Propagate check flags in patch mode #4699
Conversation
There was a problem hiding this comment.
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.
| 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); | ||
| } |
There was a problem hiding this comment.
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.
| assertThat(Files.readString(location)) | ||
| .isEqualTo( | ||
| """ | ||
| class StringConstantWrapper { | ||
| String s = "new-value"; | ||
| } | ||
| """); |
There was a problem hiding this comment.
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";
}
|
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. |
|
Effectively this change reverts #4028, though the unit tests added there still pass. So I suppose that "something else" changed that made things compatible 👀 |
|
Would it be better for Error Prone to detect the case where a disabled check is passed to |
|
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.) |
No description provided.