diff --git a/checker-qual/src/main/java/org/checkerframework/framework/qual/DefaultQualifier.java b/checker-qual/src/main/java/org/checkerframework/framework/qual/DefaultQualifier.java index ef783ca9a85..9664fc2c38a 100644 --- a/checker-qual/src/main/java/org/checkerframework/framework/qual/DefaultQualifier.java +++ b/checker-qual/src/main/java/org/checkerframework/framework/qual/DefaultQualifier.java @@ -26,6 +26,9 @@ *   class MyClass { ... } * * + *

Defaults on a package also apply to subpackages, unless the {@code applyToSubpackages} field + * is set to false. + * *

This annotation currently has no effect in stub files. * * @see org.checkerframework.framework.qual.TypeUseLocation @@ -63,6 +66,13 @@ */ TypeUseLocation[] locations() default {TypeUseLocation.ALL}; + /** + * When used on a package, whether the defaults should also apply to subpackages. + * + * @return whether the default should be inherited by subpackages + */ + boolean applyToSubpackages() default true; + /** * A wrapper annotation that makes the {@link DefaultQualifier} annotation repeatable. * diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 4f3fd2804a7..c2d562f0d8d 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -3,6 +3,10 @@ Version 3.48.2 (November 1, 2024) **User-visible changes:** +`DefaultQualifier` supports the new `applyToSubpackages` annotation attribute +to decide whether a default should also apply to subpackages. To preserve the +current behavior, the default is `true`. + **Implementation details:** **Closed issues:** diff --git a/framework/src/main/java/org/checkerframework/framework/util/defaults/Default.java b/framework/src/main/java/org/checkerframework/framework/util/defaults/Default.java index 87069d811dc..e517cbb431f 100644 --- a/framework/src/main/java/org/checkerframework/framework/util/defaults/Default.java +++ b/framework/src/main/java/org/checkerframework/framework/util/defaults/Default.java @@ -21,25 +21,40 @@ public class Default implements Comparable { /** The type use location. */ public final TypeUseLocation location; + /** Whether the default should be inherited by subpackages. */ + public final boolean applyToSubpackages; + /** * Construct a Default object. * * @param anno the default annotation mirror * @param location the type use location + * @param applyToSubpackages whether the default should be inherited by subpackages */ - public Default(AnnotationMirror anno, TypeUseLocation location) { + public Default( + final AnnotationMirror anno, + final TypeUseLocation location, + final boolean applyToSubpackages) { this.anno = anno; this.location = location; + this.applyToSubpackages = applyToSubpackages; } @Override public int compareTo(Default other) { int locationOrder = location.compareTo(other.location); - if (locationOrder == 0) { - return AnnotationUtils.compareAnnotationMirrors(anno, other.anno); - } else { + if (locationOrder != 0) { return locationOrder; } + int annoOrder = AnnotationUtils.compareAnnotationMirrors(anno, other.anno); + if (annoOrder != 0) { + return annoOrder; + } + if (applyToSubpackages == other.applyToSubpackages) { + return 0; + } else { + return applyToSubpackages ? 1 : -1; + } } @Override @@ -57,11 +72,16 @@ public boolean equals(@Nullable Object thatObj) { @Override public int hashCode() { - return Objects.hash(anno, location); + return Objects.hash(anno, location, applyToSubpackages); } @Override public String toString() { - return "( " + location.name() + " => " + anno + " )"; + return "( " + + location.name() + + " => " + + anno + + (applyToSubpackages ? "applies to subpackages" : "") + + " )"; } } 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 6a81f8bd897..ae4ab85d78c 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 @@ -90,6 +90,9 @@ public class QualifierDefaults { /** The locations() element/field of a @DefaultQualifier annotation. */ protected final ExecutableElement defaultQualifierLocationsElement; + /** The applyToSubpackages() element/field of a @DefaultQualifier annotation. */ + protected final ExecutableElement defaultQualifierApplyToSubpackagesElement; + /** The value() element/field of a @DefaultQualifier.List annotation. */ protected final ExecutableElement defaultQualifierListValueElement; @@ -189,6 +192,8 @@ public QualifierDefaults(Elements elements, AnnotatedTypeFactory atypeFactory) { TreeUtils.getMethod(DefaultQualifier.class, "value", 0, processingEnv); this.defaultQualifierLocationsElement = TreeUtils.getMethod(DefaultQualifier.class, "locations", 0, processingEnv); + this.defaultQualifierApplyToSubpackagesElement = + TreeUtils.getMethod(DefaultQualifier.class, "applyToSubpackages", 0, processingEnv); this.defaultQualifierListValueElement = TreeUtils.getMethod(DefaultQualifier.List.class, "value", 0, processingEnv); } @@ -277,11 +282,24 @@ public void addClimbStandardDefaults() { * * @param absoluteDefaultAnno the default annotation mirror * @param location the type use location + * @param applyToSubpackages whether the default should be inherited by subpackages */ public void addCheckedCodeDefault( - AnnotationMirror absoluteDefaultAnno, TypeUseLocation location) { + AnnotationMirror absoluteDefaultAnno, TypeUseLocation location, boolean applyToSubpackages) { checkDuplicates(checkedCodeDefaults, absoluteDefaultAnno, location); - checkedCodeDefaults.add(new Default(absoluteDefaultAnno, location)); + checkedCodeDefaults.add(new Default(absoluteDefaultAnno, location, applyToSubpackages)); + } + + /** + * Adds a default annotation that also applies to subpackages, if applicable. A programmer may + * override this by writing the @DefaultQualifier annotation on an element. + * + * @param absoluteDefaultAnno the default annotation mirror + * @param location the type use location + */ + public void addCheckedCodeDefault( + AnnotationMirror absoluteDefaultAnno, TypeUseLocation location) { + addCheckedCodeDefault(absoluteDefaultAnno, location, true); } /** @@ -289,13 +307,25 @@ public void addCheckedCodeDefault( * * @param uncheckedDefaultAnno the default annotation mirror * @param location the type use location + * @param applyToSubpackages whether the default should be inherited by subpackages */ public void addUncheckedCodeDefault( - AnnotationMirror uncheckedDefaultAnno, TypeUseLocation location) { + AnnotationMirror uncheckedDefaultAnno, TypeUseLocation location, boolean applyToSubpackages) { checkDuplicates(uncheckedCodeDefaults, uncheckedDefaultAnno, location); checkIsValidUncheckedCodeLocation(uncheckedDefaultAnno, location); + uncheckedCodeDefaults.add(new Default(uncheckedDefaultAnno, location, applyToSubpackages)); + } - uncheckedCodeDefaults.add(new Default(uncheckedDefaultAnno, location)); + /** + * Add a default annotation for unchecked elements that also applies to subpackages, if + * applicable. + * + * @param uncheckedDefaultAnno the default annotation mirror + * @param location the type use location + */ + public void addUncheckedCodeDefault( + AnnotationMirror uncheckedDefaultAnno, TypeUseLocation location) { + addUncheckedCodeDefault(uncheckedDefaultAnno, location, true); } /** Sets the default annotation for unchecked elements, with specific locations. */ @@ -320,6 +350,16 @@ public void addCheckedCodeDefaults( * @param elementDefaultAnno the default to set * @param location the location to apply the default to */ + /* + * TODO(cpovirk): This method looks dangerous for a type system to call early: If it "adds" a + * default for an Element before defaultsAt runs for that Element, that looks like it would + * prevent any @DefaultQualifier or similar annotation from having any effect (because + * defaultsAt would short-circuit after discovering that an entry already exists for the + * Element). Maybe this method should run defaultsAt before inserting its own entry? Or maybe + * it's too early to run defaultsAt? Or maybe we'd see new problems in existing code because + * we'd start running checkDuplicates to look for overlap between the @DefaultQualifier defaults + * and addElementDefault defaults? + */ public void addElementDefault( Element elem, AnnotationMirror elementDefaultAnno, TypeUseLocation location) { DefaultSet prevset = elementDefaults.get(elem); @@ -328,7 +368,8 @@ public void addElementDefault( } else { prevset = new DefaultSet(); } - prevset.add(new Default(elementDefaultAnno, location)); + // TODO: expose applyToSubpackages + prevset.add(new Default(elementDefaultAnno, location, true)); elementDefaults.put(elem, prevset); } @@ -355,7 +396,8 @@ private void checkDuplicates( "Only one qualifier from a hierarchy can be the default. Existing: " + previousDefaults + " and new: " - + new Default(newAnno, newLoc)); + // TODO: expose applyToSubpackages + + new Default(newAnno, newLoc, true)); } } @@ -588,9 +630,13 @@ private void applyDefaults(Tree tree, AnnotatedTypeMirror type) { defaultQualifierLocationsElement, TypeUseLocation.class, defaultQualifierValueDefault); + boolean applyToSubpackages = + AnnotationUtils.getElementValue( + dq, defaultQualifierApplyToSubpackagesElement, Boolean.class, true); + DefaultSet ret = new DefaultSet(); for (TypeUseLocation loc : locations) { - ret.add(new Default(anno, loc)); + ret.add(new Default(anno, loc, applyToSubpackages)); } return ret; } else { @@ -656,55 +702,40 @@ private DefaultSet defaultsAt(Element elt) { return elementDefaults.get(elt); } - DefaultSet qualifiers = null; - - { - AnnotationMirror dqAnno = atypeFactory.getDeclAnnotation(elt, DefaultQualifier.class); - - if (dqAnno != null) { - qualifiers = new DefaultSet(); - Set p = fromDefaultQualifier(dqAnno); - - if (p != null) { - qualifiers.addAll(p); - } - } - } + DefaultSet qualifiers = defaultsAtDirect(elt); - { - AnnotationMirror dqListAnno = - atypeFactory.getDeclAnnotation(elt, DefaultQualifier.List.class); - if (dqListAnno != null) { - if (qualifiers == null) { - qualifiers = new DefaultSet(); - } - - List values = - AnnotationUtils.getElementValueArray( - dqListAnno, defaultQualifierListValueElement, AnnotationMirror.class); - for (AnnotationMirror dqAnno : values) { - Set p = fromDefaultQualifier(dqAnno); - if (p != null) { - qualifiers.addAll(p); - } + DefaultSet parentDefaults; + if (elt.getKind() == ElementKind.PACKAGE) { + Element parent = ElementUtils.parentPackage((PackageElement) elt, elements); + DefaultSet origParentDefaults = defaultsAt(parent); + parentDefaults = new DefaultSet(); + for (Default d : origParentDefaults) { + if (d.applyToSubpackages) { + parentDefaults.add(d); } } - } - - Element parent; - if (elt.getKind() == ElementKind.PACKAGE) { - parent = ElementUtils.parentPackage((PackageElement) elt, elements); } else { - parent = elt.getEnclosingElement(); + Element parent = elt.getEnclosingElement(); + parentDefaults = defaultsAt(parent); } - DefaultSet parentDefaults = defaultsAt(parent); if (qualifiers == null || qualifiers.isEmpty()) { qualifiers = parentDefaults; } else { + // TODO(cpovirk): What should happen with conflicts? qualifiers.addAll(parentDefaults); } + /* TODO: it would seem more efficient to also cache null/empty as the result. + * However, doing so causes KeyFor tests to fail. + if (qualifiers == null) { + qualifiers = DefaultSet.EMPTY; + } + + elementDefaults.put(elt, qualifiers); + return qualifiers; + */ + if (qualifiers != null && !qualifiers.isEmpty()) { elementDefaults.put(elt, qualifiers); return qualifiers; @@ -713,6 +744,48 @@ private DefaultSet defaultsAt(Element elt) { } } + /** + * Returns the defaults that apply directly to the given Element, without considering enclosing + * Elements. + * + * @param elt the element + * @return the defaults + */ + private DefaultSet defaultsAtDirect(final Element elt) { + DefaultSet qualifiers = null; + + // Handle DefaultQualifier + AnnotationMirror dqAnno = atypeFactory.getDeclAnnotation(elt, DefaultQualifier.class); + + if (dqAnno != null) { + Set p = fromDefaultQualifier(dqAnno); + + if (p != null) { + qualifiers = new DefaultSet(); + qualifiers.addAll(p); + } + } + + // Handle DefaultQualifier.List + AnnotationMirror dqListAnno = atypeFactory.getDeclAnnotation(elt, DefaultQualifier.List.class); + if (dqListAnno != null) { + if (qualifiers == null) { + qualifiers = new DefaultSet(); + } + @SuppressWarnings("unchecked") + List values = + AnnotationUtils.getElementValue(dqListAnno, defaultQualifierListValueElement, List.class); + for (AnnotationMirror dqlAnno : values) { + Set p = fromDefaultQualifier(dqlAnno); + if (p != null) { + // TODO(cpovirk): What should happen with conflicts? + qualifiers.addAll(p); + } + } + } + return qualifiers; + } + /** * Given an element, returns whether the conservative default should be applied for it. Handles * elements from bytecode or source code. diff --git a/framework/tests/h1h2checker/pkg1/PackageDefaulting.java b/framework/tests/h1h2checker/pkg1/PackageDefaulting.java new file mode 100644 index 00000000000..c40783eff70 --- /dev/null +++ b/framework/tests/h1h2checker/pkg1/PackageDefaulting.java @@ -0,0 +1,23 @@ +package pkg1; + +import org.checkerframework.framework.testchecker.h1h2checker.quals.H1S1; +import org.checkerframework.framework.testchecker.h1h2checker.quals.H1S2; +import org.checkerframework.framework.testchecker.h1h2checker.quals.H2S1; +import org.checkerframework.framework.testchecker.h1h2checker.quals.H2S2; + +/* Defaults from package pkg1 apply within the package. */ +public class PackageDefaulting { + // Test H1 hierarchy + void m(@H1S1 @H2S1 Object p1, @H1S2 @H2S1 Object p2) { + Object l1 = p1; + // :: error: (assignment) + Object l2 = p2; + } + + // Test H2 hierarchy + void m2(@H1S1 @H2S1 Object p1, @H1S1 @H2S2 Object p2) { + Object l1 = p1; + // :: error: (assignment) + Object l2 = p2; + } +} diff --git a/framework/tests/h1h2checker/pkg1/package-info.java b/framework/tests/h1h2checker/pkg1/package-info.java new file mode 100644 index 00000000000..616b07aef5f --- /dev/null +++ b/framework/tests/h1h2checker/pkg1/package-info.java @@ -0,0 +1,14 @@ +@DefaultQualifier( + value = H1S1.class, + locations = {TypeUseLocation.LOCAL_VARIABLE}, + applyToSubpackages = false) +@DefaultQualifier( + value = H2S1.class, + locations = {TypeUseLocation.LOCAL_VARIABLE}, + applyToSubpackages = true) +package pkg1; + +import org.checkerframework.framework.qual.DefaultQualifier; +import org.checkerframework.framework.qual.TypeUseLocation; +import org.checkerframework.framework.testchecker.h1h2checker.quals.H1S1; +import org.checkerframework.framework.testchecker.h1h2checker.quals.H2S1; diff --git a/framework/tests/h1h2checker/pkg1/pkg2/PackageDefaulting.java b/framework/tests/h1h2checker/pkg1/pkg2/PackageDefaulting.java new file mode 100644 index 00000000000..c44d9fee003 --- /dev/null +++ b/framework/tests/h1h2checker/pkg1/pkg2/PackageDefaulting.java @@ -0,0 +1,22 @@ +package pkg1.pkg2; + +import org.checkerframework.framework.testchecker.h1h2checker.quals.H1S1; +import org.checkerframework.framework.testchecker.h1h2checker.quals.H1S2; +import org.checkerframework.framework.testchecker.h1h2checker.quals.H2S1; +import org.checkerframework.framework.testchecker.h1h2checker.quals.H2S2; + +/* Default from package pkg1 for H1 is not applied to subpackages, whereas H2 is applied. */ +public class PackageDefaulting { + // Test H1 hierarchy + void m(@H1S1 @H2S1 Object p1, @H1S2 @H2S1 Object p2) { + Object l1 = p1; + Object l2 = p2; + } + + // Test H2 hierarchy + void m2(@H2S1 Object p1, @H2S2 Object p2) { + Object l1 = p1; + // :: error: (assignment) + Object l2 = p2; + } +}