[Ignore] Checking effect of some code ...

[srikanth-sankaran]

No description provided.

[srikanth-sankaran]

@stephan-herrmann - can you take a look at this PR and explain what this piece of code is supposed to do ? Disabling it doesn't seem to have any ill effect as far as tests as they exist are concerned ? Does this signal a gap in testing ? Or is that code not relevant anymore ?

I was wondering if comment #4740 (comment) kicks in here too and somehow the PR #4554 could be masking a problem here and created an alternate PR #4799 that short circuits #4554 - but that one is green too

Commit 3929efa says Additional fix ensuring proper interning in TypeSystem

In #2173 (comment), you write:

`In commit 3929efa I addressed this by setting up a protocol for swapping the TVB not only in TypeSystem.types[id] but also in the PTBKey.

Since the reported symptom was already fixed in TypeDeclaration.updateWithAnnotations(..), I added a flag by which the test can disable that particular fix, so we can observe the effect of the secondary fix which is implemented in commit 3929efa`

I have not understood this fully. Your input would be appreciated.

[stephan-herrmann]

Here is a recipe to demonstrate the effect of the flag TESTING_GH_2158, starting from master:

Disable this part from #4554, which indeed masks some of the earlier work:

--- org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/Scope.java
+++ org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/Scope.java
@@ -3738,11 +3738,11 @@
 				Object value = invocations.get(mec);
 				if (value instanceof TypeBinding[]) {
 					TypeBinding[] invalidInvocations = (TypeBinding[]) value;
-					if (areSignificantlyDifferent(invalidInvocations[0], invalidInvocations[1])) {
+//					if (areSignificantlyDifferent(invalidInvocations[0], invalidInvocations[1])) {
 						problemReporter().superinterfacesCollide(invalidInvocations[0].erasure(), typeRef, invalidInvocations[0], invalidInvocations[1]);
 						type.tagBits |= TagBits.HierarchyHasProblems;
 						return true;
-					}
+//					}
 				}
 			}
 		}

Then disable the tweak from #2173 were PTBKey.arguments is modified after the fact:

--- org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeSystem.java
+++ org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeSystem.java
@@ -20,7 +20,6 @@
 package org.eclipse.jdt.internal.compiler.lookup;
 
 import java.util.HashMap;
-import java.util.function.Consumer;
 import java.util.function.Supplier;
 import org.eclipse.jdt.internal.compiler.ast.ASTNode;
 import org.eclipse.jdt.internal.compiler.util.SimpleLookupTable;
@@ -87,20 +86,20 @@
 							TypeBinding argument = arguments[i];
 							if (argument instanceof UnresolvedReferenceBinding)
 								((UnresolvedReferenceBinding) argument).addWrapper(this, environment);
-							if (argument.getClass() == TypeVariableBinding.class) {
-								final int idx = i;
-								TypeVariableBinding typeVariableBinding = (TypeVariableBinding) argument;
-								Consumer<TypeVariableBinding> previousConsumer = typeVariableBinding.updateWhenSettingTypeAnnotations;
-								typeVariableBinding.updateWhenSettingTypeAnnotations = (newTvb) -> {
-									// update the TVB argument and simulate a re-hash:
-									ParameterizedTypeBinding[] value = HashedParameterizedTypes.this.hashedParameterizedTypes.get(this);
-									arguments[idx] = newTvb;
-									HashedParameterizedTypes.this.hashedParameterizedTypes.put(this, value);
-									// for the unlikely case of multiple PTBKeys referring to this TVB chain to the next consumer:
-									if (previousConsumer != null)
-										previousConsumer.accept(newTvb);
-								};
-							}
+//							if (argument.getClass() == TypeVariableBinding.class) {
+//								final int idx = i;
+//								TypeVariableBinding typeVariableBinding = (TypeVariableBinding) argument;
+//								Consumer<TypeVariableBinding> previousConsumer = typeVariableBinding.updateWhenSettingTypeAnnotations;
+//								typeVariableBinding.updateWhenSettingTypeAnnotations = (newTvb) -> {
+//									// update the TVB argument and simulate a re-hash:
+//									ParameterizedTypeBinding[] value = HashedParameterizedTypes.this.hashedParameterizedTypes.get(this);
+//									arguments[idx] = newTvb;
+//									HashedParameterizedTypes.this.hashedParameterizedTypes.put(this, value);
+//									// for the unlikely case of multiple PTBKeys referring to this TVB chain to the next consumer:
+//									if (previousConsumer != null)
+//										previousConsumer.accept(newTvb);
+//								};
+//							}
 						}
 					}
 				}

Now, when you run NullTypeAnnotationTest.testGH2158(), the first invocation of runConformTest() succeeds, but with TESTING_GH_2158=true the same fails with:

----------\n
1. ERROR in abc\d\DMessageHandlerRegistryImpl.java (at line 4)
	public class DMessageHandlerRegistryImpl<@NonNull C extends DConnection>
	             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
The interface MessageHandlerRegistry cannot be implemented more than once with different arguments: MessageHandlerRegistry<C,CharSequence,DIncomingMessageData> and MessageHandlerRegistry<C,CharSequence,DIncomingMessageData>
----------

