False type mismatch on array initializer with null annotations (regression 2023-03)

[stephan-herrmann]

fixes #1007

Here I removed one code block from #630 and made resolving on one particular call path deferrable.

This breaks the following unfortunate chain:

1. A type's hierarchy was being connected
2. ClassScope.findSupertype() triggered initializing the null default
   (since #630, still needed and retained)
3. This triggered resolving annotations
4. An annotation had member value pairs, where values were type checked against the annotation's attributes
5. The value type contained a type that did not yet have super types connected
6. This triggered ahead of time resolving
   ( #630, now removed)
7. That ahead of time resolving caused havoc in #1007

Starting at (4) different test cases had different ways to cause havoc (different ways to trigger undesired ahead-of-time side-effects). So that point offered itself as a suitable location for severing the unfortunate chain and start deferring instead.

The complex part is making the set of deferred tasks as narrow as possible (see new method Annotation.shouldDeferResolvingDetails()).

I merged the solution into prior art around ClassScope#deferredBoundChecks, which works fine. Only the name ClassScope.checkParameterizedTypeBounds() is no longer accurate (I simply forgot to change it).

[stephan-herrmann]

@srikanth-sankaran , @iloveeclipse I plan to have a look at other issues linked from #1007 on Thursday, with a goal to have this merged before RC1.

Until then feel free to have a look, test against other reports, improve ... whatever may be needed to get this into good shape in time.

[snjeza]

The PR fixes #1007, tests pass.
It also fixes #854
The issue described at redhat-developer/vscode-java#3076 (comment) isn't fixed.
It is caused by

ClassScope.findSupertype() triggered initializing the null default
(since #630, still needed and retained)

[snjeza]

The issue described at redhat-developer/vscode-java#3076 (comment)

A simple project example:
test3076.zip

A potential patch:

diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/ClassScope.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/ClassScope.java
index fe75e26dbd..0a4c667904 100644
--- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/ClassScope.java
+++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/ClassScope.java
@@ -1671,6 +1671,7 @@ public class ClassScope extends Scope {
                        this.superTypeReference = typeReference;
                        if (this.compilerOptions().isAnnotationBasedNullAnalysisEnabled) {
                                this.hasDefaultNullnessFor(0 /*location*/, typeReference.sourceStart);
+                               this.referenceContext.binding.tagBits &= ~TagBits.HasNoMemberTypes;
                        }
                        ReferenceBinding superType = (ReferenceBinding) typeReference.resolveSuperType(this);
                        return superType;

[stephan-herrmann]

Thanks, @snjeza I can reproduce.

[stephan-herrmann]

A potential patch:

Thanks, this helps to understand the chain of causality.

Still I prefer to avoid undesired side effects (ahead of time) rather than repairing broken results after the fact. I have a patch-in-progress, that fixes this issue, just need to resolve some hiccups in existing tests :)

[stephan-herrmann]

The example from redhat-developer/vscode-java#3076 led me to revise the implementation strategy.

Before updating the PR description, here's the old version for posterity:

Here I removed one code block from #630 and made type checking on one particular call path deferrable.

This breaks the following unfortunate chain:

• A type's hierarchy was being connected
• ClassScope.findSupertype() triggered initializing the null default
  (since #630, still needed and retained)
• This triggered resolving annotations
• An annotation had member value pairs, where values were type checked against the annotation's attributes
• The value type contained a type that did not yet have super types connected
• This triggered ahead of time resolving
  (#630, now removed)
• That ahead of time resolving caused havoc in #1007

That chain is severed at the point where we went from resolving (OK) to type checking (NOK, because super type information was needed).

At first I thought I could merge the solution with prior art around ClassScope#deferredBoundChecks but that mechanism provides deferring only until the end of the current connectTypeHierarchy() but here we may have to wait until some other type has its hierarchy connected. For that reason I added a new list of DeferrableChecks to the central LookupEnvirnment, each equipped with its own enablement condition.

I wondered if iterating all deferred check tasks after each connectTypeHierarchy() could have an impact on performance, but for now the condition for deferring is so rare that I see no motivation for optimization yet (may revisit when other situations join in that mechanism).

[stephan-herrmann]

We have a cluster of defects and need a fix that will address all of them. This is what I know at the moment:

#854
eclipse-jdtls/eclipse.jdt.ls#2386
#629
#1007
#875

Of these the following have been fixed with tests to witness:

• Error marker when null analysis enabled #854
• Critical compilation bug when null analysis enabled #629 (test from #630)
• False type mismatch on array initializer with null annotations (regression 2023-03) #1007

eclipse-jdtls/eclipse.jdt.ls#2386 is just the downstream of #629.

I don't (yet) have a test case for #875.

[snjeza]

I don't (yet) have a test case for #875.

@stephan-herrmann You may want to take a look at #875 (comment)

[stephan-herrmann]

I don't (yet) have a test case for #875.

@stephan-herrmann You may want to take a look at #875 (comment)

Thanks, I saw it. But I saw no proof that it's really the same issue 😄

[stephan-herrmann]

With just a tiny improvement of my fix I could also resolve #986

[snjeza]

Thanks, I saw it. But I saw no proof that it's really the same issue

I can reproduce the issue described at #875 (comment) using the jdt.core master - https://github.com/eclipse/xtext/blob/main/org.eclipse.xtext.tests/src/org/eclipse/xtext/validation/AbstractCompositeValidatorTest.java#L140
Your PR fixes it.

[stephan-herrmann]

I can reproduce the issue described at #875 (comment) using the jdt.core master - https://github.com/eclipse/xtext/blob/main/org.eclipse.xtext.tests/src/org/eclipse/xtext/validation/AbstractCompositeValidatorTest.java#L140 Your PR fixes it.

Thanks for testing!

Looking at that test class, yes, the class, whose literal is used as an annotation argument will not have been connected yet, because it sits in the same compilation unit as the type being processed. It will be processed later based on source order.

Additionally, this declaration in ComposedChecks requires the subtype relationship which was broken due to #630:
Class<? extends AbstractDeclarativeValidator>[] validators();

With this I'm convinced it's a duplicate indeed, and forgo writing another test.

[stephan-herrmann]

Now that tests passed:

@srikanth-sankaran may I get your review for inclusion in RC1. I promise I'll stop force-pushing more changes now :)

@iloveeclipse can I get your project lead's approval?

Thanks.

[stephan-herrmann]

I'm in the middle of preparing an alternative, even safer PR. I hope to be able to complete it today.

[stephan-herrmann]

While it was fairly straight-forward to get this approach to pass all tests, there's a known weakness, for which I don't know what symptoms it might cause, but from looking at the code this approach creates the following issue:

When Annotation.resolveType() is invoked from ASTNode.resolveAnnotations(), the work in Annotation.resolveType() itself can be safely split into two chunks performed at different points in time, but when deferring, the tail of ASTNode.resolveAnnotations() will work with incomplete information and I found no easy way to catch up in this regard after the deferred task has been performed.

For that reason I created an alternative, more drastic approach in PR #1077.

[stephan-herrmann]

Moving forward I'm putting my money on #1077 instead of this PR.

[stephan-herrmann]

Resolved indeed via #1077