diff --git a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java index 0bb07e6db3f8..771d9e01649f 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeChecker.java @@ -4,6 +4,7 @@ import org.checkerframework.checker.nullness.qual.Nullable; import org.checkerframework.checker.signature.qual.ClassGetName; import org.checkerframework.dataflow.cfg.visualize.CFGVisualizer; +import org.checkerframework.framework.qual.AnnotatedFor; import org.checkerframework.framework.qual.SubtypeOf; import org.checkerframework.framework.source.SourceChecker; import org.checkerframework.framework.type.AnnotatedTypeFactory; @@ -13,6 +14,7 @@ import org.checkerframework.javacutil.AbstractTypeProcessor; import org.checkerframework.javacutil.AnnotationProvider; import org.checkerframework.javacutil.BugInCF; +import org.checkerframework.javacutil.ElementUtils; import org.checkerframework.javacutil.TypeSystemError; import org.checkerframework.javacutil.UserError; import org.plumelib.util.CollectionsPlume; @@ -22,9 +24,13 @@ import java.lang.reflect.InvocationTargetException; import java.util.Arrays; import java.util.Collection; +import java.util.IdentityHashMap; import java.util.Set; import javax.lang.model.element.AnnotationMirror; +import javax.lang.model.element.Element; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.PackageElement; /** * An abstract {@link SourceChecker} that provides a simple {@link @@ -68,6 +74,13 @@ */ public abstract class BaseTypeChecker extends SourceChecker { + /** + * A mapping from an element to whether it is in an {@code @AnnotatedFor} scope for this checker + * or an upstream checker. + */ + private final IdentityHashMap elementAnnotatedForThisCheckerOrUpstreamCache = + new IdentityHashMap<>(); + /** An array containing just {@code BaseTypeChecker.class}. */ protected static Class[] baseTypeCheckerClassArray = new Class[] {BaseTypeChecker.class}; @@ -313,4 +326,40 @@ public BaseTypeChecker getUltimateParentChecker() { causeMessage); } } + + @Override + public boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(@Nullable Element elt) { + if (elt == null) { + throw new BugInCF( + "Call of BaseTypeChecker.isElementAnnotatedForThisCheckerOrUpstreamChecker with null"); + } + + if (elementAnnotatedForThisCheckerOrUpstreamCache.containsKey(elt)) { + return elementAnnotatedForThisCheckerOrUpstreamCache.get(elt); + } + + AnnotatedTypeFactory atypeFactory = getTypeFactory(); + AnnotationMirror annotatedFor = atypeFactory.getDeclAnnotation(elt, AnnotatedFor.class); + boolean elementAnnotatedForThisChecker = + annotatedFor != null + && atypeFactory.doesAnnotatedForApplyToThisChecker(annotatedFor); + + if (!elementAnnotatedForThisChecker) { + Element parent; + if (elt.getKind() == ElementKind.PACKAGE) { + parent = + ElementUtils.parentPackage( + (PackageElement) elt, atypeFactory.getElementUtils()); + } else { + parent = elt.getEnclosingElement(); + } + + if (parent != null && isElementAnnotatedForThisCheckerOrUpstreamChecker(parent)) { + elementAnnotatedForThisChecker = true; + } + } + + elementAnnotatedForThisCheckerOrUpstreamCache.put(elt, elementAnnotatedForThisChecker); + return elementAnnotatedForThisChecker; + } } diff --git a/framework/src/main/java/org/checkerframework/common/util/count/AnnotationStatistics.java b/framework/src/main/java/org/checkerframework/common/util/count/AnnotationStatistics.java index 5415660c9874..cf7fe120969e 100644 --- a/framework/src/main/java/org/checkerframework/common/util/count/AnnotationStatistics.java +++ b/framework/src/main/java/org/checkerframework/common/util/count/AnnotationStatistics.java @@ -31,6 +31,7 @@ import javax.annotation.processing.SupportedSourceVersion; import javax.lang.model.SourceVersion; +import javax.lang.model.element.Element; import javax.lang.model.element.Name; import javax.tools.Diagnostic; @@ -326,4 +327,11 @@ public AnnotationProvider getAnnotationProvider() { throw new UnsupportedOperationException( "getAnnotationProvider is not implemented for this class."); } + + @Override + public boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(Element elt) { + // This checker only counts annotations; it does not perform any type-checking. Returning + // false keeps callers (e.g., QualifierDefaults) on safe defaults rather than crashing. + return false; + } } diff --git a/framework/src/main/java/org/checkerframework/common/util/count/JavaCodeStatistics.java b/framework/src/main/java/org/checkerframework/common/util/count/JavaCodeStatistics.java index 5eed8083588b..069d84a50b52 100644 --- a/framework/src/main/java/org/checkerframework/common/util/count/JavaCodeStatistics.java +++ b/framework/src/main/java/org/checkerframework/common/util/count/JavaCodeStatistics.java @@ -24,6 +24,7 @@ import javax.annotation.processing.SupportedSourceVersion; import javax.lang.model.SourceVersion; import javax.lang.model.element.AnnotationMirror; +import javax.lang.model.element.Element; import javax.lang.model.element.ExecutableElement; /** @@ -203,4 +204,11 @@ public AnnotationProvider getAnnotationProvider() { throw new UnsupportedOperationException( "getAnnotationProvider is not implemented for this class."); } + + @Override + public boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(Element elt) { + // This checker only counts code; it does not perform any type-checking. Returning false + // keeps callers (e.g., QualifierDefaults) on safe defaults rather than crashing. + return false; + } } diff --git a/framework/src/main/java/org/checkerframework/common/util/debug/SignaturePrinter.java b/framework/src/main/java/org/checkerframework/common/util/debug/SignaturePrinter.java index e27de1e8b599..8d3db881edc8 100644 --- a/framework/src/main/java/org/checkerframework/common/util/debug/SignaturePrinter.java +++ b/framework/src/main/java/org/checkerframework/common/util/debug/SignaturePrinter.java @@ -100,6 +100,15 @@ private void init(ProcessingEnvironment env, @Nullable @BinaryName String checke checker = new SourceChecker() { + @Override + public boolean isElementAnnotatedForThisCheckerOrUpstreamChecker( + Element elt) { + // SignaturePrinter only prints signatures; it does not perform any + // type-checking. Returning false keeps callers (e.g., + // QualifierDefaults) on safe defaults rather than crashing. + return false; + } + @Override protected SourceVisitor createSourceVisitor() { return null; diff --git a/framework/src/main/java/org/checkerframework/framework/source/AggregateChecker.java b/framework/src/main/java/org/checkerframework/framework/source/AggregateChecker.java index 727e2b8f6d6c..efb03916b8cc 100644 --- a/framework/src/main/java/org/checkerframework/framework/source/AggregateChecker.java +++ b/framework/src/main/java/org/checkerframework/framework/source/AggregateChecker.java @@ -7,6 +7,7 @@ import java.util.LinkedHashSet; import java.util.Set; +import javax.lang.model.element.Element; import javax.tools.Diagnostic; /** @@ -52,4 +53,15 @@ protected final Set> getImmediateSubcheckerClasse // the checkers in the aggregate checker do. }; } + + /** + * Always returns false. Whether an aggregate checker is annotated with {@code @AnnotatedFor} + * depends on its subcheckers. For checkers that have an upstream checker and want to handle + * {@code @AnnotatedFor} in both this and the upstream checker, see {@link + * org.checkerframework.checker.initialization.InitializationChecker#getUpstreamCheckerNames()}. + */ + @Override + public boolean isElementAnnotatedForThisCheckerOrUpstreamChecker(Element elt) { + return false; + } } diff --git a/framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java b/framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java index c3133c10c211..b48391ffd6a9 100644 --- a/framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java +++ b/framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java @@ -31,9 +31,7 @@ import org.checkerframework.checker.signature.qual.FullyQualifiedName; import org.checkerframework.common.basetype.BaseTypeChecker; import org.checkerframework.common.reflection.MethodValChecker; -import org.checkerframework.framework.qual.AnnotatedFor; import org.checkerframework.framework.type.AnnotatedTypeFactory; -import org.checkerframework.framework.util.CheckerMain; import org.checkerframework.framework.util.OptionConfiguration; import org.checkerframework.framework.util.TreePathCacher; import org.checkerframework.javacutil.AbstractTypeProcessor; @@ -152,7 +150,7 @@ // only issue errors for code inside the scope of `@NullMarked` annotations. // See // https://github.com/uber/NullAway/wiki/Configuration#only-nullmarked-version-0123-and-after. - // org.checkerframework.framework.source.SourceChecker.isAnnotatedForThisCheckerOrUpstreamChecker + // org.checkerframework.framework.source.SourceChecker.isElementAnnotatedForThisCheckerOrUpstreamChecker "onlyAnnotatedFor", // Unsoundly assume all methods have no side effects, are deterministic, or both. @@ -2773,9 +2771,7 @@ public boolean shouldSuppressWarnings(TreePath path, String errKey) { return true; } - if (isAnnotatedForThisCheckerOrUpstreamChecker(elt)) { - // Return false immediately. Do NOT check for AnnotatedFor in the enclosing - // elements as the closest AnnotatedFor is already found. + if (isElementAnnotatedForThisCheckerOrUpstreamChecker(elt)) { return false; } } else if (TreeUtils.classTreeKinds().contains(decl.getKind())) { @@ -2785,9 +2781,7 @@ public boolean shouldSuppressWarnings(TreePath path, String errKey) { return true; } - if (isAnnotatedForThisCheckerOrUpstreamChecker(elt)) { - // Return false immediately. Do NOT check for AnnotatedFor in the enclosing - // elements as the closest AnnotatedFor is already found. + if (isElementAnnotatedForThisCheckerOrUpstreamChecker(elt)) { return false; } Element packageElement = elt.getEnclosingElement(); @@ -2795,11 +2789,9 @@ public boolean shouldSuppressWarnings(TreePath path, String errKey) { if (shouldSuppressWarnings(packageElement, errKey)) { return true; } - if (isAnnotatedForThisCheckerOrUpstreamChecker(packageElement)) { - // Return false immediately. Do NOT check for AnnotatedFor in the enclosing - // elements as the closest AnnotatedFor is already found. - return false; - } + // No need to call isElementAnnotatedForThisCheckerOrUpstreamChecker on the + // package: the call above on the class element already walks enclosing + // elements (including the package) recursively. } } else { throw new BugInCF("Unexpected declaration kind: " + decl.getKind() + " " + decl); @@ -2875,11 +2867,6 @@ public boolean shouldSuppressWarnings(Element elt, String errKey) { return true; } } - if (isAnnotatedForThisCheckerOrUpstreamChecker(elt)) { - // Return false immediately. Do NOT check for AnnotatedFor in the - // enclosing elements, because they may not have an @AnnotatedFor. - return false; - } } return false; } @@ -3011,33 +2998,8 @@ protected boolean messageKeyMatches( * @param elt the source code element to check, or null * @return true if the element is annotated for this checker or an upstream checker */ - private boolean isAnnotatedForThisCheckerOrUpstreamChecker(@Nullable Element elt) { - // Return false if elt is null, or if neither useConservativeDefaultsSource nor - // issueErrorsForOnlyAnnotatedForScope is set, since the @AnnotatedFor status is irrelevant - // in that case. - // TODO: Refactor SourceChecker and QualifierDefaults to use a cache for determining if an - // element is annotated for. - if (elt == null || (!useConservativeDefaultsSource && !onlyAnnotatedFor)) { - return false; - } - - AnnotatedFor anno = elt.getAnnotation(AnnotatedFor.class); - - String[] userAnnotatedFors = (anno == null ? null : anno.value()); - - if (userAnnotatedFors != null) { - List<@FullyQualifiedName String> upstreamCheckerNames = getUpstreamCheckerNames(); - - for (String userAnnotatedFor : userAnnotatedFors) { - if (CheckerMain.matchesCheckerOrSubcheckerFromList( - userAnnotatedFor, upstreamCheckerNames)) { - return true; - } - } - } - - return false; - } + public abstract boolean isElementAnnotatedForThisCheckerOrUpstreamChecker( + @Nullable Element elt); /** * Returns a modifiable set of lower-case strings that are prefixes for SuppressWarnings 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 7f74f327a6df..136171c02174 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java +++ b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java @@ -4118,6 +4118,18 @@ protected void parseAnnotationFiles() { ajavaTypes.parseAjavaFiles(); } + /** + * Returns true if this type factory is currently parsing stub files or ajava files. While this + * is true, annotation-file-backed lookup may observe partially loaded annotation-file data. + * + * @return true if stub files or ajava files are currently being parsed + */ + public boolean isParsingAnnotationFiles() { + return stubTypes.isParsing() + || ajavaTypes.isParsing() + || (currentFileAjavaTypes != null && currentFileAjavaTypes.isParsing()); + } + /** * Returns all of the declaration annotations whose name equals the passed annotation class (or * is an alias for it) including annotations: diff --git a/framework/src/main/java/org/checkerframework/framework/util/defaults/QualifierDefaults.java b/framework/src/main/java/org/checkerframework/framework/util/defaults/QualifierDefaults.java index a92299c34f39..24b10d5deeaa 100644 --- a/framework/src/main/java/org/checkerframework/framework/util/defaults/QualifierDefaults.java +++ b/framework/src/main/java/org/checkerframework/framework/util/defaults/QualifierDefaults.java @@ -14,7 +14,6 @@ import org.checkerframework.checker.interning.qual.FindDistinct; import org.checkerframework.checker.nullness.qual.Nullable; -import org.checkerframework.framework.qual.AnnotatedFor; import org.checkerframework.framework.qual.DefaultQualifier; import org.checkerframework.framework.qual.TypeUseLocation; import org.checkerframework.framework.type.AnnotatedTypeFactory; @@ -116,9 +115,6 @@ public class QualifierDefaults { */ private final IdentityHashMap elementDefaults = new IdentityHashMap<>(); - /** A mapping of Element → Whether or not that element is AnnotatedFor this type system. */ - private final IdentityHashMap elementAnnotatedFors = new IdentityHashMap<>(); - /** CLIMB locations whose standard default is top for a given type system. */ public static final List STANDARD_CLIMB_DEFAULTS_TOP = Collections.unmodifiableList( @@ -637,46 +633,6 @@ private void applyDefaults(Tree tree, AnnotatedTypeMirror type) { } } - private boolean isElementAnnotatedForThisChecker(Element elt) { - boolean elementAnnotatedForThisChecker = false; - - if (elt == null) { - throw new BugInCF( - "Call of QualifierDefaults.isElementAnnotatedForThisChecker with null"); - } - - if (elementAnnotatedFors.containsKey(elt)) { - return elementAnnotatedFors.get(elt); - } - - AnnotationMirror annotatedFor = atypeFactory.getDeclAnnotation(elt, AnnotatedFor.class); - - if (annotatedFor != null) { - elementAnnotatedForThisChecker = - atypeFactory.doesAnnotatedForApplyToThisChecker(annotatedFor); - } - - if (!elementAnnotatedForThisChecker) { - Element parent; - if (elt.getKind() == ElementKind.PACKAGE) { - // TODO: should AnnotatedFor apply to subpackages?? - // elt.getEnclosingElement() on a package is null; therefore, - // use the dedicated method. - parent = ElementUtils.parentPackage((PackageElement) elt, elements); - } else { - parent = elt.getEnclosingElement(); - } - - if (parent != null && isElementAnnotatedForThisChecker(parent)) { - elementAnnotatedForThisChecker = true; - } - } - - elementAnnotatedFors.put(elt, elementAnnotatedForThisChecker); - - return elementAnnotatedForThisChecker; - } - /** * Returns the defaults that apply to the given Element, considering defaults from enclosing * Elements. @@ -792,6 +748,22 @@ public boolean applyConservativeDefaults(Element annotationScope) { return false; } + // Skip the conservative-defaults check while annotation files are being parsed, to avoid + // an initialization cycle. During GenericAnnotatedTypeFactory.postInit(), + // parseAnnotationFiles() + // runs the stub/ajava parser, which asks the type factory for defaulted types. That reaches + // here and would call checker.isElementAnnotatedForThisCheckerOrUpstreamChecker(...), which + // routes through BaseTypeChecker.getTypeFactory() — but the visitor (and thus the type + // factory) is not yet installed on the checker, causing an NPE. Eagerly-parsed annotation + // files (checker @StubFiles, command-line stubs, ajava files, annotated-JDK + // package-info.java) + // are the risky cases; most JDK class stubs are only parsed lazily after init completes. + // Stub-file elements are still treated as checked code by the isFromStubFile branch below + // once parsing has finished. + if (atypeFactory.isParsingAnnotationFiles()) { + return false; + } + // TODO: I would expect this: // atypeFactory.isFromByteCode(annotationScope)) { // to work instead of the @@ -805,7 +777,9 @@ public boolean applyConservativeDefaults(Element annotationScope) { && !isFromStubFile; if (isBytecode) { return useConservativeDefaultsBytecode - && !isElementAnnotatedForThisChecker(annotationScope); + && !atypeFactory + .getChecker() + .isElementAnnotatedForThisCheckerOrUpstreamChecker(annotationScope); } else if (isFromStubFile) { // TODO: Types in stub files not annotated for a particular checker should be // treated as unchecked bytecode. For now, all types in stub files are treated as @@ -814,7 +788,9 @@ public boolean applyConservativeDefaults(Element annotationScope) { // be treated like unchecked code except for methods in the scope of an @AnnotatedFor. return false; } else if (useConservativeDefaultsSource) { - return !isElementAnnotatedForThisChecker(annotationScope); + return !atypeFactory + .getChecker() + .isElementAnnotatedForThisCheckerOrUpstreamChecker(annotationScope); } return false; }