From 44a17743f131b8f02bb0fdd39755127a6de707a5 Mon Sep 17 00:00:00 2001 From: tap-prod Date: Thu, 7 Dec 2017 14:20:05 -0800 Subject: [PATCH] Automated rollback of 49eabf7fcf36bcfe46c7a2abb6132da06ab36863 *** Original change description *** Remove BugPattern.suppressibility and BugPattern.customSuppressionAnnotations suppressionAnnotations should be used instead. RELNOTES: Remove BugPattern.suppressibility and BugPattern.customSuppressionAnnotations; suppressionAnnotations should be used instead. *** ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=178294356 --- .../com/google/errorprone/BugPattern.java | 24 ++++++++ .../com/google/errorprone/BugCheckerInfo.java | 56 +++++++++++++------ .../google/errorprone/SuppressionHelper.java | 17 ++++-- .../errorprone/bugpatterns/BugChecker.java | 14 ++--- .../errorprone/matchers/Suppressible.java | 8 ++- .../errorprone/scanner/ErrorProneScanner.java | 5 +- .../scanner/ScannerSupplierTest.java | 2 +- .../google/errorprone/BugPatternInstance.java | 55 +++++++++++++++--- 8 files changed, 138 insertions(+), 43 deletions(-) diff --git a/annotation/src/main/java/com/google/errorprone/BugPattern.java b/annotation/src/main/java/com/google/errorprone/BugPattern.java index 8ea4caf41b1..3ad6e245dbe 100644 --- a/annotation/src/main/java/com/google/errorprone/BugPattern.java +++ b/annotation/src/main/java/com/google/errorprone/BugPattern.java @@ -207,9 +207,33 @@ public enum SeverityLevel { SUGGESTION } + /** @deprecated use {@link #suppressionAnnotations} instead. */ + @Deprecated + Suppressibility suppressibility() default Suppressibility.DEFAULT; + + /** @deprecated use {@link #suppressionAnnotations} instead. */ + @Deprecated + public enum Suppressibility { + /** + * Can be suppressed using the standard {@code SuppressWarnings("foo")} mechanism. This setting + * should be used unless there is a good reason otherwise, e.g. security. + */ + SUPPRESS_WARNINGS, + /** Can be suppressed with a custom annotation on a parent AST node. */ + CUSTOM_ANNOTATION, + /** Cannot be suppressed. */ + UNSUPPRESSIBLE, + /** The default - uses {@link #suppressionAnnotations} instead. */ + DEFAULT; + } + /** True if the check can be disabled using command-line flags. */ boolean disableable() default true; + /** @deprecated use {@link #suppressionAnnotations} instead. */ + @Deprecated + Class[] customSuppressionAnnotations() default {}; + /** * A set of annotation types that can be used to suppress the check. * diff --git a/check_api/src/main/java/com/google/errorprone/BugCheckerInfo.java b/check_api/src/main/java/com/google/errorprone/BugCheckerInfo.java index 5ec51c697c7..418acdeebc6 100644 --- a/check_api/src/main/java/com/google/errorprone/BugCheckerInfo.java +++ b/check_api/src/main/java/com/google/errorprone/BugCheckerInfo.java @@ -22,12 +22,14 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.Suppressibility; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.matchers.Description; import com.sun.source.tree.Tree; import java.io.Serializable; import java.lang.annotation.Annotation; import java.lang.reflect.Modifier; +import java.util.Arrays; import java.util.Map; import java.util.Set; import java.util.stream.Stream; @@ -74,12 +76,16 @@ public class BugCheckerInfo implements Serializable { */ private final String linkUrl; - private final boolean supportsSuppressWarnings; + /** + * Whether this check may be suppressed. Corresponds to the {@code suppressibility} attribute from + * its {@code BugPattern}. + */ + private final Suppressibility suppressibility; /** - * A set of suppression annotations for this check. Computed from the {@code - * suppressionAnnotations} attributes from its {@code BugPattern}. May be empty if there are no - * suppression annotations for this check. + * A set of custom suppression annotations for this check. Computed from the {@code + * suppressibility} and {@code customSuppressionAnnotations} attributes from its {@code + * BugPattern}. May be empty if there are no custom suppression annotations for this check. */ private final Set> customSuppressionAnnotations; @@ -112,16 +118,34 @@ private BugCheckerInfo(Class checker, BugPattern pattern) pattern.summary(), pattern.severity(), createLinkUrl(pattern), - Stream.of(pattern.suppressionAnnotations()).anyMatch(a -> isSuppressWarnings(a)), - Stream.of(pattern.suppressionAnnotations()) - .filter(a -> !isSuppressWarnings(a)) - .collect(toImmutableSet()), + suppressibility(pattern), + customSuppressionAnnotations(pattern), ImmutableSet.copyOf(pattern.tags()), pattern.disableable()); } - private static boolean isSuppressWarnings(Class annotation) { - return annotation.getSimpleName().equals("SuppressWarnings"); + private static Suppressibility suppressibility(BugPattern pattern) { + if (pattern.suppressibility() == Suppressibility.DEFAULT) { + if (pattern.suppressionAnnotations().length == 0) { + return Suppressibility.UNSUPPRESSIBLE; + } + if (Arrays.asList(pattern.suppressionAnnotations()).contains(SuppressWarnings.class)) { + return Suppressibility.SUPPRESS_WARNINGS; + } + return Suppressibility.CUSTOM_ANNOTATION; + } + return pattern.suppressibility(); + } + + private static Set> customSuppressionAnnotations(BugPattern pattern) { + if (pattern.suppressibility() == Suppressibility.DEFAULT) { + return Stream.of(pattern.suppressionAnnotations()) + .filter(a -> !a.equals(SuppressWarnings.class)) + .collect(toImmutableSet()); + } + return pattern.suppressibility() == Suppressibility.CUSTOM_ANNOTATION + ? ImmutableSet.copyOf(pattern.customSuppressionAnnotations()) + : ImmutableSet.of(); } private BugCheckerInfo( @@ -131,7 +155,7 @@ private BugCheckerInfo( String message, SeverityLevel defaultSeverity, String linkUrl, - boolean supportsSuppressWarnings, + Suppressibility suppressibility, Set> customSuppressionAnnotations, ImmutableSet tags, boolean disableable) { @@ -141,7 +165,7 @@ private BugCheckerInfo( this.message = message; this.defaultSeverity = defaultSeverity; this.linkUrl = linkUrl; - this.supportsSuppressWarnings = supportsSuppressWarnings; + this.suppressibility = suppressibility; this.customSuppressionAnnotations = customSuppressionAnnotations; this.tags = tags; this.disableable = disableable; @@ -163,7 +187,7 @@ public BugCheckerInfo withCustomDefaultSeverity(SeverityLevel defaultSeverity) { message, defaultSeverity, linkUrl, - supportsSuppressWarnings, + suppressibility, customSuppressionAnnotations, tags, disableable); @@ -231,8 +255,8 @@ public String linkUrl() { return linkUrl; } - public boolean supportsSuppressWarnings() { - return supportsSuppressWarnings; + public Suppressibility suppressibility() { + return suppressibility; } public Set> customSuppressionAnnotations() { @@ -240,7 +264,7 @@ public Set> customSuppressionAnnotations() { } public boolean disableable() { - return disableable; + return suppressibility() != Suppressibility.UNSUPPRESSIBLE && disableable; } public ImmutableSet getTags() { diff --git a/check_api/src/main/java/com/google/errorprone/SuppressionHelper.java b/check_api/src/main/java/com/google/errorprone/SuppressionHelper.java index 08905bf475a..63e7a51d282 100644 --- a/check_api/src/main/java/com/google/errorprone/SuppressionHelper.java +++ b/check_api/src/main/java/com/google/errorprone/SuppressionHelper.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.Suppressibility; import com.google.errorprone.matchers.Suppressible; import com.google.errorprone.util.ASTHelpers; import com.sun.tools.javac.code.Attribute; @@ -163,15 +164,21 @@ public static boolean isSuppressed( SeverityLevel severityLevel, boolean inGeneratedCode, boolean disableWarningsInGeneratedCode) { + if (suppressible.suppressibility() == Suppressibility.UNSUPPRESSIBLE) { + return false; + } if (inGeneratedCode && disableWarningsInGeneratedCode && severityLevel != SeverityLevel.ERROR) { return true; } - if (suppressible.supportsSuppressWarnings() - && !Collections.disjoint(suppressible.allNames(), suppressionsOnCurrentPath)) { - return true; + switch (suppressible.suppressibility()) { + case CUSTOM_ANNOTATION: + return !Collections.disjoint( + suppressible.customSuppressionAnnotations(), customSuppressionsOnCurrentPath); + case SUPPRESS_WARNINGS: + return !Collections.disjoint(suppressible.allNames(), suppressionsOnCurrentPath); + default: + throw new IllegalStateException("No case for: " + suppressible.suppressibility()); } - return !Collections.disjoint( - suppressible.customSuppressionAnnotations(), customSuppressionsOnCurrentPath); } private static boolean isGenerated(Symbol sym, VisitorState state) { diff --git a/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java b/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java index e92fcd310ed..d24aa9f3800 100644 --- a/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java +++ b/check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java @@ -21,6 +21,7 @@ import com.google.common.base.Preconditions; import com.google.errorprone.BugCheckerInfo; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.Suppressibility; import com.google.errorprone.VisitorState; import com.google.errorprone.fixes.Fix; import com.google.errorprone.matchers.Description; @@ -175,8 +176,8 @@ public String linkUrl() { } @Override - public boolean supportsSuppressWarnings() { - return info.supportsSuppressWarnings(); + public Suppressibility suppressibility() { + return info.suppressibility(); } @Override @@ -421,16 +422,11 @@ public boolean equals(Object obj) { BugChecker that = (BugChecker) obj; return this.canonicalName().equals(that.canonicalName()) && this.defaultSeverity().equals(that.defaultSeverity()) - && this.supportsSuppressWarnings() == that.supportsSuppressWarnings() - && this.customSuppressionAnnotations().equals(that.customSuppressionAnnotations()); + && this.suppressibility().equals(that.suppressibility()); } @Override public int hashCode() { - return Objects.hash( - canonicalName(), - defaultSeverity(), - supportsSuppressWarnings(), - customSuppressionAnnotations()); + return Objects.hash(canonicalName(), defaultSeverity(), suppressibility()); } } diff --git a/check_api/src/main/java/com/google/errorprone/matchers/Suppressible.java b/check_api/src/main/java/com/google/errorprone/matchers/Suppressible.java index e81cee14ae7..fabb0670892 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/Suppressible.java +++ b/check_api/src/main/java/com/google/errorprone/matchers/Suppressible.java @@ -16,6 +16,7 @@ package com.google.errorprone.matchers; +import com.google.errorprone.BugPattern; import java.lang.annotation.Annotation; import java.util.Set; @@ -30,8 +31,11 @@ public interface Suppressible { /** The canonical name of the check. */ String canonicalName(); - /** Returns true if this checker can be suppressed using {@code @SuppressWarnings}. */ - boolean supportsSuppressWarnings(); + /** + * Returns how this checker can be suppressed (e.g., via {@code @SuppressWarnings} or a custom + * suppression annotation. + */ + BugPattern.Suppressibility suppressibility(); /** Returns the custom suppression annotations for this checker, if custom suppression is used. */ Set> customSuppressionAnnotations(); diff --git a/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java b/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java index 8c5fd8fa45d..5fb0f9d80e3 100644 --- a/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java +++ b/check_api/src/main/java/com/google/errorprone/scanner/ErrorProneScanner.java @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.Suppressibility; import com.google.errorprone.ErrorProneError; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -250,7 +251,9 @@ protected Set> getCustomSuppressionAnnotations() { private final List wildcardMatchers = new ArrayList<>(); private void registerNodeTypes(BugChecker checker) { - customSuppressionAnnotations.addAll(checker.customSuppressionAnnotations()); + if (checker.suppressibility() == Suppressibility.CUSTOM_ANNOTATION) { + customSuppressionAnnotations.addAll(checker.customSuppressionAnnotations()); + } if (checker instanceof AnnotationTreeMatcher) { annotationMatchers.add((AnnotationTreeMatcher) checker); diff --git a/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java b/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java index e94b99d2698..1238048d1a0 100644 --- a/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java +++ b/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java @@ -474,7 +474,7 @@ public void disablingPackageLocation_suppressible() throws Exception { category = JDK, severity = ERROR, suppressionAnnotations = {}, - disableable = false + disableable = true ) public static class UnsuppressiblePackageLocation extends PackageLocation {} diff --git a/docgen_processor/src/main/java/com/google/errorprone/BugPatternInstance.java b/docgen_processor/src/main/java/com/google/errorprone/BugPatternInstance.java index 020c0066e2c..c967d8faa3d 100644 --- a/docgen_processor/src/main/java/com/google/errorprone/BugPatternInstance.java +++ b/docgen_processor/src/main/java/com/google/errorprone/BugPatternInstance.java @@ -19,6 +19,7 @@ import com.google.common.base.Preconditions; import com.google.errorprone.BugPattern.ProvidesFix; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.Suppressibility; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -58,15 +59,51 @@ public static BugPatternInstance fromElement(Element element) { instance.providesFix = annotation.providesFix(); Map keyValues = getAnnotation(element, BugPattern.class.getName()); - Object suppression = keyValues.get("suppressionAnnotations"); - if (suppression == null) { - instance.suppressionAnnotations = new String[] {SuppressWarnings.class.getName()}; - } else { - Preconditions.checkState(suppression instanceof List); - @SuppressWarnings("unchecked") // Always List, see above. - List resultList = (List) suppression; - instance.suppressionAnnotations = - resultList.stream().map(AnnotationValue::toString).toArray(String[]::new); + Suppressibility suppressibility; + try { + suppressibility = annotation.suppressibility(); + } catch (EnumConstantNotPresentException e) { + suppressibility = Suppressibility.DEFAULT; + } + + switch (suppressibility) { + case UNSUPPRESSIBLE: + instance.suppressionAnnotations = new String[0]; + break; + case SUPPRESS_WARNINGS: + instance.suppressionAnnotations = new String[] {SuppressWarnings.class.getName()}; + break; + case CUSTOM_ANNOTATION: + // Deprecated in favor of DEFAULT/suppressionAnnotations + Object customSuppression = keyValues.get("customSuppressionAnnotations"); + if (customSuppression == null) { + instance.suppressionAnnotations = new String[0]; + } else { + Preconditions.checkState(customSuppression instanceof List); + // The doc for AnnotationValue says that if the value is an array, then + // AnnotationValue#getValue() will return a List. + @SuppressWarnings("unchecked") + List resultList = + (List) customSuppression; + instance.suppressionAnnotations = + resultList.stream().map(AnnotationValue::toString).toArray(String[]::new); + } + break; + case DEFAULT: + Object suppression = keyValues.get("suppressionAnnotations"); + if (suppression == null) { + instance.suppressionAnnotations = new String[] {SuppressWarnings.class.getName()}; + } else { + Preconditions.checkState(suppression instanceof List); + @SuppressWarnings("unchecked") // Always List, see above. + List resultList = + (List) suppression; + instance.suppressionAnnotations = + resultList.stream().map(AnnotationValue::toString).toArray(String[]::new); + } + break; + default: + throw new AssertionError(suppressibility); } instance.generateExamplesFromTestCases =