From 26f1f5498321c9e2c25f5546dc9aa026affde16a Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Mon, 24 May 2021 10:25:27 -0700 Subject: [PATCH] Introduce `com.google.errorprone.annotations.Modifier` for use in `@IncompatibleModifiers` and `@RequiredModifiers`, as an alternative to `javax.lang.model.element.Modifier` which is not available at compile-time on Android. https://github.com/google/error-prone/issues/2122 PiperOrigin-RevId: 375503791 --- .../errorprone/annotations/ForOverride.java | 7 +-- .../annotations/IncompatibleModifiers.java | 10 ++-- .../errorprone/annotations/Modifier.java | 42 +++++++++++++++ .../annotations/RequiredModifiers.java | 10 ++-- .../google/errorprone/annotations/Var.java | 3 +- .../annotations/concurrent/LazyInit.java | 4 +- .../IncompatibleModifiersChecker.java | 47 ++++++++++++++-- .../bugpatterns/RequiredModifiersChecker.java | 53 ++++++++++++++++--- 8 files changed, 148 insertions(+), 28 deletions(-) create mode 100644 annotations/src/main/java/com/google/errorprone/annotations/Modifier.java diff --git a/annotations/src/main/java/com/google/errorprone/annotations/ForOverride.java b/annotations/src/main/java/com/google/errorprone/annotations/ForOverride.java index 285e14118e7..346775451d5 100644 --- a/annotations/src/main/java/com/google/errorprone/annotations/ForOverride.java +++ b/annotations/src/main/java/com/google/errorprone/annotations/ForOverride.java @@ -18,10 +18,6 @@ import static java.lang.annotation.ElementType.METHOD; import static java.lang.annotation.RetentionPolicy.CLASS; -import static javax.lang.model.element.Modifier.FINAL; -import static javax.lang.model.element.Modifier.PRIVATE; -import static javax.lang.model.element.Modifier.PUBLIC; -import static javax.lang.model.element.Modifier.STATIC; import java.lang.annotation.Documented; import java.lang.annotation.Retention; @@ -39,7 +35,8 @@ * protected or package-private visibility, although their effective visibility is actually "none". */ @Documented -@IncompatibleModifiers({PUBLIC, PRIVATE, STATIC, FINAL}) +@IncompatibleModifiers( + modifier = {Modifier.PUBLIC, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL}) @Retention(CLASS) // Parent source might not be available while compiling subclass @Target(METHOD) public @interface ForOverride {} diff --git a/annotations/src/main/java/com/google/errorprone/annotations/IncompatibleModifiers.java b/annotations/src/main/java/com/google/errorprone/annotations/IncompatibleModifiers.java index 55ff68c2717..284fb45f9b3 100644 --- a/annotations/src/main/java/com/google/errorprone/annotations/IncompatibleModifiers.java +++ b/annotations/src/main/java/com/google/errorprone/annotations/IncompatibleModifiers.java @@ -21,14 +21,13 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; -import javax.lang.model.element.Modifier; /** * Annotation declaring that the target annotation is incompatible with any one of the provided * modifiers. For example, an annotation declared as: * * 
- * {@literal @}IncompatibleModifiers(Modifier.PUBLIC)
+ * {@literal @}IncompatibleModifiers(modifier = Modifier.PUBLIC)
  * {@literal @}interface MyAnnotation {}
  *
* @@ -44,6 +43,11 @@ @Retention(RetentionPolicy.CLASS) // Element's source might not be available during analysis @Target(ElementType.ANNOTATION_TYPE) public @interface IncompatibleModifiers { + + /** @deprecated use {@link #modifier} instead */ + @Deprecated + javax.lang.model.element.Modifier[] value() default {}; + /** * The incompatible modifiers. The annotated element is illegal with the presence of any one or * more of these modifiers. @@ -51,5 +55,5 @@ *

