Skip to content
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

proposal: x/tools/go/analysis: add -category=... filter flag to driver commands #72008

Open
adonovan opened this issue Feb 27, 2025 · 9 comments · May be fixed by golang/tools#565
Open

proposal: x/tools/go/analysis: add -category=... filter flag to driver commands #72008

adonovan opened this issue Feb 27, 2025 · 9 comments · May be fixed by golang/tools#565
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsFix The path to resolution is known, but the work has not been done. Proposal Refactoring Issues related to refactoring tools ToolProposal Issues describing a requested change to a Go tool or command-line program. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@adonovan
Copy link
Member

adonovan commented Feb 27, 2025 •
edited
Loading

Background: Some analysis tools produce a range of diagnostics and fixes and it would be useful to filter them before applying fixes. For example, the modernize analyzer produces a dozen different kinds of fix, and several users have reported that it is easier to review changes if they first batch-apply only the (numerous) interface{} -> any fixes in a single very low-risk CL that can be rubberstamped, and then apply all the other fixes in a smaller CL that needs to be properly reviewed (for example, because sometimes a valuable comments may be lost).

Proposal: I propose we add a -category flag that accepts a comma-separated list of categories (or negated list of categories) and then applies only that set (or its complement).
@adonovan adonovan mentioned this issue Feb 27, 2025
Open
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 27, 2025
@gopherbot gopherbot added this to the Unreleased milestone Feb 27, 2025
@adonovan adonovan added Analysis Issues related to static analysis (vet, x/tools/go/analysis) Refactoring Issues related to refactoring tools labels Feb 27, 2025
@gabyhelp gabyhelp added the ToolProposal Issues describing a requested change to a Go tool or command-line program. label Feb 27, 2025
@silverwind
Copy link

silverwind commented Feb 27, 2025 •
edited
Loading

Definitely agree. I recently tried modernize on a big code base here, and it broke the build and did some rather disagreeable changes.

I think it should have a "safe" and "unsafe" meta-category similar to ruff (https://docs.astral.sh/ruff/linter/#fix-safety). Safe fixes should be throughly tested and never ever break code.

@adonovan
Copy link
Member Author

adonovan commented Feb 27, 2025 •
edited
Loading

None of the categories of fix should ever break code; we regard all those problems as bugs and will fix them. Modernizers are all intended to be "safe" by the ruff classification. But the highest priority bugs are those that don't break the build or even the tests, as those problems are most likely to be overlooked.

@silverwind
Copy link

silverwind commented Feb 27, 2025 •
edited
Loading

Right, thats a good stance too. If a fix is known to break code, it's better to either fix or remove it instead of labeling it "unsafe".

@adonovan adonovan mentioned this issue Feb 28, 2025
Closed
8 tasks
@prattmic prattmic added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 28, 2025
@spencerschrock spencerschrock linked a pull request Mar 6, 2025 that will close this issue
Open
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/655555 mentions this issue: go/analysis: add -category=... filter flag to driver commands

@adonovan
Copy link
Member Author

adonovan commented Mar 7, 2025

Unfortunately this very tiny change affects the CLI of cmd/vet and other analysis tools and so requires a proposal, which will add at least a month to the time to merge the (generously contributed) fix. I will try to use this as an opportunity to streamline the proposal process for tiny changes. @aclements

@adonovan adonovan changed the title x/tools/go/analysis: add -category=... filter flag to driver commands proposal: x/tools/go/analysis: add -category=... filter flag to driver commands Mar 7, 2025
@adonovan adonovan moved this to Incoming in Proposals Mar 7, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/655436 mentions this issue: gopls/internal/analysis/modernize: document workflow

@spencerschrock
Copy link

spencerschrock commented Mar 7, 2025 •
edited
Loading

Thanks, happy to see -category in modernizer, though this issue should probably be reopened until a proposal decision.

(though the cherry pick below will just close it again...)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/655956 mentions this issue: [gopls-release-branch.0.18] gopls/internal/analysis/modernize: document workflow

@adonovan
Copy link
Member Author

adonovan commented Mar 7, 2025

Oops, thanks; reopening.

@adonovan adonovan reopened this Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsFix The path to resolution is known, but the work has not been done. Proposal Refactoring Issues related to refactoring tools ToolProposal Issues describing a requested change to a Go tool or command-line program. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
Proposals
Status: Incoming
Milestone
Unreleased
Development

Successfully merging a pull request may close this issue.

go/analysis: add -category=... filter flag to driver commands spencerschrock/golang-tools
6 participants
@silverwind @prattmic @adonovan @spencerschrock @gopherbot @gabyhelp