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.

Fixes #4699 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4699 from PicnicSupermarket:sschroevers/unconditionally-apply-overrides 205c5a7 PiperOrigin-RevId: 711481047

Fixes google#4699 COPYBARA_INTEGRATE_REVIEW=google#4699 from PicnicSupermarket:sschroevers/unconditionally-apply-overrides 205c5a7 PiperOrigin-RevId: 711776790 (cherry picked from commit 85af056)

| Package | Type | Package file | Manager | Update | Change | |---|---|---|---|---|---| | [com.google.guava:guava-bom](https://github.com/google/guava) ([source](http://svn.sonatype.org/spice/trunk/oss/oss-parent-9)) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `33.4.0-jre` -> `33.4.5-jre` | | [com.google.apis:google-api-services-storage](http://nexus.sonatype.org/oss-repository-hosting.html) ([source](http://svn.sonatype.org/spice/tags/oss-parent-7)) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `v1-rev20250224-2.0.0` -> `v1-rev20250312-2.0.0` | | [com.google.errorprone:error_prone_annotations](https://errorprone.info) ([source](https://github.com/google/error-prone)) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `2.36.0` -> `2.37.0` | | [com.autonomousapps.dependency-analysis](https://github.com/autonomousapps/dependency-analysis-android-gradle-plugin) | plugin | misk/gradle/libs.versions.toml | gradle | minor | `2.12.0` -> `2.13.0` | | [software.amazon.awssdk:sdk-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.3` -> `2.31.4` | | [software.amazon.awssdk:sqs](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.3` -> `2.31.4` | | [software.amazon.awssdk:dynamodb-enhanced](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.3` -> `2.31.4` | | [software.amazon.awssdk:dynamodb](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.3` -> `2.31.4` | | [software.amazon.awssdk:aws-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.3` -> `2.31.4` | | [software.amazon.awssdk:bom](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.3` -> `2.31.4` | | [software.amazon.awssdk:auth](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.3` -> `2.31.4` | --- ### Release Notes <details> <summary>google/error-prone (com.google.errorprone:error_prone_annotations)</summary> ### [`v2.37.0`](https://github.com/google/error-prone/releases/tag/v2.37.0): Error Prone 2.37.0 Changes: - The annotations that were previously in `error_prone_type_annotations` have been been merged into `error_prone_annotations`. `error_prone_type_annotations` is now deprecated, and will be removed in a future release. New checks: - [`AssignmentExpression`](https://errorprone.info/bugpattern/AssignmentExpression) - The use of an assignment expression can be surprising and hard to read; consider factoring out the assignment to a separate statement. - [`IntFloatConversion`](https://errorprone.info/bugpattern/IntFloatConversion) - Detect calls to `scalb` that should be using the double overload instead - [`InvalidSnippet`](https://errorprone.info/bugpattern/InvalidSnippet) - Detects snippets which omit the `:` required for inline code. - [`JUnit4EmptyMethods`](https://errorprone.info/bugpattern/JUnit4EmptyMethods) - Detects empty JUnit4 `@Before`, `@After`, `@BeforeClass`, and `@AfterClass` methods. - [`MockIllegalThrows`](https://errorprone.info/bugpattern/MockIllegalThrows) - Detects cases where Mockito is configured to throw checked exception types which are impossible. - [`NegativeBoolean`](https://errorprone.info/bugpattern/NegativeBoolean) - Prefer positive boolean names. - [`RuleNotRun`](https://errorprone.info/bugpattern/RuleNotRun) - Detects `TestRule`s not annotated with `@Rule`, that won't be run. - [`StringConcatToTextBlock`](https://errorprone.info/bugpattern/StringConcatToTextBlock) - Replaces concatenated multiline strings with text blocks. - [`TimeInStaticInitializer`](https://errorprone.info/bugpattern/TimeInStaticInitializer) - Detects accesses of the system time in static contexts. Closed issues: - Propagate check flags in patch mode ([#4699](google/error-prone#4699)) - Fixes a crash in ComputeIfAbsentAmbiguousReference ([#4736](google/error-prone#4736)) - Show the field name in HidingField diagnostics ([#4775](google/error-prone#4775)) - Add support for jakarta annotations to some checks ([#4782](google/error-prone#4782)) - FloatingPointAssertionWithinEpsilonTest depends on default locale ([#4815](google/error-prone#4815)) - `@InlineMe` patching of `Strings.repeat` produces broken code ([#4819](google/error-prone#4819)) - Fix a crash in IdentifierName on unnamed (`_`) variables ([#4847](google/error-prone#4847)) - Fix a crash in ArgumentParameterSwap ([#490](google/error-prone#490)) Full changelog: google/error-prone@v2.36.0...v2.37.0 </details> <details> <summary>autonomousapps/dependency-analysis-android-gradle-plugin (com.autonomousapps.dependency-analysis)</summary> ### [`v2.13.0`](https://github.com/autonomousapps/dependency-analysis-android-gradle-plugin/blob/HEAD/CHANGELOG.md#Version-2130) - \[Feat]: `computeResolvedDependencies` to also generate a version catalog file - \[Feat]: experimenting with compressing intermediates, starting with `exploded-jars.json`. - \[Chore]: remove unused moshi functions. </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am every weekday" in timezone Australia/Melbourne, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Never, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). GitOrigin-RevId: 9a8acb12117486aa61b1d54c73d77d84629e5e79

krisso-rtb · 2025-03-26T21:21:53Z

Hi @Stephan202
Could you please take a look at #4943 and let us know if the issue is related with this PR.
Thanks in advance!

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

Closed

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

cushon approved these changes Jan 2, 2025

View reviewed changes

copybara-service bot pushed a commit that referenced this pull request Jan 3, 2025

Propagate check flags in patch mode

540289f

Fixes #4699 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4699 from PicnicSupermarket:sschroevers/unconditionally-apply-overrides 205c5a7 PiperOrigin-RevId: 711481047

copybara-service bot mentioned this pull request Jan 3, 2025

Propagate check flags in patch mode #4743

Merged

copybara-service bot closed this in 85af056 Jan 3, 2025

Stephan202 deleted the sschroevers/unconditionally-apply-overrides branch January 3, 2025 18:05

krisso-rtb mentioned this pull request Mar 26, 2025

[v2.37.0] Disabled checks are executed when XepPatchChecks: is empty #4943

Open

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

krisso-rtb commented Mar 26, 2025

Propagate check flags in patch mode #4699

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

krisso-rtb commented Mar 26, 2025

Stephan202 commented Dec 5, 2024 •

edited

Loading