[Note: please create new issues for bugs and feature requests rather than expanding the scope of this one.]

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:

  • [x] b.N → b.Loop in Benchmarks (https://go.dev/cl/638337)
  • [x] interface{} → any (CL 636276)
  • [x] 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)
  • [x] 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 #68598.~
  • [x] various append + s[::] combinations -> slices.Grow, Clip, Concat & Clone (https://go.dev/cl/637735), etc.
  • [x] append(s[:i], s[i+k:]...) -> slices.Delete(s, i, i+k), where k is a non-negative constant
  • [x] exp/{slices,maps} -> std {slices,maps} (see CL 635680 + issue #32816)
  • [x] exp/maps.Clear → clear etc (https://go.dev/cl/635680)
  • [x] 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 }~
  • [x] 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~
  • [x] 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~
  • [x] []byte(fmt.Sprintf...) → fmt.Appendf(nil, ...) (https://go.dev/cl/638436)
  • ~implementing {Text,Binary}Appender for types that currently implement {Text,Binary}Marshaler~
  • [x] replacing ctx, cancel := context.WithCancel(context.Background()); defer cancel() in tests with ctx := t.Context().
  • [ ] use cipher.NewGCMWithRandomNonce where possible (example)
  • [x] reflect.DeepEqual(x, y []comparable) -> slices.Equal(x, y) (CL 649457. Not sound! Changes nil behavior.)
  • [x] for range strings.Split -> for range strings.SplitSeq (CL 647438)
  • [ ] ditto for strings.Fields{,Seq} (split out to separate issue #72033)
  • [x] 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

Comment From: zephyrtronium

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

Comment From: 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.

Comment From: aclements

Some of these (maybe all :smile: ) 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.

Comment From: gopherbot

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

Comment From: gopherbot

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

Comment From: gopherbot

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

Comment From: gopherbot

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

Comment From: gopherbot

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

Comment From: gopherbot

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

Comment From: gopherbot

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

Comment From: gopherbot

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

Comment From: gopherbot

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

Comment From: findleyr

This is great. Let's try to get as many of these done for gopls@v0.18.0, to enhance the theme of the release.

Assigning @adonovan and @madelinekalil, who are working on it.

Comment From: gopherbot

Change https://go.dev/cl/640041 mentions this issue: gopls/internal/analysis/modernize: omitzero

Comment From: adonovan

I notice Stroustrup has long used term "rejuvenation" for this kind of program transformation.

"Source Code Rejuvenation Is Not Refactoring", Pirkelbauer, Dechev & Stroustrup (2010) https://link.springer.com/chapter/10.1007/978-3-642-11266-9_53

"Rejuvenating C++ Programs through Demacrofication", Kumar, Sutton & Stroustrup (2012) https://www.stroustrup.com/icsm-2012-demacro.pdf

"Continuous Source Code Rejuvenation in Continuous Integration Pipelines", Fülöp, Pataki & Szauter (2022) https://ceur-ws.org/Vol-3845/paper08.pdf

Comment From: gopherbot

Change https://go.dev/cl/640576 mentions this issue: gopls/internal/analysis/modernize: replace loop with slice.Contains

Comment From: aclements

Another possible modernizer was mentioned here: Replace ctx, cancel := context.WithCancel(context.Background()); defer cancel() in tests with ctx := t.Context()

Comment From: findleyr

Another possible modernizer was mentioned here: Replace ctx, cancel := context.WithCancel(context.Background()); defer cancel() in tests with ctx := t.Context()

Thanks, that looks relatively straightforward. I'll pick it up.

Comment From: gopherbot

Change https://go.dev/cl/641357 mentions this issue: gopls/internal/analysis/modernize: slicesdelete

Comment From: gopherbot

Change https://go.dev/cl/641440 mentions this issue: gopls/internal/analysis/modernize: replace WithCancel with t.Cancel

Comment From: gopherbot

Change https://go.dev/cl/641795 mentions this issue: gopls: make it easier to install the modernize singlechecker

Comment From: gopherbot

Change https://go.dev/cl/641796 mentions this issue: gopls: filter out informational and hint diagnostics for closed files

Comment From: stapelberg

@findleyr asked me to leave a note here regarding a false-positive modernizer issue I found. With gopls from x/tools at revision 1b796a93b9c5e1f507618de1d18d5908df5e3702, when I put the following code into VS Code:

package main

import "fmt"

func main() {
    cur := make(map[int32]int)
    countsB := map[int32]int{1: 23, 2: 42}
    for k, v := range countsB {
        cur[k] += v
    }
    countsA := map[int32]int{1: 23, 2: 42}
    for k, v := range countsA {
        cur[k] += v
    }
    // prints: map[1:46 2:84]
    fmt.Printf("cur: %+v\n", cur)
}

I get a blue squiggly under both “cur[k]”:

Screenshot 2025-01-10 at 08 58 26

Applying the quick fix changes the code to this:

package main

import "maps"

import "fmt"

func main() {
    cur := make(map[int32]int)
    countsB := map[int32]int{1: 23, 2: 42}
    maps.Copy(cur, countsB)
    countsA := map[int32]int{1: 23, 2: 42}
    maps.Copy(cur, countsA)
    // prints: map[1:46 2:84]
    fmt.Printf("cur: %+v\n", cur)
}

…but the docs of maps.Copy say:

Copy copies all key/value pairs in src adding them to dst. When a key in src is already present in dst, the value in dst will be overwritten by the value associated with the key in src.

…and indeed, the code no longer produces the same result: It now incorrectly prints “map[1:23 2:42]”

Comment From: adonovan

Thanks for the exemplary bug report. I forgot to check the operator used by the AssignStmt--too easily done.

Comment From: gopherbot

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

Comment From: gopherbot

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

Comment From: dsnet

Would #69820 be a good candidate for a modenizer?

Comment From: 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.

Comment From: gopherbot

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

Comment From: 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.

Comment From: adonovan

Thanks; I split this out into its own issue: https://github.com/golang/go/issues/71313

Comment From: findleyr

Thanks; I split this out into its own issue: https://github.com/golang/go/issues/71313

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

Comment From: josharian

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

Comment From: 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.

Comment From: 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.

Comment From: gopherbot

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

Comment From: 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.

Comment From: DmitriyMV

@apocelipes see https://github.com/golang/go/issues/71270

Comment From: gopherbot

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

Comment From: 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.

Comment From: 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.

Comment From: 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?

Comment From: 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.

Comment From: spencerschrock

There's one modernizer false positive reported in this issue already (https://github.com/golang/go/issues/70815#issuecomment-2581999787), 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)
}

Comment From: adonovan

Thanks @spencerschrock; I've split this into its own issue, https://github.com/golang/go/issues/71721.

Comment From: gopherbot

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

Comment From: 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.

Comment From: gopherbot

Change https://go.dev/cl/653356 mentions this issue: internal/stdlib: provide API for import graph of std library

Comment From: gopherbot

Change https://go.dev/cl/653455 mentions this issue: gopls/internal/analysis/modernize: append -> bytes.Clone

Comment From: adonovan

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.

This is now done.

Comment From: adonovan

I have split all remaining tasks out into separate issues, linked from the issue body above, with the exception of

  • use cipher.NewGCMWithRandomNonce where possible (example)

which I suspect is too infrequent to be worth the trouble. Please create new issues for bugs of feature requests; this issue is done.

Comment From: gopherbot

Change https://go.dev/cl/694975 mentions this issue: internal/task: use GenerateAutoSubmitChange in CreateUpdateStdlibIndexCL