Prevent regression

google · Dec 30, 2024 · 205c5a7 · 205c5a7
1 parent d7cd72f
commit 205c5a7
Show file tree

Hide file tree

Showing 3 changed files with 121 additions and 60 deletions.
diff --git a/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java b/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java
@@ -24,6 +24,7 @@
 import com.google.common.base.Suppliers;
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.BugPattern.SeverityLevel;
+import com.google.errorprone.ErrorProneOptions.Severity;
 import com.google.errorprone.RefactoringCollection.RefactoringResult;
 import com.google.errorprone.scanner.ErrorProneScannerTransformer;
 import com.google.errorprone.scanner.ScannerSupplier;
@@ -79,14 +80,18 @@ public static ErrorProneAnalyzer createAnalyzer(
             .or(
                 Suppliers.memoize(
                     () -> {
-                      ScannerSupplier toUse =
-                          ErrorPronePlugins.loadPlugins(scannerSupplier, context)
-                              .applyOverrides(epOptions);
                       ImmutableSet<String> namedCheckers =
                           epOptions.patchingOptions().namedCheckers();
-                      if (!namedCheckers.isEmpty()) {
-                        toUse = toUse.filter(bci -> namedCheckers.contains(bci.canonicalName()));
-                      }
+                      ScannerSupplier toUse =
+                          ErrorPronePlugins.loadPlugins(scannerSupplier, context)
+                              .applyOverrides(epOptions)
+                              .filter(
+                                  bci -> {
+                                    String name = bci.canonicalName();
+                                    return epOptions.getSeverityMap().get(name) != Severity.OFF
+                                        && (namedCheckers.isEmpty()
+                                            || namedCheckers.contains(name));
+                                  });
                       return ErrorProneScannerTransformer.create(toUse.get());
                     }));
 

diff --git a/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java b/core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java
@@ -434,34 +434,108 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
   }
 
   @Test
