Propagate check flags in patch mode #4699

Stephan202 · 2024-11-24T13:31:16Z

No description provided.

Stephan202

I found this issue while working on PicnicSupermarket/error-prone-support#1426, where I noticed that a custom flag -XepOpt:Slf4jLogDeclaration:CanonicalStaticLoggerName=LOGGER was respected during regular compilation, but not when in patch mode.

Stephan202 · 2024-11-24T13:35:29Z

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

-                          ErrorPronePlugins.loadPlugins(scannerSupplier, context);
+                          ErrorPronePlugins.loadPlugins(scannerSupplier, context)
+                              .applyOverrides(epOptions);
                      ImmutableSet<String> namedCheckers =
                          epOptions.patchingOptions().namedCheckers();
                      if (!namedCheckers.isEmpty()) {
                        toUse = toUse.filter(bci -> namedCheckers.contains(bci.canonicalName()));
-                      } else {
-                        toUse = toUse.applyOverrides(epOptions);
                      }


Besides that all existing tests still pass: this change seems reasonable, at least on a conceptual level. I didn't thoroughly assess whether the non-flag changes applied by applyOverrides are no-ops, though. Could have a closer look at that later.

Stephan202 · 2024-11-24T13:35:57Z

core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java

+    assertThat(Files.readString(location))
+        .isEqualTo(
+            """
+            class StringConstantWrapper {
+              String s = "new-value";
+            }
+            """);


Without the fix above, this test fails with:

[ERROR] Failures: [ERROR] ErrorProneJavaCompilerTest.patchWithBugPatternCustomization:465 value of: readString(...) expected: class StringConstantWrapper { String s = "new-value"; } but was: class StringConstantWrapper { String s = "flag-not-set"; }

Stephan202 · 2024-11-29T07:07:12Z

An issue that's likely fixed by this change was just reported against NullAway: uber/NullAway#1080. CC @cushon; might be worthy of a new release.

Stephan202 · 2024-11-29T07:10:39Z

Effectively this change reverts #4028, though the unit tests added there still pass. So I suppose that "something else" changed that made things compatible 👀

Stephan202 · 2024-11-29T07:34:46Z

Tested locally: this change does reintroduce the issue described by #3908 and fixed by #4028, so a more subtle fix is required. Unfortunately I have very limited availability in the coming days. Will cycle back to to this.

msridhar · 2024-12-04T00:14:48Z

Would it be better for Error Prone to detect the case where a disabled check is passed to -XepPatchChecks and throw an understandable exception for that case? It seems that with #4028 such checks are just ignored, but that could lead to some unexpected behaviors. Not sure this idea makes sense or helps with finding a simpler overall fix.

Stephan202 · 2024-12-05T09:19:03Z

A better error message may indeed be acceptable in some cases, though both the reporter of #3908 and the developer of #4028 (back then a working student at Picnic) aim to see patching of disabled checks skipped for automation purposes. So hopefully we can find a way to have our cake and eat it too.

(Managing expectations: I'm currently moving apartments, so won't have time to take a closer look in the coming days. Though it's a bit surprising that it took nearly nine months for this issue to be surfaced, one could argue that not fixing this issue is worse than re-introducing the other bug (as this impacts at least one use case for "normal" users: NullAway), and that thus a release with a revert of #4028 (possibly including a revert of its tests, that apparently no longer guard against regression) is the right thing to do for now.)

Stephan202

I rebased the PR and added a second commit. I think this PR is now ready for review. @cushon if deemed acceptable, it'd be nice to have a new release with this change. 🙏

Happy holidays!

Stephan202 · 2024-12-30T16:42:42Z

core/src/test/java/com/google/errorprone/scanner/ScannerTest.java

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


I dropped these tests, introduced in #4028, as they do not actually guard against a regression of #3908. The new tests defined above do.

Stephan202 · 2024-12-30T16:45:54Z

core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java

+  @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";
+            }
+            """);
+  }


This new test covers against regression of #3908: without the latest commit, it fails.

❗ That said, my changes impact semantics ❗. Before the changes in this PR, Error Prone would ignore the -Xep:AssignmentUpdater:OFF flag and apply any suggestions by AssignmentUpdater. After my changes, AssignmentUpdater is ignored altogether.

I can see that both the old and new behavior are desirable in different contexts. However, any user that desires the previous behavior during patching can append -XepPatchChecks:AssignmentUpdater -Xep:AssignmentUpdater:ERROR instead of just -XepPatchChecks:AssignmentUpdater (because "last flag wins"). So the new semantics make it easier to write a script/integration that allows one to patch for some check C across a large set of projects, conditionally ignoring any projects that explicitly disable C.

Stephan202 · 2024-12-30T16:46:44Z

core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java

+  @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(Path.of(fileObject.toUri())))
+        .isEqualTo(
+            """
+            class StringConstantWrapper {
+              String s = "new-value";
+            }
+            """);
+  }


This new test covers the issue for which I opened the PR (and described by uber/NullAway#1080).

Stephan202 · 2024-12-30T16:48:36Z

core/src/test/java/com/google/errorprone/ErrorProneJavaCompilerTest.java

+  @Test
+  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(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";
+            }
+            """);
+  }


