From e072376300a172ae2a1636f8adb15a3aec130583 Mon Sep 17 00:00:00 2001 From: Neil Brown Date: Fri, 20 Aug 2021 17:32:33 +0100 Subject: [PATCH] Finish records support (#4871) * Allow method annotations on compact constructors in stubs * Support automatic transfer from declaration annotations on record components in stubs to the corresponding field, method, and compact canonical constructor parameter. (Outside stubs this is done automatically by javac.) --- build.gradle | 3 +- .../checker/test/junit/DisbarUseTest.java | 33 +++++++++ .../checker/test/junit/LockTest.java | 5 +- .../disbaruse/DisbarUseChecker.java | 9 +++ .../disbaruse/DisbarUseTypeFactory.java | 22 ++++++ .../disbaruse/DisbarUseVisitor.java | 69 +++++++++++++++++++ .../testchecker/disbaruse/messages.properties | 1 + .../testchecker/disbaruse/qual/DisbarUse.java | 8 +++ .../disbaruse/qual/DisbarUseBottom.java | 12 ++++ .../disbaruse/qual/DisbarUseTop.java | 11 +++ .../disbaruse-records/DisbarredClass.java | 41 +++++++++++ .../disbaruse-records/DisbarredRecord.java | 21 ++++++ .../DisbarredRecordByStubs.java | 19 +++++ .../DisbarredRecordByStubs2.java | 22 ++++++ checker/tests/disbaruse-records/disbar.astub | 6 ++ checker/tests/lock-records/LockRecord.java | 11 +++ .../stub/AnnotationFileElementTypes.java | 38 ++++++++++ .../framework/stub/AnnotationFileParser.java | 37 +++++++++- 18 files changed, 363 insertions(+), 5 deletions(-) create mode 100644 checker/src/test/java/org/checkerframework/checker/test/junit/DisbarUseTest.java create mode 100644 checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseChecker.java create mode 100644 checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseTypeFactory.java create mode 100644 checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseVisitor.java create mode 100644 checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/messages.properties create mode 100644 checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUse.java create mode 100644 checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUseBottom.java create mode 100644 checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUseTop.java create mode 100644 checker/tests/disbaruse-records/DisbarredClass.java create mode 100644 checker/tests/disbaruse-records/DisbarredRecord.java create mode 100644 checker/tests/disbaruse-records/DisbarredRecordByStubs.java create mode 100644 checker/tests/disbaruse-records/DisbarredRecordByStubs2.java create mode 100644 checker/tests/disbaruse-records/disbar.astub create mode 100644 checker/tests/lock-records/LockRecord.java diff --git a/build.gradle b/build.gradle index 8e98e934219..eb7a08d0570 100644 --- a/build.gradle +++ b/build.gradle @@ -405,8 +405,7 @@ List getJavaFilesToFormat(projectName) { && !details.path.contains("calledmethods-delomboked") && !details.path.contains("returnsreceiverdelomboked") && !details.path.contains("build") - && (isJava16 || !details.path.contains("nullness-records")) - && (isJava16 || !details.path.contains("stubparser-records")) + && (isJava16 || !details.path.contains("-records")) && details.name.endsWith('.java')) { javaFiles.add(details.file) } diff --git a/checker/src/test/java/org/checkerframework/checker/test/junit/DisbarUseTest.java b/checker/src/test/java/org/checkerframework/checker/test/junit/DisbarUseTest.java new file mode 100644 index 00000000000..f804e80ae85 --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/test/junit/DisbarUseTest.java @@ -0,0 +1,33 @@ +package org.checkerframework.checker.test.junit; + +import java.io.File; +import java.util.List; +import org.checkerframework.checker.testchecker.disbaruse.DisbarUseChecker; +import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest; +import org.junit.runners.Parameterized.Parameters; + +public class DisbarUseTest extends CheckerFrameworkPerDirectoryTest { + + /** + * Create a DisbarUseTest. + * + * @param testFiles the files containing test code, which will be type-checked + */ + public DisbarUseTest(List testFiles) { + super( + testFiles, + DisbarUseChecker.class, + "disbaruse-records", + "-Anomsgtext", + "-Astubs=tests/disbaruse-records", + "-AstubWarnIfNotFound"); + } + + @Parameters + public static String[] getTestDirs() { + // Check for JDK 16+ without using a library: + if (System.getProperty("java.version").matches("^(1[6-9]|[2-9][0-9])\\..*")) + return new String[] {"disbaruse-records"}; + else return new String[] {}; + } +} diff --git a/checker/src/test/java/org/checkerframework/checker/test/junit/LockTest.java b/checker/src/test/java/org/checkerframework/checker/test/junit/LockTest.java index 251c01325ff..6ec8ce1a5fd 100644 --- a/checker/src/test/java/org/checkerframework/checker/test/junit/LockTest.java +++ b/checker/src/test/java/org/checkerframework/checker/test/junit/LockTest.java @@ -18,6 +18,9 @@ public LockTest(List testFiles) { @Parameters public static String[] getTestDirs() { - return new String[] {"lock", "all-systems"}; + // Check for JDK 16+ without using a library: + if (System.getProperty("java.version").matches("^(1[6-9]|[2-9][0-9])\\..*")) + return new String[] {"lock", "lock-records", "all-systems"}; + else return new String[] {"lock", "all-systems"}; } } diff --git a/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseChecker.java b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseChecker.java new file mode 100644 index 00000000000..6d603c84e07 --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseChecker.java @@ -0,0 +1,9 @@ +package org.checkerframework.checker.testchecker.disbaruse; + +import org.checkerframework.common.basetype.BaseTypeChecker; + +/** + * A checker that issues a "disbar.use" error at any use of fields, methods or parameters whose type + * is {@code @DisbarUse}. + */ +public class DisbarUseChecker extends BaseTypeChecker {} diff --git a/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseTypeFactory.java b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseTypeFactory.java new file mode 100644 index 00000000000..c3812c3ebaf --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseTypeFactory.java @@ -0,0 +1,22 @@ +package org.checkerframework.checker.testchecker.disbaruse; + +import java.lang.annotation.Annotation; +import java.util.Arrays; +import java.util.LinkedHashSet; +import java.util.Set; +import org.checkerframework.checker.testchecker.disbaruse.qual.DisbarUseBottom; +import org.checkerframework.checker.testchecker.disbaruse.qual.DisbarUseTop; +import org.checkerframework.common.basetype.BaseAnnotatedTypeFactory; +import org.checkerframework.common.basetype.BaseTypeChecker; + +public class DisbarUseTypeFactory extends BaseAnnotatedTypeFactory { + public DisbarUseTypeFactory(BaseTypeChecker checker) { + super(checker); + postInit(); + } + + @Override + protected Set> createSupportedTypeQualifiers() { + return new LinkedHashSet<>(Arrays.asList(DisbarUseTop.class, DisbarUseBottom.class)); + } +} diff --git a/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseVisitor.java b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseVisitor.java new file mode 100644 index 00000000000..bae45d78427 --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/DisbarUseVisitor.java @@ -0,0 +1,69 @@ +package org.checkerframework.checker.testchecker.disbaruse; + +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import javax.lang.model.element.Element; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.ExecutableElement; +import org.checkerframework.checker.testchecker.disbaruse.qual.DisbarUse; +import org.checkerframework.common.basetype.BaseTypeChecker; +import org.checkerframework.common.basetype.BaseTypeVisitor; +import org.checkerframework.javacutil.TreeUtils; + +public class DisbarUseVisitor extends BaseTypeVisitor { + public DisbarUseVisitor(BaseTypeChecker checker) { + super(checker); + } + + protected DisbarUseVisitor(BaseTypeChecker checker, DisbarUseTypeFactory typeFactory) { + super(checker, typeFactory); + } + + @Override + protected DisbarUseTypeFactory createTypeFactory() { + return new DisbarUseTypeFactory(checker); + } + + @Override + public Void visitMethodInvocation(MethodInvocationTree node, Void p) { + ExecutableElement methodElt = TreeUtils.elementFromUse(node); + if (methodElt != null && atypeFactory.getDeclAnnotation(methodElt, DisbarUse.class) != null) { + checker.reportError(node, "disbar.use"); + } + return super.visitMethodInvocation(node, p); + } + + @Override + public Void visitNewClass(NewClassTree node, Void p) { + ExecutableElement consElt = TreeUtils.elementFromUse(node); + if (consElt != null && atypeFactory.getDeclAnnotation(consElt, DisbarUse.class) != null) { + checker.reportError(node, "disbar.use"); + } + return super.visitNewClass(node, p); + } + + @Override + public Void visitIdentifier(IdentifierTree node, Void p) { + MemberSelectTree enclosingMemberSel = enclosingMemberSelect(); + ExpressionTree[] expressionTrees = + enclosingMemberSel == null + ? new ExpressionTree[] {node} + : new ExpressionTree[] {enclosingMemberSel, node}; + + for (ExpressionTree memberSel : expressionTrees) { + final Element elem = TreeUtils.elementFromUse(memberSel); + + // We only issue errors for variables that are fields or parameters: + if (elem != null && (elem.getKind().isField() || elem.getKind() == ElementKind.PARAMETER)) { + if (atypeFactory.getDeclAnnotation(elem, DisbarUse.class) != null) { + checker.reportError(memberSel, "disbar.use"); + } + } + } + + return super.visitIdentifier(node, p); + } +} diff --git a/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/messages.properties b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/messages.properties new file mode 100644 index 00000000000..7cad93edb50 --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/messages.properties @@ -0,0 +1 @@ +disbar.use=Use of this element is disbarred diff --git a/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUse.java b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUse.java new file mode 100644 index 00000000000..720e5cbf1be --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUse.java @@ -0,0 +1,8 @@ +package org.checkerframework.checker.testchecker.disbaruse.qual; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; + +/** The Disbar Use Checker isses a warning when an expression of this type is used. */ +@Target({ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.FIELD, ElementType.PARAMETER}) +public @interface DisbarUse {} diff --git a/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUseBottom.java b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUseBottom.java new file mode 100644 index 00000000000..43d9fecf6bb --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUseBottom.java @@ -0,0 +1,12 @@ +package org.checkerframework.checker.testchecker.disbaruse.qual; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; +import org.checkerframework.framework.qual.DefaultFor; +import org.checkerframework.framework.qual.SubtypeOf; +import org.checkerframework.framework.qual.TypeUseLocation; + +@DefaultFor(TypeUseLocation.LOWER_BOUND) +@SubtypeOf(DisbarUseTop.class) +@Target({ElementType.TYPE_USE}) +public @interface DisbarUseBottom {} diff --git a/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUseTop.java b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUseTop.java new file mode 100644 index 00000000000..39749401b0e --- /dev/null +++ b/checker/src/test/java/org/checkerframework/checker/testchecker/disbaruse/qual/DisbarUseTop.java @@ -0,0 +1,11 @@ +package org.checkerframework.checker.testchecker.disbaruse.qual; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; +import org.checkerframework.framework.qual.DefaultQualifierInHierarchy; +import org.checkerframework.framework.qual.SubtypeOf; + +@Target({ElementType.TYPE_USE}) +@SubtypeOf({}) +@DefaultQualifierInHierarchy +public @interface DisbarUseTop {} diff --git a/checker/tests/disbaruse-records/DisbarredClass.java b/checker/tests/disbaruse-records/DisbarredClass.java new file mode 100644 index 00000000000..255084d17f6 --- /dev/null +++ b/checker/tests/disbaruse-records/DisbarredClass.java @@ -0,0 +1,41 @@ +import org.checkerframework.checker.testchecker.disbaruse.qual.DisbarUse; + +class DisbarredClass { + + @DisbarUse String barred; + String fine; + + DisbarredClass() {} + + @DisbarUse + DisbarredClass(String param) {} + + DisbarredClass(@DisbarUse Integer param) {} + + DisbarredClass(@DisbarUse Long param) { + // :: error: (disbar.use) + param = 7L; + // :: error: (disbar.use) + param.toString(); + } + + @DisbarUse + void disbarredMethod() {} + + void invalid() { + // :: error: (disbar.use) + disbarredMethod(); + // :: error: (disbar.use) + new DisbarredClass(""); + // :: error: (disbar.use) + barred = ""; + // :: error: (disbar.use) + int x = barred.length(); + } + + void valid() { + new DisbarredClass(); + fine = ""; + int x = fine.length(); + } +} diff --git a/checker/tests/disbaruse-records/DisbarredRecord.java b/checker/tests/disbaruse-records/DisbarredRecord.java new file mode 100644 index 00000000000..34c9cdae464 --- /dev/null +++ b/checker/tests/disbaruse-records/DisbarredRecord.java @@ -0,0 +1,21 @@ +import org.checkerframework.checker.testchecker.disbaruse.qual.DisbarUse; + +record DisbarredRecord(@DisbarUse String barred, String fine) { + + DisbarredRecord { + // :: error: (disbar.use) + int x = barred.length(); + } + + void invalid() { + // :: error: (disbar.use) + barred(); + // :: error: (disbar.use) + int x = barred.length(); + } + + void valid() { + fine(); + int x = fine.length(); + } +} diff --git a/checker/tests/disbaruse-records/DisbarredRecordByStubs.java b/checker/tests/disbaruse-records/DisbarredRecordByStubs.java new file mode 100644 index 00000000000..13730e728f1 --- /dev/null +++ b/checker/tests/disbaruse-records/DisbarredRecordByStubs.java @@ -0,0 +1,19 @@ +record DisbarredRecordByStubs(String barred, String fine) { + + DisbarredRecordByStubs { + // :: error: (disbar.use) + int x = barred.length(); + } + + void invalid() { + // :: error: (disbar.use) + barred(); + // :: error: (disbar.use) + int x = barred.length(); + } + + void valid() { + fine(); + int x = fine.length(); + } +} diff --git a/checker/tests/disbaruse-records/DisbarredRecordByStubs2.java b/checker/tests/disbaruse-records/DisbarredRecordByStubs2.java new file mode 100644 index 00000000000..8df03d950aa --- /dev/null +++ b/checker/tests/disbaruse-records/DisbarredRecordByStubs2.java @@ -0,0 +1,22 @@ +record DisbarredRecordByStubs2(String barred, String fine) { + + // Annotation isn't copied to explicitly declared constructor parameters: + DisbarredRecordByStubs2(String barred, String fine) { + int x = barred.length(); + // :: error: (disbar.use) + this.barred = ""; + this.fine = fine; + } + + void invalid() { + // :: error: (disbar.use) + barred(); + // :: error: (disbar.use) + int x = barred.length(); + } + + void valid() { + fine(); + int x = fine.length(); + } +} diff --git a/checker/tests/disbaruse-records/disbar.astub b/checker/tests/disbaruse-records/disbar.astub new file mode 100644 index 00000000000..ee6f712dbd4 --- /dev/null +++ b/checker/tests/disbaruse-records/disbar.astub @@ -0,0 +1,6 @@ +import org.checkerframework.checker.testchecker.disbaruse.qual.DisbarUse; + +record DisbarredRecordByStubs(@DisbarUse String barred, String fine) { +} +record DisbarredRecordByStubs2(@DisbarUse String barred, String fine) { +} diff --git a/checker/tests/lock-records/LockRecord.java b/checker/tests/lock-records/LockRecord.java new file mode 100644 index 00000000000..c9f179e6bfa --- /dev/null +++ b/checker/tests/lock-records/LockRecord.java @@ -0,0 +1,11 @@ +import java.util.concurrent.locks.ReentrantLock; +import org.checkerframework.checker.lock.qual.LockingFree; + +public record LockRecord(String s, ReentrantLock lock) { + @LockingFree + // :: error: (method.guarantee.violated) + public LockRecord { + // :: error: (method.guarantee.violated) + lock.lock(); + } +} diff --git a/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java b/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java index 818aafb00a2..54388dde761 100644 --- a/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java +++ b/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java @@ -390,6 +390,44 @@ public Set getDeclAnnotations(Element elt) { String eltName = ElementUtils.getQualifiedName(elt); if (annotationFileAnnos.declAnnos.containsKey(eltName)) { return annotationFileAnnos.declAnnos.get(eltName); + } else { + // Handle annotations on record declarations. + boolean canTransferAnnotationsToSameName; + Element enclosingType; // Do nothing unless this element is a record. + switch (elt.getKind()) { + case METHOD: + // Annotations transfer to zero-arg accessor methods of same name: + canTransferAnnotationsToSameName = ((ExecutableElement) elt).getParameters().isEmpty(); + enclosingType = elt.getEnclosingElement(); + break; + case FIELD: + // Annotations transfer to fields of same name: + canTransferAnnotationsToSameName = true; + enclosingType = elt.getEnclosingElement(); + break; + case PARAMETER: + // Annotations transfer to compact canonical constructor parameter of same name: + canTransferAnnotationsToSameName = + ElementUtils.isCompactCanonicalRecordConstructor(elt.getEnclosingElement()) + && elt.getEnclosingElement().getKind() == ElementKind.CONSTRUCTOR; + enclosingType = elt.getEnclosingElement().getEnclosingElement(); + break; + default: + canTransferAnnotationsToSameName = false; + enclosingType = null; + break; + } + + if (canTransferAnnotationsToSameName && enclosingType.getKind().toString().equals("RECORD")) { + AnnotationFileParser.RecordStub recordStub = + annotationFileAnnos.records.get(enclosingType.getSimpleName().toString()); + if (recordStub != null + && recordStub.componentsByName.containsKey(elt.getSimpleName().toString())) { + RecordComponentStub recordComponentStub = + recordStub.componentsByName.get(elt.getSimpleName().toString()); + return recordComponentStub.getAnnotationsForTarget(elt.getKind()); + } + } } return Collections.emptySet(); } diff --git a/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileParser.java b/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileParser.java index 108fea21d4f..922b843fa70 100644 --- a/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileParser.java +++ b/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileParser.java @@ -343,6 +343,13 @@ public static class RecordComponentStub { /** The type of the record component. */ public final AnnotatedTypeMirror type; + /** + * The set of all annotations on the declaration of the record component. If applicable these + * will be copied to the corresponding field, accessor method, and compact canonical constructor + * parameter. + */ + private final Set allAnnotations; + /** Whether this component has an accessor of exactly the same name in the stubs file. */ private boolean hasAccessorInStubs = false; @@ -350,9 +357,30 @@ public static class RecordComponentStub { * Creates a new RecordComponentStub with the given type. * * @param type the type of the record component + * @param allAnnotations the declaration annotations on the component */ - public RecordComponentStub(AnnotatedTypeMirror type) { + public RecordComponentStub(AnnotatedTypeMirror type, Set allAnnotations) { this.type = type; + this.allAnnotations = allAnnotations; + } + + /** + * Get the record component annotations that are applicable to the given element kind. + * + * @param elementKind the element kind to apply to (e.g., FIELD, METHOD) + * @return the set of annotations from the component that apply + */ + public Set getAnnotationsForTarget(ElementKind elementKind) { + Set filtered = AnnotationUtils.createAnnotationSet(); + for (AnnotationMirror annoMirror : allAnnotations) { + Target target = annoMirror.getAnnotationType().asElement().getAnnotation(Target.class); + // Only add the declaration annotation if the annotation applies to the element. + if (AnnotationUtils.getElementKindsForTarget(target).contains(elementKind)) { + // `annoMirror` is applicable to `elt` + filtered.add(annoMirror); + } + } + return filtered; } /** @@ -1568,7 +1596,12 @@ private RecordComponentStub processRecordField(Parameter decl, VariableElement e annotate(fieldType, decl.getType(), decl.getAnnotations(), decl); putMerge(annotationFileAnnos.atypes, elt, fieldType); - return new RecordComponentStub(fieldType); + Set annos = AnnotationUtils.createAnnotationSet(); + for (AnnotationExpr annotation : decl.getAnnotations()) { + AnnotationMirror annoMirror = getAnnotation(annotation, allAnnotations); + annos.add(annoMirror); + } + return new RecordComponentStub(fieldType, annos); } /**