diff --git a/annotation/src/main/java/com/google/errorprone/BugPattern.java b/annotation/src/main/java/com/google/errorprone/BugPattern.java index 77a77f93aa0..55170b690ed 100644 --- a/annotation/src/main/java/com/google/errorprone/BugPattern.java +++ b/annotation/src/main/java/com/google/errorprone/BugPattern.java @@ -16,12 +16,12 @@ package com.google.errorprone; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + import java.lang.annotation.Annotation; import java.lang.annotation.Retention; import java.util.Comparator; -import static java.lang.annotation.RetentionPolicy.RUNTIME; - /** * Describes a bug pattern detected by error-prone. Used to generate compiler error messages, * for @SuppressWarnings, and to generate wiki documentation. @@ -141,15 +141,6 @@ public enum MaturityLevel { } } - /** - * Whether this check should be disableable by a command-line flag. - * - *

You are strongly encouraged to keep the default value of false since error-prone checks - * should have a zero false positive rate. A check should only be disabled when bugs exist in - * legacy code and are infeasible to fix. - */ - boolean disableable() default false; - /** * Whether this checker should be suppressible, and if so, by what means. */ @@ -160,15 +151,25 @@ 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, + SUPPRESS_WARNINGS(true), /** * Can be suppressed with a custom annotation on a parent AST node. */ - CUSTOM_ANNOTATION, + CUSTOM_ANNOTATION(false), /** * Cannot be suppressed. */ - UNSUPPRESSIBLE + UNSUPPRESSIBLE(false); + + private final boolean disableable; + + Suppressibility(boolean disableable) { + this.disableable = disableable; + } + + public boolean disableable() { + return disableable; + } } /** diff --git a/core/src/main/java/com/google/errorprone/BugCheckerSupplier.java b/core/src/main/java/com/google/errorprone/BugCheckerSupplier.java index e60752ec655..23da158ad81 100644 --- a/core/src/main/java/com/google/errorprone/BugCheckerSupplier.java +++ b/core/src/main/java/com/google/errorprone/BugCheckerSupplier.java @@ -19,6 +19,7 @@ import com.google.common.base.Supplier; import com.google.errorprone.BugPattern.MaturityLevel; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.Suppressibility; import com.google.errorprone.bugpatterns.BugChecker; import java.util.Objects; @@ -62,6 +63,12 @@ public static BugCheckerSupplier fromInstance(BugChecker checker) { */ public abstract SeverityLevel severity(); + /** + * The {@link Suppressibility} of the {@link BugChecker}, e.g. + * {@link Suppressibility#UNSUPPRESSIBLE} + */ + public abstract Suppressibility suppressibility(); + /** * Returns a new {@link BugCheckerSupplier} in which the previous {@link SeverityLevel} * has been overridden to the given value. @@ -73,11 +80,6 @@ public static BugCheckerSupplier fromInstance(BugChecker checker) { */ public abstract MaturityLevel maturity(); - /** - * Whether this {@link BugChecker} may be disabled. - */ - public abstract boolean disableable(); - @Override public boolean equals(Object obj) { if (obj instanceof BugCheckerSupplier) { @@ -85,13 +87,13 @@ public boolean equals(Object obj) { return this.canonicalName().equals(that.canonicalName()) && this.severity().equals(that.severity()) && this.maturity().equals(that.maturity()) - && this.disableable() == that.disableable(); + && this.suppressibility().equals(that.suppressibility()); } return false; } @Override public int hashCode() { - return Objects.hash(canonicalName(), severity(), maturity(), disableable()); + return Objects.hash(canonicalName(), severity(), maturity(), suppressibility()); } } diff --git a/core/src/main/java/com/google/errorprone/FromClassBugCheckerSupplier.java b/core/src/main/java/com/google/errorprone/FromClassBugCheckerSupplier.java index c293192c15c..d978c8ff453 100644 --- a/core/src/main/java/com/google/errorprone/FromClassBugCheckerSupplier.java +++ b/core/src/main/java/com/google/errorprone/FromClassBugCheckerSupplier.java @@ -19,6 +19,7 @@ import com.google.common.base.Preconditions; import com.google.errorprone.BugPattern.MaturityLevel; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.Suppressibility; import com.google.errorprone.bugpatterns.BugChecker; /** @@ -29,7 +30,7 @@ class FromClassBugCheckerSupplier extends BugCheckerSupplier { private final String canonicalName; private final SeverityLevel severity; private final MaturityLevel maturity; - private final boolean disableable; + private final Suppressibility suppressibility; FromClassBugCheckerSupplier(Class checkerClass) { BugPattern pattern = checkerClass.getAnnotation(BugPattern.class); @@ -39,16 +40,17 @@ class FromClassBugCheckerSupplier extends BugCheckerSupplier { this.canonicalName = pattern.name(); this.severity = pattern.severity(); this.maturity = pattern.maturity(); - this.disableable = pattern.disableable(); + this.suppressibility = pattern.suppressibility(); } private FromClassBugCheckerSupplier(Class checkerClass, - String canonicalName, SeverityLevel severity, MaturityLevel maturity, boolean disableable) { + String canonicalName, SeverityLevel severity, MaturityLevel maturity, + Suppressibility suppressibility) { this.checkerClass = checkerClass; this.canonicalName = canonicalName; this.severity = severity; this.maturity = maturity; - this.disableable = disableable; + this.suppressibility = suppressibility; } @Override @@ -76,7 +78,7 @@ public SeverityLevel severity() { @Override public BugCheckerSupplier overrideSeverity(SeverityLevel severity) { return new FromClassBugCheckerSupplier( - this.checkerClass, this.canonicalName, severity, this.maturity, this.disableable); + this.checkerClass, this.canonicalName, severity, this.maturity, this.suppressibility); } @Override @@ -85,8 +87,8 @@ public MaturityLevel maturity() { } @Override - public boolean disableable() { - return disableable; + public Suppressibility suppressibility() { + return suppressibility; } @Override diff --git a/core/src/main/java/com/google/errorprone/FromInstanceBugCheckerSupplier.java b/core/src/main/java/com/google/errorprone/FromInstanceBugCheckerSupplier.java index 4b5f07cc6d6..c070bf07e1e 100644 --- a/core/src/main/java/com/google/errorprone/FromInstanceBugCheckerSupplier.java +++ b/core/src/main/java/com/google/errorprone/FromInstanceBugCheckerSupplier.java @@ -19,6 +19,7 @@ import com.google.common.base.Preconditions; import com.google.errorprone.BugPattern.MaturityLevel; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.Suppressibility; import com.google.errorprone.bugpatterns.BugChecker; /** @@ -74,7 +75,7 @@ public MaturityLevel maturity() { } @Override - public boolean disableable() { - return checker.disableable(); + public Suppressibility suppressibility() { + return checker.suppressibility(); } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java index d8fbcc26118..4cf0877e8be 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java @@ -135,12 +135,6 @@ public abstract class BugChecker implements Suppressible, Serializable { */ private final String linkUrl; - /** - * Whether this check may be disabled. Corresponds to the {@code disableable} attribute from - * its {@code BugPattern}. - */ - private final boolean disableable; - /** * Whether this check may be suppressed. Corresponds to the {@code suppressibility} attribute * from its {@code BugPattern}. @@ -171,7 +165,6 @@ public BugChecker() { maturity = pattern.maturity(); severity = pattern.severity(); linkUrl = createLinkUrl(pattern); - disableable = pattern.disableable(); suppressibility = pattern.suppressibility(); if (suppressibility == Suppressibility.CUSTOM_ANNOTATION) { customSuppressionAnnotation = pattern.customSuppressionAnnotation(); @@ -252,10 +245,6 @@ public String linkUrl() { return linkUrl; } - public boolean disableable() { - return disableable; - } - @Override public Suppressibility suppressibility() { return suppressibility; diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ChainingConstructorIgnoresParameter.java b/core/src/main/java/com/google/errorprone/bugpatterns/ChainingConstructorIgnoresParameter.java index 027816a96ec..f85cf4a8f73 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ChainingConstructorIgnoresParameter.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ChainingConstructorIgnoresParameter.java @@ -68,7 +68,7 @@ * @author cpovirk@google.com (Chris Povirk) */ @BugPattern(name = "ChainingConstructorIgnoresParameter", - maturity = MATURE, category = JDK, severity = ERROR, disableable = true, + maturity = MATURE, category = JDK, severity = ERROR, explanation = "A constructor parameter might not be being used as expected", summary = "The called constructor accepts a parameter with the same name and type as one of " + "its caller's parameters, but its caller doesn't pass that parameter to it. It's likely " diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DepAnn.java b/core/src/main/java/com/google/errorprone/bugpatterns/DepAnn.java index e25464738b3..ceccf0ccab5 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/DepAnn.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/DepAnn.java @@ -46,7 +46,7 @@ explanation = "A declaration has the @deprecated Javadoc tag but no @Deprecated annotation. " + "Please add an @Deprecated annotation to this declaration in addition to the @deprecated " + "tag in the Javadoc.", - disableable = true, category = JDK, severity = ERROR, maturity = MATURE) + category = JDK, severity = ERROR, maturity = MATURE) public class DepAnn extends BugChecker implements MethodTreeMatcher, ClassTreeMatcher, VariableTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/LongLiteralLowerCaseSuffix.java b/core/src/main/java/com/google/errorprone/bugpatterns/LongLiteralLowerCaseSuffix.java index 671551ed10f..64247469115 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/LongLiteralLowerCaseSuffix.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/LongLiteralLowerCaseSuffix.java @@ -44,7 +44,7 @@ summary = "Prefer 'L' to 'l' for the suffix to long literals", explanation = "A long literal can have a suffix of 'L' or 'l', but the former is less " + "likely to be confused with a '1' in most fonts.", - disableable = true, category = JDK, severity = ERROR, maturity = MATURE) + category = JDK, severity = ERROR, maturity = MATURE) public class LongLiteralLowerCaseSuffix extends BugChecker implements LiteralTreeMatcher { private static final Matcher matcher = new Matcher() { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NarrowingCompoundAssignment.java b/core/src/main/java/com/google/errorprone/bugpatterns/NarrowingCompoundAssignment.java index 76d9e6e9587..4d35f1c62c8 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/NarrowingCompoundAssignment.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NarrowingCompoundAssignment.java @@ -55,7 +55,7 @@ + " For example, 'byte b = 0; b = b << 1;' does not compile, but 'byte b = 0; b <<= 1;'" + " does!\n\n" + " (See Puzzle #9 in 'Java Puzzlers: Traps, Pitfalls, and Corner Cases'.)", - category = JDK, severity = ERROR, maturity = MATURE, disableable = true) + category = JDK, severity = ERROR, maturity = MATURE) public class NarrowingCompoundAssignment extends BugChecker implements CompoundAssignmentTreeMatcher { diff --git a/core/src/main/java/com/google/errorprone/scanner/ScannerSupplier.java b/core/src/main/java/com/google/errorprone/scanner/ScannerSupplier.java index c7484c3944a..7094ad3b084 100644 --- a/core/src/main/java/com/google/errorprone/scanner/ScannerSupplier.java +++ b/core/src/main/java/com/google/errorprone/scanner/ScannerSupplier.java @@ -156,7 +156,7 @@ public ScannerSupplier applyOverrides(ErrorProneOptions errorProneOptions) } switch (entry.getValue()) { case OFF: - if (!supplier.disableable()) { + if (!supplier.suppressibility().disableable()) { throw new InvalidCommandLineOptionException( supplier.canonicalName() + " may not be disabled"); } @@ -168,7 +168,7 @@ public ScannerSupplier applyOverrides(ErrorProneOptions errorProneOptions) case WARN: // Demoting an enabled check from an error to a warning is a form of disabling if (enabledChecks.contains(supplier) - && !supplier.disableable() + && !supplier.suppressibility().disableable() && supplier.severity() == SeverityLevel.ERROR) { throw new InvalidCommandLineOptionException(supplier.canonicalName() + " is not disableable and may not be demoted to a warning"); diff --git a/core/src/test/java/com/google/errorprone/CommandLineFlagTest.java b/core/src/test/java/com/google/errorprone/CommandLineFlagTest.java index 1febe1f983c..a667cdff1ff 100644 --- a/core/src/test/java/com/google/errorprone/CommandLineFlagTest.java +++ b/core/src/test/java/com/google/errorprone/CommandLineFlagTest.java @@ -21,6 +21,7 @@ import static com.google.errorprone.BugPattern.MaturityLevel.MATURE; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.BugPattern.Suppressibility.UNSUPPRESSIBLE; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher; @@ -53,7 +54,7 @@ public class CommandLineFlagTest { @BugPattern(name = "DisableableChecker", altNames = "foo", summary = "Disableable checker that flags all return statements as errors", explanation = "Disableable checker that flags all return statements as errors", - disableable = true, category = ONE_OFF, severity = ERROR, maturity = MATURE) + category = ONE_OFF, severity = ERROR, maturity = MATURE) private static class DisableableChecker extends BugChecker implements ReturnTreeMatcher { @Override public Description matchReturn(ReturnTree tree, VisitorState state) { @@ -64,7 +65,7 @@ public Description matchReturn(ReturnTree tree, VisitorState state) { @BugPattern(name = "NondisableableChecker", summary = "NondisableableChecker checker that flags all return statements as errors", explanation = "NondisableableChecker checker that flags all return statements as errors", - category = ONE_OFF, severity = ERROR, maturity = MATURE) + suppressibility = UNSUPPRESSIBLE, category = ONE_OFF, severity = ERROR, maturity = MATURE) private static class NondisableableChecker extends BugChecker implements ReturnTreeMatcher { @Override public Description matchReturn(ReturnTree tree, VisitorState state) { @@ -86,7 +87,7 @@ public Description matchReturn(ReturnTree tree, VisitorState state) { @BugPattern(name = "ErrorChecker", summary = "Checker that flags all return statements as errors", explanation = "Checker that flags all return statements as errors", - category = ONE_OFF, severity = ERROR, maturity = MATURE, disableable = true) + category = ONE_OFF, severity = ERROR, maturity = MATURE) private static class ErrorChecker extends BugChecker implements ReturnTreeMatcher { @Override public Description matchReturn(ReturnTree tree, VisitorState state) { diff --git a/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java b/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java index e9f7c33ff19..73f602ca6de 100644 --- a/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java +++ b/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java @@ -17,6 +17,10 @@ package com.google.errorprone; import static com.google.common.truth.Truth.assertThat; +import static com.google.errorprone.BugPattern.Category.JDK; +import static com.google.errorprone.BugPattern.MaturityLevel.MATURE; +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.BugPattern.Suppressibility.UNSUPPRESSIBLE; import static com.google.errorprone.DiagnosticTestHelper.diagnosticMessage; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasItem; @@ -26,7 +30,9 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; +import com.google.errorprone.bugpatterns.ArrayEquals; import com.google.errorprone.bugpatterns.ArrayEqualsTest; import com.google.errorprone.bugpatterns.BadShiftAmount; import com.google.errorprone.bugpatterns.BugChecker; @@ -230,6 +236,12 @@ public void testBadFlagThrowsException() throws Exception { } } + @BugPattern(name = "ArrayEquals", + summary = "Reference equality used to compare arrays", + explanation = "", category = JDK, severity = ERROR, maturity = MATURE, + suppressibility = UNSUPPRESSIBLE) + public static class UnsuppressibleArrayEquals extends ArrayEquals {} + @Test public void testCantDisableNonDisableableCheck() throws Exception { try { @@ -237,7 +249,7 @@ public void testCantDisableNonDisableableCheck() throws Exception { ArrayEqualsTest.class, Arrays.asList("ArrayEqualsPositiveCases.java"), Arrays.asList("-Xep:ArrayEquals:OFF"), - Collections.>emptyList()); + ImmutableList.>of(UnsuppressibleArrayEquals.class)); fail(); } catch (RuntimeException expected) { assertThat(expected.getMessage()).contains("ArrayEquals may not be disabled"); 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 48b8fa6ed8d..832fd98767d 100644 --- a/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java +++ b/core/src/test/java/com/google/errorprone/scanner/ScannerSupplierTest.java @@ -28,6 +28,7 @@ import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugCheckerSupplier; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.ErrorProneJavaCompilerTest; import com.google.errorprone.ErrorProneOptions; import com.google.errorprone.InvalidCommandLineOptionException; import com.google.errorprone.bugpatterns.ArrayEquals; @@ -184,7 +185,8 @@ public void applyOverridesDisablesChecks() throws Exception { // Calling ScannerSupplier.applyOverrides() just to make sure it throws the right exception @SuppressWarnings("CheckReturnValue") public void applyOverridesThrowsExceptionWhenDisablingNonDisablableCheck() throws Exception { - ScannerSupplier ss = ScannerSupplier.fromBugCheckers(new ArrayEquals()); + ScannerSupplier ss = ScannerSupplier.fromBugCheckers( + new ErrorProneJavaCompilerTest.UnsuppressibleArrayEquals()); ErrorProneOptions epOptions = ErrorProneOptions.processArgs( ImmutableList.of("-Xep:ArrayEquals:OFF")); @@ -200,7 +202,8 @@ public void applyOverridesThrowsExceptionWhenDisablingNonDisablableCheck() throw // Calling ScannerSupplier.applyOverrides() just to make sure it throws the right exception @SuppressWarnings("CheckReturnValue") public void applyOverridesThrowsExceptionWhenDemotingNonDisablableCheck() throws Exception { - ScannerSupplier ss = ScannerSupplier.fromBugCheckers(new ArrayEquals()); + ScannerSupplier ss = ScannerSupplier.fromBugCheckers( + new ErrorProneJavaCompilerTest.UnsuppressibleArrayEquals()); ErrorProneOptions epOptions = ErrorProneOptions.processArgs( ImmutableList.of("-Xep:ArrayEquals:WARN"));