Skip to content

x/tools/gopls: modernize slices.Delete suggestion not considering uint type #73663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
anderseknert opened this issue May 10, 2025 · 4 comments
Closed
Labels
gopls Issues related to the Go language server, gopls. ToolProposal Issues describing a requested change to a Go tool or command-line program. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@anderseknert
Copy link

gopls version

v0.18.1

go env

-

What did you do?

package main

import "fmt"

func main() {
	var s = []string{"a", "b", "c"}
	var i uint32 = 1

	s = append(s[:i], s[i+1:]...)

	fmt.Println(s) // [a c]
}
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"

func main() {
	var s = []string{"a", "b", "c"}
	var i uint32 = 1

	s = slices.Delete(s, i, i+1)

	fmt.Println(s)
}
go build
./main.go:11:23: cannot use i (variable of type uint32) as int value in argument to slices.Delete
./main.go:11:26: cannot use i + 1 (value of type uint32) as int value in argument to slices.Delete

What did you expect to see?

That the change wasn't suggested in this case (or possibly with a cast to make it work? I haven't thought much about that).

This issue also encountered when running modernize against OPA, specifically here.

Editor and settings

No response

Logs

No response

@anderseknert anderseknert added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels May 10, 2025
@gopherbot gopherbot added this to the Unreleased milestone May 10, 2025
anderseknert added a commit to anderseknert/opa that referenced this issue May 10, 2025
Earlier this evening I tried to run the Go
[modernize](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize)
analyzer on OPA. That didn't go as planned:

- golang/go#73661
- golang/go#73663

While we wait for that to be fixed, I figured an old-fashioned
search-and-replace across the repo may work for at least the
`interface{}` to `any` conversion. That should help make it easier
to see the other fixes as applied by the modernize tool once it has
had those issues resolved.

Signed-off-by: Anders Eknert <[email protected]>
@gabyhelp
Copy link

@gabyhelp gabyhelp added the ToolProposal Issues describing a requested change to a Go tool or command-line program. label May 10, 2025
anderseknert added a commit to anderseknert/opa that referenced this issue May 11, 2025
Earlier this evening I tried to run the Go
[modernize](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize)
analyzer on OPA. That didn't go as planned:

- golang/go#73661
- golang/go#73663

While we wait for that to be fixed, I figured an old-fashioned
search-and-replace across the repo may work for at least the
`interface{}` to `any` conversion. That should help make it easier
to see the other fixes as applied by the modernize tool once it has
had those issues resolved.

Signed-off-by: Anders Eknert <[email protected]>
@xieyuschen
Copy link
Member

xieyuschen commented May 11, 2025

Thanks for reporting the issue. I believe modernizer needs to check untyped type additionally, so after applying fixes, it won't cause type-check errors. I will send a fix with the similar approach in CL-665195.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/671735 mentions this issue: gopls/internal/analysis/modernize/slicesdelete: convert index type if needed

anderseknert added a commit to anderseknert/opa that referenced this issue May 12, 2025
Earlier this evening I tried to run the Go
[modernize](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize)
analyzer on OPA. That didn't go as planned:

- golang/go#73661
- golang/go#73663

While we wait for that to be fixed, I figured an old-fashioned
search-and-replace across the repo may work for at least the
`interface{}` to `any` conversion. That should help make it easier
to see the other fixes as applied by the modernize tool once it has
had those issues resolved.

Signed-off-by: Anders Eknert <[email protected]>
anderseknert added a commit to anderseknert/opa that referenced this issue May 12, 2025
Earlier this evening I tried to run the Go
[modernize](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize)
analyzer on OPA. That didn't go as planned:

- golang/go#73661
- golang/go#73663

While we wait for that to be fixed, I figured an old-fashioned
search-and-replace across the repo may work for at least the
`interface{}` to `any` conversion. That should help make it easier
to see the other fixes as applied by the modernize tool once it has
had those issues resolved.

Signed-off-by: Anders Eknert <[email protected]>
anderseknert added a commit to anderseknert/opa that referenced this issue May 12, 2025
Earlier this evening I tried to run the Go
[modernize](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize)
analyzer on OPA. That didn't go as planned:

- golang/go#73661
- golang/go#73663

While we wait for that to be fixed, I figured an old-fashioned
search-and-replace across the repo may work for at least the
`interface{}` to `any` conversion. That should help make it easier
to see the other fixes as applied by the modernize tool once it has
had those issues resolved.

Signed-off-by: Anders Eknert <[email protected]>
anderseknert added a commit to open-policy-agent/opa that referenced this issue May 12, 2025
Earlier this evening I tried to run the Go
[modernize](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize)
analyzer on OPA. That didn't go as planned:

- golang/go#73661
- golang/go#73663

While we wait for that to be fixed, I figured an old-fashioned
search-and-replace across the repo may work for at least the
`interface{}` to `any` conversion. That should help make it easier
to see the other fixes as applied by the modernize tool once it has
had those issues resolved.

Signed-off-by: Anders Eknert <[email protected]>
@gopherbot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. ToolProposal Issues describing a requested change to a Go tool or command-line program. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants