x/tools/gopls: modernize slices.Clone suggestion wrong when copying between aliased types

[anderseknert]

gopls version

v0.18.1

go env

-

What did you do?

Ran the modernize linter on this example:

package main

import "fmt"

type ssa []string
type ssb []string

var a ssa = ssa{"a", "b", "c"}

func main() {
	var b ssb = append([]string{}, a...)

	fmt.Println(b)
}

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

What did you see happen?

package main

import "slices"

import "fmt"

type ssa []string
type ssb []string

var a ssa = ssa{"a", "b", "c"}

func main() {
	var b ssb = slices.Clone(a)

	fmt.Println(b)
}

go build
./main.go:13:14: cannot use slices.Clone(a) (value of slice type ssa) as ssb value in variable declaration

What did you expect to see?

slices.Clone not being suggested here, or at least not in a form that breaks compilation.

The example is silly, but I encountered this in real code when trying to run modernize against the OPA project, here specifically.

Editor and settings

No response

Logs

No response

[gabyhelp]

Related Issues

• x/tools/gopls/internal/analysis/modernize: append([]T{}, x...) -> slices.Clone(x) fix may unsoundly return nil #73557

Related Code Changes

• all: use slices.Clone or bytes.Clone to simplify code

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[xieyuschen]

Similar to #73663 , I will help to send a fix.

Edit: according to go spec, If f is variadic with a final parameter p of type ...T, then within f the type of p is equivalent to type []T..

The named type ssa could be understood as []string when type checker checks function variadic argument. However, type checker has a stricter rule on slices.Clone function call, which explicitly requires a slice. As ssa is a named type, it's no longer a slice but a separated type.

[anderseknert]

@xieyuschen yeah, var b ssb = append(ssb{}, a...) works too (instead of append to []string{}), but neither can be directly converted to use slices.Clone. Explicit conversion to the common type type (or whatever []string would be called here) works, e.g.

var b ssb = slices.Clone([]string(a))

And perhaps that's the way to go if this should remain a suggestion/fix for this particular case.

[gopherbot]

Change https://go.dev/cl/671915 mentions this issue: gopls/internal/analysis/modernize: appendclipped: narrow down fitted cases

[firelizzard18]

slices.Clone[[]string](a) also works

[xieyuschen]

func Clone[S ~[]E, E any](s S) S returns the S which is the exact same type with input.

However, the append([]string{}, a...) returns a value typed []string, which makes it possible to be implicitly converted to another type here. The current offered fix from append to slices.Slice has missed this case.

[gopherbot]

Change https://go.dev/cl/671975 mentions this issue: gopls/internal/analysis/modernize: preserve return type of append

[firelizzard18]

IMO the correct solution is to update the modernizer so that slices.Clone preserves the return type of append. To that end I opened CL 671915 as a suggested change to @xieyuschen's CL. This updates the modernizer to pass a value for the type parameter S when the base and slice in append(base, slice...) have different types.

[anderseknert]

Whichever solution is merged, thank you both for jumping in here so quickly 👍

[firelizzard18]

We've merged CLs; @anderseknert if you want to test the fix you could check out https://go-review.googlesource.com/c/tools/+/671915 and cd gopls && go install .. Then "Go: Restart Language Server" or restarting/reloading vscode should use the new binary.

[anderseknert]

Thanks!

[anderseknert]

@firelizzard18 I just tried to do that but I'm afraid I got stuck, as this workflow is new to me :)
I've checked out the repo, but can't find any branch matching the change. Which one is it on? (and for future me — where do I see that?)

[firelizzard18]

@anderseknert Gerrit doesn't use branches, at least not for CLs (which are roughly the equivalent to PRs). When you open the CL, there's a menu in the top-right (click Download Patch):

Here's a copy of the first option. You can change change-671915 to whatever you like.

git fetch https://go.googlesource.com/tools refs/changes/15/671915/3 && git checkout -b change-671915 FETCH_HEAD

[anderseknert]

Thanks for those instructions, @firelizzard18 — much appreciated. I guess someone really looked at git and thought another layer of complexity was needed :) All good though, and I'm sure I'll learn to find my way around eventually.

As for the issue, the simple example I provided with this issue seems to convert just fine now. However, the real-world code in OPA where I encountered this first seems to break still, although at least in a new and exciting way :P

Screen.Recording.2025-05-19.at.21.33.38.mov

Repeating the location here for convenience in case you want to use it for a test: https://github.com/open-policy-agent/opa/blob/main/v1/ast/parser.go#L982