Skip to content

Commit

Permalink
Improved documentation and naming for annotation comparisons
Browse files Browse the repository at this point in the history
  • Loading branch information
@mernst
mernst authored Oct 9, 2024
1 parent 09f423a commit b9dd1af
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 45 deletions.
9 changes: 5 additions & 4 deletions ...r/src/main/java/org/checkerframework/checker/formatter/FormatterAnnotatedTypeFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,11 @@ private boolean hasFormatMethodAnno(AMethod methodAnnos) {
private boolean hasFormatMethodAnno(
WholeProgramInferenceJavaParserStorage.CallableDeclarationAnnos methodAnnos) {
AnnotationMirrorSet declarationAnnos = methodAnnos.getDeclarationAnnotations();
return containsSameByClass(
declarationAnnos, org.checkerframework.checker.formatter.qual.FormatMethod.class)
|| AnnotationUtils.containsSameByName(
declarationAnnos, "com.google.errorprone.annotations.FormatMethod");
return !declarationAnnos.isEmpty()
&& (containsSameByClass(
declarationAnnos, org.checkerframework.checker.formatter.qual.FormatMethod.class)
|| AnnotationUtils.containsSameByName(
declarationAnnos, "com.google.errorprone.annotations.FormatMethod"));
}

/** The tree annotator for the Format String Checker. */
Expand Down
6 changes: 3 additions & 3 deletions checker/src/main/java/org/checkerframework/checker/nullness/NullnessTransfer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,9 +222,9 @@ protected TransferResult<NullnessValue, NullnessStore> strengthenAnnotationOfEqu
}
}

