From 134895090fa885feda697c0460e275c223282ac3 Mon Sep 17 00:00:00 2001 From: Nathan Kooij Date: Thu, 30 Sep 2021 17:03:06 +0200 Subject: [PATCH] Support banned fields --- ....java => SimplifyTimeAnnotationCheck.java} | 83 ++++++++++++------- ...a => SimplifyTimeAnnotationCheckTest.java} | 22 ++++- 2 files changed, 71 insertions(+), 34 deletions(-) rename error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/{SimplifyTimeBasedAnnotationCheck.java => SimplifyTimeAnnotationCheck.java} (82%) rename error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/{SimplifyTimeBasedAnnotationCheckTest.java => SimplifyTimeAnnotationCheckTest.java} (84%) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeBasedAnnotationCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeAnnotationCheck.java similarity index 82% rename from error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeBasedAnnotationCheck.java rename to error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeAnnotationCheck.java index 2aa25619..398ab9a8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeBasedAnnotationCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeAnnotationCheck.java @@ -23,6 +23,7 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Scope; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.VarSymbol; @@ -34,13 +35,12 @@ import java.util.stream.Stream; @AutoService(BugChecker.class) @BugPattern( - name = "SimplifyTimeBasedAnnotation", + name = "SimplifyTimeAnnotation", summary = "Simplifies annotations which express an amount of time using TimeUnit", linkType = BugPattern.LinkType.NONE, severity = BugPattern.SeverityLevel.WARNING, tags = BugPattern.StandardTags.SIMPLIFICATION) -public final class SimplifyTimeBasedAnnotationCheck extends BugChecker - implements AnnotationTreeMatcher { +public final class SimplifyTimeAnnotationCheck extends BugChecker implements AnnotationTreeMatcher { private static final AnnotationAttributeMatcher ARGUMENT_SELECTOR = getMatcher(); @Override @@ -61,8 +61,12 @@ public final class SimplifyTimeBasedAnnotationCheck extends BugChecker AnnotationTree annotation, ImmutableList arguments) { checkArgument(!arguments.isEmpty()); - SimplifiableAnnotation simplifiableAnnotation = - SimplifiableAnnotation.from(getAnnotationFqcn(annotation)); + AnnotationDescriptor annotationDescriptor = + AnnotationDescriptor.from(getAnnotationFqcn(annotation)); + if (containsAnyAttributeOf(annotation, annotationDescriptor.bannedFields)) { + return Optional.empty(); + } + ImmutableMap indexedArguments = Maps.uniqueIndex( arguments, @@ -71,10 +75,10 @@ public final class SimplifyTimeBasedAnnotationCheck extends BugChecker .getSimpleName() .toString()); TimeUnit timeUnit = - getTimeUnit(annotation, simplifiableAnnotation.getTimeUnitField(), indexedArguments); + getTimeUnit(annotation, annotationDescriptor.timeUnitField, indexedArguments); ImmutableMap fieldValues = - simplifiableAnnotation.getTimeFields().stream() + annotationDescriptor.timeFields.stream() .map(field -> Maps.immutableEntry(field, getValue(field, indexedArguments))) .filter(entry -> entry.getValue().isPresent()) .collect(toImmutableMap(Map.Entry::getKey, entry -> entry.getValue().orElseThrow())); @@ -98,7 +102,7 @@ public final class SimplifyTimeBasedAnnotationCheck extends BugChecker getImplicitValueAttributeFix( annotation, simplification.getValue(), - simplifiableAnnotation.getTimeUnitField(), + annotationDescriptor.timeUnitField, simplification.getUnit())); } @@ -112,7 +116,20 @@ public final class SimplifyTimeBasedAnnotationCheck extends BugChecker .values())); return getExplicitAttributesFix( - annotation, simplifications, simplifiableAnnotation.getTimeUnitField(), commonUnit); + annotation, simplifications, annotationDescriptor.timeUnitField, commonUnit); + } + + private static boolean containsAnyAttributeOf( + AnnotationTree annotation, ImmutableSet attributes) { + return annotation.getArguments().stream() + .map( + expr -> + expr.getKind() == Tree.Kind.ASSIGNMENT + ? ASTHelpers.getSymbol(((AssignmentTree) expr).getVariable()) + .getSimpleName() + .toString() + : "value") + .anyMatch(attributes::contains); } private static Fix getImplicitValueAttributeFix( @@ -189,11 +206,10 @@ public final class SimplifyTimeBasedAnnotationCheck extends BugChecker private static AnnotationAttributeMatcher getMatcher() { ImmutableList toMatch = - Arrays.stream(SimplifiableAnnotation.values()) + Arrays.stream(AnnotationDescriptor.values()) .flatMap( annotation -> - annotation.getTimeFields().stream() - .map(field -> annotation.getFqcn() + '#' + field)) + annotation.timeFields.stream().map(field -> annotation.fqcn + '#' + field)) .collect(toImmutableList()); return AnnotationAttributeMatcher.create(Optional.of(toMatch), ImmutableList.of()); } @@ -210,25 +226,39 @@ public final class SimplifyTimeBasedAnnotationCheck extends BugChecker return ImmutableSortedSet.copyOf(units).first(); } - // XXX: Support "banned" fields which prevent simplifications altogether. - private enum SimplifiableAnnotation { + private enum AnnotationDescriptor { JUNIT_TIMEOUT("org.junit.jupiter.api.Timeout", ImmutableSet.of("value"), "unit"), SPRING_SCHEDULED( "org.springframework.scheduling.annotation.Scheduled", ImmutableSet.of("fixedDelay", "fixedRate", "initialDelay"), - "timeUnit"); + "timeUnit", + ImmutableSet.of("fixedDelayString", "fixedRateString", "initialDelayString")); + /** The fully-qualified class name of the annotation to simplify. */ private final String fqcn; + /** The attributes containing a value of time. */ private final ImmutableSet timeFields; + /** The attribute containing the time unit. */ private final String timeUnitField; + /** The set of attributes that cause the check to backoff. */ + private final ImmutableSet bannedFields; - SimplifiableAnnotation(String fqcn, ImmutableSet timeFields, String timeUnitField) { + AnnotationDescriptor(String fqcn, ImmutableSet timeFields, String timeUnitField) { + this(fqcn, timeFields, timeUnitField, ImmutableSet.of()); + } + + AnnotationDescriptor( + String fqcn, + ImmutableSet timeFields, + String timeUnitField, + ImmutableSet bannedFields) { this.fqcn = fqcn; this.timeFields = timeFields; this.timeUnitField = timeUnitField; + this.bannedFields = bannedFields; } - public static SimplifiableAnnotation from(String fqcn) { + public static AnnotationDescriptor from(String fqcn) { return Arrays.stream(values()) .filter(annotation -> annotation.fqcn.equals(fqcn)) .findFirst() @@ -237,19 +267,7 @@ public final class SimplifyTimeBasedAnnotationCheck extends BugChecker new IllegalArgumentException( String.format( "Unknown enum constant: %s.%s", - SimplifiableAnnotation.class.getName(), fqcn))); - } - - public String getFqcn() { - return fqcn; - } - - public ImmutableSet getTimeFields() { - return timeFields; - } - - public String getTimeUnitField() { - return timeUnitField; + AnnotationDescriptor.class.getName(), fqcn))); } } @@ -285,6 +303,7 @@ public final class SimplifyTimeBasedAnnotationCheck extends BugChecker .reverse(); } + /** Represents a simplification in terms of the new value and new unit. */ private static final class Simplification { private final Number value; private final TimeUnit unit; @@ -319,6 +338,10 @@ public final class SimplifyTimeBasedAnnotationCheck extends BugChecker : new Simplification(value.intValue(), unit); } + /** + * Converts the value with the unit represented by this simplification to an equivalent value + * in the given {@code unit}. + */ public long toUnit(TimeUnit unit) { return unit.convert(value.longValue(), this.unit); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeBasedAnnotationCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeAnnotationCheckTest.java similarity index 84% rename from error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeBasedAnnotationCheckTest.java rename to error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeAnnotationCheckTest.java index ff2b4439..7ae82717 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeBasedAnnotationCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/SimplifyTimeAnnotationCheckTest.java @@ -4,12 +4,11 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; -public final class SimplifyTimeBasedAnnotationCheckTest { +public final class SimplifyTimeAnnotationCheckTest { private final CompilationTestHelper compilationTestHelper = - CompilationTestHelper.newInstance(SimplifyTimeBasedAnnotationCheck.class, getClass()); + CompilationTestHelper.newInstance(SimplifyTimeAnnotationCheck.class, getClass()); private final BugCheckerRefactoringTestHelper refactoringTestHelper = - BugCheckerRefactoringTestHelper.newInstance( - SimplifyTimeBasedAnnotationCheck.class, getClass()); + BugCheckerRefactoringTestHelper.newInstance(SimplifyTimeAnnotationCheck.class, getClass()); @Test void identification() { @@ -29,6 +28,21 @@ public final class SimplifyTimeBasedAnnotationCheckTest { .doTest(); } + @Test + void identificationBannedField() { + compilationTestHelper + .addSourceLines( + "A.java", + "import org.springframework.scheduling.annotation.Scheduled;", + "", + "interface A {", + " // BUG: Diagnostic contains:", + " @Scheduled(fixedDelay = 6_000) A scheduledFixedDelay();", + " @Scheduled(fixedDelay = 6_000, fixedRateString = \"\") A bannedAttribute();", + "}") + .doTest(); + } + @Test void replacement() { refactoringTestHelper