From 33667c9f2b56d859f541b5a96bbe816c74a22bde Mon Sep 17 00:00:00 2001 From: Stephan Herrmann Date: Sun, 17 Mar 2024 18:32:32 +0100 Subject: [PATCH 1/2] Null-Analysis - The interface ... cannot be implemented more than once with different arguments ... fixes #2158 --- .../compiler/ast/TypeDeclaration.java | 11 +++- .../compiler/lookup/SourceTypeBinding.java | 1 + .../regression/NullTypeAnnotationTest.java | 65 +++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/TypeDeclaration.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/TypeDeclaration.java index 417c389a75a..263c7115500 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/TypeDeclaration.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/TypeDeclaration.java @@ -1927,6 +1927,13 @@ public void updateSupertypesWithAnnotations(Map outerUpdates, Map updates) { + if (previousType instanceof ParameterizedTypeBinding previousPTB + && previousPTB.original() instanceof SourceTypeBinding previousOriginal + && previousOriginal.supertypeAnnotationsUpdated) { + // re-initialized parameterized type with updated annotations from the original: + typeRef.resolvedType = this.scope.environment().createParameterizedType(previousOriginal, previousPTB.arguments, previousType.enclosingType()); + } + typeRef.updateWithAnnotations(this.scope, 0); ReferenceBinding updatedType = (ReferenceBinding) typeRef.resolvedType; if (updatedType instanceof ParameterizedTypeBinding) { @@ -1940,8 +1947,10 @@ protected ReferenceBinding updateWithAnnotations(TypeReference typeRef, Referenc if (previousType != null) { if (previousType.id == TypeIds.T_JavaLangObject && ((this.binding.tagBits & TagBits.HierarchyHasProblems) != 0)) return previousType; // keep this cycle breaker - if (previousType != updatedType) //$IDENTITY-COMPARISON$ + if (previousType != updatedType) { //$IDENTITY-COMPARISON$ updates.put(previousType, updatedType); + this.binding.supertypeAnnotationsUpdated = true; + } } return updatedType; } diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/SourceTypeBinding.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/SourceTypeBinding.java index 8a95de66160..b27b39c5921 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/SourceTypeBinding.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/SourceTypeBinding.java @@ -138,6 +138,7 @@ public class SourceTypeBinding extends ReferenceBinding { public boolean isVarArgs = false; // for record declaration private FieldBinding[] implicitComponentFields; // cache private MethodBinding[] recordComponentAccessors = null; // hash maybe an overkill + public boolean supertypeAnnotationsUpdated = false; // have any supertype annotations been updated during CompleteTypeBindingsSteps.INTEGRATE_ANNOTATIONS_IN_HIERARCHY? public SourceTypeBinding(char[][] compoundName, PackageBinding fPackage, ClassScope scope) { this.compoundName = compoundName; diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java index e2bb6f5fa1c..842ab123207 100644 --- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java +++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java @@ -19242,4 +19242,69 @@ public boolean hasNext() { "----------\n"; runner.runNegativeTest(); } + +public void testGH2158() { + Runner runner = new Runner(); + runner.testFiles = new String[] { + "abc/Connection.java", + """ + package abc; + public interface Connection<@org.eclipse.jdt.annotation.NonNull M> { } + """, + "abc/IncomingMessageData.java", + """ + package abc; + public interface IncomingMessageData<@org.eclipse.jdt.annotation.NonNull T> { } + """, + "abc/MessageHandlerRegistry.java", + """ + package abc; + import org.eclipse.jdt.annotation.*; + public interface MessageHandlerRegistry + <@NonNull C extends Connection, @NonNull T, @NonNull D extends IncomingMessageData> { } + """, + "abc/MessageHandlerRegistryImpl.java", + """ + package abc; + import org.eclipse.jdt.annotation.*; + public class MessageHandlerRegistryImpl + <@NonNull C extends Connection, @NonNull T, @NonNull D extends IncomingMessageData> + implements MessageHandlerRegistry { } + """, + "abc/d/DConnection.java", + """ + package abc.d; + import abc.*; + import org.eclipse.jdt.annotation.*; + public interface DConnection extends Connection<@NonNull CharSequence> { } + """, + "abc/d/DIncomingMessageData.java", + """ + package abc.d; + import org.eclipse.jdt.annotation.*; + import abc.*; + public interface DIncomingMessageData extends IncomingMessageData<@NonNull CharSequence> { } + """, + "abc/d/DMessageHandlerRegistry.java", + """ + package abc.d; + import org.eclipse.jdt.annotation.*; + import abc.*; + public interface DMessageHandlerRegistry<@NonNull C extends DConnection> + extends MessageHandlerRegistry { } + """, + "abc/d/DMessageHandlerRegistryImpl.java", + """ + package abc.d; + import org.eclipse.jdt.annotation.*; + import abc.*; + public class DMessageHandlerRegistryImpl<@NonNull C extends DConnection> + extends MessageHandlerRegistryImpl + implements DMessageHandlerRegistry { } + """ + }; + runner.customOptions = getCompilerOptions(); + runner.classLibraries = this.LIBS; + runner.runConformTest(); +} } From 3929efa8014b12f898357f039268aa780443548e Mon Sep 17 00:00:00 2001 From: Stephan Herrmann Date: Tue, 19 Mar 2024 17:58:26 +0100 Subject: [PATCH 2/2] Null-Analysis - The interface ... cannot be implemented more than once with different arguments ... Additional fix ensuring proper interning in TypeSystem --- .../compiler/ast/TypeDeclaration.java | 11 ++++++--- .../internal/compiler/lookup/TypeSystem.java | 23 +++++++++++++++++-- .../compiler/lookup/TypeVariableBinding.java | 7 ++++++ .../regression/NullTypeAnnotationTest.java | 9 ++++++++ 4 files changed, 45 insertions(+), 5 deletions(-) diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/TypeDeclaration.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/TypeDeclaration.java index 263c7115500..bb3cc2dd6bc 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/TypeDeclaration.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/TypeDeclaration.java @@ -98,6 +98,9 @@ public class TypeDeclaration extends Statement implements ProblemSeverities, Ref // 15 Sealed Type preview support public TypeReference[] permittedTypes; + // TEST ONLY: disable one fix here to challenge another related fix (in TypeSystem): + public static boolean TESTING_GH_2158 = false; + static { disallowedComponentNames = new HashSet<>(6); disallowedComponentNames.add("clone"); //$NON-NLS-1$ @@ -1927,11 +1930,13 @@ public void updateSupertypesWithAnnotations(Map outerUpdates, Map updates) { - if (previousType instanceof ParameterizedTypeBinding previousPTB + if (!TESTING_GH_2158 + && previousType instanceof ParameterizedTypeBinding previousPTB && previousPTB.original() instanceof SourceTypeBinding previousOriginal && previousOriginal.supertypeAnnotationsUpdated) { - // re-initialized parameterized type with updated annotations from the original: - typeRef.resolvedType = this.scope.environment().createParameterizedType(previousOriginal, previousPTB.arguments, previousType.enclosingType()); + // re-initialized parameterized type with updated annotations from the original: + typeRef.resolvedType = this.scope.environment().createParameterizedType(previousOriginal, // <- has been updated + previousPTB.arguments, previousType.enclosingType(), previousType.getAnnotations()); // <- no changes here } typeRef.updateWithAnnotations(this.scope, 0); diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeSystem.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeSystem.java index d1f7c9ac0ef..df729cb7d29 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeSystem.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeSystem.java @@ -20,6 +20,7 @@ package org.eclipse.jdt.internal.compiler.lookup; import java.util.HashMap; +import java.util.function.Consumer; import org.eclipse.jdt.internal.compiler.ast.ASTNode; import org.eclipse.jdt.internal.compiler.util.SimpleLookupTable; @@ -82,11 +83,26 @@ public PTBKey(ReferenceBinding type, TypeBinding[] arguments, ReferenceBinding e if (type instanceof UnresolvedReferenceBinding) ((UnresolvedReferenceBinding) type).addWrapper(this, environment); if (arguments != null) { - for (TypeBinding argument : arguments) { + for (int i = 0; i < arguments.length; i++) { + TypeBinding argument = arguments[i]; if (argument instanceof UnresolvedReferenceBinding) ((UnresolvedReferenceBinding) argument).addWrapper(this, environment); if (argument.hasNullTypeAnnotations()) this.tagBits |= TagBits.HasNullTypeAnnotation; + if (argument.getClass() == TypeVariableBinding.class) { + final int idx = i; + TypeVariableBinding typeVariableBinding = (TypeVariableBinding) argument; + Consumer 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); + }; + } } } } @@ -253,13 +269,16 @@ public final TypeBinding getUnannotatedType(TypeBinding type) { * If it itself is already registered as the key unannotated type of its family, * create a clone to play that role from now on and swap types in the types cache. */ - public void forceRegisterAsDerived(TypeBinding derived) { + public void forceRegisterAsDerived(TypeVariableBinding derived) { int id = derived.id; if (id != TypeIds.NoId && this.types[id] != null) { TypeBinding unannotated = this.types[id][0]; if (unannotated == derived) { //$IDENTITY-COMPARISON$ // was previously registered as unannotated, replace by a fresh clone to remain unannotated: this.types[id][0] = unannotated = derived.clone(null); + if (derived.updateWhenSettingTypeAnnotations != null) { + derived.updateWhenSettingTypeAnnotations.accept((TypeVariableBinding) unannotated); + } } // proceed as normal: cacheDerivedType(unannotated, derived); diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeVariableBinding.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeVariableBinding.java index 2e5b6955bce..f7ef22e0f74 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeVariableBinding.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/TypeVariableBinding.java @@ -44,6 +44,7 @@ import java.util.Arrays; import java.util.Set; +import java.util.function.Consumer; import org.eclipse.jdt.core.compiler.CharOperation; import org.eclipse.jdt.internal.compiler.ast.ASTNode; @@ -77,6 +78,12 @@ public class TypeVariableBinding extends ReferenceBinding { public char[] genericTypeSignature; LookupEnvironment environment; + /* + * In one particular situation a TVB will be cloned and the clone will be used as the 'naked' type + * within TypeSystem. This may require some updating inside TypeSystem's hash structure. + */ + Consumer updateWhenSettingTypeAnnotations; + public TypeVariableBinding(char[] sourceName, Binding declaringElement, int rank, LookupEnvironment environment) { this.sourceName = sourceName; this.declaringElement = declaringElement; diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java index 842ab123207..739123e7c50 100644 --- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java +++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java @@ -29,6 +29,7 @@ import org.eclipse.jdt.core.JavaCore; import org.eclipse.jdt.core.tests.compiler.regression.AbstractRegressionTest.JavacTestOptions.Excuse; +import org.eclipse.jdt.internal.compiler.ast.TypeDeclaration; import org.eclipse.jdt.internal.compiler.classfmt.ClassFileConstants; import org.eclipse.jdt.internal.compiler.impl.CompilerOptions; @@ -19306,5 +19307,13 @@ public class DMessageHandlerRegistryImpl<@NonNull C extends DConnection> runner.customOptions = getCompilerOptions(); runner.classLibraries = this.LIBS; runner.runConformTest(); + + // challenge other part of the fix: + TypeDeclaration.TESTING_GH_2158 = true; + try { + runner.runConformTest(); + } finally { + TypeDeclaration.TESTING_GH_2158 = false; + } } }