x/tools/gopls/internal/analysis: add "modernizer" analyzers

[adonovan]

Between generics, iterators, min and max, new standard packages such as slices, maps, and cmp, there are a large number of new ways to write Go code more concisely and no less clearly. We should develop "modernizer" analyzer(s) that identify such opportunities and offer suggested fixes.

Many of these are already implemented in staticcheck (see S1000 et seq); we would just need to enable them.

When the appropriate fix is to inline a call to a deprecated function whose body illustrates the recommended approach (e.g. a call to a more modern function, as in the case of the functions in the golang.org/x/exp/maps package), these will be handled by annotating the deprecated function as prescribed by proposal #32816, and letting the inliner take care of it.

Examples:

• b.N → b.Loop in Benchmarks (https://go.dev/cl/638337)
• interface{} → any (CL 636276)
• explicit loops → slices.{Equal,Compare,Index,Contains}{,Func}. The challenge is treating the return false and return true steps in the slices package as wildcards that match "equal and opposite" actions--not just return statements--in the user code. (CL 640576)
• explicit loops → maps.{Clone,Collect.Insert,Copy} (https://go.dev/cl/638875)
• maps.Keys(m) + slices.Collect (or loop equivalent of Keys+Collect) + sort -> slices.Sorted(maps.Keys(m)). Eliminate temporaries when used once in range _. Downside: de-optimizes preallocation of correct array size; perhaps best left until resolution of proposal: maps: helper to loop over the maps in predictable order #68598.
• various append + s[::] combinations -> slices.Grow, Clip, Concat & Clone (https://go.dev/cl/637735), etc.
• append(s[:i], s[i+k:]...) -> slices.Delete(s, i, i+k), where k is a non-negative constant
• exp/{slices,maps} -> std {slices,maps} (see CL 635680 + issue cmd/fix: automate migrations for simple deprecations #32816)
• exp/maps.Clear → clear etc (https://go.dev/cl/635680)
• if/else → min/max (CL 635777)
• awkward logic in slice.Sort's less function → if cmp := cmp.Compare(x, y); cmp != 0 { return cmp < 0 }
• sort.Slice -> slices.Sort, avoiding the less func when it is the natural order (https://go.dev/cl/635681)
• sort.Slice -> slices.SortFunc, eliminating the indices
• json:omitempty → json:omitzero for basic types
• SetFinalizer → AddCleanup
• use strings.Lines instead of strings.Split("\n") (but: "\r"?!) or text.scanner(strings.NewReader)/Scan/Text
• []byte(fmt.Sprintf...) → fmt.Appendf(nil, ...) (https://go.dev/cl/638436)
• implementing {Text,Binary}Appender for types that currently implement {Text,Binary}Marshaler
• replacing ctx, cancel := context.WithCancel(context.Background()); defer cancel() in tests with ctx := t.Context().
• use cipher.NewGCMWithRandomNonce where possible (example)
• reflect.DeepEqual(x, y []comparable) -> slices.Equal(x, y) (CL 649457)
• for range strings.Split -> for range strings.SplitSeq (CL 647438)
• ditto for strings.Fields{,Seq}
• for i := 0; i < n; i++ {} => for i := range n {} (CL 646916)

They should of course all respect the effective version of Go determined by go.mod and build tags.

@aclements

[zephyrtronium]

Would it make sense for some of these to be depreciations instead? Particularly looking at x/exp/maps.Clear and the go/* upgrades.

[adonovan]

Would it make sense for some of these to be depreciations instead? Particularly looking at x/exp/maps.Clear and the go/* upgrades.

Even if we were to deprecate maps.Clear (which is perfectly fine function), users would still want a way to migrate their code away from it. So I see deprecation as independent. And in many cases the pattern being "modernized" is not just a simple use of one symbol that could be deprecated.

[aclements]

Some of these (maybe all 😄 ) are subtler than they appear on the tin.

b.N → b.Loop in Benchmarks

The core of this is just replacing for range b.N with for b.Loop(), but a major point of b.Loop() is that it allows other simplifications, like you don't have to reset the timer after setup, or go out of your way to cache complex setup steps, or stop the timer after the loop for expensive cleanup steps.

SetFinalizer → AddCleanup

Here you have to identify the underlying "resource" that's being freed by the finalizer since that's what AddCleanup attaches.

use strings.Lines

There are so many patterns for splitting a string into lines right now. I'm really curious how we'd pick up on that.

implementing {Text,Binary}Appender for types that currently implement {Text,Binary}Marshaler

In addition to creating Append methods, ideally this should also rewrite MarshalText/MarshalBinary to use the new Append methods.

[gopherbot]

Change https://go.dev/cl/635777 mentions this issue: gopls/internal/analysis/modernize: quick fixes to modernize Go code

[gopherbot]

Change https://go.dev/cl/635680 mentions this issue: slices,maps: delegate go go1.21 packages of the same name

[gopherbot]

Change https://go.dev/cl/635681 mentions this issue: gopls/internal/analysis/modernize: sort.Slice -> slices.Sort

[gopherbot]

Change https://go.dev/cl/636276 mentions this issue: gopls/internal/analysis/modernize: replace interface{} with any

[gopherbot]

Change https://go.dev/cl/637735 mentions this issue: gopls/internal/analysis/modernize: append(zerocap, s...) -> slices.Clone(s)

[gopherbot]

Change https://go.dev/cl/638337 mentions this issue: gopls/internal/analysis/modernize: b.N -> b.Loop()

[gopherbot]

Change https://go.dev/cl/638595 mentions this issue: gopls/internal/analysis/modernize: simplify append(base, s...)

[gopherbot]

Change https://go.dev/cl/638875 mentions this issue: gopls/internal/analysis/modernize: for...{m[k]=v} -> maps.Collect et al

[gopherbot]

Change https://go.dev/cl/638436 mentions this issue: gopls/internal/analysis/modernize: string formatting

[adonovan]

Thanks; I split this out into its own issue: #71313

[findleyr]

Thanks; I split this out into its own issue: #71313

Made that a sub-issue of this one. Neat! We're living in the future.

[josharian]

A modernization I often do by hand is using {strings,bytes}.Cut{,Prefix,Suffix}.

[adonovan]

Would [array to slice conversions] be a good candidate for a modenizer?
https://go.dev/cl/642815 contains a quick stab.

It doesn't turn up a single instance in the entire k8s codebase (~3MLoC), so the juice is not worth the squeeze.

[dsnet]

It doesn't turn up a single instance in the entire k8s codebase (~3MLoC), so the juice is not worth the squeeze.

😅 Thank you for testing that out.

[gopherbot]

Change https://go.dev/cl/646916 mentions this issue: gopls/internal/analysis/modernize: for i := 0; i < n; i++ -> range n

[apocelipes]

Would cmp.Compare(str1, str2) -> strings.Compare(str1, str2) be a good candidate for a modenizer? The strings.Compare is more efficient and seems to have clearer semantics.

[DmitriyMV]

@apocelipes see #71270

[gopherbot]

Change https://go.dev/cl/647355 mentions this issue: gopls/internal/analysis/modernize/cmd/modernize: create

[earthboundkid]

When I tried to run this, I got an error. Is this expected?

$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@1320197d6c7ed6da48496e5e311c7ee76701e035
 -fix
go: golang.org/x/tools/gopls@v0.0.0-20250206210647-1320197d6c7e requires go >= 1.23.4; switching to go1.23.6
go: golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@1320197d6c7ed6da48496e5e311c7ee76701e035 (in golang.org/x/tools/gopls@v0.0.0-20250206210647-1320197d6c7e):
        The go.mod file for the module providing named packages contains one or
        more replace directives. It must not contain directives that would cause
        it to be interpreted differently than if it were the main module.

[adonovan]

Yes, it is expected: gopls release tags are on a branch that doesn't have the replace golang.org/x/tools => ../ directive, allowing them to be go installed. You'll have to check out the x/tools repo and build it from there, or wait for the release.

Also: If building from head, beware that the three-way merge CL hasn't landed yet, so -fix may make a mess of things.

[nightlyone]

@adonovan When you are done, could you please also turn this into a more regular pass so we can monitor and fix regressions in that area using a golanci-lint integration of that linter?

[adonovan]

@adonovan When you are done, could you please also turn this into a more regular pass so we can monitor and fix regressions in that area using a golanci-lint integration of that linter?

Do you mean: can we publish the modernize analyzer as a subpackage of go/analysis/passes, so that golangci-lint can include it in its set? That's certainly something we could do, though of course every new API symbol requires a proposal; but we are at the beginning of a series of discussions about how to generalize modernizers (and by implication analyzers in general) so that they can be defined by third-party projects too. We should probably wait till that picture emerges clearly before committing to publish the modernize analyzer.

[spencerschrock]

There's one modernizer false positive reported in this issue already (#70815 (comment)), so I'll add another.

Currently at HEAD (44b61a1d174cc06329b20f5de60c2b0c800741a4):

main.go:7:5: if/else statement can be modernized using max

package main

import "fmt"

func main() {
	var x, y int
	if x <= 0 { // a value of -1 or 0 will use a default value (30)
		y = 30
	} else {
		y = x
	}
	fmt.Print(y)
}

Applying the suggested fix doesn't yield an equivalent result:

package main

import "fmt"

func main() {
	var x, y int
	y = max(x, 0)
	fmt.Print(y)
}

[adonovan]

Thanks @spencerschrock; I've split this into its own issue, #71721.

[gopherbot]

Change https://go.dev/cl/649457 mentions this issue: gopls/internal/analysis/modernize: reflect.DeepEqual -> slices.Equal

[mvdan]

One thought: when suggesting to replace append([]byte{}, p...) with slices.Clone, if the file or package in question already uses the bytes package elsewhere but not the slices package, I think that implies that bytes.Clone would be a mildly better replacement. Because in one example I just ran into, the package in question was relatively low level, so mixing the bytes and slices packages seemed unnecessary.

In general I think the same applies to any suggestions to use the slices package for operations on []byte where the file already uses the bytes package but not the slices package. IMHO local consistency is relatively important.