From ea94dfab25b781c3fca6de006f7318216aabe86b Mon Sep 17 00:00:00 2001 From: Srikanth Sankaran <131454720+srikanth-sankaran@users.noreply.github.com> Date: Tue, 22 Oct 2024 16:58:19 +0530 Subject: [PATCH] Code review driven incremental clean up of sealed types implementation (#3141) * Get rid of stale @noreference tags * Fix javadoc * Fix indentation problems * Align method names with JLS terminology * Reuse standard code patterns * Prevent further checks on a `iterator.remove()`d element * Get rid of type annotation handling code where no type annotations are legal --- .../jdt/internal/compiler/ClassFile.java | 2 +- .../compiler/ast/SwitchStatement.java | 23 ++++++++++--------- .../compiler/classfmt/ClassFileReader.java | 6 ++--- .../internal/compiler/env/IBinaryType.java | 6 ++--- .../compiler/lookup/BinaryTypeBinding.java | 13 +++-------- .../eclipse/jdt/internal/core/BinaryType.java | 1 - .../eclipse/jdt/internal/core/SourceType.java | 1 - 7 files changed, 21 insertions(+), 31 deletions(-) diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ClassFile.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ClassFile.java index ae0a9f56079..354576a4d49 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ClassFile.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ClassFile.java @@ -6043,7 +6043,7 @@ public void initialize(SourceTypeBinding aType, ClassFile parentClassFile, boole if (aType.isAnonymousType()) { ReferenceBinding superClass = aType.superclass; if (superClass == null || !(superClass.isEnum() && superClass.isSealed())) - accessFlags &= ~ClassFileConstants.AccFinal; + accessFlags &= ~ClassFileConstants.AccFinal; } int finalAbstract = ClassFileConstants.AccFinal | ClassFileConstants.AccAbstract; if ((accessFlags & finalAbstract) == finalAbstract) { diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java index a2af4daa373..e33f7e345bf 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/SwitchStatement.java @@ -367,7 +367,7 @@ public boolean visit(TNode node) { } if (node.type instanceof ReferenceBinding ref && ref.isSealed()) { List allAllowedTypes = ref.getAllEnumerableReferenceTypes(); - this.covers &= isExhaustiveWithCaseTypes(allAllowedTypes, availableTypes); + this.covers &= caseElementsCoverSelectorType(allAllowedTypes, availableTypes); return this.covers; } this.covers = false; @@ -1456,7 +1456,7 @@ private boolean checkAndFlagDefaultSealed(BlockScope skope, CompilerOptions comp if (!(ref.isClass() || ref.isInterface() || ref.isTypeVariable() || ref.isIntersectionType())) return false; - if (!isExhaustiveWithCaseTypes(ref.getAllEnumerableReferenceTypes(), this.caseLabelElementTypes)) { + if (!caseElementsCoverSelectorType(ref.getAllEnumerableReferenceTypes(), this.caseLabelElementTypes)) { if (this instanceof SwitchExpression) // non-exhaustive switch expressions will be flagged later. return false; skope.problemReporter().enhancedSwitchMissingDefaultCase(this.expression); @@ -1470,7 +1470,7 @@ private boolean checkAndFlagDefaultRecord(BlockScope skope, CompilerOptions comp List allallowedTypes = new ArrayList<>(); allallowedTypes.add(ref); if (comps == null || comps.length == 0) { - if (!isExhaustiveWithCaseTypes(allallowedTypes, this.caseLabelElementTypes)) { + if (!caseElementsCoverSelectorType(allallowedTypes, this.caseLabelElementTypes)) { skope.problemReporter().enhancedSwitchMissingDefaultCase(this.expression); return true; } @@ -1490,7 +1490,7 @@ private boolean checkAndFlagDefaultRecord(BlockScope skope, CompilerOptions comp this.switchBits |= SwitchStatement.Exhaustive; return false; } - private boolean isExhaustiveWithCaseTypes(List allAllowedTypes, List listedTypes) { + private boolean caseElementsCoverSelectorType(List allAllowedTypes, List listedTypes) { Iterator iterator = allAllowedTypes.iterator(); while (iterator.hasNext()) { ReferenceBinding next = iterator.next(); @@ -1502,18 +1502,19 @@ private boolean isExhaustiveWithCaseTypes(List allAllowedTypes iterator.remove(); continue; } - for (TypeBinding type : listedTypes) { - // permits specifies classes, not parameterizations - if (next.erasure().isCompatibleWith(type.erasure())) { - iterator.remove(); - break; - } - } if (next.isEnum()) { int constantCount = this.otherConstants == null ? 0 : this.otherConstants.length; Set unenumeratedConstants = unenumeratedConstants(next, constantCount); if (unenumeratedConstants.size() == 0) { iterator.remove(); + continue; + } + } + for (TypeBinding type : listedTypes) { + // permits specifies classes, not parameterizations + if (next.erasure().isCompatibleWith(type.erasure())) { + iterator.remove(); + break; } } } diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/classfmt/ClassFileReader.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/classfmt/ClassFileReader.java index 7173a363b8d..d93ad08048c 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/classfmt/ClassFileReader.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/classfmt/ClassFileReader.java @@ -528,9 +528,7 @@ public ClassFileReader(byte[] classFileBytes, char[] fileName, boolean fullyInit offset += 2; this.permittedSubtypesNames = new char[this.permittedSubtypesCount][]; for (int j = 0; j < this.permittedSubtypesCount; j++) { - utf8Offset = - this.constantPoolOffsets[u2At(this.constantPoolOffsets[u2At(offset)] + 1)]; - this.permittedSubtypesNames[j] = CharDeduplication.intern(utf8At(utf8Offset + 3, u2At(utf8Offset + 1))); + this.permittedSubtypesNames[j] = CharDeduplication.intern(getConstantClassNameAt(u2At(offset))); offset += 2; } } @@ -1113,7 +1111,7 @@ && hasStructuralTypeAnnotationChanges(getTypeAnnotations(), newClassFile.getType return true; } - // permitted sub-types + // permitted subtypes char[][] newPermittedSubtypesNames = newClassFile.getPermittedSubtypesNames(); if (this.permittedSubtypesNames != newPermittedSubtypesNames) { int newPermittedSubtypesLength = newPermittedSubtypesNames == null ? 0 : newPermittedSubtypesNames.length; diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/env/IBinaryType.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/env/IBinaryType.java index fdfb8b64aa0..cce631129c9 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/env/IBinaryType.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/env/IBinaryType.java @@ -94,11 +94,11 @@ public interface IBinaryType extends IGenericType, IBinaryInfo { char[][] getInterfaceNames(); /** - * Answer the unresolved names of the receiver's permitted sub types + * Answer the unresolved names of the receiver's permitted subtypes in the + * class file format as specified in section 4.2 of the Java 2 VM spec * or null if the array is empty. * - * A name is a simple name or a qualified, dot separated name. - * For example, Hashtable or java.util.Hashtable. + * For example, java.lang.String is java/lang/String. */ default char[][] getPermittedSubtypesNames() { return null; diff --git a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java index 8cd977bb712..e5f6ffa46a2 100644 --- a/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java +++ b/org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java @@ -567,12 +567,6 @@ void cachePartsFrom(IBinaryType binaryType, boolean needFieldsAndMethods) { break; } } - for (TypeBinding permsub : this.permittedTypes) { - if (permsub.hasNullTypeAnnotations()) { - this.externalAnnotationStatus = ExternalAnnotationStatus.TYPE_IS_ANNOTATED; - break; - } - } } } @@ -2558,9 +2552,8 @@ public ReferenceBinding[] permittedTypes() { return this.permittedTypes = this.prototype.permittedTypes(); } for (int i = this.permittedTypes.length; --i >= 0;) - this.permittedTypes[i] = (ReferenceBinding) resolveType(this.permittedTypes[i], this.environment, false); + this.permittedTypes[i] = (ReferenceBinding) resolveType(this.permittedTypes[i], this.environment, false); // re-resolution seems harmless - // Note: unlike for superinterfaces() hierarchy check not required here since these are subtypes return this.permittedTypes; } @Override @@ -2635,13 +2628,13 @@ public String toString() { if (this.permittedTypes != Binding.NO_PERMITTED_TYPES) { buffer.append("\n\tpermits : "); //$NON-NLS-1$ for (int i = 0, length = this.permittedTypes.length; i < length; i++) { - if (i > 0) + if (i > 0) buffer.append(", "); //$NON-NLS-1$ buffer.append((this.permittedTypes[i] != null) ? this.permittedTypes[i].debugName() : "NULL TYPE"); //$NON-NLS-1$ } } } else { - buffer.append("NULL PERMITTEDSUBTYPES"); //$NON-NLS-1$ + buffer.append("NULL PERMITTED SUBTYPES"); //$NON-NLS-1$ } if (this.enclosingType != null) { diff --git a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/BinaryType.java b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/BinaryType.java index afce48bc719..17a149aa1a3 100644 --- a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/BinaryType.java +++ b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/BinaryType.java @@ -770,7 +770,6 @@ public boolean isRecord() throws JavaModelException { } /** * @see IType#isSealed() - * @noreference This method is not intended to be referenced by clients as it is a part of Java preview feature. */ @Override public boolean isSealed() throws JavaModelException { diff --git a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/SourceType.java b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/SourceType.java index a16c96a8103..0d8dc47af17 100644 --- a/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/SourceType.java +++ b/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/SourceType.java @@ -705,7 +705,6 @@ public boolean isRecord() throws JavaModelException { } /** * @see IType#isSealed() - * @noreference This method is not intended to be referenced by clients as it is a part of Java preview feature. */ @Override public boolean isSealed() throws JavaModelException {