From 76180cf235fd1e7620a3d480c5c71113d6a4070c Mon Sep 17 00:00:00 2001 From: Michael Ernst Date: Sat, 12 Oct 2024 14:45:43 -0700 Subject: [PATCH] Support adding declaration annotation aliases using annotation names (#150) Co-authored-by: Werner Dietl --- .../UpperBoundAnnotatedTypeFactory.java | 3 +- .../checker/mustcall/MustCallVisitor.java | 3 +- .../cfg/builder/CFGTranslationPhaseOne.java | 3 +- .../framework/source/SourceChecker.java | 3 +- .../framework/type/AnnotatedTypeFactory.java | 119 ++++++++++++------ .../framework/util/AnnotatedTypes.java | 3 +- .../InvocationTypeInference.java | 6 +- .../typeinference8/constraint/Typing.java | 3 +- 8 files changed, 99 insertions(+), 44 deletions(-) diff --git a/checker/src/main/java/org/checkerframework/checker/index/upperbound/UpperBoundAnnotatedTypeFactory.java b/checker/src/main/java/org/checkerframework/checker/index/upperbound/UpperBoundAnnotatedTypeFactory.java index a32d7640ec5e..9dbc8f893fc8 100644 --- a/checker/src/main/java/org/checkerframework/checker/index/upperbound/UpperBoundAnnotatedTypeFactory.java +++ b/checker/src/main/java/org/checkerframework/checker/index/upperbound/UpperBoundAnnotatedTypeFactory.java @@ -642,7 +642,8 @@ public Void visitCompoundAssignment(CompoundAssignmentTree tree, AnnotatedTypeMi @Override public Void visitBinary(BinaryTree tree, AnnotatedTypeMirror type) { // This implementation does NOT call getAnnotatedType on the left or right operands. - // Doing so would lead to re-examination of subexpressions many times (which is too slow). + // Doing so would lead to re-examination of subexpressions many times (which is too + // slow). // A few small rules for addition/subtraction by 0/1, etc. if (TreeUtils.isStringConcatenation(tree)) { diff --git a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java index f5711cc42e0e..8da9b7af298c 100644 --- a/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java +++ b/checker/src/main/java/org/checkerframework/checker/mustcall/MustCallVisitor.java @@ -263,7 +263,8 @@ protected boolean skipReceiverSubtypeCheck( // If you think of the receiver of the method call as an implicit parameter, it has some // MustCall type. For example, consider the method call: // void foo(@MustCall("bar") ThisClass this) - // If we now call o.foo() where o has @MustCall({"bar, baz"}), the receiver subtype check would + // If we now call o.foo() where o has @MustCall({"bar, baz"}), the receiver subtype check + // would // throw an error, since o is not a subtype of @MustCall("bar"). However, since foo cannot // take ownership of its receiver, it does not matter what it 'thinks' the @MustCall methods // of the receiver are. Hence, it is always sound to skip this check. diff --git a/dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java b/dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java index 8bb453ac57ca..1eeeaacf0536 100644 --- a/dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java +++ b/dataflow/src/main/java/org/checkerframework/dataflow/cfg/builder/CFGTranslationPhaseOne.java @@ -2739,7 +2739,8 @@ public Node visitConditionalExpression(ConditionalExpressionTree tree, Void p) { Tree parent = TreePathUtil.getContextForPolyExpression(getCurrentPath()); if (parent != null) { exprType = TreeUtils.typeOf(parent); - // exprType is null when the condition is non-atomic, e.g.: x.isEmpty() ? null : null + // exprType is null when the condition is non-atomic, e.g.: x.isEmpty() ? null : + // null } if (parent == null || exprType == null) { exprType = TypesUtils.getObjectTypeMirror(env); 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 61c6dac6d7e2..dba01035d8cb 100644 --- a/framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java +++ b/framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java @@ -1253,7 +1253,8 @@ public void typeProcess(TypeElement e, TreePath p) { // All other messages are printed immediately. This includes errors issued because the // checker threw an exception. - // Update errsOnLastExit for all checkers, so that no matter which one is run next, its test of + // Update errsOnLastExit for all checkers, so that no matter which one is run next, its test + // of // whether a Java error occurred is correct. Context context = ((JavacProcessingEnvironment) processingEnv).getContext(); 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 26ff32eb39a8..9bf19314dd7b 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java +++ b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java @@ -355,9 +355,6 @@ public class AnnotatedTypeFactory implements AnnotationProvider { /** The checker to use for option handling and resource management. */ protected final BaseTypeChecker checker; - /** Map keys are canonical names of aliased annotations. */ - private final Map<@FullyQualifiedName String, Alias> aliases = new HashMap<>(); - /** * Scans all parts of the {@link AnnotatedTypeMirror} so that all of its fields are initialized. */ @@ -394,6 +391,19 @@ public void initializeAtm(AnnotatedTypeMirror type) { atmInitializer.visit(type); } + /** Map keys are canonical names of aliased annotations. */ + private final Map<@FullyQualifiedName String, Alias> aliases = new HashMap<>(); + + /** + * A map from the canonical name of a declaration annotation to the mapping of the canonical name + * of a declaration annotation with the same meaning (an alias) to the annotation mirror that + * should be used instead (an instance of the canonical declaration annotation). + */ + // A further generalization is to do something similar to `aliases`, where we allow copying + // elements from the alias to the canonical annotation. + private final Map<@FullyQualifiedName String, Map<@FullyQualifiedName String, AnnotationMirror>> + declAliases = new HashMap<>(); + /** * Information about one annotation alias. * @@ -460,14 +470,6 @@ void checkRep(String aliasName) { } } - /** - * A map from the class of an annotation to the set of classes for annotations with the same - * meaning, as well as the annotation mirror that should be used. - */ - private final Map< - Class, IPair>>> - declAliases = new HashMap<>(); - /** Unique ID counter; for debugging purposes. */ private static int uidCounter = 0; @@ -2464,9 +2466,12 @@ protected ParameterizedExecutableType methodFromUse( } if (typeArguments.inferenceCrash && tree instanceof MethodInvocationTree) { - // If inference crashed, then the return type will not be the correct Java type. This can - // cause crashes elsewhere in the framework. To avoid those crashes, create an ATM with the - // correct Java type and default annotations. (If inference crashes an error will be issued + // If inference crashed, then the return type will not be the correct Java type. This + // can + // cause crashes elsewhere in the framework. To avoid those crashes, create an ATM with + // the + // correct Java type and default annotations. (If inference crashes an error will be + // issued // in the BaseTypeVisitor.) TypeMirror type = TreeUtils.typeOf(tree); AnnotatedTypeMirror returnType = AnnotatedTypeMirror.createType(type, this, false); @@ -2855,9 +2860,12 @@ protected ParameterizedExecutableType constructorFromUse( stubTypes.injectRecordComponentType(types, ctor, con); if (typeArguments.inferenceCrash) { - // If inference crashed, then the return type will not be the correct Java type. This can - // cause crashes elsewhere in the framework. To avoid those crashes, create an ATM with the - // correct Java type and default annotations. (If inference crashes an error will be issued + // If inference crashed, then the return type will not be the correct Java type. This + // can + // cause crashes elsewhere in the framework. To avoid those crashes, create an ATM with + // the + // correct Java type and default annotations. (If inference crashes an error will be + // issued // in the BaseTypeVisitor.) TypeMirror typeTM = TreeUtils.typeOf(tree); AnnotatedTypeMirror returnType = AnnotatedTypeMirror.createType(typeTM, this, false); @@ -3507,18 +3515,37 @@ protected void addAliasedDeclAnnotation( Class alias, Class annotationClass, AnnotationMirror annotationToUse) { - IPair>> pair = - declAliases.get(annotationClass); - if (pair != null) { - if (!AnnotationUtils.areSame(annotationToUse, pair.first)) { - throw new BugInCF("annotationToUse should be the same: %s %s", pair.first, annotationToUse); - } - } else { - pair = IPair.of(annotationToUse, new HashSet<>()); - declAliases.put(annotationClass, pair); + addAliasedDeclAnnotation( + alias.getCanonicalName(), annotationClass.getCanonicalName(), annotationToUse); + } + + /** + * Add the annotation {@code alias} as an alias for the declaration annotation {@code annotation}, + * where the annotation mirror {@code annotationToUse} will be used instead. If multiple calls are + * made with the same {@code annotation}, then the {@code annotationToUse} must be the same. + * + *

The point of {@code annotationToUse} is that it may include elements/fields. + * + * @param alias the fully-qualified name of the alias annotation + * @param annotationName the fully-qualified name of the canonical annotation + * @param annotationToUse the annotation mirror to use + */ + protected void addAliasedDeclAnnotation( + @FullyQualifiedName String alias, + @FullyQualifiedName String annotationName, + AnnotationMirror annotationToUse) { + Map<@FullyQualifiedName String, AnnotationMirror> mapping = declAliases.get(annotationName); + if (mapping == null) { + mapping = new HashMap<>(1); + declAliases.put(annotationName, mapping); + } + AnnotationMirror prev = mapping.put(alias, annotationToUse); + // There already was a mapping. Raise an error. + if (prev != null && !AnnotationUtils.areSame(prev, annotationToUse)) { + throw new TypeSystemError( + "Multiple aliases for %s: %s cannot map to %s and %s.", + annotationName, alias, prev, annotationToUse); } - Set> aliases = pair.second; - aliases.add(alias); } /** @@ -3939,10 +3966,32 @@ public boolean shouldWarnIfStubRedundantWithBytecode() { */ private @Nullable AnnotationMirror getDeclAnnotation( Element elt, Class annoClass, boolean checkAliases) { + return getDeclAnnotation(elt, annoClass.getCanonicalName(), checkAliases); + } + + /** + * Returns the actual annotation mirror used to annotate this element, whose name equals the + * passed canonical annotation name (or is an alias for it). Returns null if none exists. May + * return the canonical annotation that annotationName is an alias for. + * + *

An option is provided not to check for aliases of annotations. For example, an annotated + * type factory may use aliasing for a pair of annotations for convenience while needing in some + * cases to determine a strict ordering between them, such as when determining whether the + * annotations on an overrider method are more specific than the annotations of an overridden + * method. + * + * @param elt the element to retrieve the annotation from + * @param annoName the canonical annotation name to retrieve + * @param checkAliases whether to return an annotation mirror for an alias of the requested + * annotation class name + * @return the annotation mirror for the requested annotation, or null if not found + */ + private AnnotationMirror getDeclAnnotation( + Element elt, @FullyQualifiedName String annoName, boolean checkAliases) { AnnotationMirrorSet declAnnos = getDeclAnnotations(elt); for (AnnotationMirror am : declAnnos) { - if (areSameByClass(am, annoClass)) { + if (AnnotationUtils.areSameByName(am, annoName)) { return am; } } @@ -3950,16 +3999,14 @@ public boolean shouldWarnIfStubRedundantWithBytecode() { return null; } // Look through aliases. - IPair>> aliases = declAliases.get(annoClass); + Map<@FullyQualifiedName String, AnnotationMirror> aliases = declAliases.get(annoName); if (aliases == null) { return null; } - for (Class alias : aliases.second) { - for (AnnotationMirror am : declAnnos) { - if (areSameByClass(am, alias)) { - // TODO: need to copy over elements/fields - return aliases.first; - } + for (AnnotationMirror am : declAnnos) { + AnnotationMirror match = aliases.get(AnnotationUtils.annotationName(am)); + if (match != null) { + return match; } } diff --git a/framework/src/main/java/org/checkerframework/framework/util/AnnotatedTypes.java b/framework/src/main/java/org/checkerframework/framework/util/AnnotatedTypes.java index 8a32cb9f7f77..022e4b7e5ab9 100644 --- a/framework/src/main/java/org/checkerframework/framework/util/AnnotatedTypes.java +++ b/framework/src/main/java/org/checkerframework/framework/util/AnnotatedTypes.java @@ -900,7 +900,8 @@ public static AnnotatedTypeMirror annotatedGLB( if (type2.getKind() == TypeKind.TYPEVAR) { return type2; } - // I think the only way error happens is when one of the types is a typevarible, but just in + // I think the only way error happens is when one of the types is a typevarible, but + // just in // case, just return type1. return type1; } diff --git a/framework/src/main/java/org/checkerframework/framework/util/typeinference8/InvocationTypeInference.java b/framework/src/main/java/org/checkerframework/framework/util/typeinference8/InvocationTypeInference.java index eb9bbf1fa140..c7169073d400 100644 --- a/framework/src/main/java/org/checkerframework/framework/util/typeinference8/InvocationTypeInference.java +++ b/framework/src/main/java/org/checkerframework/framework/util/typeinference8/InvocationTypeInference.java @@ -670,8 +670,10 @@ private BoundSet getB4(BoundSet b3, ConstraintSet c) { c.remove(subset); BoundSet newBounds = subset.reduceAdditionalArgOnce(context); if (!subset.isEmpty()) { - // The subset is not empty at this point if an additional argument constraint was found. - // In this case, a new subset needs to be picked so that dependencies of the constraints + // The subset is not empty at this point if an additional argument constraint was + // found. + // In this case, a new subset needs to be picked so that dependencies of the + // constraints // from reducing the additional argument constraint can be taken into account. c.addAll(subset); } diff --git a/framework/src/main/java/org/checkerframework/framework/util/typeinference8/constraint/Typing.java b/framework/src/main/java/org/checkerframework/framework/util/typeinference8/constraint/Typing.java index c1014e07c323..682cc5eb3aa5 100644 --- a/framework/src/main/java/org/checkerframework/framework/util/typeinference8/constraint/Typing.java +++ b/framework/src/main/java/org/checkerframework/framework/util/typeinference8/constraint/Typing.java @@ -211,7 +211,8 @@ private ReductionResult reduceSubtypeClass(Java8InferenceContext context) { // constraint reduces to the following new constraints: // for all i (1 <= i <= n), . - // Capturing is not in the JLS, but otherwise wildcards appear in the constraints against + // Capturing is not in the JLS, but otherwise wildcards appear in the constraints + // against // the type arguments, which causes crashes. AbstractType sAsSuper = S.asSuper(T.getJavaType()).capture(context); if (sAsSuper == null) {