You rightly identified that there is some subtle interaction between several of these changes. It would be super cool if in the end several of these changes can be replaced by just one systematic change. For the cluster of issues relating to type variables, unique IDs and type annotations, there may indeed be a common solution. It's only the wildcard thing from #4554 that sits oddly in the mix: the latter tries to simulate a javac bug strictly relating to the semantics of wildcards, which should not interact with the technical issues of type interning :(

[stephan-herrmann]

Trying to describe the interaction between type interning and #4554 (this is an extended version of #4740 (comment)):

Experiment:

• disable the tweak from Null-Analysis - The interface ... cannot be implemented more than once with different arguments ... #2173 like above
• debug that part of NullTypeAnnotationTest.testGH2158 that sets TESTING_GH_2158

The interning bug strikes in this call stack:

TypeBinding.notEquals(TypeBinding, TypeBinding) line: 1718	
ClassScope(Scope).minimalErasedCandidates(TypeBinding[], Map<TypeBinding,Object>) line: 4442	
ClassScope(Scope).hasErasedCandidatesCollisions(TypeBinding, TypeBinding, Map<TypeBinding,Object>, ReferenceBinding, ASTNode) line: 3734	
ClassScope.checkParameterizedSuperTypeCollisions() line: 928	
CompilationUnitScope.checkParameterizedTypes() line: 340	

Here equivalence of these types is not recognized, although they only differ in null annotations:

• MessageHandlerRegistry<@NonNull C extends DConnection,CharSequence,DIncomingMessageData> id = 168
• MessageHandlerRegistry<@NonNull C extends DConnection,@NonNull CharSequence,@NonNull DIncomingMessageData> id = 175

Next, Scope.minimalErasedCandidates() adds the above types as a seeming conflict into the same slot of allInvocations.

Right when we are about to report problemReporter().superinterfacesCollide() the tweak from #4554 breaks down the equivalence checks to the components of both PTB: type and arguments. Indeed all these bindings have pairwise identical ids. As we didn't even apply any wildcard related adjustment, this method in a way "proves" that the id-difference at the PTB level was wrong, since no substantial difference was found in its ingredients. So the #4554 tweak does no harm in terms of type interning, it only covers up a bogus difference in one particular situation.

[srikanth-sankaran]

I am away from my laptop and will try these experiments tomorrow but ignoring for the moment this special flag to exercise some part of the fix, what is that block of code itself doing ??

Any explanation as to why removing it does not cause any failures ? Is it a gap in testing ? Or is that code rendered irrelevant somehow ??

[stephan-herrmann]

I am away from my laptop and will try these experiments tomorrow but ignoring for the moment this special flag to exercise some part of the fix, what is that block of code itself doing ??

Any explanation as to why removing it does not cause any failures ?

#2173 implements two fixes, each of which alone fixes the new test case. To see a failure remove both fixes (plus the "repair" tweak from #4554).

In normal mode, testGH2158 uses the fix in TypeDeclaration, with TESTING_GH_2158 set, the test uses the fix in TypeSystem.

Since at the code level I couldn't prove that one fix truly subsumes the other one (either way), I kept both fixes, and added the flag that allows tests to cover the second fix while disabling the first.

Is it a gap in testing ? Or is that code rendered irrelevant somehow ??

You may call it a gap in testing: at that time I believed that the change in TypeSystem had value in itself, but I couldn't find a test case that bypasses the fix in TypeDeclaration, to actually demonstrate necessity of the change in TypeSystem.

[srikanth-sankaran]

Ok, thanks for the picture. I think I am ready to start attempting an alternate fix - stay tuned.

[stephan-herrmann]

@srikanth-sankaran I still owe you an answer here:

what is that block of code itself doing ??

While the exact sequence of events is a pain to put down, the main description from #2173 is still valid:

... It happened because a ParameterizedTypeBinding was created (as a super interface binding) before the underlying generic type was fully updated (CompleteTypeBindingsSteps.INTEGRATE_ANNOTATIONS_IN_HIERARCHY) which clearly is a bug.

While further untangling the order of processing steps doesn't look very feasible, I found a way to fix this super type after the fact (during TypeDeclaration.updateSupertypesWithAnnotations(), which already exists for this kind of udpate).
This is done by (a) detecting this situation (with help of SourceTypeBinding.supertypeAnnotationsUpdated) and if necessary (b) re-create the parameterized type from the updated generic type. ...

IOW, resolving super types intentionally works without any regard of type annotations. If this includes creating a PTB from an STB with an annotated super type, those super type annotations will also be missing in the PTB.

When later we update the STB we mark it as supertypeAnnotationsUpdated.

Next when we encounter a PTB whose type has that flag set, also the PTB is recreated this time including information about annotated supertypes. All this happens during phase INTEGRATE_ANNOTATIONS_IN_HIERARCHY.

This is the part of #2173 that I'm quite confident about, as it can be explained in semantic terms of resolving supertypes and integrating type annotations into the hierarchy lateron. The tweaks concerning type interning are more of a hack I'd say.