Add all the supporting infrastructure for removeUnused mode#203
Add all the supporting infrastructure for removeUnused mode#203bulldozer-bot[bot] merged 15 commits intodevelopfrom
removeUnused mode#203Conversation
Generate changelog in
|
✅ Successfully generated changelog entry!What happened?Your changelog entries have been stored in the database as part of our migration to ChangelogV3. Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. 📋Changelog Preview✨ Features
|
kelvinou01
changed the title
removeUnused mode and it's interferencesremoveUnused
| return EntryStream.of(CommonOptions.this.extraErrorProneCheckOptions()) | ||
| .append(other.extraErrorProneCheckOptions()) | ||
| .toMap(); | ||
| .collect(Collectors.toMap( |
There was a problem hiding this comment.
We have to specify how we collect duplicate keys here, or else we get an error
| @Override | ||
| public Map<String, String> extraErrorProneCheckOptions() { | ||
| return Map.of("SuppressibleErrorProne:Mode", "RemoveUnused"); | ||
| } |
There was a problem hiding this comment.
This will be used in VisitorStateModifications to branch to the removeUnused procedure.
| @Override | ||
| public Map<String, String> extraErrorProneCheckOptions() { | ||
| return Map.of("SuppressibleErrorProne:Mode", "Apply"); | ||
| } |
There was a problem hiding this comment.
To maintain parity with RemoveUnusedMode. Also, potentially a way to move mode interference logic into VisitorStateModifications in the future (see #180)
| "SuppressibleErrorProne:Mode", | ||
| "RemoveRollout"); |
There was a problem hiding this comment.
To maintain parity with RemoveUnusedMode. Also, potentially a way to move mode interference logic into VisitorStateModifications in the future (see #180)
| @Override | ||
| public Map<String, String> extraErrorProneCheckOptions() { | ||
| return Map.of("SuppressibleErrorProne:Mode", "Suppress"); | ||
| } |
There was a problem hiding this comment.
To maintain parity with RemoveUnusedMode. Also, potentially a way to move mode interference logic into VisitorStateModifications in the future (see #180)
kelvinou01
changed the title
removeUnusedremoveUnused mode
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| public final class RemoveUnusedAndApplyingInterference extends SpecificModeInterference { |
There was a problem hiding this comment.
Isn't this identical to the suppressing + applying interference? What happens when we have suppressing + applying + removeUnused?
I'm honestly not convinced that this interference concept was necessary (mea culpa) - I think we may have been able to always use -XepPatchChecks: to enable patching on every bugchecker and always send a list of checks that need patching to VisitorStateModifications, which would remove fixes if it is not to be patched (when suppressing/applying/etc).
If we still need to keep interferences, I think we could use the ModeInterference interface directly, return all of suppress/apply/removeUnused as interfering if they have been enabled. The combine all of them together in interfere (and only have one interference between all three).
There was a problem hiding this comment.
Now that I think about it, if we are reporting rollout suppressions with SuppressibleErrorProne bugchecker, I think we can get rid of PreferPatchChecks entirely. -XepPatchChecks will retain it's original semantics of actually applying the autofixes.
There was a problem hiding this comment.
As discussed privately, let's consolidate the duplicate logic to a ApplyingAndSuppressingOrRemoveUnusedInterference
kelvinou01
force-pushed
the
okelvin/remove-unused-mode
branch
from
e898d04 to
9de5225
Compare
| * Initialize a {@code LazySuppressionFix} on {@code declaration}, choosing which suppressions to keep with | ||
| * {@code filterExisting}. | ||
| */ | ||
| @SuppressWarnings("PreferSafeLoggableExceptions") // It doesn't matter in our internal codebases |
There was a problem hiding this comment.
Should probably disable it in the build.gradle for this project.
There was a problem hiding this comment.
Actually, I think the it shouldn't even suggest this unless safe-logging is on the classpath. Not sure why it would be.
There was a problem hiding this comment.
Seems like it's being suggested everywhere. Maybe the safe logging errorprones should live in safe-logging instead after #178 is out
|
|
||
| @Override | ||
| public Map<String, String> extraErrorProneCheckOptions() { | ||
| return Map.of("SuppressibleErrorProne:Mode", "Apply"); |
There was a problem hiding this comment.
I would at least make "SuppressibleErrorProne:Mode" a constant across all these files.
It could actually be a higher level concept, ie have it's own method on CommonOptions or - even better - the modes framework could automatically add check option based on the modes in use, meaning this isn't even necessary.
There was a problem hiding this comment.
Let's make it a constant for now, and tweak the concept in the next PR
|
:+1L |
|
👍 |
|
Released 2.22.0 |
2 participants
Before this PR
See #173 for background
FLUP to #192 and #200
We are working on
-PerrorProneRemoveUnused, a mode which removes all unused suppressions. This is the penultimate PR which brings in all the infrastructure required to support removeUnused mode:removeUnusedleads to UnsupportedOperationException for the time being)ReportedFixCacheto encapsulate creating and reporting new suppression fixes. Note that this usesstate.reportMatch, c.f. the old mechanism of returning a populatedDescriptionfrominterceptDescriptionSuppressibleErrorPronebugchecker, used to prevent infinite recursion fromReportedFixCachecallingstate.reportMatchThe actual implementation for removeUnused will be brought in the next PR for ease of review.
After this PR
==COMMIT_MSG==
Add the
removeUnusedmode and it's interferences==COMMIT_MSG==
Possible downsides?