From e75a471d73e5cc2dfcf5377199b64648f9b9f9b0 Mon Sep 17 00:00:00 2001 From: zcai Date: Fri, 22 Jul 2022 16:40:25 -0400 Subject: [PATCH] Stubs: Allow recursively parsing enclosing classes (#305) --- .../default-on-class/List.java | 3 + .../default-on-class/MutableList.java | 4 + .../default-on-class/Test.java | 22 +++++ .../default-on-class/list.astub | 8 ++ .../default-on-class/test.out | 3 + .../AnnotatedJdkTest.java | 16 ++++ docs/CHANGELOG.md | 6 ++ .../stub/AnnotationFileElementTypes.java | 95 +++++++++++-------- .../framework/stub/AnnotationFileParser.java | 24 +++-- .../framework/type/AnnotatedTypeFactory.java | 17 +--- 10 files changed, 137 insertions(+), 61 deletions(-) create mode 100644 checker/jtreg/stubs/defaultqualinstub/default-on-class/List.java create mode 100644 checker/jtreg/stubs/defaultqualinstub/default-on-class/MutableList.java create mode 100644 checker/jtreg/stubs/defaultqualinstub/default-on-class/Test.java create mode 100644 checker/jtreg/stubs/defaultqualinstub/default-on-class/list.astub create mode 100644 checker/jtreg/stubs/defaultqualinstub/default-on-class/test.out create mode 100644 checker/tests/nullness-safedefaultsbytecode/AnnotatedJdkTest.java diff --git a/checker/jtreg/stubs/defaultqualinstub/default-on-class/List.java b/checker/jtreg/stubs/defaultqualinstub/default-on-class/List.java new file mode 100644 index 00000000000..56ccb10f9c4 --- /dev/null +++ b/checker/jtreg/stubs/defaultqualinstub/default-on-class/List.java @@ -0,0 +1,3 @@ +public abstract class List { + abstract void retainAll(List other); +} diff --git a/checker/jtreg/stubs/defaultqualinstub/default-on-class/MutableList.java b/checker/jtreg/stubs/defaultqualinstub/default-on-class/MutableList.java new file mode 100644 index 00000000000..a4a6b8377bd --- /dev/null +++ b/checker/jtreg/stubs/defaultqualinstub/default-on-class/MutableList.java @@ -0,0 +1,4 @@ +public abstract class MutableList extends List { + @Override + abstract void retainAll(List other); +} diff --git a/checker/jtreg/stubs/defaultqualinstub/default-on-class/Test.java b/checker/jtreg/stubs/defaultqualinstub/default-on-class/Test.java new file mode 100644 index 00000000000..6b3689ef26f --- /dev/null +++ b/checker/jtreg/stubs/defaultqualinstub/default-on-class/Test.java @@ -0,0 +1,22 @@ +import org.checkerframework.checker.nullness.qual.*; + +/* + * @test + * @summary Defaults applied on class will not be inherited. + * + * @compile -XDrawDiagnostics -Xlint:unchecked List.java + * @compile -XDrawDiagnostics -Xlint:unchecked MutableList.java + * @compile/fail/ref=test.out -XDrawDiagnostics -Xlint:unchecked -processor org.checkerframework.checker.nullness.NullnessChecker -Anomsgtext -Astubs=list.astub -Werror Test.java + */ + +public class Test { + // l1 has type List + // mutableList has type MutableList + void foo(List l1, MutableList mutableList) { + // retainAll only accepts List with non-null elements + l1.retainAll(mutableList); + + // l2 has type List + List l2 = mutableList; + } +} diff --git a/checker/jtreg/stubs/defaultqualinstub/default-on-class/list.astub b/checker/jtreg/stubs/defaultqualinstub/default-on-class/list.astub new file mode 100644 index 00000000000..86e3ede5431 --- /dev/null +++ b/checker/jtreg/stubs/defaultqualinstub/default-on-class/list.astub @@ -0,0 +1,8 @@ +import org.checkerframework.framework.qual.DefaultQualifier; +import org.checkerframework.framework.qual.TypeUseLocation; +import org.checkerframework.checker.nullness.qual.NonNull; + +@DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.UPPER_BOUND) +public class List { + abstract void retainAll(List other); +} diff --git a/checker/jtreg/stubs/defaultqualinstub/default-on-class/test.out b/checker/jtreg/stubs/defaultqualinstub/default-on-class/test.out new file mode 100644 index 00000000000..7561b6e9ae2 --- /dev/null +++ b/checker/jtreg/stubs/defaultqualinstub/default-on-class/test.out @@ -0,0 +1,3 @@ +Test.java:17:22: compiler.err.proc.messager: (argument.type.incompatible) +Test.java:20:22: compiler.err.proc.messager: (assignment.type.incompatible) +2 errors diff --git a/checker/tests/nullness-safedefaultsbytecode/AnnotatedJdkTest.java b/checker/tests/nullness-safedefaultsbytecode/AnnotatedJdkTest.java new file mode 100644 index 00000000000..8fd522c1d31 --- /dev/null +++ b/checker/tests/nullness-safedefaultsbytecode/AnnotatedJdkTest.java @@ -0,0 +1,16 @@ +import org.checkerframework.checker.nullness.qual.*; + +import java.util.HashMap; +import java.util.Set; + +// There should be no warnings for the following operations +// if the annotated JDK is loaded properly +public class AnnotatedJdkTest { + String toStringTest(Object v) { + return v.toString(); + } + + Set<@KeyFor("#1") String> keySetTest(HashMap map) { + return map.keySet(); + } +} diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 706677c59ca..5230495fa2d 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -5,6 +5,12 @@ Version 3.23.0-eisop2 (July 22, 2022) **Implementation details:** +Improved defaulting in stub files: +As an extension to the fix for eisop#270, we now allow internally parsing +multiple stub files at the same time. This should make `AnnotatedTypeFactory.getDeclAnnotations` +return the expected declaration annotations for all kinds of elements, +even if it is parsing a different stub file. + **Closed issues:** eisop#308. 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 de249d1a4fc..1bc3576d2c6 100644 --- a/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java +++ b/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileElementTypes.java @@ -37,6 +37,7 @@ import java.util.Collections; import java.util.Enumeration; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -64,11 +65,8 @@ public class AnnotationFileElementTypes { /** Annotations from annotation files (but not from annotated JDK files). */ private final AnnotationFileAnnotations annotationFileAnnos; - /** - * Whether or not a file is currently being parsed. (If one is being parsed, don't try to parse - * another.) - */ - private boolean parsing; + /** The number of ongoing parsing tasks. */ + private int parsingCount; /** AnnotatedTypeFactory. */ private final AnnotatedTypeFactory factory; @@ -94,6 +92,12 @@ public class AnnotationFileElementTypes { /** Parse all JDK files at startup rather than as needed. */ private final boolean parseAllJdkFiles; + /** + * Stores the fully qualified name of classes that are currently being parsed in {@link + * #parseEnclosingClass} + */ + private final Set processingClasses = new LinkedHashSet<>(); + /** * Creates an empty annotation source. * @@ -102,7 +106,7 @@ public class AnnotationFileElementTypes { public AnnotationFileElementTypes(AnnotatedTypeFactory factory) { this.factory = factory; this.annotationFileAnnos = new AnnotationFileAnnotations(); - this.parsing = false; + this.parsingCount = 0; String release = SystemUtil.getReleaseValue(factory.getProcessingEnv()); this.annotatedJdkVersion = release != null ? release : String.valueOf(SystemUtil.getJreVersion()); @@ -117,7 +121,7 @@ public AnnotationFileElementTypes(AnnotatedTypeFactory factory) { * @return true if files are currently being parsed; otherwise, false */ public boolean isParsing() { - return parsing; + return parsingCount > 0; } /** @@ -142,7 +146,8 @@ public boolean isParsing() { * annotation is requested from a class in that file. */ public void parseStubFiles() { - parsing = true; + assert parsingCount == 0; + ++parsingCount; BaseTypeChecker checker = factory.getChecker(); ProcessingEnvironment processingEnv = factory.getProcessingEnv(); // 1. jdk.astub @@ -174,10 +179,6 @@ public void parseStubFiles() { // This preps but does not parse the JDK files (except package-info.java files). // The JDK source code files will be parsed later, on demand. prepJdkStubs(); - // prepping the JDK parses all package-info.java files, which sets the `parsing` field - // to - // false, so re-set it to true. - parsing = true; } // 3. Stub files listed in @StubFiles annotation on the checker @@ -198,12 +199,14 @@ public void parseStubFiles() { AnnotationFileType.COMMAND_LINE_STUB); } - parsing = false; + --parsingCount; + assert parsingCount == 0; } /** Parses the ajava files passed through the -Aajava command-line option. */ public void parseAjavaFiles() { - parsing = true; + assert parsingCount == 0; + ++parsingCount; // TODO: Error if this is called more than once? SourceChecker checker = factory.getChecker(); List ajavaFiles = new ArrayList<>(); @@ -213,7 +216,8 @@ public void parseAjavaFiles() { } parseAnnotationFiles(ajavaFiles, AnnotationFileType.AJAVA); - parsing = false; + --parsingCount; + assert parsingCount == 0; } /** @@ -226,7 +230,8 @@ public void parseAjavaFiles() { * @param root javac tree for the compilation unit stored in {@code ajavaFile} */ public void parseAjavaFileWithTree(String ajavaPath, CompilationUnitTree root) { - parsing = true; + assert parsingCount == 0; + ++parsingCount; SourceChecker checker = factory.getChecker(); ProcessingEnvironment processingEnv = factory.getProcessingEnv(); try { @@ -237,7 +242,8 @@ public void parseAjavaFileWithTree(String ajavaPath, CompilationUnitTree root) { checker.message(Kind.NOTE, "Could not read ajava file: " + ajavaPath); } - parsing = false; + --parsingCount; + assert parsingCount == 0; } /** @@ -356,9 +362,6 @@ private void parseAnnotationFiles(List annotationFiles, AnnotationFileTy * does not appear in an annotation file. */ public AnnotatedTypeMirror getAnnotatedTypeMirror(Element e) { - if (parsing) { - return null; - } parseEnclosingClass(e); AnnotatedTypeMirror type = annotationFileAnnos.atypes.get(e); return type == null ? null : type.deepCopy(); @@ -391,13 +394,6 @@ public Set getDeclAnnotation(Element elt) { * does not appear in an annotation file. */ public Set getDeclAnnotations(Element elt) { - // If currently parsing a file, return an empty set. - // The only exception is package because we always load package-info eagerly - // and there is no parent element to parse. - if (parsing && elt.getKind() != ElementKind.PACKAGE) { - return Collections.emptySet(); - } - parseEnclosingClass(elt); String eltName = ElementUtils.getQualifiedName(elt); if (annotationFileAnnos.declAnnos.containsKey(eltName)) { @@ -460,7 +456,7 @@ public Set getDeclAnnotations(Element elt) { */ public void injectRecordComponentType( Types types, Element elt, AnnotatedExecutableType memberType) { - if (parsing) { + if (isParsing()) { throw new BugInCF("parsing while calling injectRecordComponentType"); } @@ -530,7 +526,7 @@ private void replaceAnnotations(AnnotatedTypeMirror destType, AnnotatedTypeMirro */ public @Nullable AnnotatedExecutableType getFakeOverride( Element elt, AnnotatedTypeMirror receiverType) { - if (parsing) { + if (isParsing()) { throw new BugInCF("parsing while calling getFakeOverride"); } @@ -624,19 +620,38 @@ private void replaceAnnotations(AnnotatedTypeMirror destType, AnnotatedTypeMirro * @param e element whose outermost enclosing class will be parsed */ private void parseEnclosingClass(Element e) { - if (!shouldParseJdk) { + if (!shouldParseJdk + || e.getKind() == ElementKind.PACKAGE + || e.getKind() == ElementKind.MODULE) { return; } + String className = getOutermostEnclosingClass(e); if (className == null || className.isEmpty()) { + // TODO: maybe investigate other situations where the enclosing class is missing + // if (e.getKind() != ElementKind.PACKAGE && e.getKind() != + // ElementKind.MODULE) { + // factory.getChecker().reportWarning(e, "unexpected element " + e + " of + // kind " + e.getKind()); + // } + + return; + } + + if (!processingClasses.add(className)) { + // TODO: some declaration annotations in the enclosing class may still + // be missing, we can revisit this part if it's causing issues return; } - if (jdkStubFiles.containsKey(className)) { - parseJdkStubFile(jdkStubFiles.get(className)); - jdkStubFiles.remove(className); - } else if (jdkStubFilesJar.containsKey(className)) { - parseJdkJarEntry(jdkStubFilesJar.get(className)); - jdkStubFilesJar.remove(className); + + try { + if (jdkStubFiles.containsKey(className)) { + parseJdkStubFile(jdkStubFiles.remove(className)); + } else if (jdkStubFilesJar.containsKey(className)) { + parseJdkJarEntry(jdkStubFilesJar.remove(className)); + } + } finally { + processingClasses.remove(className); } } @@ -678,7 +693,7 @@ private void parseEnclosingClass(Element e) { * @param path path to file to parse */ private void parseJdkStubFile(Path path) { - parsing = true; + ++parsingCount; try (FileInputStream jdkStub = new FileInputStream(path.toFile())) { AnnotationFileParser.parseJdkFileAsStub( path.toFile().getName(), @@ -689,7 +704,7 @@ private void parseJdkStubFile(Path path) { } catch (IOException e) { throw new BugInCF("cannot open the jdk stub file " + path, e); } finally { - parsing = false; + --parsingCount; } } @@ -700,7 +715,7 @@ private void parseJdkStubFile(Path path) { */ private void parseJdkJarEntry(String jarEntryName) { JarURLConnection connection = getJarURLConnectionToJdk(); - parsing = true; + ++parsingCount; try (JarFile jarFile = connection.getJarFile()) { InputStream jdkStub; try { @@ -719,7 +734,7 @@ private void parseJdkJarEntry(String jarEntryName) { } catch (BugInCF e) { throw new BugInCF("Exception while parsing " + jarEntryName + ": " + e.getMessage(), e); } finally { - parsing = false; + --parsingCount; } } 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 a6ff3bf57de..ef665e321a7 100644 --- a/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileParser.java +++ b/framework/src/main/java/org/checkerframework/framework/stub/AnnotationFileParser.java @@ -97,6 +97,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.stream.Collectors; import javax.annotation.processing.ProcessingEnvironment; import javax.lang.model.element.AnnotationMirror; @@ -2952,17 +2953,22 @@ private void putOrAddToDeclAnnos(String key, Set annos) { if (stored == null) { annotationFileAnnos.declAnnos.put(key, new HashSet<>(annos)); } else { - if (fileType != AnnotationFileType.JDK_STUB) { - stored.addAll(annos); - } else { + // TODO: Currently, we assume there can be at most one annotation of the same name + // in both `stored` and `annos`. Maybe we should consider the situation of multiple + // entries having the same name. + Set annotationsToAdd = annos; + if (fileType == AnnotationFileType.JDK_STUB) { // JDK annotations should not replace any annotation of the same type. - List origStored = new ArrayList<>(stored); - for (AnnotationMirror anno : annos) { - if (!AnnotationUtils.containsSameByName(origStored, anno)) { - stored.add(anno); - } - } + annotationsToAdd = + annos.stream() + .filter(am -> !AnnotationUtils.containsSameByName(stored, am)) + .collect(Collectors.toSet()); + } else { + // Annotations that are not from the annotated JDK may replace existing + // annotations of the same type. + stored.removeIf(am -> AnnotationUtils.containsSameByName(annos, am)); } + stored.addAll(annotationsToAdd); } } diff --git a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java index d5247996a32..9bdfaf90582 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java +++ b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java @@ -4215,17 +4215,6 @@ public Set getDeclAnnotations(Element elt) { } } - // If parsing annotation files, return only the annotations in the element. - // The only exception is package because we always load package-info eagerly - // and there is no parent element to parse. - boolean isParsing = - stubTypes.isParsing() - || ajavaTypes.isParsing() - || (currentFileAjavaTypes != null && currentFileAjavaTypes.isParsing()); - if (isParsing && elt.getKind() != ElementKind.PACKAGE) { - return results; - } - // Add annotations from annotation files. results.addAll(stubTypes.getDeclAnnotations(elt)); results.addAll(ajavaTypes.getDeclAnnotations(elt)); @@ -4241,7 +4230,11 @@ public Set getDeclAnnotations(Element elt) { } // Add the element and its annotations to the cache. - cacheDeclAnnos.put(elt, results); + if (!stubTypes.isParsing() + && !ajavaTypes.isParsing() + && (currentFileAjavaTypes == null || !currentFileAjavaTypes.isParsing())) { + cacheDeclAnnos.put(elt, results); + } return results; }