Skip to content

x/tools/go/analysis/internal/checker: remove imports that are unused after a batch of fixes are applied #73578

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
cuishuang opened this issue May 2, 2025 · 4 comments
Labels
Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@cuishuang
Copy link
Contributor

Go version

go 1.24.2

Output of go env in your module/workspace:

AR='ar'
CC='clang'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='0'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='clang++'
GCCGO='gccgo'
GO111MODULE='auto'
GOARCH='arm64'
GOARM64='v8.0'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/Users/xxx/Library/Caches/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/Users/xxx/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -arch arm64 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/9t/839s3jmj73bcgyp5x_xh3gw00000gn/T/go-build2973783802=/tmp/go-build -gno-record-gcc-switches -fno-common'
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMOD='/Users/xxxx/20250428/tools/gopls/go.mod'
GOMODCACHE='/Users/xxx/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/xxx/go'
GOPRIVATE=''
GOPROXY='https://goproxy.cn,direct'
GOROOT='/usr/local/go'
GOSUMDB='off'
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/xxx/Library/Application Support/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.24.2'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

package main

import (
	"fmt"
	"sort"
)

func main() {

	a := []uint32{3, 1, 4, 1, 5, 9}
	Uint32Sort(a)

	fmt.Println(a)

}

func Uint32Sort(arr []uint32) {
	sort.Slice(arr, func(i, j int) bool { return arr[i] < arr[j] })
}

Output:

[1 1 3 4 5 9]

What did you see happen?

After executing modernize -fix -test ./..., it will become.

package main

import (
	"fmt"
	"slices"
	"sort"
)

func main() {

	a := []uint32{3, 1, 4, 1, 5, 9}
	Uint32Sort(a)

	fmt.Println(a)

}

func Uint32Sort(arr []uint32) {
	slices.Sort(arr)
}

Output:

# command-line-arguments
./main.go:6:2: "sort" imported and not used

What did you expect to see?

The sort package should be handled correctly; if there are other places in this file using methods from the sort package, they should be retained. Otherwise, they should be removed.

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels May 2, 2025
@gopherbot gopherbot added this to the Unreleased milestone May 2, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/669515 mentions this issue: gopls/internal/analysis/modernize: fix the package import logic for the sortslice analyzer

@adonovan
Copy link
Member

adonovan commented May 5, 2025

This is not a problem of the modernize analyzer, but a general problem across all our refactoring tools that may remove a reference to an imported package. If a fix removes the last reference, the import needs to be deleted. However, the fix may be one of several, and it is only when all of them are composed that the last reference disappears, so the responsibility to remove the import is a property of the collection of fixes. That means it can really only be addressed by the analysis driver. The correct fix is for the driver to (in effect) run goimports when formatting the code after all fixes have been applied.

@adonovan adonovan changed the title x/tools/gopls/internal/analysis/modernize: sortslice analyzer fails to remove unused sort import after replacing sort.Slice x/tools/go/analysis/internal/checker: remove imports that are unused after a batch of fixes are applied May 5, 2025
@adonovan adonovan added Refactoring Issues related to refactoring tools and removed gopls Issues related to the Go language server, gopls. labels May 5, 2025
@adonovan
Copy link
Member

adonovan commented May 5, 2025

Closing as dup of #72035

@adonovan adonovan closed this as completed May 5, 2025
@cuishuang
Copy link
Contributor Author

This is not a problem of the modernize analyzer, but a general problem across all our refactoring tools that may remove a reference to an imported package. If a fix removes the last reference, the import needs to be deleted. However, the fix may be one of several, and it is only when all of them are composed that the last reference disappears, so the responsibility to remove the import is a property of the collection of fixes. That means it can really only be addressed by the analysis driver. The correct fix is for the driver to (in effect) run goimports when formatting the code after all fixes have been applied.

Thanks for your guidance. It should indeed be handled this way.

I tried running modernize -fix -test ./... on this code:

package main

import "fmt"

func main() {
	sourceMap := map[string]int{
		"apple":  1,
		"banana": 2,
		"cherry": 3,
	}

	destMap := map[string]int{
		"banana": 10,
		"date":   20,
	}

	for key, value := range sourceMap {
		destMap[key] = value
	}

	fmt.Println(destMap)
}

After executing modernize -fix -test ./..., it changes to:

package main

import "maps"

import "fmt"

func main() {
	sourceMap := map[string]int{
		"apple":  1,
		"banana": 2,
		"cherry": 3,
	}

	destMap := map[string]int{
		"banana": 10,
		"date":   20,
	}

	maps.Copy(destMap, sourceMap)

	fmt.Println(destMap)
}

It requires explicitly running goimports -w *.go to change it to the expected:

import (
	"fmt"
	"maps"
)

This work should indeed not be done by a single analyzer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

3 participants