Skip to content

Commit

Permalink
Suppress ImmutableAnnotationChecker for AutoAnnotation-generated anno…
Browse files Browse the repository at this point in the history
…tations

Which contain array fields, but make defensive copies to ensure the class is
immutable.

RELNOTES: N/A

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=178042508
  • Loading branch information
cushon committed Dec 6, 2017
1 parent 50a78bd commit 11e28aa
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 0 deletions.
54 changes: 54 additions & 0 deletions check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@

package com.google.errorprone.util;

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.errorprone.matchers.JUnitMatchers.JUNIT4_RUN_WITH_ANNOTATION;
import static com.sun.tools.javac.code.Scope.LookupKind.NON_RECURSIVE;

import com.google.common.base.CharMatcher;
import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.VisitorState;
import com.google.errorprone.dataflow.nullnesspropagation.Nullness;
import com.google.errorprone.dataflow.nullnesspropagation.NullnessAnalysis;
Expand All @@ -46,6 +48,8 @@
import com.sun.source.tree.TypeParameterTree;
import com.sun.source.tree.VariableTree;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Attribute;
import com.sun.tools.javac.code.Attribute.Compound;
import com.sun.tools.javac.code.Flags;
import com.sun.tools.javac.code.Scope;
import com.sun.tools.javac.code.Symbol;
Expand Down Expand Up @@ -96,8 +100,10 @@
import java.util.stream.Stream;
import javax.annotation.Nullable;
import javax.lang.model.element.AnnotationMirror;
import javax.lang.model.element.AnnotationValue;
import javax.lang.model.element.ElementKind;
import javax.lang.model.type.TypeKind;
import javax.lang.model.util.SimpleAnnotationValueVisitor8;

/** This class contains utility methods to work with the javac AST. */
public class ASTHelpers {
Expand Down Expand Up @@ -943,4 +949,52 @@ public static MethodSymbol resolveExistingMethod(
log.popDiagnosticHandler(handler);
}
}

/**
* Returns the values of the given symbol's {@code javax.annotation.Generated} or {@code
* javax.annotation.processing.Generated} annotation, if present.
*/
public static ImmutableSet<String> getGeneratedBy(ClassSymbol symbol, VisitorState state) {
checkNotNull(symbol);
Optional<Compound> c =
Stream.of("javax.annotation.Generated", "javax.annotation.processing.Generated")
.map(state::getSymbolFromString)
.filter(a -> a != null)
.map(symbol::attribute)
.filter(a -> a != null)
.findFirst();
if (!c.isPresent()) {
return ImmutableSet.of();
}
Optional<Attribute> values =
c.get()
.getElementValues()
.entrySet()
.stream()
.filter(e -> e.getKey().getSimpleName().contentEquals("value"))
.map(e -> e.getValue())
.findAny();
if (!values.isPresent()) {
return ImmutableSet.of();
}
ImmutableSet.Builder<String> suppressions = ImmutableSet.builder();
values
.get()
.accept(
new SimpleAnnotationValueVisitor8<Void, Void>() {
@Override
public Void visitString(String s, Void aVoid) {
suppressions.add(s);
return super.visitString(s, aVoid);
}

@Override
public Void visitArray(List<? extends AnnotationValue> vals, Void aVoid) {
vals.stream().forEachOrdered(v -> v.accept(this, null));
return super.visitArray(vals, aVoid);
}
},
null);
return suppressions.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.errorprone.BugPattern.Category.JDK;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.util.ASTHelpers.getGeneratedBy;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.google.errorprone.util.ASTHelpers.getType;

Expand All @@ -39,6 +40,7 @@
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Symbol.ClassSymbol;
import java.util.Collections;
import java.util.Optional;

/** @author cushon@google.com (Liam Miller-Cushon) */
Expand All @@ -57,6 +59,11 @@ public class ImmutableAnnotationChecker extends BugChecker implements ClassTreeM
"annotations are immutable by default; annotating them with"
+ " @com.google.errorprone.annotations.Immutable is unnecessary";

private static final ImmutableSet<String> PROCESSOR_BLACKLIST =
ImmutableSet.of(
"com.google.auto.value.processor.AutoAnnotationProcessor"
);

private final WellKnownMutability wellKnownMutability;

@Deprecated // Used reflectively, but you should pass in ErrorProneFlags to get custom mutability
Expand All @@ -76,6 +83,9 @@ public Description matchClass(ClassTree tree, VisitorState state) {
|| !WellKnownMutability.isAnnotation(state, symbol.type)) {
return NO_MATCH;
}
if (!Collections.disjoint(getGeneratedBy(symbol, state), PROCESSOR_BLACKLIST)) {
return NO_MATCH;
}

This comment has been minimized.

Copy link
@Stephan202

Stephan202 Dec 7, 2017

Contributor

Unless the processor exposes defensive copies of any arbitrary mutable public property, isn't this too optimistic?

(An alternative would be for Google Auto value to annotate the array fields it generates with @SuppressWarnings("Immutable"). That's what I did for Immutables in immutables/immutables@80d0633.)

This comment has been minimized.

Copy link
@cushon

cushon Dec 7, 2017

Author Collaborator

Annotations can't have arbitrary mutable properties, just arrays and compile-time constants (literals including enums and class literals). AutoAnnotation makes defensive copies of arrays.

This comment has been minimized.

Copy link
@Stephan202

Stephan202 Dec 7, 2017

Contributor

Thanks; I see I mistook AutoAnnotationProcessor for AutoValueProcessor. Then it makes sense :).

Just checking: having the processor generate @SuppressWarnings("Immutable") would have been a valid alternative, right?

This comment has been minimized.

Copy link
@cushon

cushon Dec 7, 2017

Author Collaborator

Yep, @SuppressWarnings works fine too. I decided to bake knowledge of the annotation processor into the check, instead of baking knowledge of the check into the annotation processor.


if (ASTHelpers.hasAnnotation(symbol, Immutable.class, state)) {
AnnotationTree annotation =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.errorprone.bugpatterns.threadsafety;

import com.google.errorprone.CompilationTestHelper;
import java.lang.reflect.Method;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -329,4 +330,41 @@ public void jucImmutable() {
"}")
.doTest();
}

@Test
public void generated() {
compilationHelper
.addSourceLines(
"Test.java",
"import java.lang.annotation.Annotation;",
(isJdk8OrEarlier()
? "import javax.annotation.Generated;"
: "import javax.annotation.processing.Generated;"),
"@Generated(\"com.google.auto.value.processor.AutoAnnotationProcessor\")",
"class Test implements Deprecated {",
" public Class<? extends Annotation> annotationType() { return Deprecated.class; }",
" int x;",
" private Test(int x) {",
" this.x = x;",
" }",
" public boolean forRemoval() {",
" return false;",
" }",
" public String since() {",
" return \"\";",
" }",
"}")
.doTest();
}

static boolean isJdk8OrEarlier() {
try {
Method versionMethod = Runtime.class.getMethod("version");
Object version = versionMethod.invoke(null);
int majorVersion = (int) version.getClass().getMethod("major").invoke(version);
return majorVersion <= 8;
} catch (ReflectiveOperationException e) {
return true;
}
}
}

0 comments on commit 11e28aa

Please sign in to comment.