Stop the Xplugin integration from infinitely recursing into compilesW…

…ithFix RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=218100949
google · Nov 2, 2018 · 02cebcd · 02cebcd
1 parent dd4e326
commit 02cebcd
Show file tree

Hide file tree

Showing 2 changed files with 84 additions and 3 deletions.
diff --git a/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java b/check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java
@@ -789,20 +789,26 @@ public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOExcept
     Context context = new Context();
     Options.instance(context).putAll(Options.instance(javacTask.getContext()));
     context.put(Arguments.class, arguments);
+    // When using the -Xplugin Error Prone integration, disable Error Prone for speculative
+    // recompiles to avoid infinite recurison.
+    ImmutableList<String> options =
+        arguments.getPluginOpts().stream().anyMatch(x -> x.iterator().next().equals("ErrorProne"))
+            ? ImmutableList.of("-Xplugin:ErrorProne -XepDisableAllChecks")
+            : ImmutableList.of();
     JavacTask newTask =
         JavacTool.create()
             .getTask(
                 CharStreams.nullWriter(),
                 state.context.get(JavaFileManager.class),
                 diagnosticListener,
-                ImmutableList.of(),
+                options,
                 arguments.getClassNames(),
                 fileObjects,
                 context);
     try {
       newTask.analyze();
-    } catch (Throwable e) {
-      return false; // ¯\_(ツ)_/¯
+    } catch (IOException e) {
+      throw new UncheckedIOException(e);
     }
     return countErrors(diagnosticListener) == 0;
   }

diff --git a/core/src/test/java/com/google/errorprone/ErrorProneJavacPluginTest.java b/core/src/test/java/com/google/errorprone/ErrorProneJavacPluginTest.java
@@ -23,15 +23,27 @@
 import static java.util.Locale.ENGLISH;
 import static org.junit.Assert.assertThrows;
 
+import com.google.auto.service.AutoService;
 import com.google.common.base.Joiner;
+import com.google.common.base.Splitter;
 import com.google.common.base.StandardSystemProperty;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Streams;
 import com.google.common.jimfs.Configuration;
 import com.google.common.jimfs.Jimfs;
+import com.google.errorprone.BugPattern.ProvidesFix;
+import com.google.errorprone.BugPattern.SeverityLevel;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.fixes.SuggestedFixes;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.ReturnTree;
 import com.sun.source.util.JavacTask;
 import com.sun.tools.javac.api.JavacTool;
 import com.sun.tools.javac.file.JavacFileManager;
 import com.sun.tools.javac.util.Context;
+import java.io.File;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.io.StringWriter;
@@ -42,6 +54,7 @@
 import javax.tools.Diagnostic;
 import javax.tools.DiagnosticCollector;
 import javax.tools.JavaFileObject;
+import javax.tools.StandardLocation;
 import org.junit.Assume;
 import org.junit.Rule;
 import org.junit.Test;
@@ -298,4 +311,66 @@ public void stopOnErrorPolicy() throws IOException {
     assertThat(diagnostics.get(0)).contains("[CollectionIncompatibleType]");
     assertThat(diagnostics.get(1)).contains("[DeadException]");
   }
+
+  /** A bugpattern for testing. */
+  @AutoService(BugChecker.class)
+  @BugPattern(
+      name = "TestCompilesWithFix",
+      summary = "",
+      severity = SeverityLevel.ERROR,
+      providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION)
+  public static class TestCompilesWithFix extends BugChecker implements ReturnTreeMatcher {
+
+    @Override
+    public Description matchReturn(ReturnTree tree, VisitorState state) {
+      // add a no-op fix to exercise compilesWithFix
+      SuggestedFix fix = SuggestedFix.postfixWith(tree, "//");
+      return SuggestedFixes.compilesWithFix(fix, state)
+          ? describeMatch(tree, fix)
+          : Description.NO_MATCH;
+    }
+  }
+
+  @Test
+  public void compilesWithFix() throws IOException {
+    FileSystem fileSystem = Jimfs.newFileSystem(Configuration.unix());
+    Path source = fileSystem.getPath("Test.java");
+    Files.write(
+        source,
+        ImmutableList.of(
+            "import java.util.HashSet;",
+            "import java.util.Set;",
+            "class Test {",
+            "  void f() {",
+            "    return;",
+            "  }",
+            "}"),
+        UTF_8);
+    JavacFileManager fileManager = new JavacFileManager(new Context(), false, UTF_8);
+    fileManager.setLocation(
+        StandardLocation.ANNOTATION_PROCESSOR_PATH,
+        Streams.stream(
+                Splitter.on(File.pathSeparatorChar)
+                    .split(StandardSystemProperty.JAVA_CLASS_PATH.value()))
+            .map(File::new)
+            .collect(toImmutableList()));
+    DiagnosticCollector<JavaFileObject> diagnosticCollector = new DiagnosticCollector<>();
+    JavacTask task =
+        JavacTool.create()
+            .getTask(
+                null,
+                fileManager,
+                diagnosticCollector,
+                ImmutableList.of(
+                    "-Xplugin:ErrorProne -XepDisableAllChecks -Xep:TestCompilesWithFix:ERROR",
+                    "-XDcompilePolicy=byfile"),
+                ImmutableList.of(),
+                fileManager.getJavaFileObjects(source));
+    assertThat(task.call()).isFalse();
+    Diagnostic<? extends JavaFileObject> diagnostic =
+        diagnosticCollector.getDiagnostics().stream()
+            .filter(d -> d.getKind() == Diagnostic.Kind.ERROR)
+            .collect(onlyElement());
+    assertThat(diagnostic.getMessage(ENGLISH)).contains("[TestCompilesWithFix]");
+  }
 }