Change javac's compilation policy in open-source Error Prone to requi…

…re that

classes in the same source file must always be at the same compiler phase, and
remove Error Prone code related to dealing with multiple top-level classes
that are at different compiler phases.

javac by default processes each class individually through all the compiler
phases.  This means that if you have multiple top-level classes in the same
source file, you cannot analyze the whole compilation unit at once since
one of the classes will have been lowered already.  This CL passes an option
to javac to tell it to group classes in the same file and process them in
lockstep.  This lets us remove complexity from Error Prone, and makes it
easier to unify with Refaster.

RELNOTES: none
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=101266905

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

@@ -25,7 +25,6 @@
 import com.google.errorprone.scanner.Scanner;
 
 import com.sun.source.tree.CompilationUnitTree;
-import com.sun.source.tree.ImportTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.util.TaskEvent;
 import com.sun.source.util.TaskEvent.Kind;
@@ -136,34 +135,18 @@ public void reportReadyForAnalysis(TaskEvent taskEvent, TreePath path, boolean h
     try {
       // Assert that the event is unique and scan the current tree.
       verify(seen.add(path.getLeaf()), "Duplicate FLOW event for: %s", taskEvent.getTypeElement());
-      errorProneScanner.scan(path, createVisitorState(path.getCompilationUnit()));
 
-      // We only get TaskEvents for compilation units if they contain no package declarations
-      // (e.g. package-info.java files). Otherwise there are events for each individual
-      // declaration. Once we've processed all of the declarations we manually start a post-order
-      // visit of the compilation unit.
-      if (path.getLeaf().getKind() != Tree.Kind.COMPILATION_UNIT
-          && finishedCompilation(path.getCompilationUnit())) {
-        CompilationUnitTree tree = path.getCompilationUnit();
-        VisitorState visitorState = createVisitorState(path.getCompilationUnit());
-
-
-        errorProneScanner.matchCompilationUnit(tree, visitorState);
-
-        // Manually traverse into the components of the compilation tree we are interested in, and
-        // skip type decls: top-level declarations are visited separately first, and at this point
-        // parts of the classes could be lowered away.
-        if (tree.getPackage() != null) {
-          errorProneScanner.scan(new TreePath(path, tree.getPackage()), visitorState);
-        }
-        for (ImportTree importTree : tree.getImports()) {
-          if (importTree == null) {
-            continue;
-          }
-          errorProneScanner.scan(new TreePath(path, importTree), visitorState);
-        }
+      VisitorState state = createVisitorState(path.getCompilationUnit());
+      if (path.getLeaf().getKind() == Tree.Kind.COMPILATION_UNIT) {
+        // We only get TaskEvents for compilation units if they contain no package declarations
+        // (e.g. package-info.java files).  In this case it's safe to analyze the
+        // CompilationUnitTree immediately.
+        errorProneScanner.scan(path, state);
+      } else if (finishedCompilation(path.getCompilationUnit())) {
+        // Otherwise this TaskEvent is for a ClassTree, and we can scan the whole
+        // CompilationUnitTree once we've seen all the enclosed classes.
+        errorProneScanner.scan(new TreePath(path.getCompilationUnit()), state);
       }
-
     } catch (CompletionFailure e) {
       // A CompletionFailure can be triggered when error-prone tries to complete a symbol
       // that isn't on the compilation classpath. This can occur when a check performs an

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

@@ -19,7 +19,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.StandardSystemProperty.JAVA_SPECIFICATION_VERSION;
 
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.errorprone.scanner.BuiltInCheckerSuppliers;
 import com.google.errorprone.scanner.Scanner;
 import com.google.errorprone.scanner.ScannerSupplier;
@@ -163,7 +163,7 @@ public Result run(String[] args) {
    *
    * <p>This prevents, e.g., targeting Java 8 by default when using error-prone on JDK7.
    */
-  private List<String> defaultToLatestSupportedLanguageLevel(String[] argv) {
+  private Iterable<String> defaultToLatestSupportedLanguageLevel(Iterable<String> args) {
 
     String overrideLanguageLevel;
     switch (JAVA_SPECIFICATION_VERSION.value()) {
@@ -174,23 +174,50 @@ private List<String> defaultToLatestSupportedLanguageLevel(String[] argv) {
         overrideLanguageLevel = "8";
         break;
       default:
-        return ImmutableList.copyOf(argv);
+        return args;
     }
 
-    return ImmutableList.<String>builder()
-        // suppress xlint 'options' warnings to avoid diagnostics like:
-        // 'bootstrap class path not set in conjunction with -source 1.7'
-        .add("-Xlint:-options")
-        .add("-source").add(overrideLanguageLevel)
-        .add("-target").add(overrideLanguageLevel)
-        .add(argv)
-        .build();
+    return Iterables.concat(
+        Arrays.asList(
+          // suppress xlint 'options' warnings to avoid diagnostics like:
+          // 'bootstrap class path not set in conjunction with -source 1.7'
+          "-Xlint:-options",
+          "-source", overrideLanguageLevel,
+          "-target", overrideLanguageLevel),
+        args);
+  }
+
+  /**
+   * Sets javac's {@code -XDcompilePolicy} flag to {@code byfile}.  This ensures that all classes in
+   * a file are attributed before any of them are lowered.  Error Prone depends on this behavior
+   * when analyzing files that contain multiple classes.
+   *
+   * @throws InvalidCommandLineOptionException if the {@code -XDcompilePolicy} flag is passed
+   * in the existing arguments with a value other than {@code byfile}
+   */
+  private Iterable<String> setCompilePolicyToByFile(Iterable<String> args)
+      throws InvalidCommandLineOptionException {
+    for (String arg : args) {
+      if (arg.startsWith("-XDcompilePolicy")) {
+        String value = arg.substring(arg.indexOf('=') + 1);
+        if (!value.equals("byfile")) {
+          throw new InvalidCommandLineOptionException(
+              "-XDcompilePolicy must be byfile for Error Prone to work properly");
+        }
+        // If there is already an "-XDcompilePolicy=byfile" flag, don't do anything.
+        return args;
+      }
+    }
+    return Iterables.concat(
+        args,
+        Arrays.asList("-XDcompilePolicy=byfile"));
   }
 
   private String[] prepareCompilation(String[] argv, Context context)
       throws InvalidCommandLineOptionException {
 
-    List<String> newArgs = defaultToLatestSupportedLanguageLevel(argv);
+    Iterable<String> newArgs = defaultToLatestSupportedLanguageLevel(Arrays.asList(argv));
+    newArgs = setCompilePolicyToByFile(newArgs);
     ErrorProneOptions epOptions = ErrorProneOptions.processArgs(newArgs);
 
     argv = epOptions.getRemainingArgs();

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

@@ -64,15 +64,15 @@ public SuppressionHelper(
   }
 
   /**
-   * Used for the return type of {@code handleSuppressions()}.  Either field may be null, which
+   * Container for information about suppressions.  Either reference field may be null, which
    * indicates that the suppression sets are unchanged.
    */
-  public static class NewSuppressions {
+  public static class SuppressionInfo {
     public Set<String> suppressWarningsStrings;
     public Set<Class<? extends Annotation>> customSuppressions;
     public boolean inGeneratedCode;
 
-    public NewSuppressions(
+    public SuppressionInfo(
         Set<String> suppressWarningsStrings,
         Set<Class<? extends Annotation>> customSuppressions,
         boolean inGeneratedCode) {
@@ -99,7 +99,7 @@ public NewSuppressions(
    *        annotations on the current path through the AST
    * @param customSuppressionsOnCurrentPath The set of all custom suppression annotations
    */
-  public NewSuppressions extendSuppressionSets(
+  public SuppressionInfo extendSuppressionSets(
       Symbol sym,
       Type suppressWarningsType,
       Set<String> suppressionsOnCurrentPath,
@@ -148,7 +148,7 @@ public NewSuppressions extendSuppressionSets(
       }
     }
 
-    return new NewSuppressions(newSuppressions, newCustomSuppressions, newInGeneratedCode);
+    return new SuppressionInfo(newSuppressions, newCustomSuppressions, newInGeneratedCode);
   }
 
   /**

core/src/main/java/com/google/errorprone/analysis/BugCheckerLocalAnalysis.java

@@ -32,7 +32,6 @@
 import com.google.errorprone.bugpatterns.BugChecker.CaseTreeMatcher;
 import com.google.errorprone.bugpatterns.BugChecker.CatchTreeMatcher;
 import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
-import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeInfo;
 import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
 import com.google.errorprone.bugpatterns.BugChecker.CompoundAssignmentTreeMatcher;
 import com.google.errorprone.bugpatterns.BugChecker.ConditionalExpressionTreeMatcher;
@@ -205,7 +204,7 @@ public Void visitAnnotation(AnnotationTree node, VisitorState state) {
   public Void visitCompilationUnit(CompilationUnitTree node, VisitorState state) {
     if (checker instanceof CompilationUnitTreeMatcher) {
       CompilationUnitTreeMatcher matcher = (CompilationUnitTreeMatcher) checker;
-      report(matcher.matchCompilationUnit(CompilationUnitTreeInfo.create(node), state), state);
+      report(matcher.matchCompilationUnit(node, state), state);
     }
     return null;
   }

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

@@ -18,10 +18,7 @@
 
 import static com.google.common.base.MoreObjects.firstNonNull;
 
-import com.google.auto.value.AutoValue;
-import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
 import com.google.errorprone.BugCheckerInfo;
 import com.google.errorprone.BugPattern.MaturityLevel;
 import com.google.errorprone.BugPattern.SeverityLevel;
@@ -51,7 +48,6 @@
 import com.sun.source.tree.EmptyStatementTree;
 import com.sun.source.tree.EnhancedForLoopTree;
 import com.sun.source.tree.ExpressionStatementTree;
-import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.ForLoopTree;
 import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.IfTree;
@@ -87,14 +83,11 @@
 
 import java.io.Serializable;
 import java.lang.annotation.Annotation;
-import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 
 import javax.annotation.CheckReturnValue;
-import javax.lang.model.element.Modifier;
-import javax.tools.JavaFileObject;
 
 /**
  * A base class for implementing bug checkers. The {@code BugChecker} supplies a Scanner
@@ -155,6 +148,7 @@ public static Description.Builder buildDescriptionFromChecker(Tree node, BugChec
         checker.message());
   }
 
+  @Override
   public String canonicalName() {
     return info.canonicalName();
   }
@@ -242,72 +236,8 @@ public static interface ClassTreeMatcher extends Suppressible {
     Description matchClass(ClassTree tree, VisitorState state);
   }
 
-  /**
-   * The information that is safe for a {@link CompilationUnitTreeMatcher} to access.
-   *
-   * <p>Error-prone does not support matching entire compilation unit trees, due to a limitation of
-   * javac. Class declarations must be inspected one at a time via {@link ClassTreeMatcher}.
-   *
-   * <p>CAUTION: checks can still access the compilation unit tree using
-   * {@link VisitorState#getPath()}, but the AST nodes for type declarations may be in an
-   * inconsistent state.
-   */
-  @AutoValue
-  public abstract static class CompilationUnitTreeInfo {
-
-    /**
-     * Information about the top-level types in a compilation unit.
-     */
-    @AutoValue
-    public abstract static class DeclarationInfo {
-      public abstract String name();
-      public abstract Tree.Kind kind();
-      public abstract Set<Modifier> modifiers();
-
-      public static DeclarationInfo create(String name, Tree.Kind kind, Set<Modifier> modifiers) {
-        return new AutoValue_BugChecker_CompilationUnitTreeInfo_DeclarationInfo(
-            name, kind, modifiers);
-      }
-    }
-
-    /** Wrapper for {@link CompilationUnitTree#getPackageAnnotations()}. */
-    public abstract List<? extends AnnotationTree> packageAnnotations();
-
-    /** Wrapper for {@link CompilationUnitTree#getPackageName()}. */
-    public abstract Optional<ExpressionTree> packageName();
-
-    /** Wrapper for {@link CompilationUnitTree#getImports()}. */
-    public abstract List<? extends ImportTree> imports();
-
-    /** Wrapper for {@link CompilationUnitTree#getTypeDecls()}. */
-    public abstract ImmutableList<DeclarationInfo> typeDeclarations();
-
-    /** Wrapper for {@link CompilationUnitTree#getSourceFile()}. */
-    public abstract JavaFileObject sourceFile();
-
-    public static CompilationUnitTreeInfo create(final CompilationUnitTree node) {
-      final ImmutableList.Builder<DeclarationInfo> members = ImmutableList.builder();
-      for (Tree tree : node.getTypeDecls()) {
-        if (tree instanceof ClassTree) {
-          ClassTree classTree = (ClassTree) tree;
-          members.add(
-              DeclarationInfo.create(
-                  classTree.getSimpleName().toString(),
-                  classTree.getKind(),
-                  classTree.getModifiers().getFlags()));
-        }
-      }
-      return new AutoValue_BugChecker_CompilationUnitTreeInfo(
-          node.getPackageAnnotations(),
-          Optional.fromNullable(node.getPackageName()),
-          node.getImports(),
-          members.build(),
-          node.getSourceFile());
-    }
-  }
-
   public static interface CompilationUnitTreeMatcher extends Suppressible {
-    Description matchCompilationUnit(CompilationUnitTreeInfo info, VisitorState state);
+    Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state);
   }
 
   public static interface CompoundAssignmentTreeMatcher extends Suppressible {

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

@@ -38,6 +38,7 @@
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 
+import com.sun.source.tree.CompilationUnitTree;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.MethodInvocationTree;
@@ -77,7 +78,7 @@ public final class ChainingConstructorIgnoresParameter extends BugChecker
   private final Multimap<MethodSymbol, Caller> callersToEvaluate = ArrayListMultimap.create();
 
   @Override
-  public Description matchCompilationUnit(CompilationUnitTreeInfo info, VisitorState state) {
+  public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
     /*
      * Clear the collections to save memory. (I wonder if it also helps to handle weird cases when a
      * class has multiple definitions. But I would expect for multiple definitions within the same

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

@@ -24,10 +24,13 @@
 import com.google.common.io.Files;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
-import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeInfo.DeclarationInfo;
 import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
 import com.google.errorprone.matchers.Description;
 
+import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.CompilationUnitTree;
+import com.sun.source.tree.Tree;
+
 import java.util.ArrayList;
 import java.util.List;
 
@@ -49,36 +52,30 @@
 public class ClassName extends BugChecker implements CompilationUnitTreeMatcher {
 
   @Override
-  public Description matchCompilationUnit(CompilationUnitTreeInfo info, VisitorState state) {
-    if (info.typeDeclarations().isEmpty() || !info.packageName().isPresent()) {
+  public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
+    if (tree.getTypeDecls().isEmpty() || tree.getPackageName() == null) {
       return Description.NO_MATCH;
     }
-    String filename = Files.getNameWithoutExtension(info.sourceFile().getName());
+    String filename = Files.getNameWithoutExtension(tree.getSourceFile().getName());
     List<String> names = new ArrayList<>();
-    for (DeclarationInfo member : info.typeDeclarations()) {
-      switch (member.kind()) {
-        case CLASS:
-        case INTERFACE:
-        case ANNOTATION_TYPE:
-        case ENUM:
-          if (member.name().equals(filename)) {
-            return Description.NO_MATCH;
-          }
-          if (member.modifiers().contains(Modifier.PUBLIC)) {
-            // If any of the top-level types are public, javac will complain
-            // if the filename doesn't match. We don't want to double-report
-            // the error.
-            return Description.NO_MATCH;
-          }
-          names.add(member.name());
-          break;
-        default:
-          break;
+    for (Tree member : tree.getTypeDecls()) {
+      if (member instanceof ClassTree) {
+        ClassTree classMember = (ClassTree) member;
+        if (classMember.getSimpleName().toString().equals(filename)) {
+          return Description.NO_MATCH;
+        }
+        if (classMember.getModifiers().getFlags().contains(Modifier.PUBLIC)) {
+          // If any of the top-level types are public, javac will complain
+          // if the filename doesn't match. We don't want to double-report
+          // the error.
+          return Description.NO_MATCH;
+        }
+        names.add(classMember.getSimpleName().toString());
       }
     }
     String message = String.format(
         "Expected a class declaration named %s inside %s.java, instead found: %s",
         filename, filename, Joiner.on(", ").join(names));
-    return buildDescription(info.packageName().get()).setMessage(message).build();
+    return buildDescription(tree.getPackageName()).setMessage(message).build();
   }
 }