AnnotationMirrorSet secondAnnos =
secondValue != null ? secondValue.getAnnotations() : new AnnotationMirrorSet();
if (nullnessTypeFactory.containsSameByClass(secondAnnos, PolyNull.class)) {
if (secondValue != null
&& nullnessTypeFactory.containsSameByClass(
secondValue.getAnnotations(), PolyNull.class)) {
thenStore = thenStore == null ? res.getThenStore() : thenStore;
elseStore = elseStore == null ? res.getElseStore() : elseStore;
// TODO: methodTree is null for lambdas. Handle that case. See Issue3850.java.
Expand Down
71 changes: 39 additions & 32 deletions framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3500,21 +3500,22 @@ protected void addAliasedTypeAnnotation(
* <p>The point of {@code annotationToUse} is that it may include elements/fields.
*
* @param alias the class of the alias annotation
* @param annotation the class of the canonical annotation
* @param annotationClass the class of the canonical annotation
* @param annotationToUse the annotation mirror to use
*/
protected void addAliasedDeclAnnotation(
Class<? extends Annotation> alias,
Class<? extends Annotation> annotation,
Class<? extends Annotation> annotationClass,
AnnotationMirror annotationToUse) {
IPair<AnnotationMirror, Set<Class<? extends Annotation>>> pair = declAliases.get(annotation);
IPair<AnnotationMirror, Set<Class<? extends Annotation>>> 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(annotation, pair);
declAliases.put(annotationClass, pair);
}
Set<Class<? extends Annotation>> aliases = pair.second;
aliases.add(alias);
Expand Down Expand Up @@ -3845,19 +3846,21 @@ protected void parseAnnotationFiles() {
*
* @see #getDeclAnnotationNoAliases
* @param elt the element to retrieve the declaration annotation from
* @param anno annotation class
* @return the annotation mirror for anno
* @param annoClass annotation class
* @return the annotation mirror for annoClass
*/
@Override
public final AnnotationMirror getDeclAnnotation(Element elt, Class<? extends Annotation> anno) {
logGat("entering getDeclAnnotation(%s [%s], %s)%n", elt, elt.getKind(), anno);
public final AnnotationMirror getDeclAnnotation(
Element elt, Class<? extends Annotation> annoClass) {
logGat("entering getDeclAnnotation(%s [%s], %s)%n", elt, elt.getKind(), annoClass);
if (debugGat) {
if (elt.toString().equals("java.lang.CharSequence")) {
new Error("stack trace").printStackTrace();
}
}
AnnotationMirror result = getDeclAnnotation(elt, anno, true);
logGat(" exiting getDeclAnnotation(%s [%s], %s) => %s%n", elt, elt.getKind(), anno, result);
AnnotationMirror result = getDeclAnnotation(elt, annoClass, true);
logGat(
" exiting getDeclAnnotation(%s [%s], %s) => %s%n", elt, elt.getKind(), annoClass, result);
return result;
}

Expand All @@ -3875,12 +3878,12 @@ public final AnnotationMirror getDeclAnnotation(Element elt, Class<? extends Ann
*
* @see #getDeclAnnotation
* @param elt the element to retrieve the declaration annotation from
* @param anno annotation class
* @return the annotation mirror for anno
* @param annoClass annotation class
* @return the annotation mirror for annoClass
*/
public final @Nullable AnnotationMirror getDeclAnnotationNoAliases(
Element elt, Class<? extends Annotation> anno) {
return getDeclAnnotation(elt, anno, false);
Element elt, Class<? extends Annotation> annoClass) {
return getDeclAnnotation(elt, annoClass, false);
}

/**
Expand Down Expand Up @@ -5448,8 +5451,9 @@ public static String negateConstant(String constantExpression) {
/**
* Checks that the annotation {@code am} has the name of {@code annoClass}. Values are ignored.
*
* <p>This method is faster than {@link AnnotationUtils#areSameByClass(AnnotationMirror, Class)}
* because it caches the name of the class rather than computing it each time.
* <p>In the end, all annotation comparisons are by name. This method is faster than {@link
* AnnotationUtils#areSameByClass(AnnotationMirror, Class)} because it caches the name of {@code
* annoClass} rather than computing it on each invocation of this method.
*
* @param am the AnnotationMirror whose class to compare
* @param annoClass the class to compare
Expand All @@ -5466,35 +5470,38 @@ public boolean areSameByClass(AnnotationMirror am, Class<? extends Annotation> a

/**
* Checks that the collection contains the annotation. Using {@code Collection.contains} does not
* always work, because it does not use {@code areSame()} for comparison.
* always work, because it does not use {@link AnnotationUtils#areSame} for comparison.
*
* <p>This method is faster than {@link AnnotationUtils#containsSameByClass(Collection, Class)}
* because is caches the name of the class rather than computing it each time.
* <p>In the end, all annotation comparisons are by name. This method is faster than {@link
* AnnotationUtils#containsSameByClass(Collection, Class)} because it (actually, its callee)
* caches the name of {@code annoClass} rather than computing it each time.
*
* @param c a collection of AnnotationMirrors
* @param anno the annotation class to search for in c
* @return true iff c contains anno, according to areSameByClass
* @param annoClass the annotation class to search for in c
* @return true iff c contains an annotation of class annoClass, according to {@link
* #areSameByClass}
*/
public boolean containsSameByClass(
Collection<? extends AnnotationMirror> c, Class<? extends Annotation> anno) {
return getAnnotationByClass(c, anno) != null;
Collection<? extends AnnotationMirror> c, Class<? extends Annotation> annoClass) {
return getAnnotationByClass(c, annoClass) != null;
}

/**
* Returns the AnnotationMirror in {@code c} that has the same class as {@code anno}.
* Returns the AnnotationMirror in {@code c} that has class {@code annoClass}.
*
* <p>This method is faster than {@link AnnotationUtils#getAnnotationByClass(Collection, Class)}
* because is caches the name of the class rather than computing it each time.
* because it (actually, its callee) caches the name of the class rather than computing it each
* time.
*
* @param c a collection of AnnotationMirrors
* @param anno the class to search for in c
* @return AnnotationMirror with the same class as {@code anno} iff c contains anno, according to
* @param annoClass the class to search for in c
* @return an AnnotationMirror with class {@code annoClass} iff c contains one, according to
* areSameByClass; otherwise, {@code null}
*/
public @Nullable AnnotationMirror getAnnotationByClass(
Collection<? extends AnnotationMirror> c, Class<? extends Annotation> anno) {
Collection<? extends AnnotationMirror> c, Class<? extends Annotation> annoClass) {
for (AnnotationMirror an : c) {
if (areSameByClass(an, anno)) {
if (areSameByClass(an, annoClass)) {
return an;
}
}
Expand Down Expand Up @@ -5630,11 +5637,11 @@ protected void makeConditionConsistentWithOtherMethod(
}

/**
* Does {@code anno}, which is an {@link org.checkerframework.framework.qual.AnnotatedFor}
* annotation, apply to this checker?
* Does {@code annotatedForAnno}, which is an {@link
* org.checkerframework.framework.qual.AnnotatedFor} annotation, apply to this checker?
*
* @param annotatedForAnno an {@link AnnotatedFor} annotation
* @return whether {@code anno} applies to this checker
* @return whether {@code annotatedForAnno} applies to this checker
*/
public boolean doesAnnotatedForApplyToThisChecker(AnnotationMirror annotatedForAnno) {
List<String> annotatedForCheckers =
Expand Down
5 changes: 3 additions & 2 deletions ...main/java/org/checkerframework/framework/util/typeinference8/types/AbstractQualifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.util.function.BinaryOperator;
import javax.lang.model.element.AnnotationMirror;
import org.checkerframework.checker.interning.qual.Interned;
import org.checkerframework.checker.signature.qual.CanonicalName;
import org.checkerframework.framework.util.typeinference8.util.Java8InferenceContext;
import org.checkerframework.javacutil.AnnotationMirrorMap;
import org.checkerframework.javacutil.AnnotationMirrorSet;
Expand All @@ -21,7 +22,7 @@
public abstract class AbstractQualifier {

/** The (interned) name of the top qualifier in the same hierarchy as the qualifier. */
protected final @Interned String hierarchyName;
protected final @Interned @CanonicalName String hierarchyName;

/** The context. */
protected final Java8InferenceContext context;
Expand All @@ -34,7 +35,7 @@ public abstract class AbstractQualifier {
*/
AbstractQualifier(AnnotationMirror anno, Java8InferenceContext context) {
AnnotationMirror top = context.typeFactory.getQualifierHierarchy().getTopAnnotation(anno);
hierarchyName = AnnotationUtils.annotationNameInterned(top);
hierarchyName = AnnotationUtils.annotationName(top).intern();
this.context = context;
}

Expand Down
17 changes: 13 additions & 4 deletions javacutil/src/main/java/org/checkerframework/javacutil/AnnotationUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ private AnnotationUtils() {
/**
* Returns the fully-qualified name of an annotation as a String.
*
* <p>This method is efficient for {@code AnnotationBuilder.CheckerFrameworkAnnotationMirror}, for
* which it looks up the name. This method may be inefficient for other subclasses of {@code
* AnnotationMirror}, because it may compute a new string.
*
* @param annotation the annotation whose name to return
* @return the fully-qualified name of an annotation as a String
*/
Expand All @@ -81,6 +85,9 @@ private AnnotationUtils() {
/**
* Returns the fully-qualified name of an annotation as a String.
*
* <p>This is more efficient than calling {@link annotationName} and {@link
* java.lang.String#intern}.
*
* @param annotation the annotation whose name to return
* @return the fully-qualified name of an annotation as a String
*/
Expand Down Expand Up @@ -150,6 +157,8 @@ public static int compareByName(AnnotationMirror a1, AnnotationMirror a2) {
throw new BugInCF("Unexpected null argument: compareByName(%s, %s)", a1, a2);
}

// This is largely duplicated code. The point of this block is that
// the `if (name1 == name2)` test is very fast.
if (a1 instanceof CheckerFrameworkAnnotationMirror
&& a2 instanceof CheckerFrameworkAnnotationMirror) {
@Interned @CanonicalName String name1 = ((CheckerFrameworkAnnotationMirror) a1).annotationName;
Expand Down Expand Up @@ -190,7 +199,7 @@ public static boolean areSameByName(AnnotationMirror am, String aname) {
}

/**
* Checks that the annotation {@code am} has the name of {@code annoClass}. Values are ignored.
* Checks that the annotation {@code am} has class {@code annoClass}. Values are ignored.
*
* <p>This method is not very efficient. It is more efficient to use {@code
* AnnotatedTypeFactory#areSameByClass} or {@link #areSameByName}.
Expand Down Expand Up @@ -386,7 +395,7 @@ public static int compareAnnotationMirrors(AnnotationMirror a1, AnnotationMirror
return nameComparison;
}

// The annotations have the same name, but different values, so compare values.
// The annotations have the same name, but possibly different values, so compare values.
Map<? extends ExecutableElement, ? extends AnnotationValue> vals1 = a1.getElementValues();
Map<? extends ExecutableElement, ? extends AnnotationValue> vals2 = a2.getElementValues();
Set<ExecutableElement> sortedElements =
Expand All @@ -413,11 +422,11 @@ public static int compareAnnotationMirrors(AnnotationMirror a1, AnnotationMirror
}

/**
* Return 0 iff the two AnnotationValue objects are the same.
* Compare the two AnnotationValue objects for order.
*
* @param av1 the first AnnotationValue to compare
* @param av2 the second AnnotationValue to compare
* @return 0 if the two annotation values are the same
* @return -1 if the first is lesser, 0 if they are the same, or 1 if the first is greater
*/
@CompareToMethod
private static int compareAnnotationValue(AnnotationValue av1, AnnotationValue av2) {
Expand Down

0 comments on commit b9dd1af

Please sign in to comment.