Remove the 'disableable' attribute from @BugPattern.

There's usually not a good reason to make checks undisableable, and it
interferes with clean-up. Checks that should be undisableable (e.g. for
security reasons) are already unsuppressible.

RELNOTES: none
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=84155911

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.
-   *
-   * <p>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;
+    }
   }
 
   /**

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,25 +80,20 @@ 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) {
       BugCheckerSupplier that = (BugCheckerSupplier) 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());
   }
 }

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<? extends BugChecker> 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<? extends BugChecker> 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

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();
   }
 }

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;

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 "

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 {
 

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<LiteralTree> matcher = new Matcher<LiteralTree>() {

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 {
 

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");

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) {

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,14 +236,20 @@ 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 {
       doCompile(
           ArrayEqualsTest.class,
           Arrays.asList("ArrayEqualsPositiveCases.java"),
           Arrays.asList("-Xep:ArrayEquals:OFF"),
-          Collections.<Class<? extends BugChecker>>emptyList());
+          ImmutableList.<Class<? extends BugChecker>>of(UnsuppressibleArrayEquals.class));
       fail();
     } catch (RuntimeException expected) {
       assertThat(expected.getMessage()).contains("ArrayEquals may not be disabled");

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"));