Empty array has the same effect as not applying this annotation at all; duplicates are * allowed but have no effect. */ - Modifier[] value(); + Modifier[] modifier() default {}; } diff --git a/annotations/src/main/java/com/google/errorprone/annotations/Modifier.java b/annotations/src/main/java/com/google/errorprone/annotations/Modifier.java new file mode 100644 index 00000000000..c8b9bd83d09 --- /dev/null +++ b/annotations/src/main/java/com/google/errorprone/annotations/Modifier.java @@ -0,0 +1,42 @@ +/* + * Copyright 2021 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.annotations; + +/** + * Modifiers in the Java language, as specified in: + * + *

+ */ +public enum Modifier { + PUBLIC, + PROTECTED, + PRIVATE, + ABSTRACT, + DEFAULT, + STATIC, + FINAL, + TRANSIENT, + VOLATILE, + SYNCHRONIZED, + NATIVE, + STRICTFP +} diff --git a/annotations/src/main/java/com/google/errorprone/annotations/RequiredModifiers.java b/annotations/src/main/java/com/google/errorprone/annotations/RequiredModifiers.java index ca14b836836..e63a77b9401 100644 --- a/annotations/src/main/java/com/google/errorprone/annotations/RequiredModifiers.java +++ b/annotations/src/main/java/com/google/errorprone/annotations/RequiredModifiers.java @@ -21,14 +21,13 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; -import javax.lang.model.element.Modifier; /** * Annotation declaring that the target annotation requires all the specified modifiers. For * example, an annotation declared as: * * 
- * {@literal @}RequiredModifiers(Modifier.PUBLIC)
+ * {@literal @}RequiredModifiers(modifier = Modifier.PUBLIC)
  * {@literal @}interface MyAnnotation {}
  *
* @@ -44,6 +43,11 @@ @Retention(RetentionPolicy.CLASS) // Element's source might not be available during analysis @Target(ElementType.ANNOTATION_TYPE) public @interface RequiredModifiers { + + /** @deprecated use {@link #modifier} instead */ + @Deprecated + javax.lang.model.element.Modifier[] value() default {}; + /** * The required modifiers. The annotated element is illegal if any one or more of these modifiers * are absent. @@ -51,5 +55,5 @@ *

Empty array has the same effect as not applying this annotation at all; duplicates are * allowed but have no effect. */ - Modifier[] value(); + Modifier[] modifier() default {}; } diff --git a/annotations/src/main/java/com/google/errorprone/annotations/Var.java b/annotations/src/main/java/com/google/errorprone/annotations/Var.java index a0e2a63f2b4..ebf3b639f42 100644 --- a/annotations/src/main/java/com/google/errorprone/annotations/Var.java +++ b/annotations/src/main/java/com/google/errorprone/annotations/Var.java @@ -20,7 +20,6 @@ import static java.lang.annotation.ElementType.LOCAL_VARIABLE; import static java.lang.annotation.ElementType.PARAMETER; import static java.lang.annotation.RetentionPolicy.RUNTIME; -import static javax.lang.model.element.Modifier.FINAL; import java.lang.annotation.Retention; import java.lang.annotation.Target; @@ -42,5 +41,5 @@ */ @Target({FIELD, PARAMETER, LOCAL_VARIABLE}) @Retention(RUNTIME) -@IncompatibleModifiers(FINAL) +@IncompatibleModifiers(modifier = {Modifier.FINAL}) public @interface Var {} diff --git a/annotations/src/main/java/com/google/errorprone/annotations/concurrent/LazyInit.java b/annotations/src/main/java/com/google/errorprone/annotations/concurrent/LazyInit.java index c264e6aa761..380c2d7f54e 100644 --- a/annotations/src/main/java/com/google/errorprone/annotations/concurrent/LazyInit.java +++ b/annotations/src/main/java/com/google/errorprone/annotations/concurrent/LazyInit.java @@ -15,9 +15,9 @@ */ package com.google.errorprone.annotations.concurrent; -import static javax.lang.model.element.Modifier.FINAL; import com.google.errorprone.annotations.IncompatibleModifiers; +import com.google.errorprone.annotations.Modifier; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -61,7 +61,7 @@ * unless you really understand this and you really need the performance benefits of * introducing the data race, do not use this construct. */ -@IncompatibleModifiers({FINAL}) +@IncompatibleModifiers(modifier = {Modifier.FINAL}) @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.FIELD) public @interface LazyInit {} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/IncompatibleModifiersChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/IncompatibleModifiersChecker.java index 0a26c96d506..99dc0a1a48a 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/IncompatibleModifiersChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/IncompatibleModifiersChecker.java @@ -19,12 +19,11 @@ import static com.google.errorprone.BugPattern.LinkType.NONE; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.MoreAnnotations.getValue; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; -import com.google.errorprone.annotations.IncompatibleModifiers; import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; @@ -32,8 +31,16 @@ import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ModifiersTree; import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Attribute; +import com.sun.tools.javac.code.Symbol; +import java.util.Collection; +import java.util.LinkedHashSet; +import java.util.List; import java.util.Set; +import javax.lang.model.element.AnnotationValue; import javax.lang.model.element.Modifier; +import javax.lang.model.element.VariableElement; +import javax.lang.model.util.SimpleAnnotationValueVisitor8; /** @author sgoldfeder@google.com (Steven Goldfeder) */ @BugPattern( @@ -45,15 +52,28 @@ severity = ERROR) public class IncompatibleModifiersChecker extends BugChecker implements AnnotationTreeMatcher { + private static final String INCOMPATIBLE_MODIFIERS = + "com.google.errorprone.annotations.IncompatibleModifiers"; + @Override public Description matchAnnotation(AnnotationTree tree, VisitorState state) { - IncompatibleModifiers annotation = ASTHelpers.getAnnotation(tree, IncompatibleModifiers.class); + Symbol sym = ASTHelpers.getSymbol(tree); + if (sym == null) { + return NO_MATCH; + } + Attribute.Compound annotation = + sym.getRawAttributes().stream() + .filter(a -> a.type.tsym.getQualifiedName().contentEquals(INCOMPATIBLE_MODIFIERS)) + .findAny() + .orElse(null); if (annotation == null) { return NO_MATCH; } - ImmutableSet incompatibleModifiers = ImmutableSet.copyOf(annotation.value()); + Set incompatibleModifiers = new LinkedHashSet<>(); + getValue(annotation, "value").ifPresent(a -> getModifiers(incompatibleModifiers, a)); + getValue(annotation, "modifier").ifPresent(a -> getModifiers(incompatibleModifiers, a)); if (incompatibleModifiers.isEmpty()) { - return Description.NO_MATCH; + return NO_MATCH; } Tree parent = state.getPath().getParentPath().getLeaf(); @@ -83,4 +103,21 @@ public Description matchAnnotation(AnnotationTree tree, VisitorState state) { .setMessage(message) .build(); } + + private static void getModifiers(Collection modifiers, Attribute attribute) { + class Visitor extends SimpleAnnotationValueVisitor8 { + @Override + public Void visitEnumConstant(VariableElement c, Void unused) { + modifiers.add(Modifier.valueOf(c.getSimpleName().toString())); + return null; + } + + @Override + public Void visitArray(List vals, Void unused) { + vals.forEach(val -> val.accept(this, null)); + return null; + } + } + attribute.accept(new Visitor(), null); + } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/RequiredModifiersChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/RequiredModifiersChecker.java index cc59e688889..66fc60ff2cf 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/RequiredModifiersChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/RequiredModifiersChecker.java @@ -18,12 +18,12 @@ import static com.google.errorprone.BugPattern.LinkType.NONE; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.MoreAnnotations.getValue; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; -import com.google.errorprone.annotations.RequiredModifiers; import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; @@ -31,8 +31,16 @@ import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ModifiersTree; import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Attribute; +import com.sun.tools.javac.code.Symbol; +import java.util.Collection; +import java.util.LinkedHashSet; +import java.util.List; import java.util.Set; +import javax.lang.model.element.AnnotationValue; import javax.lang.model.element.Modifier; +import javax.lang.model.element.VariableElement; +import javax.lang.model.util.SimpleAnnotationValueVisitor8; /** @author sgoldfeder@google.com (Steven Goldfeder) */ @BugPattern( @@ -46,28 +54,40 @@ public class RequiredModifiersChecker extends BugChecker implements AnnotationTr private static final String MESSAGE_TEMPLATE = "%s has specified that it must be used together with the following modifiers: %s"; + private static final String REQUIRED_MODIFIERS = + "com.google.errorprone.annotations.RequiredModifiers"; @Override public Description matchAnnotation(AnnotationTree tree, VisitorState state) { - RequiredModifiers annotation = ASTHelpers.getAnnotation(tree, RequiredModifiers.class); + Symbol sym = ASTHelpers.getSymbol(tree); + if (sym == null) { + return NO_MATCH; + } + Attribute.Compound annotation = + sym.getRawAttributes().stream() + .filter(a -> a.type.tsym.getQualifiedName().contentEquals(REQUIRED_MODIFIERS)) + .findAny() + .orElse(null); if (annotation == null) { - return Description.NO_MATCH; + return NO_MATCH; } - Set requiredModifiers = ImmutableSet.copyOf(annotation.value()); + Set requiredModifiers = new LinkedHashSet<>(); + getValue(annotation, "value").ifPresent(a -> getModifiers(requiredModifiers, a)); + getValue(annotation, "modifier").ifPresent(a -> getModifiers(requiredModifiers, a)); if (requiredModifiers.isEmpty()) { - return Description.NO_MATCH; + return NO_MATCH; } Tree parent = state.getPath().getParentPath().getLeaf(); if (!(parent instanceof ModifiersTree)) { // e.g. An annotated package name - return Description.NO_MATCH; + return NO_MATCH; } Set missing = Sets.difference(requiredModifiers, ((ModifiersTree) parent).getFlags()); if (missing.isEmpty()) { - return Description.NO_MATCH; + return NO_MATCH; } String annotationName = ASTHelpers.getAnnotationName(tree); @@ -86,4 +106,21 @@ public Description matchAnnotation(AnnotationTree tree, VisitorState state) { .setMessage(customMessage) .build(); } + + private static void getModifiers(Collection modifiers, Attribute attribute) { + class Visitor extends SimpleAnnotationValueVisitor8 { + @Override + public Void visitEnumConstant(VariableElement c, Void unused) { + modifiers.add(Modifier.valueOf(c.getSimpleName().toString())); + return null; + } + + @Override + public Void visitArray(List vals, Void unused) { + vals.forEach(val -> val.accept(this, null)); + return null; + } + } + attribute.accept(new Visitor(), null); + } }