gopls/internal/analysis/unusedfunc: use non-heuristic std check This CL changes the logic for detecting whether a package is part of std, from packagepath.IsStdPackage (a purely formal check) to stdlib.HasPackage, which checks the manifest. Also, rename IsStdPackage to MaybeStdPackage to indicate its heuristic nature, flag this in its documentation, and audit all other uses. And fix two tests that had the same latent bug. Fixes golang/go#80555 Change-Id: Ifc2bbc5ebaeddf45f3d59edef9c1a01fbe16ad9d Reviewed-on: https://go-review.googlesource.com/c/tools/+/805440 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alex Putman <aputman@golang.org> Auto-Submit: Alan Donovan <adonovan@google.com>
diff --git a/go/analysis/passes/modernize/modernize.go b/go/analysis/passes/modernize/modernize.go index 2231abf..0483a37 100644 --- a/go/analysis/passes/modernize/modernize.go +++ b/go/analysis/passes/modernize/modernize.go
@@ -123,7 +123,7 @@ // specified standard packages or their dependencies. func within(pass *analysis.Pass, pkgs ...string) bool { path := pass.Pkg.Path() - return packagepath.IsStdPackage(path) && + return packagepath.MaybeStdPackage(path) && moreiters.Contains(stdlib.Dependencies(pkgs...), path) }
diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go index 244f650..f93ec0e 100644 --- a/go/packages/packages_test.go +++ b/go/packages/packages_test.go
@@ -3493,7 +3493,7 @@ type result struct{ Dir, ForTest string } got := make(map[string]result) for pkg := range packages.Postorder(pkgs) { - if !packagepath.IsStdPackage(pkg.PkgPath) { + if !packagepath.MaybeStdPackage(pkg.PkgPath) { rel, err := filepath.Rel(dir, pkg.Dir) if err != nil { t.Errorf("Rel(%q, %q) failed: %v", dir, pkg.Dir, err)
diff --git a/gopls/internal/analysis/unusedfunc/testdata/issue80555.txtar b/gopls/internal/analysis/unusedfunc/testdata/issue80555.txtar new file mode 100644 index 0000000..05bdfa4 --- /dev/null +++ b/gopls/internal/analysis/unusedfunc/testdata/issue80555.txtar
@@ -0,0 +1,11 @@ +Regression test for a false negative on a private module with name +that looks like a standard package. + +-- go.mod -- +module myapp +go 1.21 + +-- a/a.go -- +package a + +func unused() // want `function "unused" is unused`
diff --git a/gopls/internal/analysis/unusedfunc/unusedfunc.go b/gopls/internal/analysis/unusedfunc/unusedfunc.go index 48380c0..7dcf1db 100644 --- a/gopls/internal/analysis/unusedfunc/unusedfunc.go +++ b/gopls/internal/analysis/unusedfunc/unusedfunc.go
@@ -19,8 +19,8 @@ "golang.org/x/tools/internal/analysis/analyzerutil" typeindexanalyzer "golang.org/x/tools/internal/analysis/typeindex" "golang.org/x/tools/internal/astutil" - "golang.org/x/tools/internal/packagepath" "golang.org/x/tools/internal/refactor" + "golang.org/x/tools/internal/stdlib" "golang.org/x/tools/internal/typesinternal/typeindex" ) @@ -74,7 +74,7 @@ func run(pass *analysis.Pass) (any, error) { // The standard library makes heavy use of intrinsics, linknames, etc, // that confuse this algorithm; so skip it (#74130). - if packagepath.IsStdPackage(pass.Pkg.Path()) { + if stdlib.HasPackage(pass.Pkg.Path()) { return nil, nil }
diff --git a/gopls/internal/analysis/unusedfunc/unusedfunc_test.go b/gopls/internal/analysis/unusedfunc/unusedfunc_test.go index db117b1..5f376e1 100644 --- a/gopls/internal/analysis/unusedfunc/unusedfunc_test.go +++ b/gopls/internal/analysis/unusedfunc/unusedfunc_test.go
@@ -5,7 +5,6 @@ package unusedfunc_test import ( - "path/filepath" "testing" "golang.org/x/tools/go/analysis/analysistest" @@ -14,6 +13,6 @@ ) func Test(t *testing.T) { - dir := testfiles.ExtractTxtarFileToTmp(t, filepath.Join(analysistest.TestData(), "basic.txtar")) - analysistest.RunWithSuggestedFixes(t, dir, unusedfunc.Analyzer, "example.com/a") + analysistest.RunWithSuggestedFixes(t, testfiles.ExtractTxtarFileToTmp(t, "testdata/basic.txtar"), unusedfunc.Analyzer, "example.com/a") + analysistest.Run(t, testfiles.ExtractTxtarFileToTmp(t, "testdata/issue80555.txtar"), unusedfunc.Analyzer, "myapp/a") }
diff --git a/gopls/internal/mcp/context.go b/gopls/internal/mcp/context.go index f7ec8ff..13b665d 100644 --- a/gopls/internal/mcp/context.go +++ b/gopls/internal/mcp/context.go
@@ -108,7 +108,7 @@ // Skip the standard library to reduce token usage, operating on // the assumption that the LLM is already familiar with its // symbols and documentation. - if packagepath.IsStdPackage(spec.Path.Value) { + if packagepath.MaybeStdPackage(spec.Path.Value) { continue } toSummarize = append(toSummarize, spec)
diff --git a/gopls/internal/test/marker/testdata/codeaction/extract_variable-toplevel.txt b/gopls/internal/test/marker/testdata/codeaction/extract_variable-toplevel.txt index a5a5e58..d0e3fad 100644 --- a/gopls/internal/test/marker/testdata/codeaction/extract_variable-toplevel.txt +++ b/gopls/internal/test/marker/testdata/codeaction/extract_variable-toplevel.txt
@@ -1,6 +1,9 @@ This test checks the behavior of the 'extract variable/constant' code action at top level (outside any function). See issue #70665. +-- settings.json -- +{"analyses": {"unusedfunc": false}} + -- a.go -- package a
diff --git a/gopls/internal/test/marker/testdata/token/shadowing.txt b/gopls/internal/test/marker/testdata/token/shadowing.txt index 81e91b8..546035c 100644 --- a/gopls/internal/test/marker/testdata/token/shadowing.txt +++ b/gopls/internal/test/marker/testdata/token/shadowing.txt
@@ -2,7 +2,8 @@ -- settings.json -- { - "semanticTokens": true + "semanticTokens": true, + "analyses": {"unusedfunc": false} } -- capabilities.json -- {
diff --git a/internal/analysis/analyzerutil/version.go b/internal/analysis/analyzerutil/version.go index 700d53e..60d7253 100644 --- a/internal/analysis/analyzerutil/version.go +++ b/internal/analysis/analyzerutil/version.go
@@ -38,7 +38,7 @@ // The bootstrap rule does not cover tests, // and some tests (e.g. debug/elf/file_test.go) rely on this. pkgpath := pass.Pkg.Path() - if packagepath.IsStdPackage(pkgpath) && + if packagepath.MaybeStdPackage(pkgpath) && stdlib.IsBootstrapPackage(pkgpath) && // (excludes "*_test" external test packages) !strings.HasSuffix(pass.Fset.File(file.Pos()).Name(), "_test.go") { // (excludes all tests) fileVersion = stdlib.BootstrapVersion.String() // package must bootstrap
diff --git a/internal/packagepath/packagepath.go b/internal/packagepath/packagepath.go index fa39a13..7b25340 100644 --- a/internal/packagepath/packagepath.go +++ b/internal/packagepath/packagepath.go
@@ -36,9 +36,19 @@ return true } -// IsStdPackage reports whether the specified package path belongs to a -// package in the standard library (including internal dependencies). -func IsStdPackage(path string) bool { +// MaybeStdPackage reports whether the specified package path might +// belong to a package in the standard library (including internal +// dependencies), based only on its form. +// +// It may spuriously return true, but a result of false is definitive: +// +// MaybeStdPackage("fmt") = true +// MaybeStdPackage("maybe/tomorrow") = true // false positive +// MaybeStdPackage("example.com/foo") = false +// +// For a definitive answer, use [stdlib.HasPackage], which consults a +// huge table. +func MaybeStdPackage(path string) bool { // A standard package has no dot in its first segment. // (It may yet have a dot, e.g. "vendor/golang.org/x/foo".) slash := strings.IndexByte(path, '/')
diff --git a/internal/packagepath/packagepath_test.go b/internal/packagepath/packagepath_test.go index 18ee7b1..ac7e323 100644 --- a/internal/packagepath/packagepath_test.go +++ b/internal/packagepath/packagepath_test.go
@@ -39,7 +39,7 @@ } } -func TestIsStdPackage(t *testing.T) { +func TestMaybeStdPackage(t *testing.T) { testCases := []struct { pkgpath string isStd bool @@ -49,11 +49,12 @@ {pkgpath: "vendor/golang.org/x/net/dns/dnsmessage", isStd: true}, {pkgpath: "golang.org/x/net/dns/dnsmessage", isStd: false}, {pkgpath: "testdata", isStd: false}, + {pkgpath: "myprivateapp", isStd: true}, // a false positive (go.dev/issue/80555) } for _, tc := range testCases { t.Run(tc.pkgpath, func(t *testing.T) { - got := IsStdPackage(tc.pkgpath) + got := MaybeStdPackage(tc.pkgpath) if got != tc.isStd { t.Fatalf("got %t want %t", got, tc.isStd) }
diff --git a/internal/refactor/imports.go b/internal/refactor/imports.go index 5ce70ae..046038c 100644 --- a/internal/refactor/imports.go +++ b/internal/refactor/imports.go
@@ -125,13 +125,13 @@ var pos token.Pos if gd, ok := decl0.(*ast.GenDecl); ok && gd.Tok == token.IMPORT && gd.Rparen.IsValid() { // Have existing grouped import ( ... ) decl. - if packagepath.IsStdPackage(pkgpath) && len(gd.Specs) > 0 { + if packagepath.MaybeStdPackage(pkgpath) && len(gd.Specs) > 0 { // Add spec for a std package before // first existing spec, followed by // a blank line if the next one is non-std. first := gd.Specs[0].(*ast.ImportSpec) pos = first.Pos() - if !packagepath.IsStdPackage(first.Path.Value) { + if !packagepath.MaybeStdPackage(first.Path.Value) { newText += "\n" } newText += "\n\t"