-PerrorProneRemoveUnused implementation (works with suppress and apply): remove unused suppressions on every encountered compilation unit in VisitorStateModifications #167
Conversation
Generate changelog in
|
| private final List<String> existingSuppressions; | ||
| private final String suffix; | ||
| private final Set<String> newSuppressions; | ||
| private final Set<String> desiredSuppressions; |
There was a problem hiding this comment.
This implicitly uses a listener/accumulator pattern, like error-prone's DescriptionListener. Maybe make this explicit
There was a problem hiding this comment.
In that it's a mutable set?
| } | ||
|
|
||
| /** | ||
| * Only these trees are suppressible, as per |
There was a problem hiding this comment.
We need to be careful here. We've decided that suppressible-error-prone will not put suppressions in certain locations, but if we're removing unused human suppressions they may be in these locations. This may complicate things somewhat.
There was a problem hiding this comment.
Especially as we might want to "move" for-rollout: suppressions to be in a more specific location (eg because we changed up some errorprone logic), but we probably don't want to do this to human authored suppressions unless we need to (eg someone suppresses a whole class to stop loads of errors - we probably want to keep this).
There was a problem hiding this comment.
we probably don't want to do this to human authored suppressions unless we need to
yep, RemoveUnusedMode has tests for this specific case
kelvinou01
changed the title
| def 'errorProneRemoveUnused + errorProneApply + errorProneSuppress applies fixes and suppressions on previously suppressed elements'() { | ||
| // language=Java | ||
| writeJavaSourceFileToSourceSets ''' | ||
| package app; | ||
| @SuppressWarnings("ArrayEquals") | ||
| public final class App { | ||
| private static final String EMPTY_STRING = ""; | ||
|
|
||
| // Although InlineTrivialConstant can be placed lower in the AST hierarchy, | ||
| // we preserve existing suppressions whenever possible rather than move suppressions around. | ||
| // Also, note that we don't add for-rollout here. | ||
| @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) | ||
| static class Inner { | ||
| private static final String EMPTY = ""; | ||
| boolean truism = new int[3].equals(new int[3]); | ||
|
|
||
| @SuppressWarnings("InlineTrivialConstant") | ||
| static class InnerInner { | ||
| @SuppressWarnings({"ArrayEquals", "InlineTrivialConstant"}) | ||
| void method() { | ||
| new int[3].equals(new int[3]); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| '''.stripIndent(true) | ||
| when: | ||
| runTasksSuccessfully('compileAllErrorProne', '-PerrorProneRemoveUnused', '-PerrorProneSuppress', '-PerrorProneApply=ArrayEquals') | ||
| then: | ||
| // language=Java | ||
| resultIsSyntacticallyEqualTo ''' | ||
| package app; | ||
| import java.util.Arrays; | ||
|
|
||
| public final class App { | ||
| @SuppressWarnings("for-rollout:InlineTrivialConstant") | ||
| private static final String EMPTY_STRING = ""; | ||
|
|
||
| // Although InlineTrivialConstant can be placed lower in the AST hierarchy, | ||
| // we preserve existing suppressions whenever possible rather than move suppressions around. | ||
| // Also, note that we don't add for-rollout here. | ||
| @SuppressWarnings("InlineTrivialConstant") | ||
| static class Inner { | ||
| private static final String EMPTY = ""; | ||
| boolean truism = Arrays.equals(new int[3], new int[3]); | ||
|
|
||
| static class InnerInner { | ||
| void method() { | ||
| Arrays.equals(new int[3], new int[3]); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| '''.stripIndent(true) | ||
| } |
There was a problem hiding this comment.
🚨 TLDR of what this PR enables
| * <p>This class uses bytecode manipulation to patch {@code SuppressibleTreePathScanner::isSuppressed} | ||
| * to respect the ErrorProneOptions setting. | ||
| */ | ||
| final class SuppressibleTreePathScannerClassVisitor extends ClassVisitor { |
There was a problem hiding this comment.
As discussed in person, we should try to get the issue fixed in error-prone before resorting to further asm modification.
There was a problem hiding this comment.
yep, lets see how the PR does
| return description; | ||
| } | ||
|
|
||
| Set<String> modes = visitorState.errorProneOptions().getFlags().getSetOrEmpty("SuppressibleErrorProne:Mode"); |
There was a problem hiding this comment.
Mmm, putting the mode in here kinda odd, especially as we could be in multiple modes. just realised this is a Set, but maybe we could have a nice abstract here - would need to think about this more.
…e into okelvin/remove-apply-suppress # Conflicts: # gradle-suppressible-error-prone/src/test/groovy/com/palantir/gradle/suppressibleerrorprone/SuppressibleErrorPronePluginIntegrationTest.groovy # suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/RemoveRolloutSuppressions.java # suppressible-error-prone/src/main/java/com/palantir/suppressibleerrorprone/SuppressWarningsUtils.java # versions.lock # versions.props
Before this PR
After this PR
==COMMIT_MSG==
==COMMIT_MSG==
Possible downsides?