-  public void patchWithBugPatternCustomization() throws IOException {
-    // Patching modifies files on disk, so we must create an actual file that matches the
-    // `SimpleJavaFileObject` defined below.
-    Path location = tempDir.getRoot().toPath().resolve("StringConstantWrapper.java");
-    String source =
-        """
-        class StringConstantWrapper {
-          String s = "old-value";
-        }
-        """;
-    Files.writeString(location, source);
+  public void patchAll() throws IOException {
+    JavaFileObject fileObject =
+        createOnDiskFileObject(
+            "StringConstantWrapper.java",
+            """
+            class StringConstantWrapper {
+              String s = "old-value";
+            }
+            """);
+
+    CompilationResult result =
+        doCompile(
+            Collections.singleton(fileObject),
+            Arrays.asList("-XepPatchChecks:", "-XepPatchLocation:IN_PLACE"),
+            Collections.singletonList(AssignmentUpdater.class));
+    assertThat(result.succeeded).isTrue();
+    assertThat(Files.readString(Path.of(fileObject.toUri())))
+        .isEqualTo(
+            """
+            class StringConstantWrapper {
+              String s = "flag-not-set";
+            }
+            """);
+  }
+
+  @Test
+  public void patchAllWithCheckDisabled() throws IOException {
+    JavaFileObject fileObject =
+        createOnDiskFileObject(
+            "StringConstantWrapper.java",
+            """
+            class StringConstantWrapper {
+              String s = "old-value";
+            }
+            """);
 
     CompilationResult result =
         doCompile(
-            Collections.singleton(
-                new SimpleJavaFileObject(location.toUri(), SimpleJavaFileObject.Kind.SOURCE) {
-                  @Override
-                  public CharSequence getCharContent(boolean ignoreEncodingErrors) {
-                    return source;
-                  }
-                }),
+            Collections.singleton(fileObject),
+            Arrays.asList(
+                "-XepPatchChecks:", "-XepPatchLocation:IN_PLACE", "-Xep:AssignmentUpdater:OFF"),
+            Collections.singletonList(AssignmentUpdater.class));
+    assertThat(result.succeeded).isTrue();
+    assertThat(Files.readString(Path.of(fileObject.toUri())))
+        .isEqualTo(
+            """
+            class StringConstantWrapper {
+              String s = "old-value";
+            }
+            """);
+  }
+
+  @Test
+  public void patchSingleWithCheckDisabled() throws IOException {
+    JavaFileObject fileObject =
+        createOnDiskFileObject(
+            "StringConstantWrapper.java",
+            """
+            class StringConstantWrapper {
+              String s = "old-value";
+            }
+            """);
+
+    CompilationResult result =
+        doCompile(
+            Collections.singleton(fileObject),
+            Arrays.asList(
+                "-XepPatchChecks:AssignmentUpdater",
+                "-XepPatchLocation:IN_PLACE",
+                "-Xep:AssignmentUpdater:OFF"),
+            Collections.singletonList(AssignmentUpdater.class));
+    assertThat(result.succeeded).isTrue();
+    assertThat(Files.readString(Path.of(fileObject.toUri())))
+        .isEqualTo(
+            """
+            class StringConstantWrapper {
+              String s = "old-value";
+            }
+            """);
+  }
+
+  @Test
+  public void patchSingleWithBugPatternCustomization() throws IOException {
+    JavaFileObject fileObject =
+        createOnDiskFileObject(
+            "StringConstantWrapper.java",
+            """
+            class StringConstantWrapper {
+              String s = "old-value";
+            }
+            """);
+
+    CompilationResult result =
+        doCompile(
+            Collections.singleton(fileObject),
             Arrays.asList(
                 "-XepPatchChecks:AssignmentUpdater",
                 "-XepPatchLocation:IN_PLACE",
                 "-XepOpt:AssignmentUpdater:NewValue=new-value"),
             Collections.singletonList(AssignmentUpdater.class));
     assertThat(result.succeeded).isTrue();
-    assertThat(Files.readString(location))
+    assertThat(Files.readString(Path.of(fileObject.toUri())))
         .isEqualTo(
             """
             class StringConstantWrapper {
@@ -470,6 +544,23 @@ class StringConstantWrapper {
             """);
   }
 
+  /**
+   * Creates a {@link JavaFileObject} with matching on-disk contents.
+   *
+   * <p>This method aids in testing patching functionality, as {@code IN_PLACE} patching requires
+   * that compiled code actually exists on disk.
+   */
+  private JavaFileObject createOnDiskFileObject(String fileName, String source) throws IOException {
+    Path location = tempDir.getRoot().toPath().resolve(fileName);
+    Files.writeString(location, source);
+    return new SimpleJavaFileObject(location.toUri(), SimpleJavaFileObject.Kind.SOURCE) {
+      @Override
+      public CharSequence getCharContent(boolean ignoreEncodingErrors) {
+        return source;
+      }
+    };
+  }
+
   private static class CompilationResult {
     public final boolean succeeded;
     public final String output;

diff --git a/core/src/test/java/com/google/errorprone/scanner/ScannerTest.java b/core/src/test/java/com/google/errorprone/scanner/ScannerTest.java
@@ -106,41 +106,6 @@ class Test {
         .doTest();
   }
 
-  @Test
-  public void dontRunPatchForDisabledChecks() {
-    compilationHelper
-        .addSourceLines(
-            "Test.java",
-            """
-            import com.google.errorprone.scanner.ScannerTest.Foo;
-
-            class Test {
-              Foo foo;
-            }
-            """)
-        .setArgs("-XepPatchLocation:IN_PLACE", "-XepPatchChecks:", "-Xep:ShouldNotUseFoo:OFF")
-        .doTest();
-  }
-
-  @Test
-  public void dontRunPatchUnlessRequested() {
-    compilationHelper
-        .addSourceLines(
-            "Test.java",
-            """
-            import com.google.errorprone.scanner.ScannerTest.Foo;
-
-            class Test {
-              Foo foo;
-            }
-            """)
-        .setArgs(
-            "-XepPatchLocation:IN_PLACE",
-            "-Xep:ShouldNotUseFoo:WARN",
-            "-XepPatchChecks:DeadException")
-        .doTest();
-  }
-
   @OkToUseFoo // Foo can use itself. But this shouldn't suppress errors on *usages* of Foo.
   public static final class Foo<T> {}