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)
• for range strings.Split -> for range strings.SplitSeq (CL 647438)
• 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 for the exemplary bug report. I forgot to check the operator used by the AssignStmt--too easily done.

[gopherbot]

Change https://go.dev/cl/642076 mentions this issue: gopls/internal/analysis/modernize: fix bug in mapsloop

[gopherbot]

Change https://go.dev/cl/642078 mentions this issue: gopls/internal/analysis/modernize: fix bug in minmax

[dsnet]

Would #69820 be a good candidate for a modenizer?

[adonovan]

Would #69820 be a good candidate for a modenizer?

It's certainly an easy thing to implement. I'm not sure how frequent it is. The summary is:

var slice []T
const k
*(*[k]T)(slice)  =>  [k]T(slice)  // when used as rvalue

https://go.dev/cl/642815 contains a quick stab.

[gopherbot]

Change https://go.dev/cl/642815 mentions this issue: gopls/internal/analysis/modernize: simplify *(*[k]T)(x) -> ([k]T)(x)

[findleyr]

@adonovan I encountered a bug in the slices.Contains modernizer today.

Modernizing using slices.Contains here results in an error:
https://cs.opensource.google/go/x/tools/+/master:gopls/internal/golang/highlight.go;l=348;drc=344e48255740736de8c8277e9a286cf3231c7e13

		case *ast.CallExpr:
			// If cursor is an arg in a callExpr, we don't want control flow highlighting.
			if i > 0 {
				for _, arg := range n.Args {
					if arg == path[i-1] {
						return
					}
				}
			}

Error is: S (type []ast.Expr) does not satisfy ~[]E.
For now, I think we need to check that the types in the match condition are identical.

[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/[email protected] 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/[email protected]):
        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.