These two tests describe the "common case": they pass both before and after the fix.

Stephan202 · 2024-12-30T17:06:37Z

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

            .or(
                Suppliers.memoize(
                    () -> {
-                      ScannerSupplier toUse =
-                          ErrorPronePlugins.loadPlugins(scannerSupplier, context);
                      ImmutableSet<String> namedCheckers =
                          epOptions.patchingOptions().namedCheckers();
-                      if (!namedCheckers.isEmpty()) {
-                        toUse = toUse.filter(bci -> namedCheckers.contains(bci.canonicalName()));
-                      } else {
-                        toUse = toUse.applyOverrides(epOptions);
-                      }
+                      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());
                    }));


So patching now ignores checks:

Whose severity is OFF, or

That are not explicitly enabled for patching, while other checks are.

Stephan202 commented Nov 24, 2024

View reviewed changes

This was referenced Nov 24, 2024

Introduce Dropwizard Metrics integration test PicnicSupermarket/error-prone-support#1426

Merged

Patching not working since Errorprone 2.26.0 uber/NullAway#1080

Open

Propagate check flags in patch mode

d7cd72f

Stephan202 force-pushed the sschroevers/unconditionally-apply-overrides branch 2 times, most recently from c54e413 to 574cea1 Compare December 30, 2024 16:41

Prevent regression

205c5a7

Stephan202 force-pushed the sschroevers/unconditionally-apply-overrides branch from 574cea1 to 205c5a7 Compare December 30, 2024 17:00

Stephan202 commented Dec 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate check flags in patch mode #4699

Propagate check flags in patch mode #4699

Stephan202 commented Nov 24, 2024

Stephan202 left a comment

Stephan202 Nov 24, 2024

Stephan202 Nov 24, 2024

Stephan202 commented Nov 29, 2024

Stephan202 commented Nov 29, 2024

Stephan202 commented Nov 29, 2024

msridhar commented Dec 4, 2024

Stephan202 commented Dec 5, 2024 •

edited

Loading

Stephan202 left a comment

Stephan202 Dec 30, 2024

Stephan202 Dec 30, 2024

Stephan202 Dec 30, 2024

Stephan202 Dec 30, 2024

Stephan202 Dec 30, 2024

Propagate check flags in patch mode #4699

Are you sure you want to change the base?

Propagate check flags in patch mode #4699

Conversation

Stephan202 commented Nov 24, 2024

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Nov 24, 2024

Choose a reason for hiding this comment

Stephan202 Nov 24, 2024

Choose a reason for hiding this comment

Stephan202 commented Nov 29, 2024

Stephan202 commented Nov 29, 2024

Stephan202 commented Nov 29, 2024

msridhar commented Dec 4, 2024

Stephan202 commented Dec 5, 2024 • edited Loading

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Dec 30, 2024

Choose a reason for hiding this comment

Stephan202 Dec 30, 2024

Choose a reason for hiding this comment

Stephan202 Dec 30, 2024

Choose a reason for hiding this comment

Stephan202 Dec 30, 2024

Choose a reason for hiding this comment

Stephan202 Dec 30, 2024

Choose a reason for hiding this comment

Stephan202 commented Dec 5, 2024 •

edited

Loading