Skip to content

Add removeUnused mode #202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Oct 23, 2025
Merged

Add removeUnused mode #202

merged 4 commits into from
Oct 23, 2025

Conversation

@kelvinou01
Copy link
Contributor

@kelvinou01 kelvinou01 commented Oct 17, 2025 •
edited
Loading

Before this PR

See #173 for background

FLUP to #192, #200, #203

Features

  • removeUnused mode removes all unused error-prone suppressions (both human-authored and for-rollout suppressions) in all compilation units
  • Only removes error-prone suppressions, and ignores unknown ones like checkstyles or SomeRandomSuppression
  • Works seamlessly with apply and suppress mode, allowing you to remove unused + apply + suppress in one compilation

After this PR

==COMMIT_MSG==
Add removeUnused mode
==COMMIT_MSG==

Possible downsides?

  • Adds yet another layer of complexity onto VisitorStateModifications, e.g. the SuppressibleErrorProne check used to prevent infinite recursion when calling state.reportMatch
  • Maybe performance regressions?

@changelog-app
Copy link

changelog-app bot commented Oct 17, 2025 •
edited by kelvinou01
Loading

Generate changelog in changelog/@unreleased

Type (Select exactly one)

  • Feature (Adding new functionality)
  • Improvement (Improving existing functionality)
  • Fix (Fixing an issue with existing functionality)
  • Break (Creating a new major version by breaking public APIs)
  • Deprecation (Removing functionality in a non-breaking way)
  • Migration (Automatically moving data/functionality to a new system)

Description

Add removeUnused mode

Check the box to generate changelog(s)

  • Generate changelog entry

@changelog-app
Copy link

changelog-app bot commented Oct 17, 2025

✅ 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

  • Add removeUnused mode (#202)

@kelvinou01 kelvinou01 marked this pull request as draft October 17, 2025 12:53
@kelvinou01 kelvinou01 force-pushed the okelvin/remove-unused-pr branch 2 times, most recently from 79ab57d to a25c7f4 Compare October 22, 2025 09:01
@kelvinou01 kelvinou01 force-pushed the okelvin/remove-unused-pr branch from a25c7f4 to 27f96dd Compare October 22, 2025 09:08
@kelvinou01 kelvinou01 marked this pull request as ready for review October 22, 2025 09:09
@kelvinou01
Copy link
Contributor Author

kelvinou01 commented Oct 22, 2025

FLUPS: #203 (comment)

CRogers
CRogers reviewed Oct 22, 2025
View reviewed changes
...error-prone/src/main/java/com/palantir/suppressibleerrorprone/VisitorStateModifications.java Outdated
private static final Logger log = Logger.getLogger(VisitorStateModifications.class.getName());

private static final ReportedFixCache FIXES = new ReportedFixCache();
private static final Set<CompilationUnitTree> SEEN = new HashSet<>();
Copy link
Contributor

@CRogers CRogers Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give SEEN a slightly better name - maybe ALREADY_REMOVED_EXISTING_SUPPRESSIONS?

Copy link
Contributor

@CRogers CRogers Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think we should make a new type to remove some code from VisitorStateModifications. It can hold and manage this Set, then have the removeAllSuppressionsOnErrorpronesInCompilationUnit method in there too.

Copy link
Contributor

@CRogers CRogers Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to ReportedFixCache (will need to take it as a field)

Copy link
Contributor Author

@kelvinou01 kelvinou01 Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I've created a SuppressionRemover which encapsulates a weak set and the "report on everything" logic.

@kelvinou01 kelvinou01 force-pushed the okelvin/remove-unused-pr branch from 5a1d8f3 to c642485 Compare October 22, 2025 16:56
@CRogers
Copy link
Contributor

CRogers commented Oct 23, 2025

👍

@bulldozer-bot bulldozer-bot bot merged commit 691db97 into develop Oct 23, 2025
5 checks passed
@bulldozer-bot bulldozer-bot bot deleted the okelvin/remove-unused-pr branch October 23, 2025 13:26
@autorelease3
Copy link

autorelease3 bot commented Oct 23, 2025

Released 2.24.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CRogers CRogers CRogers left review comments

Assignees

No one assigned

Labels

autorelease merge when ready

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kelvinou01 @CRogers