Stubs: Allow recursively parsing enclosing classes (#305)

checker/jtreg/stubs/defaultqualinstub/default-on-class/List.java

@@ -0,0 +1,3 @@
+public abstract class List<T> {
+    abstract void retainAll(List<?> other);
+}

checker/jtreg/stubs/defaultqualinstub/default-on-class/MutableList.java

@@ -0,0 +1,4 @@
+public abstract class MutableList<T> extends List<T> {
+    @Override
+    abstract void retainAll(List<?> other);
+}

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<? extends @NonNull Object>
+    // mutableList has type MutableList<? extends @Nullable Object>
+    void foo(List<?> l1, MutableList<?> mutableList) {
+        // retainAll only accepts List with non-null elements
+        l1.retainAll(mutableList);
+
+        // l2 has type List<? extends @NonNull Object>
+        List<?> l2 = mutableList;
+    }
+}

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<T> {
+    abstract void retainAll(List<?> other);
+}

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

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<String, Object> map) {
+        return map.keySet();
+    }
+}

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.

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<String> 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<String> 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<String> 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<AnnotationMirror> getDeclAnnotation(Element elt) {
      *     does not appear in an annotation file.
      */
     public Set<AnnotationMirror> 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<AnnotationMirror> 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;
         }
     }
 

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<AnnotationMirror> 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<AnnotationMirror> annotationsToAdd = annos;
+            if (fileType == AnnotationFileType.JDK_STUB) {
                 // JDK annotations should not replace any annotation of the same type.
-                List<AnnotationMirror> 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);
         }
     }
 

framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java

@@ -4215,17 +4215,6 @@ public Set<AnnotationMirror> 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<AnnotationMirror> 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;
     }