Make BugCheckers immutable.

Severity override is now accomplished by keeping a separate immutable map
between checkers and severity levels.

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

annotation/src/main/java/com/google/errorprone/BugPattern.java

@@ -121,12 +121,22 @@ public enum Category {
   SeverityLevel severity();
 
   public enum SeverityLevel {
-    ERROR,
-    WARNING,
+    ERROR(true),
+    WARNING(true),
     /**
      * Should not be used for general code.
      */
-    NOT_A_PROBLEM
+    NOT_A_PROBLEM(false);
+
+    private final boolean enabled;
+
+    SeverityLevel(boolean enabled) {
+      this.enabled = enabled;
+    }
+
+    public boolean enabled() {
+      return enabled;
+    }
   }
 
   MaturityLevel maturity();

core/src/main/java/com/google/errorprone/BugCheckerSupplier.java

@@ -22,6 +22,7 @@
 import com.google.errorprone.BugPattern.Suppressibility;
 import com.google.errorprone.bugpatterns.BugChecker;
 
+import java.util.Map;
 import java.util.Objects;
 
 /**
@@ -59,21 +60,21 @@ public static BugCheckerSupplier fromInstance(BugChecker checker) {
   public abstract String canonicalName();
 
   /**
-   * The {@link SeverityLevel} of the {@link BugChecker}, e.g. {@link SeverityLevel#ERROR}.
+   * The default {@link SeverityLevel} of the {@link BugChecker}, e.g. {@link SeverityLevel#ERROR}.
    */
-  public abstract SeverityLevel severity();
-
+  public abstract SeverityLevel defaultSeverity();
+  
   /**
    * 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.
+   * The per-compilation {@link SeverityLevel} of the {@link BugChecker}, e.g.
+   * {@link SeverityLevel#ERROR}.
    */
-  public abstract BugCheckerSupplier overrideSeverity(SeverityLevel severity);
+  public abstract SeverityLevel severity(Map<String, SeverityLevel> severityMap);
 
   /**
    * The {@link MaturityLevel} of the {@link BugChecker}, e.g. {@link MaturityLevel#EXPERIMENTAL}.
@@ -85,7 +86,7 @@ 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.defaultSeverity().equals(that.defaultSeverity())
           && this.maturity().equals(that.maturity())
           && this.suppressibility().equals(that.suppressibility());
     }
@@ -94,6 +95,6 @@ public boolean equals(Object obj) {
 
   @Override
   public int hashCode() {
-    return Objects.hash(canonicalName(), severity(), maturity(), suppressibility());
+    return Objects.hash(canonicalName(), defaultSeverity(), maturity(), suppressibility());
   }
 }

core/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java

@@ -179,6 +179,6 @@ private VisitorState createVisitorState(CompilationUnitTree compilation) {
         log,
         ((JCCompilationUnit) compilation).endPositions,
         compilation.getSourceFile());
-    return new VisitorState(context, logReporter);
+    return new VisitorState(context, logReporter, errorProneScanner.severityMap());
   }
 }

core/src/main/java/com/google/errorprone/FromClassBugCheckerSupplier.java

@@ -22,6 +22,8 @@
 import com.google.errorprone.BugPattern.Suppressibility;
 import com.google.errorprone.bugpatterns.BugChecker;
 
+import java.util.Map;
+
 /**
  * A {@link BugCheckerSupplier} that supplies {@link BugChecker}s given a class.
  */
@@ -56,11 +58,9 @@ private FromClassBugCheckerSupplier(Class<? extends BugChecker> checkerClass,
   @Override
   public BugChecker get() {
     try {
-      BugChecker checker = checkerClass.newInstance();
-      checker.setSeverity(severity);
-      return checker;
+      return checkerClass.newInstance();
     } catch (ReflectiveOperationException e) {
-      throw new RuntimeException(
+      throw new LinkageError(
           "Could not reflectively create error prone checker " + checkerClass.getName(), e);
     }
   }
@@ -69,16 +69,15 @@ public BugChecker get() {
   public String canonicalName() {
     return canonicalName;
   }
-
+  
   @Override
-  public SeverityLevel severity() {
-    return severity;
+  public SeverityLevel severity(Map<String, SeverityLevel> severityMap) {
+    return severityMap.get(canonicalName);
   }
 
   @Override
-  public BugCheckerSupplier overrideSeverity(SeverityLevel severity) {
-    return new FromClassBugCheckerSupplier(
-        this.checkerClass, this.canonicalName, severity, this.maturity, this.suppressibility);
+  public SeverityLevel defaultSeverity() {
+    return severity;
   }
 
   @Override

core/src/main/java/com/google/errorprone/FromInstanceBugCheckerSupplier.java

@@ -22,35 +22,21 @@
 import com.google.errorprone.BugPattern.Suppressibility;
 import com.google.errorprone.bugpatterns.BugChecker;
 
+import java.util.Map;
+
 /**
  * A {@link BugCheckerSupplier} that supplies the {@link BugChecker} instance that was passed
  * in its constructor.
  */
 class FromInstanceBugCheckerSupplier extends BugCheckerSupplier {
   private final BugChecker checker;
-  private final SeverityLevel severity;
 
   FromInstanceBugCheckerSupplier(BugChecker checker) {
-    this(Preconditions.checkNotNull(checker), checker.severity());
-  }
-
-  private FromInstanceBugCheckerSupplier(BugChecker checker, SeverityLevel severity) {
-    this.checker = checker;
-    this.severity = severity;
+    this.checker = Preconditions.checkNotNull(checker);
   }
 
   @Override
   public BugChecker get() {
-    /* Note that we mutate the severity of the BugChecker instance here, which is not ideal.
-     * Ideally BugChecker instances would be immutable, and we would ask for a copy of this
-     * BugChecker with a different severity.
-     *
-     * Instead, we make BugCheckerSupplier and ScannerSupplier immutable. When we process severity
-     * overrides, we do not store a reference to the BugCheckerSupplier with the overridden
-     * severity; we keep only the original BugCheckerSupplier.  This ensures that the default
-     * severity is used for subsequent compilations.
-     */
-    checker.setSeverity(severity);
     return checker;
   }
 
@@ -60,13 +46,13 @@ public String canonicalName() {
   }
 
   @Override
-  public SeverityLevel severity() {
-    return severity;
+  public SeverityLevel defaultSeverity() {
+    return checker.defaultSeverity();
   }
-
+  
   @Override
-  public BugCheckerSupplier overrideSeverity(SeverityLevel severity) {
-    return new FromInstanceBugCheckerSupplier(this.checker, severity);
+  public SeverityLevel severity(Map<String, SeverityLevel> severityMap) {
+    return checker.severity(severityMap);
   }
 
   @Override

core/src/main/java/com/google/errorprone/VisitorState.java

@@ -16,6 +16,8 @@
 
 package com.google.errorprone;
 
+import com.google.common.collect.ImmutableMap;
+import com.google.errorprone.BugPattern.SeverityLevel;
 import com.google.errorprone.matchers.Description;
 
 import com.sun.source.tree.Tree;
@@ -38,6 +40,8 @@
 import com.sun.tools.javac.util.Names;
 
 import java.io.IOException;
+import java.util.Collections;
+import java.util.Map;
 
 /**
  * @author alexeagle@google.com (Alex Eagle)
@@ -47,6 +51,7 @@ public class VisitorState {
   private final DescriptionListener descriptionListener;
   public final Context context;
   private final TreePath path;
+  private final Map<String, SeverityLevel> severityMap;
 
   // The default no-op implementation of DescriptionListener. We use this instead of null so callers
   // of getDescriptionListener() don't have to do null-checking.
@@ -55,22 +60,28 @@ public class VisitorState {
   };
 
   public VisitorState(Context context) {
-    this(context, null, NULL_LISTENER);
+    this(context, null, NULL_LISTENER, ImmutableMap.<String, SeverityLevel>of());
   }
 
   public VisitorState(Context context, DescriptionListener listener) {
-    this(context, null, listener);
+    this(context, null, listener, Collections.<String, SeverityLevel>emptyMap());
+  }
+
+  public VisitorState(Context context, DescriptionListener listener,
+      Map<String, SeverityLevel> severityMap) {
+    this(context, null, listener, severityMap);
   }
 
   private VisitorState(Context context, TreePath path,
-      DescriptionListener descriptionListener) {
+      DescriptionListener descriptionListener, Map<String, SeverityLevel> severityMap) {
     this.context = context;
     this.path = path;
     this.descriptionListener = descriptionListener;
+    this.severityMap = severityMap;
   }
 
   public VisitorState withPath(TreePath path) {
-    return new VisitorState(context, path, descriptionListener);
+    return new VisitorState(context, path, descriptionListener, severityMap);
   }
 
   public TreePath getPath() {
@@ -89,8 +100,17 @@ public Symtab getSymtab() {
     return Symtab.instance(context);
   }
 
-  public DescriptionListener getDescriptionListener() {
-    return descriptionListener;
+  public void reportMatch(Description description) {
+    // TODO(user): creating Descriptions with the default severity and updating them here isn't
+    // ideal (we could forget to do the update), so consider removing severity from Description.
+    // Instead, there could be another method on the listener that took a description and a
+    // (separate) SeverityLevel. Adding the method to the interface would require updating the
+    // existing implementations, though. Wait for default methods?
+    SeverityLevel override = severityMap.get(description.checkName);
+    if (override != null) {
+      description = description.applySeverityOverride(override);
+    }
+    descriptionListener.onDescribed(description);
   }
 
   public Name getName(String nameStr) {

core/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java

@@ -16,6 +16,8 @@
 
 package com.google.errorprone.bugpatterns;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
+
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.BugPattern.MaturityLevel;
@@ -84,6 +86,7 @@
 import java.io.Serializable;
 import java.lang.annotation.Annotation;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 /**
@@ -117,10 +120,9 @@ public abstract class BugChecker implements Suppressible, Serializable {
   private final String message;
 
   /**
-   * The type of diagnostic (error or warning) to emit when this check triggers.  Initialized to
-   * the {@code severity} attribute from its {@code BugPattern}, but may be overridden.
+   * The default type of diagnostic (error or warning) to emit when this check triggers.
    */
-  private SeverityLevel severity;
+  private final SeverityLevel defaultSeverity;
 
   /**
    * The maturity of this checker.  Used to decide whether to enable this check.  Corresponds to
@@ -163,7 +165,7 @@ public BugChecker() {
         .build();
     message = pattern.summary();
     maturity = pattern.maturity();
-    severity = pattern.severity();
+    defaultSeverity = pattern.severity();
     linkUrl = createLinkUrl(pattern);
     suppressibility = pattern.suppressibility();
     if (suppressibility == Suppressibility.CUSTOM_ANNOTATION) {
@@ -233,12 +235,12 @@ public MaturityLevel maturity() {
     return maturity;
   }
 
-  public SeverityLevel severity() {
-    return severity;
+  public SeverityLevel defaultSeverity() {
+    return defaultSeverity;
   }
-
-  public void setSeverity(SeverityLevel severity) {
-    this.severity = severity;
+  
+  public SeverityLevel severity(Map<String, SeverityLevel> severities) {
+    return firstNonNull(severities.get(canonicalName), defaultSeverity);
   }
 
   public String linkUrl() {

core/src/main/java/com/google/errorprone/bugpatterns/ChainingConstructorIgnoresParameter.java

@@ -195,12 +195,7 @@ private static Map<String, Type> indexTypeByName(List<? extends VariableTree> pa
 
   private void reportMatch(
       Tree diagnosticPosition, VisitorState state, Tree toReplace, String replaceWith) {
-    report(describeMatch(diagnosticPosition, replace(toReplace, replaceWith)), state);
-  }
-
-  // Cloned from Scanner. This also appears in ThreadSafe. See the TODO there.
-  private void report(Description description, VisitorState state) {
-    state.getDescriptionListener().onDescribed(description);
+    state.reportMatch(describeMatch(diagnosticPosition, replace(toReplace, replaceWith)));
   }
 
   private static boolean referencesIdentifierWithName(

core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/GuardedByChecker.java

@@ -134,6 +134,6 @@ private void report(Description description, VisitorState state) {
     if (description == null || description == Description.NO_MATCH) {
       return;
     }
-    state.getDescriptionListener().onDescribed(description);
+    state.reportMatch(description);
   }
 }