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

[adonovan]

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).

[silverwind]

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]

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]

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".

[gopherbot]

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

[adonovan]

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

[gopherbot]

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

[spencerschrock]

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]

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

[adonovan]

Oops, thanks; reopening.

[cuishuang]

Friendly inquiry, how is the proposal progressing? I really need this feature, as using interface{} -> any is too broad. It would be great to enable or disable specific items.

Additionally, are there any plans for other optimizations, such as slices.Equal and t.TempDir?

I would be happy to implement them. Should I submit a proposal?

[adonovan]

Friendly inquiry, how is the proposal progressing?

Slowly, but that's the nature of the proposal process. I expect this will get approved within a couple of weeks.

I really need this feature, as using interface{} -> any is too broad. It would be great to enable or disable specific items.

As a stopgap, i added a -category flag to the modernize analyzer, so you can use modernize -category=efaceany right away on the experimental golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest` command.

[cuishuang]

As a stopgap, i added a -category flag to the modernize analyzer, so you can use modernize -category=efaceany right away on the experimental golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest` command.

Thank you for your response.

However, I tried it yesterday and saw the TODO in the code (stopgap until CL 655555 lands), but it doesn’t seem to be working. Here are the execution results.

flag provided but not defined: -category
modernize: simplify code by using modern constructs

Usage: modernize [-flag] [package]

This analyzer reports opportunities for simplifying and clarifying
existing code by using more modern features of Go, such as:

  - replacing an if/else conditional assignment by a call to the
    built-in min or max functions added in go1.21;
  - replacing sort.Slice(x, func(i, j int) bool) { return s[i] < s[j] }
    by a call to slices.Sort(s), added in go1.21;
  - replacing interface{} by the 'any' type added in go1.18;
  - replacing append([]T(nil), s...) by slices.Clone(s) or
    slices.Concat(s), added in go1.21;
  - replacing a loop around an m[k]=v map update by a call
    to one of the Collect, Copy, Clone, or Insert functions
    from the maps package, added in go1.21;
  - replacing []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),
    added in go1.19;
  - replacing uses of context.WithCancel in tests with t.Context, added in
    go1.24;
  - replacing omitempty by omitzero on structs, added in go1.24;
  - replacing append(s[:i], s[i+1]...) by slices.Delete(s, i, i+1),
    added in go1.21
  - replacing a 3-clause for i := 0; i < n; i++ {} loop by
    for i := range n {}, added in go1.22;
  - replacing Split in "for range strings.Split(...)" by go1.24's
    more efficient SplitSeq;

To apply all modernization fixes en masse, you can use the
following command:

	$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -test ./...

....

[spencerschrock]

However, I tried it yesterday and saw the TODO in the code (stopgap until CL 655555 lands), but it doesn’t seem to be working

latest refers to gopls/v0.18.1 here, which was before -category was committed in (7435a81). You can install modernize from the master branch if you want a version with the patch.

[adonovan]

Thanks @spencerschrock, I misspoke when I suggested @latest. Beware that, unlike many applications, gopls uses replace directives and so cannot be installed from the master branch using @master; you must clone x/tools and build ./gopls.