From f8f3c13ff3cad44c8cf62bea13bf846e9d7e8b5a Mon Sep 17 00:00:00 2001 From: Tim King Date: Thu, 3 Oct 2024 12:57:35 -0700 Subject: [PATCH 01/95] internal/aliases: add a function to conditionally enable aliases Conditionally enable the gotypesalias GODEBUG setting based on the gotoolchain used to compile the tool. Change-Id: I1e698249f943c8f2cb95c3acbc45aedf64260667 Reviewed-on: https://go-review.googlesource.com/c/tools/+/617636 Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI Commit-Queue: Tim King --- internal/aliases/aliases.go | 54 +++++++++++++++++++++++ internal/aliases/enable_test.go | 76 +++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+) create mode 100644 internal/aliases/enable_test.go diff --git a/internal/aliases/aliases.go b/internal/aliases/aliases.go index b9425f5a209..7598deae1b8 100644 --- a/internal/aliases/aliases.go +++ b/internal/aliases/aliases.go @@ -5,8 +5,13 @@ package aliases import ( + "go/build" "go/token" "go/types" + "os" + "slices" + "strings" + "sync" ) // Package aliases defines backward compatible shims @@ -36,3 +41,52 @@ func NewAlias(enabled bool, pos token.Pos, pkg *types.Package, name string, rhs } return types.NewTypeName(pos, pkg, name, rhs) } + +// ConditionallyEnableGoTypesAlias enables the gotypesalias GODEBUG setting if +// * the version of go used to compile the program is between 1.23 and 1.26, +// * gotypesalias are not already enabled, and +// * gotypesalias is not set in GODEBUG already +// exactly once. Otherwise it does nothing. +// +// Recommended usage is to do the following within a main package: +// +// func init() { ConditionallyEnableGoTypesAlias() } +// +// within a module with go version 1.22 or in GOPATH mode. +func ConditionallyEnableGoTypesAlias() { conditionallyEnableGoTypesAliasOnce() } + +var conditionallyEnableGoTypesAliasOnce = sync.OnceFunc(conditionallyEnableGoTypesAlias) + +func conditionallyEnableGoTypesAlias() { + // Let R be the version of go the program was compiled with. Then + // if R < 1.22, do nothing as gotypesalias is meaningless, + // if R == 1.22, do not turn on gotypesalias (latent bugs), + // if 1.23 <= R && R <= 1.26, turn on gotypesalias, and + // if R >= 1.27, this is a guaranteed no-op. + + if !slices.Contains(build.Default.ReleaseTags, "go1.23") { + return // R < 1.23 (do nothing) + } + if slices.Contains(build.Default.ReleaseTags, "go1.27") { + return // R >= 1.27 (do nothing) + } + // 1.23 <= R <= 1.26 + _, anyIsAlias := types.Universe.Lookup("any").Type().(*types.Alias) + if anyIsAlias { + return // gotypesalias are already enabled. + } + + // Does GODEBUG have gotypesalias set already? + godebugs := strings.Split(os.Getenv("GODEBUG"), ",") + for _, p := range godebugs { + if strings.HasPrefix(strings.TrimSpace(p), "gotypesalias=") { + // gotypesalias is set in GODEBUG already. + // Do not override this setting. + return + } + } + + // Enable gotypesalias. + godebugs = append(godebugs, "gotypesalias=1") + os.Setenv("GODEBUG", strings.Join(godebugs, ",")) +} diff --git a/internal/aliases/enable_test.go b/internal/aliases/enable_test.go new file mode 100644 index 00000000000..3b05960e5f8 --- /dev/null +++ b/internal/aliases/enable_test.go @@ -0,0 +1,76 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package aliases_test + +import ( + "fmt" + "os" + "os/exec" + "runtime" + "strings" + "testing" + + "golang.org/x/tools/internal/aliases" + "golang.org/x/tools/internal/testenv" +) + +func init() { + if os.Getenv("ConditionallyEnableGoTypesAlias_CHILD") == "1" { + go aliases.ConditionallyEnableGoTypesAlias() // Throw in an extra call. Should be fine. + aliases.ConditionallyEnableGoTypesAlias() + } +} + +func TestConditionallyEnableGoTypesAlias(t *testing.T) { + if !(runtime.GOOS == "linux" || runtime.GOOS == "darwin") { + t.Skipf("skipping fork/exec test on this platform") + } + + if os.Getenv("ConditionallyEnableGoTypesAlias_CHILD") == "1" { + // child process + enabled := aliases.Enabled() + fmt.Printf("gotypesalias is enabled %v", enabled) + return + } + + testenv.NeedsTool(t, "go") + + var wants map[string]string + const ( + enabled = "gotypesalias is enabled true" + disabled = "gotypesalias is enabled false" + ) + goversion := testenv.Go1Point() + if goversion <= 22 { + wants = map[string]string{ + "": disabled, + "0": disabled, + "1": enabled, + } + } else { + wants = map[string]string{ + "": enabled, + "0": disabled, + "1": enabled, + } + } + + for _, test := range []string{"", "0", "1"} { + cmd := exec.Command(os.Args[0], "-test.run=TestConditionallyEnableGoTypesAlias") + cmd.Env = append(os.Environ(), "ConditionallyEnableGoTypesAlias_CHILD=1") + if test != "" { + cmd.Env = append(cmd.Env, fmt.Sprintf("GODEBUG=gotypesalias=%s", test)) + } + out, err := cmd.CombinedOutput() + if err != nil { + t.Error(err) + } + want := wants[test] + if !strings.Contains(string(out), want) { + t.Errorf("gotypesalias=%q: want %s", test, want) + t.Logf("(go 1.%d) %q: out=<<%s>>", goversion, test, out) + } + } +} From 89a53114db861f183d705bd429099987759ce138 Mon Sep 17 00:00:00 2001 From: Meng Zhuo Date: Mon, 9 Sep 2024 18:16:35 +0800 Subject: [PATCH 02/95] go/analysis/passes/asmdecl: allow syscall write registers implicitly Syscall will write register implicity, which asmdecl should allow this writes. Fixes golang/go#69352 Change-Id: I7e0dece5428e9ef359b89ce418adc533f7515b1b Reviewed-on: https://go-review.googlesource.com/c/tools/+/611815 Reviewed-by: Michael Knyszek LUCI-TryBot-Result: Go LUCI Reviewed-by: Cherry Mui --- go/analysis/passes/asmdecl/asmdecl.go | 20 +++++++++++++------ .../passes/asmdecl/testdata/src/a/asm.go | 1 + .../passes/asmdecl/testdata/src/a/asm1.s | 7 ++++++- .../passes/asmdecl/testdata/src/a/asm11.s | 6 ++++++ .../passes/asmdecl/testdata/src/a/asm7.s | 6 ++++++ 5 files changed, 33 insertions(+), 7 deletions(-) diff --git a/go/analysis/passes/asmdecl/asmdecl.go b/go/analysis/passes/asmdecl/asmdecl.go index c9ba1a375d3..92671ca0082 100644 --- a/go/analysis/passes/asmdecl/asmdecl.go +++ b/go/analysis/passes/asmdecl/asmdecl.go @@ -57,6 +57,8 @@ type asmArch struct { // include the first integer register and first floating-point register. Accessing // any of them counts as writing to result. retRegs []string + // writeResult is a list of instructions that will change result register implicity. + writeResult []string // calculated during initialization sizes types.Sizes intSize int @@ -85,18 +87,18 @@ type asmVar struct { var ( asmArch386 = asmArch{name: "386", bigEndian: false, stack: "SP", lr: false} asmArchArm = asmArch{name: "arm", bigEndian: false, stack: "R13", lr: true} - asmArchArm64 = asmArch{name: "arm64", bigEndian: false, stack: "RSP", lr: true, retRegs: []string{"R0", "F0"}} - asmArchAmd64 = asmArch{name: "amd64", bigEndian: false, stack: "SP", lr: false, retRegs: []string{"AX", "X0"}} + asmArchArm64 = asmArch{name: "arm64", bigEndian: false, stack: "RSP", lr: true, retRegs: []string{"R0", "F0"}, writeResult: []string{"SVC"}} + asmArchAmd64 = asmArch{name: "amd64", bigEndian: false, stack: "SP", lr: false, retRegs: []string{"AX", "X0"}, writeResult: []string{"SYSCALL"}} asmArchMips = asmArch{name: "mips", bigEndian: true, stack: "R29", lr: true} asmArchMipsLE = asmArch{name: "mipsle", bigEndian: false, stack: "R29", lr: true} asmArchMips64 = asmArch{name: "mips64", bigEndian: true, stack: "R29", lr: true} asmArchMips64LE = asmArch{name: "mips64le", bigEndian: false, stack: "R29", lr: true} - asmArchPpc64 = asmArch{name: "ppc64", bigEndian: true, stack: "R1", lr: true, retRegs: []string{"R3", "F1"}} - asmArchPpc64LE = asmArch{name: "ppc64le", bigEndian: false, stack: "R1", lr: true, retRegs: []string{"R3", "F1"}} - asmArchRISCV64 = asmArch{name: "riscv64", bigEndian: false, stack: "SP", lr: true, retRegs: []string{"X10", "F10"}} + asmArchPpc64 = asmArch{name: "ppc64", bigEndian: true, stack: "R1", lr: true, retRegs: []string{"R3", "F1"}, writeResult: []string{"SYSCALL"}} + asmArchPpc64LE = asmArch{name: "ppc64le", bigEndian: false, stack: "R1", lr: true, retRegs: []string{"R3", "F1"}, writeResult: []string{"SYSCALL"}} + asmArchRISCV64 = asmArch{name: "riscv64", bigEndian: false, stack: "SP", lr: true, retRegs: []string{"X10", "F10"}, writeResult: []string{"ECALL"}} asmArchS390X = asmArch{name: "s390x", bigEndian: true, stack: "R15", lr: true} asmArchWasm = asmArch{name: "wasm", bigEndian: false, stack: "SP", lr: false} - asmArchLoong64 = asmArch{name: "loong64", bigEndian: false, stack: "R3", lr: true, retRegs: []string{"R4", "F0"}} + asmArchLoong64 = asmArch{name: "loong64", bigEndian: false, stack: "R3", lr: true, retRegs: []string{"R4", "F0"}, writeResult: []string{"SYSCALL"}} arches = []*asmArch{ &asmArch386, @@ -351,6 +353,12 @@ Files: } if abi == "ABIInternal" && !haveRetArg { + for _, ins := range archDef.writeResult { + if strings.HasPrefix(line, ins) { + haveRetArg = true + break + } + } for _, reg := range archDef.retRegs { if strings.Contains(line, reg) { haveRetArg = true diff --git a/go/analysis/passes/asmdecl/testdata/src/a/asm.go b/go/analysis/passes/asmdecl/testdata/src/a/asm.go index 1413b74696f..7569d19ceb8 100644 --- a/go/analysis/passes/asmdecl/testdata/src/a/asm.go +++ b/go/analysis/passes/asmdecl/testdata/src/a/asm.go @@ -30,6 +30,7 @@ func returnint() int func returnbyte(x int) byte func returnnamed(x byte) (r1 int, r2 int16, r3 string, r4 byte) func returnintmissing() int +func returnsyscall() func leaf(x, y int) int func noprof(x int) diff --git a/go/analysis/passes/asmdecl/testdata/src/a/asm1.s b/go/analysis/passes/asmdecl/testdata/src/a/asm1.s index c3ef9f40fec..9f17a03da76 100644 --- a/go/analysis/passes/asmdecl/testdata/src/a/asm1.s +++ b/go/analysis/passes/asmdecl/testdata/src/a/asm1.s @@ -307,7 +307,6 @@ TEXT ·returnnamed(SB),0,$0-41 TEXT ·returnintmissing(SB),0,$0-8 RET // want `RET without writing to 8-byte ret\+0\(FP\)` - // issue 15271 TEXT ·f15271(SB), NOSPLIT, $0-4 // Stick 123 into the low 32 bits of X0. @@ -354,6 +353,12 @@ TEXT ·returnmissingABIInternal(SB), NOSPLIT, $32 MOVQ $123, CX RET // want `RET without writing to result register` +// issue 69352 +TEXT ·returnsyscall(SB),0,$0-0 + MOVQ $123, CX + SYSCALL + RET + // return jump TEXT ·retjmp(SB), NOSPLIT, $0-8 RET retjmp1(SB) // It's okay to not write results if there's a tail call. diff --git a/go/analysis/passes/asmdecl/testdata/src/a/asm11.s b/go/analysis/passes/asmdecl/testdata/src/a/asm11.s index e81e8ee179f..3e3f5a36234 100644 --- a/go/analysis/passes/asmdecl/testdata/src/a/asm11.s +++ b/go/analysis/passes/asmdecl/testdata/src/a/asm11.s @@ -11,3 +11,9 @@ TEXT ·returnABIInternal(SB), NOSPLIT, $8 TEXT ·returnmissingABIInternal(SB), NOSPLIT, $8 MOV $123, X20 RET // want `RET without writing to result register` + +// issue 69352 +TEXT ·returnsyscall(SB),0,$0-0 + MOV $123, X20 + ECALL + RET diff --git a/go/analysis/passes/asmdecl/testdata/src/a/asm7.s b/go/analysis/passes/asmdecl/testdata/src/a/asm7.s index 51b5a841313..28bca66ee11 100644 --- a/go/analysis/passes/asmdecl/testdata/src/a/asm7.s +++ b/go/analysis/passes/asmdecl/testdata/src/a/asm7.s @@ -198,3 +198,9 @@ TEXT ·returnABIInternal(SB), NOSPLIT, $8 TEXT ·returnmissingABIInternal(SB), NOSPLIT, $8 MOVD $123, R10 RET // want `RET without writing to result register` + +// issue 69352 +TEXT ·returnsyscall(SB),0,$0-0 + MOVD $123, R10 + SYSCALL + RET From 0b989c812dcb662451db0b70a8267fe7e211886e Mon Sep 17 00:00:00 2001 From: Tim King Date: Wed, 2 Oct 2024 15:51:48 -0700 Subject: [PATCH 03/95] internal/versions: update test expectations Updates test expecations now that go.dev/cl/607955 has been submitted. Change-Id: I795a14f7690026620f270e2db329945286764046 Reviewed-on: https://go-review.googlesource.com/c/tools/+/617635 Reviewed-by: Michael Matloob LUCI-TryBot-Result: Go LUCI Commit-Queue: Tim King --- internal/versions/types_test.go | 205 +++++++++++++++++++++++++++----- 1 file changed, 173 insertions(+), 32 deletions(-) diff --git a/internal/versions/types_test.go b/internal/versions/types_test.go index 377369ffed7..2cd145d2f20 100644 --- a/internal/versions/types_test.go +++ b/internal/versions/types_test.go @@ -11,51 +11,195 @@ import ( "go/parser" "go/token" "go/types" + "strings" "testing" + "golang.org/x/tools/internal/testenv" "golang.org/x/tools/internal/versions" ) +var contents = map[string]string{ + "gobuild122.go": ` +//go:build go1.22 +package p +`, + "gobuild121.go": ` +//go:build go1.21 +package p +`, + "gobuild120.go": ` +//go:build go1.20 +package p +`, + "gobuild119.go": ` +//go:build go1.19 +package p +`, + "noversion.go": ` +package p +`, +} + func Test(t *testing.T) { - var contents = map[string]string{ - "gobuild.go": ` - //go:build go1.23 - package p - `, - "noversion.go": ` - package p - `, + testenv.NeedsGo1Point(t, 23) // TODO(#69749): Allow on 1.22 if a fix for #69749 is submitted. + + for _, item := range []struct { + goversion string + pversion string + tests []fileTest + }{ + { + "", "", []fileTest{ + {"noversion.go", ""}, + {"gobuild119.go", "go1.21"}, + {"gobuild120.go", "go1.21"}, + {"gobuild121.go", "go1.21"}, + {"gobuild122.go", "go1.22"}}, + }, + { + "go1.20", "go1.20", []fileTest{ + {"noversion.go", "go1.20"}, + {"gobuild119.go", "go1.21"}, + {"gobuild120.go", "go1.21"}, + {"gobuild121.go", "go1.21"}, + {"gobuild122.go", "go1.22"}}, + }, + { + "go1.21", "go1.21", []fileTest{ + {"noversion.go", "go1.21"}, + {"gobuild119.go", "go1.21"}, + {"gobuild120.go", "go1.21"}, + {"gobuild121.go", "go1.21"}, + {"gobuild122.go", "go1.22"}}, + }, + { + "go1.22", "go1.22", []fileTest{ + {"noversion.go", "go1.22"}, + {"gobuild119.go", "go1.21"}, + {"gobuild120.go", "go1.21"}, + {"gobuild121.go", "go1.21"}, + {"gobuild122.go", "go1.22"}}, + }, + } { + name := fmt.Sprintf("types.Config{GoVersion:%q}", item.goversion) + t.Run(name, func(t *testing.T) { + testFiles(t, item.goversion, item.pversion, item.tests) + }) } - type fileTest struct { - fname string - want string +} + +func TestToolchain122(t *testing.T) { + // TestToolchain122 tests the 1.22 toolchain for the FileVersion it returns. + // These results are at the moment unique to 1.22. So test it with distinct + // expectations. + + // TODO(#69749): Remove requirement if a fix for #69749 is submitted. + if testenv.Go1Point() != 22 { + t.Skip("Expectations are only for 1.22 toolchain") } + for _, item := range []struct { goversion string pversion string tests []fileTest }{ - // {"", "", []fileTest{{"noversion.go", ""}, {"gobuild.go", ""}}}, // TODO(matloob): re-enable this test (with modifications) once CL 607955 has been submitted - {"go1.22", "go1.22", []fileTest{{"noversion.go", "go1.22"}, {"gobuild.go", "go1.23"}}}, + { + "", "", []fileTest{ + {"noversion.go", ""}, + {"gobuild119.go", ""}, // differs + {"gobuild120.go", ""}, // differs + {"gobuild121.go", ""}, // differs + {"gobuild122.go", ""}}, // differs + }, + { + "go1.20", "go1.20", []fileTest{ + {"noversion.go", "go1.20"}, + {"gobuild119.go", "go1.20"}, // differs + {"gobuild120.go", "go1.20"}, // differs + {"gobuild121.go", "go1.21"}, + {"gobuild122.go", "go1.22"}}, + }, + { + "go1.21", "go1.21", []fileTest{ + {"noversion.go", "go1.21"}, + {"gobuild119.go", "go1.19"}, // differs + {"gobuild120.go", "go1.20"}, // differs + {"gobuild121.go", "go1.21"}, + {"gobuild122.go", "go1.22"}}, + }, + { + "go1.22", "go1.22", []fileTest{ + {"noversion.go", "go1.22"}, + {"gobuild119.go", "go1.19"}, // differs + {"gobuild120.go", "go1.20"}, // differs + {"gobuild121.go", "go1.21"}, + {"gobuild122.go", "go1.22"}}, + }, } { name := fmt.Sprintf("types.Config{GoVersion:%q}", item.goversion) + t.Run(name, func(t *testing.T) { + testFiles(t, item.goversion, item.pversion, item.tests) + }) + } +} + +type fileTest struct { + fname string + want string +} + +func testFiles(t *testing.T, goversion string, pversion string, tests []fileTest) { + + fset := token.NewFileSet() + files := make([]*ast.File, len(tests)) + for i, test := range tests { + files[i] = parse(t, fset, test.fname, contents[test.fname]) + } + pkg, info, err := typeCheck(fset, files, goversion) + if err != nil { + t.Fatal(err) + } + if got, want := pkg.GoVersion(), pversion; versions.Compare(got, want) != 0 { + t.Errorf("GoVersion()=%q. expected %q", got, want) + } + if got := versions.FileVersion(info, nil); got != "" { + t.Errorf(`FileVersions(nil)=%q. expected ""`, got) + } + for i, test := range tests { + if got, want := versions.FileVersion(info, files[i]), test.want; got != want { + t.Errorf("FileVersions(%s)=%q. expected %q", test.fname, got, want) + } + } +} + +func TestTooNew(t *testing.T) { + testenv.NeedsGo1Point(t, 23) // TODO(#69749): Allow on 1.22 if a fix for #69749 is submitted. + + const contents = ` + //go:build go1.99 + package p + ` + type fileTest struct { + fname string + want string + } + + for _, goversion := range []string{ + "", + "go1.22", + } { + name := fmt.Sprintf("types.Config{GoVersion:%q}", goversion) t.Run(name, func(t *testing.T) { fset := token.NewFileSet() - files := make([]*ast.File, len(item.tests)) - for i, test := range item.tests { - files[i] = parse(t, fset, test.fname, contents[test.fname]) - } - pkg, info := typeCheck(t, fset, files, item.goversion) - if got, want := pkg.GoVersion(), item.pversion; versions.Compare(got, want) != 0 { - t.Errorf("GoVersion()=%q. expected %q", got, want) - } - if got := versions.FileVersion(info, nil); got != "" { - t.Errorf(`FileVersions(nil)=%q. expected ""`, got) + files := []*ast.File{parse(t, fset, "p.go", contents)} + _, _, err := typeCheck(fset, files, goversion) + if err == nil { + t.Fatal("Expected an error from a using a TooNew file version") } - for i, test := range item.tests { - if got, want := versions.FileVersion(info, files[i]), test.want; got != want { - t.Errorf("FileVersions(%s)=%q. expected %q", test.fname, got, want) - } + got := err.Error() + want := "file requires newer Go version go1.99" + if !strings.Contains(got, want) { + t.Errorf("Error message %q did not include %q", got, want) } }) } @@ -69,7 +213,7 @@ func parse(t *testing.T, fset *token.FileSet, name, src string) *ast.File { return file } -func typeCheck(t *testing.T, fset *token.FileSet, files []*ast.File, goversion string) (*types.Package, *types.Info) { +func typeCheck(fset *token.FileSet, files []*ast.File, goversion string) (*types.Package, *types.Info, error) { conf := types.Config{ Importer: importer.Default(), GoVersion: goversion, @@ -77,8 +221,5 @@ func typeCheck(t *testing.T, fset *token.FileSet, files []*ast.File, goversion s info := types.Info{} versions.InitFileVersions(&info) pkg, err := conf.Check("", fset, files, &info) - if err != nil { - t.Fatal(err) - } - return pkg, &info + return pkg, &info, err } From de11c555dbd50b89094ccf55408d69f21192408a Mon Sep 17 00:00:00 2001 From: Jack Baldry Date: Sun, 6 Oct 2024 17:46:07 +0000 Subject: [PATCH 04/95] gopls/doc/codelenses: fix link typo Change-Id: I1105cec5cf851932a15410a674196e32c97ac832 GitHub-Last-Rev: a3242676494e94f94c050b52fd62ef874421edc5 GitHub-Pull-Request: golang/tools#527 Reviewed-on: https://go-review.googlesource.com/c/tools/+/617617 LUCI-TryBot-Result: Go LUCI Reviewed-by: Robert Findley Reviewed-by: Ian Lance Taylor Auto-Submit: Ian Lance Taylor Reviewed-by: Tim King --- gopls/doc/codelenses.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gopls/doc/codelenses.md b/gopls/doc/codelenses.md index 71425ce09c6..0930076bec6 100644 --- a/gopls/doc/codelenses.md +++ b/gopls/doc/codelenses.md @@ -16,7 +16,7 @@ Their features are subject to change. Client support: - **VS Code**: Code Lenses appear as small text links above a line of source code. -- **Emacs + eglot**: Not supported, but prototype exists at https://github.joaotavora/eglot/pull/71. +- **Emacs + eglot**: Not supported, but prototype exists at https://github.com/joaotavora/eglot/pull/71. - **Vim + coc.nvim**: ?? - **CLI**: `gopls codelens`. For example, `gopls codelens -exec file.go:123 "run test"` runs the test at the specified line. From a30b2075cac0157b6d73110a6bdc5135fcb98a63 Mon Sep 17 00:00:00 2001 From: Tim King Date: Sat, 5 Oct 2024 14:39:40 -0700 Subject: [PATCH 05/95] internal/versions: remove InitFileVersions InitFileVersions is not longer conditionally needed at x/tools supports Go >= 1.22. Change-Id: I5df1b46463d23acda569e1378027d542ac749f93 Reviewed-on: https://go-review.googlesource.com/c/tools/+/617637 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan Commit-Queue: Tim King --- go/analysis/unitchecker/unitchecker.go | 17 ++++++++--------- go/loader/loader.go | 17 ++++++++--------- go/packages/packages.go | 19 +++++++++---------- go/ssa/ssautil/load.go | 17 ++++++++--------- go/types/typeutil/callee_test.go | 9 ++++----- .../analysis/infertypeargs/infertypeargs.go | 5 ++--- gopls/internal/cache/analysis.go | 17 ++++++++--------- gopls/internal/cache/check.go | 16 ++++++++-------- gopls/internal/golang/change_signature.go | 17 ++++++++--------- gopls/internal/golang/identifier_test.go | 16 +++++++--------- internal/versions/types.go | 5 ----- internal/versions/types_test.go | 5 +++-- internal/versions/versions_test.go | 5 +++-- refactor/satisfy/find_test.go | 17 ++++++++--------- 14 files changed, 84 insertions(+), 98 deletions(-) diff --git a/go/analysis/unitchecker/unitchecker.go b/go/analysis/unitchecker/unitchecker.go index 71ebbfaef15..2301ccfc0e4 100644 --- a/go/analysis/unitchecker/unitchecker.go +++ b/go/analysis/unitchecker/unitchecker.go @@ -51,7 +51,6 @@ import ( "golang.org/x/tools/go/analysis/internal/analysisflags" "golang.org/x/tools/internal/analysisinternal" "golang.org/x/tools/internal/facts" - "golang.org/x/tools/internal/versions" ) // A Config describes a compilation unit to be analyzed. @@ -257,15 +256,15 @@ func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]re GoVersion: cfg.GoVersion, } info := &types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Defs: make(map[*ast.Ident]types.Object), - Uses: make(map[*ast.Ident]types.Object), - Implicits: make(map[ast.Node]types.Object), - Instances: make(map[*ast.Ident]types.Instance), - Scopes: make(map[ast.Node]*types.Scope), - Selections: make(map[*ast.SelectorExpr]*types.Selection), + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Instances: make(map[*ast.Ident]types.Instance), + Scopes: make(map[ast.Node]*types.Scope), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + FileVersions: make(map[*ast.File]string), } - versions.InitFileVersions(info) pkg, err := tc.Check(cfg.ImportPath, fset, files, info) if err != nil { diff --git a/go/loader/loader.go b/go/loader/loader.go index 013c0f505bb..5a260e0d880 100644 --- a/go/loader/loader.go +++ b/go/loader/loader.go @@ -23,7 +23,6 @@ import ( "golang.org/x/tools/go/ast/astutil" "golang.org/x/tools/go/internal/cgo" - "golang.org/x/tools/internal/versions" ) var ignoreVendor build.ImportMode @@ -1029,18 +1028,18 @@ func (imp *importer) newPackageInfo(path, dir string) *PackageInfo { info := &PackageInfo{ Pkg: pkg, Info: types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Defs: make(map[*ast.Ident]types.Object), - Uses: make(map[*ast.Ident]types.Object), - Implicits: make(map[ast.Node]types.Object), - Instances: make(map[*ast.Ident]types.Instance), - Scopes: make(map[ast.Node]*types.Scope), - Selections: make(map[*ast.SelectorExpr]*types.Selection), + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Instances: make(map[*ast.Ident]types.Instance), + Scopes: make(map[ast.Node]*types.Scope), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + FileVersions: make(map[*ast.File]string), }, errorFunc: imp.conf.TypeChecker.Error, dir: dir, } - versions.InitFileVersions(&info.Info) // Copy the types.Config so we can vary it across PackageInfos. tc := imp.conf.TypeChecker diff --git a/go/packages/packages.go b/go/packages/packages.go index f227f1bab10..fc6ac1e0671 100644 --- a/go/packages/packages.go +++ b/go/packages/packages.go @@ -31,7 +31,6 @@ import ( "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/packagesinternal" "golang.org/x/tools/internal/typesinternal" - "golang.org/x/tools/internal/versions" ) // A LoadMode controls the amount of detail to return when loading. @@ -1171,15 +1170,15 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) { } lpkg.TypesInfo = &types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Defs: make(map[*ast.Ident]types.Object), - Uses: make(map[*ast.Ident]types.Object), - Implicits: make(map[ast.Node]types.Object), - Instances: make(map[*ast.Ident]types.Instance), - Scopes: make(map[ast.Node]*types.Scope), - Selections: make(map[*ast.SelectorExpr]*types.Selection), - } - versions.InitFileVersions(lpkg.TypesInfo) + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Instances: make(map[*ast.Ident]types.Instance), + Scopes: make(map[ast.Node]*types.Scope), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + FileVersions: make(map[*ast.File]string), + } lpkg.TypesSizes = ld.sizes importer := importerFunc(func(path string) (*types.Package, error) { diff --git a/go/ssa/ssautil/load.go b/go/ssa/ssautil/load.go index 51fba054541..c64b03f177f 100644 --- a/go/ssa/ssautil/load.go +++ b/go/ssa/ssautil/load.go @@ -13,7 +13,6 @@ import ( "golang.org/x/tools/go/packages" "golang.org/x/tools/go/ssa" - "golang.org/x/tools/internal/versions" ) // Packages creates an SSA program for a set of packages. @@ -134,15 +133,15 @@ func BuildPackage(tc *types.Config, fset *token.FileSet, pkg *types.Package, fil } info := &types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Defs: make(map[*ast.Ident]types.Object), - Uses: make(map[*ast.Ident]types.Object), - Implicits: make(map[ast.Node]types.Object), - Instances: make(map[*ast.Ident]types.Instance), - Scopes: make(map[ast.Node]*types.Scope), - Selections: make(map[*ast.SelectorExpr]*types.Selection), + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Instances: make(map[*ast.Ident]types.Instance), + Scopes: make(map[ast.Node]*types.Scope), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + FileVersions: make(map[*ast.File]string), } - versions.InitFileVersions(info) if err := types.NewChecker(tc, fset, pkg, info).Files(files); err != nil { return nil, nil, err } diff --git a/go/types/typeutil/callee_test.go b/go/types/typeutil/callee_test.go index faee0f88721..1d48bc743a9 100644 --- a/go/types/typeutil/callee_test.go +++ b/go/types/typeutil/callee_test.go @@ -14,7 +14,6 @@ import ( "testing" "golang.org/x/tools/go/types/typeutil" - "golang.org/x/tools/internal/versions" ) func TestStaticCallee(t *testing.T) { @@ -122,11 +121,11 @@ func testStaticCallee(t *testing.T, contents []string) { packages := make(map[string]*types.Package) cfg := &types.Config{Importer: closure(packages)} info := &types.Info{ - Instances: make(map[*ast.Ident]types.Instance), - Uses: make(map[*ast.Ident]types.Object), - Selections: make(map[*ast.SelectorExpr]*types.Selection), + Instances: make(map[*ast.Ident]types.Instance), + Uses: make(map[*ast.Ident]types.Object), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + FileVersions: make(map[*ast.File]string), } - versions.InitFileVersions(info) var files []*ast.File for i, content := range contents { diff --git a/gopls/internal/analysis/infertypeargs/infertypeargs.go b/gopls/internal/analysis/infertypeargs/infertypeargs.go index 9a514ad620c..0ce43e67079 100644 --- a/gopls/internal/analysis/infertypeargs/infertypeargs.go +++ b/gopls/internal/analysis/infertypeargs/infertypeargs.go @@ -13,7 +13,6 @@ import ( "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/internal/typeparams" - "golang.org/x/tools/internal/versions" ) const Doc = `check for unnecessary type arguments in call expressions @@ -91,9 +90,9 @@ func diagnose(fset *token.FileSet, inspect *inspector.Inspector, start, end toke Rparen: call.Rparen, } info := &types.Info{ - Instances: make(map[*ast.Ident]types.Instance), + Instances: make(map[*ast.Ident]types.Instance), + FileVersions: make(map[*ast.File]string), } - versions.InitFileVersions(info) if err := types.CheckExpr(fset, pkg, call.Pos(), newCall, info); err != nil { // Most likely inference failed. break diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index 9debc609048..b18c30fd7ee 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -50,7 +50,6 @@ import ( "golang.org/x/tools/internal/facts" "golang.org/x/tools/internal/gcimporter" "golang.org/x/tools/internal/typesinternal" - "golang.org/x/tools/internal/versions" ) /* @@ -965,17 +964,17 @@ func (an *analysisNode) typeCheck(parsed []*parsego.File) *analysisPackage { compiles: len(mp.Errors) == 0, // false => list error types: types.NewPackage(string(mp.PkgPath), string(mp.Name)), typesInfo: &types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Defs: make(map[*ast.Ident]types.Object), - Instances: make(map[*ast.Ident]types.Instance), - Implicits: make(map[ast.Node]types.Object), - Selections: make(map[*ast.SelectorExpr]*types.Selection), - Scopes: make(map[ast.Node]*types.Scope), - Uses: make(map[*ast.Ident]types.Object), + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Instances: make(map[*ast.Ident]types.Instance), + Implicits: make(map[ast.Node]types.Object), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + Scopes: make(map[ast.Node]*types.Scope), + Uses: make(map[*ast.Ident]types.Object), + FileVersions: make(map[*ast.File]string), }, typesSizes: mp.TypesSizes, } - versions.InitFileVersions(pkg.typesInfo) // Unsafe has no syntax. if mp.PkgPath == "unsafe" { diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index 08d57f4e657..f93a8ab42ad 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -1544,16 +1544,16 @@ func (b *typeCheckBatch) checkPackage(ctx context.Context, ph *packageHandle) (* types: types.NewPackage(string(inputs.pkgPath), string(inputs.name)), typesSizes: inputs.sizes, typesInfo: &types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Defs: make(map[*ast.Ident]types.Object), - Uses: make(map[*ast.Ident]types.Object), - Implicits: make(map[ast.Node]types.Object), - Instances: make(map[*ast.Ident]types.Instance), - Selections: make(map[*ast.SelectorExpr]*types.Selection), - Scopes: make(map[ast.Node]*types.Scope), + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Instances: make(map[*ast.Ident]types.Instance), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + Scopes: make(map[ast.Node]*types.Scope), + FileVersions: make(map[*ast.File]string), }, } - versions.InitFileVersions(pkg.typesInfo) // Collect parsed files from the type check pass, capturing parse errors from // compiled files. diff --git a/gopls/internal/golang/change_signature.go b/gopls/internal/golang/change_signature.go index 72cbe4c2d90..41c56ba6c2c 100644 --- a/gopls/internal/golang/change_signature.go +++ b/gopls/internal/golang/change_signature.go @@ -28,7 +28,6 @@ import ( "golang.org/x/tools/internal/refactor/inline" "golang.org/x/tools/internal/tokeninternal" "golang.org/x/tools/internal/typesinternal" - "golang.org/x/tools/internal/versions" ) // RemoveUnusedParameter computes a refactoring to remove the parameter @@ -482,15 +481,15 @@ func rewriteCalls(ctx context.Context, rw signatureRewrite) (map[protocol.Docume func reTypeCheck(logf func(string, ...any), orig *cache.Package, fileMask map[protocol.DocumentURI]*ast.File, expectErrors bool) (*types.Package, *types.Info, error) { pkg := types.NewPackage(string(orig.Metadata().PkgPath), string(orig.Metadata().Name)) info := &types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Defs: make(map[*ast.Ident]types.Object), - Uses: make(map[*ast.Ident]types.Object), - Implicits: make(map[ast.Node]types.Object), - Selections: make(map[*ast.SelectorExpr]*types.Selection), - Scopes: make(map[ast.Node]*types.Scope), - Instances: make(map[*ast.Ident]types.Instance), + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + Scopes: make(map[ast.Node]*types.Scope), + Instances: make(map[*ast.Ident]types.Instance), + FileVersions: make(map[*ast.File]string), } - versions.InitFileVersions(info) { var files []*ast.File for _, pgf := range orig.CompiledGoFiles() { diff --git a/gopls/internal/golang/identifier_test.go b/gopls/internal/golang/identifier_test.go index d78d8fe99f5..8206d8731ae 100644 --- a/gopls/internal/golang/identifier_test.go +++ b/gopls/internal/golang/identifier_test.go @@ -11,8 +11,6 @@ import ( "go/token" "go/types" "testing" - - "golang.org/x/tools/internal/versions" ) func TestSearchForEnclosing(t *testing.T) { @@ -95,13 +93,13 @@ func posAt(line, column int, fset *token.FileSet, fname string) token.Pos { // newInfo returns a types.Info with all maps populated. func newInfo() *types.Info { info := &types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Defs: make(map[*ast.Ident]types.Object), - Uses: make(map[*ast.Ident]types.Object), - Implicits: make(map[ast.Node]types.Object), - Selections: make(map[*ast.SelectorExpr]*types.Selection), - Scopes: make(map[ast.Node]*types.Scope), + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + Scopes: make(map[ast.Node]*types.Scope), + FileVersions: make(map[*ast.File]string), } - versions.InitFileVersions(info) return info } diff --git a/internal/versions/types.go b/internal/versions/types.go index f0bb0d15f03..0fc10ce4eb5 100644 --- a/internal/versions/types.go +++ b/internal/versions/types.go @@ -31,8 +31,3 @@ func FileVersion(info *types.Info, file *ast.File) string { // This would act as a max version on what a tool can support. return Future } - -// InitFileVersions initializes info to record Go versions for Go files. -func InitFileVersions(info *types.Info) { - info.FileVersions = make(map[*ast.File]string) -} diff --git a/internal/versions/types_test.go b/internal/versions/types_test.go index 2cd145d2f20..bf459a5829c 100644 --- a/internal/versions/types_test.go +++ b/internal/versions/types_test.go @@ -218,8 +218,9 @@ func typeCheck(fset *token.FileSet, files []*ast.File, goversion string) (*types Importer: importer.Default(), GoVersion: goversion, } - info := types.Info{} - versions.InitFileVersions(&info) + info := types.Info{ + FileVersions: make(map[*ast.File]string), + } pkg, err := conf.Check("", fset, files, &info) return pkg, &info, err } diff --git a/internal/versions/versions_test.go b/internal/versions/versions_test.go index 0886f8c80be..2599b8f26e5 100644 --- a/internal/versions/versions_test.go +++ b/internal/versions/versions_test.go @@ -205,8 +205,9 @@ func TestFileVersions(t *testing.T) { {GoVersion: versions.Go1_22}, {}, // GoVersion is unset. } { - info := &types.Info{} - versions.InitFileVersions(info) + info := &types.Info{ + FileVersions: make(map[*ast.File]string), + } _, err = conf.Check("P", fset, []*ast.File{f}, info) if err != nil { diff --git a/refactor/satisfy/find_test.go b/refactor/satisfy/find_test.go index daa8b219ef2..cb755601c78 100644 --- a/refactor/satisfy/find_test.go +++ b/refactor/satisfy/find_test.go @@ -15,7 +15,6 @@ import ( "sort" "testing" - "golang.org/x/tools/internal/versions" "golang.org/x/tools/refactor/satisfy" ) @@ -216,15 +215,15 @@ func constraints(t *testing.T, src string) []string { // type-check info := &types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Defs: make(map[*ast.Ident]types.Object), - Uses: make(map[*ast.Ident]types.Object), - Implicits: make(map[ast.Node]types.Object), - Instances: make(map[*ast.Ident]types.Instance), - Scopes: make(map[ast.Node]*types.Scope), - Selections: make(map[*ast.SelectorExpr]*types.Selection), + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Instances: make(map[*ast.Ident]types.Instance), + Scopes: make(map[ast.Node]*types.Scope), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + FileVersions: make(map[*ast.File]string), } - versions.InitFileVersions(info) conf := types.Config{ Importer: importer.Default(), } From c19060b012d905a012532dd55dddab9725802971 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 19 Sep 2024 16:49:05 +0000 Subject: [PATCH 06/95] gopls/internal/cache: use packageHandles to hold an active package cache Previously, gopls was keeping a common import graph up to date between snapshots, to optimize re-typechecking open packages. However, this had several downsides: - It required rather complicated accounting - Working in package A could get slower after opening package B, if package B was in the forward closure of A, since it reduced the common import graph. - Since imports were constructed a-priori to type checking, we imported *all* packages in the forward closure of open packages, even if they wouldn't be needed for type checking, inflating the memory footprint. This CL changes gopls to instead keep track of the active package on the packageHandle, and invalidate its imports independently of other active packages. As a result, we can eliminate the complicated importGraph logic and the associated relationships between open packages. We also reduce memory by holding on to a minimal set of imports for open packages. Change-Id: I82c49bb7002ab748497f34b43844b34176bdef9c Reviewed-on: https://go-review.googlesource.com/c/tools/+/614165 Auto-Submit: Robert Findley LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan --- gopls/internal/cache/check.go | 618 ++++++++---------- gopls/internal/cache/load.go | 1 - gopls/internal/cache/session.go | 1 - gopls/internal/cache/snapshot.go | 73 +-- .../test/integration/misc/definition_test.go | 46 ++ gopls/internal/util/persistent/map.go | 3 + 6 files changed, 313 insertions(+), 429 deletions(-) diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index f93a8ab42ad..99ca025eecd 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -58,9 +58,11 @@ type typeCheckBatch struct { // handleMu guards _handles, which must only be accessed via addHandles or // getHandle. // - // TODO(rfindley): refactor such that we can simply prepare the type checking - // pass by ensuring that handles are present on the Snapshot, and access them - // directly, rather than copying maps for each caller. + // An alternative would be to simply verify that package handles are present + // on the Snapshot, and access them directly, rather than copying maps for + // each caller. However, handles are accessed very frequently during type + // checking, and ordinary go maps are measurably faster than the + // persistent.Map used to store handles on the snapshot. handleMu sync.Mutex _handles map[PackageID]*packageHandle @@ -77,13 +79,9 @@ func (b *typeCheckBatch) addHandles(handles map[PackageID]*packageHandle) { b.handleMu.Lock() defer b.handleMu.Unlock() for id, ph := range handles { - assert(ph.state == validKey, "invalid handle") - if alt, ok := b._handles[id]; ok { - // Once handles have been reevaluated, they should not change. Therefore, - // we should only ever encounter exactly one handle instance for a given - // ID. - assert(alt == ph, "mismatching handle") - } else { + assert(ph.state >= validKey, "invalid handle") + + if alt, ok := b._handles[id]; !ok || alt.state < ph.state { b._handles[id] = ph } } @@ -136,209 +134,6 @@ func (s *Snapshot) TypeCheck(ctx context.Context, ids ...PackageID) ([]*Package, return pkgs, s.forEachPackage(ctx, ids, nil, post) } -// getImportGraph returns a shared import graph use for this snapshot, or nil. -// -// This is purely an optimization: holding on to more imports allows trading -// memory for CPU and latency. Currently, getImportGraph returns an import -// graph containing all packages imported by open packages, since these are -// highly likely to be needed when packages change. -// -// Furthermore, since we memoize active packages, including their imports in -// the shared import graph means we don't run the risk of pinning duplicate -// copies of common imports, if active packages are computed in separate type -// checking batches. -func (s *Snapshot) getImportGraph(ctx context.Context) *importGraph { - if !preserveImportGraph { - return nil - } - s.mu.Lock() - - // Evaluate the shared import graph for the snapshot. There are three major - // codepaths here: - // - // 1. importGraphDone == nil, importGraph == nil: it is this goroutine's - // responsibility to type-check the shared import graph. - // 2. importGraphDone == nil, importGraph != nil: it is this goroutine's - // responsibility to resolve the import graph, which may result in - // type-checking only if the existing importGraph (carried over from the - // preceding snapshot) is invalid. - // 3. importGraphDone != nil: some other goroutine is doing (1) or (2), wait - // for the work to be done. - done := s.importGraphDone - if done == nil { - done = make(chan unit) - s.importGraphDone = done - release := s.Acquire() // must acquire to use the snapshot asynchronously - go func() { - defer release() - importGraph, err := s.resolveImportGraph() // may be nil - if err != nil { - // resolveImportGraph operates on the background context, because it is - // a shared resource across the snapshot. If the snapshot is cancelled, - // don't log an error. - if s.backgroundCtx.Err() == nil { - event.Error(ctx, "computing the shared import graph", err) - } - importGraph = nil - } - s.mu.Lock() - s.importGraph = importGraph - s.mu.Unlock() - close(done) - }() - } - s.mu.Unlock() - - select { - case <-done: - return s.importGraph - case <-ctx.Done(): - return nil - } -} - -// resolveImportGraph evaluates the shared import graph to use for -// type-checking in this snapshot. This may involve re-using the import graph -// of the previous snapshot (stored in s.importGraph), or computing a fresh -// import graph. -// -// resolveImportGraph should only be called from getImportGraph. -// -// TODO(rfindley): resolveImportGraph can be eliminated (greatly simplifying -// things) by instead holding on to imports of open packages after each type -// checking pass. -func (s *Snapshot) resolveImportGraph() (*importGraph, error) { - ctx := s.backgroundCtx - ctx, done := event.Start(event.Detach(ctx), "cache.resolveImportGraph") - defer done() - - s.mu.Lock() - lastImportGraph := s.importGraph - g := s.meta - s.mu.Unlock() - - openPackages := make(map[PackageID]bool) - for _, fh := range s.Overlays() { - // golang/go#66145: don't call MetadataForFile here. This function, which - // builds a shared import graph, is an optimization. We don't want it to - // have the side effect of triggering a load. - // - // In the past, a call to MetadataForFile here caused a bunch of - // unnecessary loads in multi-root workspaces (and as a result, spurious - // diagnostics). - var mps []*metadata.Package - for _, id := range g.IDs[fh.URI()] { - mps = append(mps, g.Packages[id]) - } - metadata.RemoveIntermediateTestVariants(&mps) - for _, mp := range mps { - openPackages[mp.ID] = true - } - } - - var openPackageIDs []PackageID - for id := range openPackages { - openPackageIDs = append(openPackageIDs, id) - } - - // Subtlety: we erase the upward cone of open packages from the shared import - // graph, to increase reusability. - // - // This is easiest to understand via an example: suppose A imports B, and B - // imports C. Now suppose A and B are open. If we preserve the entire set of - // shared deps by open packages, deps will be {B, C}. But this means that any - // change to the open package B will invalidate the shared import graph, - // meaning we will experience no benefit from sharing when B is edited. - // Consider that this will be a common scenario, when A is foo_test and B is - // foo. Better to just preserve the shared import C. - // - // With precise pruning, we may want to truncate this search based on - // reachability. - // - // TODO(rfindley): this logic could use a unit test. - volatile := make(map[PackageID]bool) - var isVolatile func(PackageID) bool - isVolatile = func(id PackageID) (v bool) { - v, ok := volatile[id] - if !ok { - volatile[id] = false // defensive: break cycles - for _, dep := range g.Packages[id].DepsByPkgPath { - if isVolatile(dep) { - v = true - // Keep going, to ensure that we traverse all dependencies. - } - } - if openPackages[id] { - v = true - } - volatile[id] = v - } - return v - } - for _, id := range openPackageIDs { - if ctx.Err() != nil { - return nil, ctx.Err() - } - _ = isVolatile(id) // populate volatile map - } - - var ids []PackageID - for id, v := range volatile { - if !v { - ids = append(ids, id) - } - } - - handles, err := s.getPackageHandles(ctx, ids) - if err != nil { - return nil, err - } - - // We reuse the last import graph if and only if none of the dependencies - // have changed. Doing better would involve analyzing dependencies to find - // subgraphs that are still valid. Not worth it, especially when in the - // common case nothing has changed. - unchanged := lastImportGraph != nil && len(ids) == len(lastImportGraph.depKeys) - depKeys := make(map[PackageID]file.Hash) - for id, ph := range handles { - depKeys[id] = ph.key - if unchanged { - prevKey, ok := lastImportGraph.depKeys[id] - unchanged = ok && prevKey == ph.key - } - } - - if unchanged { - return lastImportGraph, nil - } - - b := newTypeCheckBatch(s.view.parseCache, nil) - if err := b.query(ctx, ids, nil, nil, nil, handles); err != nil { - return nil, err - } - - next := &importGraph{ - fset: b.fset, - depKeys: depKeys, - imports: make(map[PackageID]pkgOrErr), - } - for id, fut := range b.importPackages.cache { - if fut.v == nil && fut.err == nil { - panic(fmt.Sprintf("internal error: import node %s is not evaluated", id)) - } - next.imports[id] = pkgOrErr{fut.v, fut.err} - } - return next, nil -} - -// An importGraph holds selected results of a type-checking pass, to be re-used -// by subsequent snapshots. -type importGraph struct { - fset *token.FileSet // fileset used for type checking imports - depKeys map[PackageID]file.Hash // hash of direct dependencies for this graph - imports map[PackageID]pkgOrErr // results of type checking -} - // Package visiting functions used by forEachPackage; see the documentation of // forEachPackage for details. type ( @@ -376,8 +171,11 @@ func (s *Snapshot) forEachPackage(ctx context.Context, ids []PackageID, pre preT // requests for package information for the modified package (semantic // tokens, code lens, inlay hints, etc.) for i, id := range ids { - if pkg := s.getActivePackage(id); pkg != nil { - post(i, pkg) + s.mu.Lock() + ph, ok := s.packages.Get(id) + s.mu.Unlock() + if ok && ph.state >= validPackage { + post(i, ph.pkgData.pkg) } else { needIDs = append(needIDs, id) indexes = append(indexes, i) @@ -388,6 +186,14 @@ func (s *Snapshot) forEachPackage(ctx context.Context, ids []PackageID, pre preT return nil // short cut: many call sites do not handle empty ids } + b, release := s.acquireTypeChecking() + defer release() + + handles, err := s.getPackageHandles(ctx, needIDs) + if err != nil { + return err + } + // Wrap the pre- and post- funcs to translate indices. var pre2 preTypeCheck if pre != nil { @@ -396,18 +202,28 @@ func (s *Snapshot) forEachPackage(ctx context.Context, ids []PackageID, pre preT } } post2 := func(i int, pkg *Package) { - s.setActivePackage(pkg.metadata.ID, pkg) - post(indexes[i], pkg) - } + id := pkg.metadata.ID + if ph := handles[id]; ph.isOpen && ph.state < validPackage { + // Cache open type checked packages. + ph = ph.clone() + ph.pkgData = &packageData{ + fset: pkg.FileSet(), + imports: pkg.Types().Imports(), + pkg: pkg, + } + ph.state = validPackage - b, release := s.acquireTypeChecking(ctx) - defer release() + s.mu.Lock() + if alt, ok := s.packages.Get(id); !ok || alt.state < ph.state { + s.packages.Set(id, ph, nil) + } + s.mu.Unlock() + } - handles, err := s.getPackageHandles(ctx, needIDs) - if err != nil { - return err + post(indexes[i], pkg) } - return b.query(ctx, nil, needIDs, pre2, post2, handles) + + return b.query(ctx, needIDs, pre2, post2, handles) } // acquireTypeChecking joins or starts a concurrent type checking batch. @@ -415,14 +231,13 @@ func (s *Snapshot) forEachPackage(ctx context.Context, ids []PackageID, pre preT // The batch may be queried for package information using [typeCheckBatch.query]. // The second result must be called when the batch is no longer needed, to // release the resource. -func (s *Snapshot) acquireTypeChecking(ctx context.Context) (*typeCheckBatch, func()) { +func (s *Snapshot) acquireTypeChecking() (*typeCheckBatch, func()) { s.typeCheckMu.Lock() defer s.typeCheckMu.Unlock() if s.batch == nil { assert(s.batchRef == 0, "miscounted type checking") - impGraph := s.getImportGraph(ctx) - s.batch = newTypeCheckBatch(s.view.parseCache, impGraph) + s.batch = newTypeCheckBatch(s.view.parseCache) } s.batchRef++ @@ -441,8 +256,8 @@ func (s *Snapshot) acquireTypeChecking(ctx context.Context) (*typeCheckBatch, fu // shared parseCache. // // If a non-nil importGraph is provided, imports in this graph will be reused. -func newTypeCheckBatch(parseCache *parseCache, importGraph *importGraph) *typeCheckBatch { - b := &typeCheckBatch{ +func newTypeCheckBatch(parseCache *parseCache) *typeCheckBatch { + return &typeCheckBatch{ _handles: make(map[PackageID]*packageHandle), parseCache: parseCache, fset: fileSetWithBase(reservedForParsing), @@ -450,20 +265,6 @@ func newTypeCheckBatch(parseCache *parseCache, importGraph *importGraph) *typeCh syntaxPackages: newFutureCache[PackageID, *Package](false), // don't persist syntax packages importPackages: newFutureCache[PackageID, *types.Package](true), // ...but DO persist imports } - - if importGraph != nil { - // Clone the file set every time, to ensure we do not leak files. - b.fset = tokeninternal.CloneFileSet(importGraph.fset) - // Pre-populate future cache with 'done' futures. - done := make(chan unit) - close(done) - for id, res := range importGraph.imports { - b.importPackages.cache[id] = &future[*types.Package]{done: done, v: res.pkg, err: res.err} - } - } else { - b.fset = fileSetWithBase(reservedForParsing) - } - return b } // query executes a traversal of package information in the given typeCheckBatch. @@ -478,7 +279,7 @@ func newTypeCheckBatch(parseCache *parseCache, importGraph *importGraph) *typeCh // // TODO(rfindley): simplify this API by clarifying shared import graph and // package handle logic. -func (b *typeCheckBatch) query(ctx context.Context, importIDs, syntaxIDs []PackageID, pre preTypeCheck, post postTypeCheck, handles map[PackageID]*packageHandle) error { +func (b *typeCheckBatch) query(ctx context.Context, syntaxIDs []PackageID, pre preTypeCheck, post postTypeCheck, handles map[PackageID]*packageHandle) error { b.addHandles(handles) // Start a single goroutine for each requested package. @@ -486,15 +287,6 @@ func (b *typeCheckBatch) query(ctx context.Context, importIDs, syntaxIDs []Packa // Other packages are reached recursively, and will not be evaluated if they // are not needed. var g errgroup.Group - for _, id := range importIDs { - g.Go(func() error { - if ctx.Err() != nil { - return ctx.Err() - } - _, err := b.getImportPackage(ctx, id) - return err - }) - } for i, id := range syntaxIDs { g.Go(func() error { if ctx.Err() != nil { @@ -554,13 +346,36 @@ func (b *typeCheckBatch) handleSyntaxPackage(ctx context.Context, i int, id Pack return nil // skip: not needed } + // Check if we have a syntax package stored on ph. + // + // This was checked in [Snapshot.forEachPackage], but may have since changed. + if ph.state >= validPackage { + post(i, ph.pkgData.pkg) + return nil + } + pkg, err := b.syntaxPackages.get(ctx, id, func(ctx context.Context) (*Package, error) { // Wait for predecessors. - { + // Record imports of this package to avoid redundant work in typesConfig. + imports := make(map[PackagePath]*types.Package) + fset := b.fset + if ph.state >= validImports { + for _, imp := range ph.pkgData.imports { + imports[PackagePath(imp.Path())] = imp + } + // Reusing imports requires that their positions are mapped by the FileSet. + fset = tokeninternal.CloneFileSet(ph.pkgData.fset) + } else { + var impMu sync.Mutex var g errgroup.Group - for _, depID := range ph.mp.DepsByPkgPath { + for depPath, depID := range ph.mp.DepsByPkgPath { g.Go(func() error { - _, err := b.getImportPackage(ctx, depID) + imp, err := b.getImportPackage(ctx, depID) + if err == nil { + impMu.Lock() + imports[depPath] = imp + impMu.Unlock() + } return err }) } @@ -588,7 +403,7 @@ func (b *typeCheckBatch) handleSyntaxPackage(ctx context.Context, i int, id Pack } // Compute the syntax package. - p, err := b.checkPackage(ctx, ph) + p, err := b.checkPackage(ctx, fset, ph, imports) if err != nil { return nil, err // e.g. I/O error, cancelled } @@ -707,7 +522,7 @@ func (b *typeCheckBatch) checkPackageForImport(ctx context.Context, ph *packageH onError := func(e error) { // Ignore errors for exporting. } - cfg := b.typesConfig(ctx, ph.localInputs, onError) + cfg := b.typesConfig(ctx, ph.localInputs, nil, onError) cfg.IgnoreFuncBodies = true // Parse the compiled go files, bypassing the parse cache as packages checked @@ -837,9 +652,11 @@ func (b *typeCheckBatch) importLookup(mp *metadata.Package) func(PackagePath) Pa type packageState uint8 const ( - validMetadata packageState = iota // the package has valid metadata (initial state) + validMetadata packageState = iota // the package has valid metadata validLocalData // local package files have been analyzed validKey // dependencies have been analyzed, and key produced + validImports // pkgData.fset and pkgData.imports are valid + validPackage // pkgData.pkg is valid ) // A packageHandle holds information derived from a metadata.Package, and @@ -866,12 +683,20 @@ const ( // we sometimes refer to as "precise pruning", or fine-grained invalidation: // https://go.dev/blog/gopls-scalability#invalidation // +// After type checking, package information for open packages is cached in the +// pkgData field (validPackage), to optimize subsequent requests oriented +// around open files. +// // Following a change, the packageHandle is cloned in the new snapshot with a // new state set to its least known valid state, as described above: if package // files changed, it is reset to validMetadata; if dependencies changed, it is // reset to validLocalData. However, the derived data from its previous state // is not yet removed, as keys may not have changed after they are reevaluated, -// in which case we can avoid recomputing the derived data. +// in which case we can avoid recomputing the derived data. In particular, if +// the cache key did not change, the pkgData field (if set) remains valid. As a +// special case, if the cache key did change, but none of the keys of +// dependencies changed, the pkgData.fset and pkgData.imports fields are still +// valid, though the pkgData.pkg field is not (validImports). // // See [packageHandleBuilder.evaluatePackageHandle] for more details of the // reevaluation algorithm. @@ -909,6 +734,8 @@ type packageHandle struct { // localInputs holds all local type-checking localInputs, excluding // dependencies. localInputs *typeCheckInputs + // isOpen reports whether the package has any open files. + isOpen bool // localKey is a hash of localInputs. localKey file.Hash // refs is the result of syntactic dependency analysis produced by the @@ -925,6 +752,24 @@ type packageHandle struct { // It includes the all bits of the transitive closure of // dependencies's sources. key file.Hash + + // pkgData caches data derived from type checking the package. + // This data is set during [Snapshot.forEachPackage], and may be partially + // invalidated in [packageHandleBuilder.evaluatePackageHandle]. + // + // If state == validPackage, all fields of pkgData are valid. If state == + // validImports, only fset and imports are valid. + pkgData *packageData +} + +// packageData holds the (possibly partial) result of type checking this +// package. See the pkgData field of [packageHandle]. +// +// packageData instances are immutable. +type packageData struct { + fset *token.FileSet // pkg.FileSet() + imports []*types.Package // pkg.Types().Imports() + pkg *Package // pkg, if state==validPackage; nil in lower states } // clone returns a shallow copy of the receiver. @@ -1178,51 +1023,53 @@ func (b *packageHandleBuilder) getOneTransitiveRefLocked(sym typerefs.Symbol) *t // // evaluatePackageHandle must only be called from getPackageHandles. func (b *packageHandleBuilder) evaluatePackageHandle(ctx context.Context, n *handleNode) (err error) { - // Initialize n.ph. - var hit bool b.s.mu.Lock() - n.ph, hit = b.s.packages.Get(n.mp.ID) + ph, hit := b.s.packages.Get(n.mp.ID) b.s.mu.Unlock() - if hit && n.ph.state >= validKey { - return nil // already valid - } else { - // We'll need to update the package handle. Since this could happen - // concurrently, make a copy. - if hit { - n.ph = n.ph.clone() - } else { - n.ph = &packageHandle{ - mp: n.mp, - state: validMetadata, - } - } - } - defer func() { if err == nil { - assert(n.ph.state == validKey, "invalid handle") + assert(ph.state >= validKey, "invalid handle") + // Record the now valid key in the snapshot. // There may be a race, so avoid the write if the recorded handle is // already valid. b.s.mu.Lock() - if alt, ok := b.s.packages.Get(n.mp.ID); !ok || alt.state < n.ph.state { - b.s.packages.Set(n.mp.ID, n.ph, nil) + if alt, ok := b.s.packages.Get(n.mp.ID); !ok || alt.state < ph.state { + b.s.packages.Set(n.mp.ID, ph, nil) } else { - n.ph = alt + ph = alt } b.s.mu.Unlock() + + // Initialize n.ph. + n.ph = ph } }() - // Invariant: n.ph is either + if hit && ph.state >= validKey { + return nil // already valid + } else { + // We'll need to update the package handle. Since this could happen + // concurrently, make a copy. + if hit { + ph = ph.clone() // state < validKey + } else { + ph = &packageHandle{ + mp: n.mp, + state: validMetadata, + } + } + } + + // Invariant: ph is either // - a new handle in state validMetadata, or // - a clone of an existing handle in state validMetadata or validLocalData. // State transition: validMetadata -> validLocalInputs. localKeyChanged := false - if n.ph.state < validLocalData { - prevLocalKey := n.ph.localKey // may be zero + if ph.state < validLocalData { + prevLocalKey := ph.localKey // may be zero // No package handle: read and analyze the package syntax. inputs, err := b.s.typeCheckInputs(ctx, n.mp) if err != nil { @@ -1232,15 +1079,47 @@ func (b *packageHandleBuilder) evaluatePackageHandle(ctx context.Context, n *han if err != nil { return err } - n.ph.loadDiagnostics = computeLoadDiagnostics(ctx, b.s, n.mp) - n.ph.localInputs = inputs - n.ph.localKey = localPackageKey(inputs) - n.ph.refs = refs - n.ph.state = validLocalData - localKeyChanged = n.ph.localKey != prevLocalKey + ph.loadDiagnostics = computeLoadDiagnostics(ctx, b.s, n.mp) + ph.localInputs = inputs + + checkOpen: + for _, files := range [][]file.Handle{inputs.goFiles, inputs.compiledGoFiles} { + for _, fh := range files { + if _, ok := fh.(*overlay); ok { + ph.isOpen = true + break checkOpen + } + } + } + if !ph.isOpen { + // ensure we don't hold data for closed packages + ph.pkgData = nil + } + ph.localKey = localPackageKey(inputs) + ph.refs = refs + ph.state = validLocalData + localKeyChanged = ph.localKey != prevLocalKey } - assert(n.ph.state == validLocalData, "unexpected handle state") + assert(ph.state == validLocalData, "unexpected handle state") + + // State transition: validLocalInputs -> validKey + + // Check if any dependencies have actually changed. + depsChanged := true + if ph.depKeys != nil { // ph was previously evaluated + depsChanged = len(ph.depKeys) != len(n.succs) + if !depsChanged { + for id, succ := range n.succs { + oldKey, ok := ph.depKeys[id] + assert(ok, "missing dep") + if oldKey != succ.ph.key { + depsChanged = true + break + } + } + } + } // Optimization: if the local package information did not change, nor did any // of the dependencies, we don't need to re-run the reachability algorithm. @@ -1252,73 +1131,83 @@ func (b *packageHandleBuilder) evaluatePackageHandle(ctx context.Context, n *han // package key of B will not change. We still need to re-run the reachability // algorithm on B to confirm. But if the key of B did not change, we don't // even need to run the reachability algorithm on A. - if !localKeyChanged && - n.ph.depKeys != nil && // n.ph was previously evaluated - len(n.ph.depKeys) == len(n.succs) { - - unchanged := true - for id, succ := range n.succs { - oldKey, ok := n.ph.depKeys[id] - assert(ok, "missing dep") - if oldKey != succ.ph.key { - unchanged = false - break + if !localKeyChanged && !depsChanged { + ph.state = validKey + } + + keyChanged := false + if ph.state < validKey { + prevKey := ph.key + + // If we get here, it must be the case that deps have changed, so we must + // run the reachability algorithm. + ph.depKeys = make(map[PackageID]file.Hash) + + // See the typerefs package: the reachable set of packages is defined to be + // the set of packages containing syntax that is reachable through the + // symbol reference graph starting at the exported symbols in the + // dependencies of ph. + reachable := b.s.view.pkgIndex.NewSet() + for depID, succ := range n.succs { + ph.depKeys[depID] = succ.ph.key + reachable.Add(succ.idxID) + trefs := b.getTransitiveRefs(succ.mp.ID) + assert(trefs != nil, "nil trefs") + for _, set := range trefs { + reachable.Union(set) } } - if unchanged { - n.ph.state = validKey - return nil - } - } - // State transition: validLocalInputs -> validKey - // - // If we get here, it must be the case that deps have changed, so we must - // run the reachability algorithm. - n.ph.depKeys = make(map[PackageID]file.Hash) - - // See the typerefs package: the reachable set of packages is defined to be - // the set of packages containing syntax that is reachable through the - // exported symbols in the dependencies of n.ph. - reachable := b.s.view.pkgIndex.NewSet() - for depID, succ := range n.succs { - n.ph.depKeys[depID] = succ.ph.key - reachable.Add(succ.idxID) - trefs := b.getTransitiveRefs(succ.mp.ID) - assert(trefs != nil, "nil trefs") - for _, set := range trefs { - reachable.Union(set) - } - } - - // Collect reachable nodes. - var reachableNodes []*handleNode - // In the presence of context cancellation, any package may be missing. - // We need all dependencies to produce a valid key. - reachable.Elems(func(id typerefs.IndexID) { - dh := b.nodes[id] - if dh == nil { - // Previous code reported an error (not a bug) here. - bug.Reportf("missing reachable node for %q", id) - } else { - reachableNodes = append(reachableNodes, dh) + // Collect reachable nodes. + var reachableNodes []*handleNode + // In the presence of context cancellation, any package may be missing. + // We need all dependencies to produce a key. + reachable.Elems(func(id typerefs.IndexID) { + dh := b.nodes[id] + if dh == nil { + // Previous code reported an error (not a bug) here. + bug.Reportf("missing reachable node for %q", id) + } else { + reachableNodes = append(reachableNodes, dh) + } + }) + + // Sort for stability. + sort.Slice(reachableNodes, func(i, j int) bool { + return reachableNodes[i].mp.ID < reachableNodes[j].mp.ID + }) + + // Key is the hash of the local key of this package, and the local key of + // all reachable packages. + depHasher := sha256.New() + depHasher.Write(ph.localKey[:]) + for _, dh := range reachableNodes { + depHasher.Write(dh.ph.localKey[:]) } - }) + depHasher.Sum(ph.key[:0]) + ph.state = validKey + keyChanged = ph.key != prevKey + } - // Sort for stability. - sort.Slice(reachableNodes, func(i, j int) bool { - return reachableNodes[i].mp.ID < reachableNodes[j].mp.ID - }) + assert(ph.state == validKey, "unexpected handle state") - // Key is the hash of the local key, and the local key of all reachable - // packages. - depHasher := sha256.New() - depHasher.Write(n.ph.localKey[:]) - for _, dh := range reachableNodes { - depHasher.Write(dh.ph.localKey[:]) + // Validate ph.pkgData, upgrading state if the package or its imports are + // still valid. + if ph.pkgData != nil { + pkgData := *ph.pkgData // make a copy + ph.pkgData = &pkgData + ph.state = validPackage + if keyChanged || ph.pkgData.pkg == nil { + ph.pkgData.pkg = nil // ensure we don't hold on to stale packages + ph.state = validImports + } + if depsChanged { + ph.pkgData = nil + ph.state = validKey + } } - depHasher.Sum(n.ph.key[:0]) - n.ph.state = validKey + + // Postcondition: state >= validKey return nil } @@ -1533,14 +1422,14 @@ func localPackageKey(inputs *typeCheckInputs) file.Hash { // checkPackage type checks the parsed source files in compiledGoFiles. // (The resulting pkg also holds the parsed but not type-checked goFiles.) // deps holds the future results of type-checking the direct dependencies. -func (b *typeCheckBatch) checkPackage(ctx context.Context, ph *packageHandle) (*Package, error) { +func (b *typeCheckBatch) checkPackage(ctx context.Context, fset *token.FileSet, ph *packageHandle, imports map[PackagePath]*types.Package) (*Package, error) { inputs := ph.localInputs ctx, done := event.Start(ctx, "cache.typeCheckBatch.checkPackage", label.Package.Of(string(inputs.id))) defer done() pkg := &syntaxPackage{ id: inputs.id, - fset: b.fset, // must match parse call below + fset: fset, // must match parse call below types: types.NewPackage(string(inputs.pkgPath), string(inputs.name)), typesSizes: inputs.sizes, typesInfo: &types.Info{ @@ -1558,11 +1447,11 @@ func (b *typeCheckBatch) checkPackage(ctx context.Context, ph *packageHandle) (* // Collect parsed files from the type check pass, capturing parse errors from // compiled files. var err error - pkg.goFiles, err = b.parseCache.parseFiles(ctx, b.fset, parsego.Full, false, inputs.goFiles...) + pkg.goFiles, err = b.parseCache.parseFiles(ctx, pkg.fset, parsego.Full, false, inputs.goFiles...) if err != nil { return nil, err } - pkg.compiledGoFiles, err = b.parseCache.parseFiles(ctx, b.fset, parsego.Full, false, inputs.compiledGoFiles...) + pkg.compiledGoFiles, err = b.parseCache.parseFiles(ctx, pkg.fset, parsego.Full, false, inputs.compiledGoFiles...) if err != nil { return nil, err } @@ -1591,7 +1480,7 @@ func (b *typeCheckBatch) checkPackage(ctx context.Context, ph *packageHandle) (* onError := func(e error) { pkg.typeErrors = append(pkg.typeErrors, e.(types.Error)) } - cfg := b.typesConfig(ctx, inputs, onError) + cfg := b.typesConfig(ctx, inputs, imports, onError) check := types.NewChecker(cfg, pkg.fset, pkg.types, pkg.typesInfo) var files []*ast.File @@ -1677,7 +1566,7 @@ func (b *typeCheckBatch) checkPackage(ctx context.Context, ph *packageHandle) (* // e.g. "go1" or "go1.2" or "go1.2.3" var goVersionRx = regexp.MustCompile(`^go[1-9][0-9]*(?:\.(0|[1-9][0-9]*)){0,2}$`) -func (b *typeCheckBatch) typesConfig(ctx context.Context, inputs *typeCheckInputs, onError func(e error)) *types.Config { +func (b *typeCheckBatch) typesConfig(ctx context.Context, inputs *typeCheckInputs, imports map[PackagePath]*types.Package, onError func(e error)) *types.Config { cfg := &types.Config{ Sizes: inputs.sizes, Error: onError, @@ -1701,6 +1590,19 @@ func (b *typeCheckBatch) typesConfig(ctx context.Context, inputs *typeCheckInput if !metadata.IsValidImport(inputs.pkgPath, depPH.mp.PkgPath, inputs.viewType != GoPackagesDriverView) { return nil, fmt.Errorf("invalid use of internal package %q", path) } + // For syntax packages, the set of required imports is known and + // precomputed. For import packages (checkPackageForImport), imports are + // constructed lazily, because they may not have been needed if we could + // have imported from export data. + // + // TODO(rfindley): refactor to move this logic to the callsite. + if imports != nil { + imp, ok := imports[depPH.mp.PkgPath] + if !ok { + return nil, fmt.Errorf("missing import %s", id) + } + return imp, nil + } return b.getImportPackage(ctx, id) }), } diff --git a/gopls/internal/cache/load.go b/gopls/internal/cache/load.go index 9373766b413..5aa5d27626c 100644 --- a/gopls/internal/cache/load.go +++ b/gopls/internal/cache/load.go @@ -301,7 +301,6 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc workspacePackages := computeWorkspacePackagesLocked(ctx, s, meta) s.meta = meta s.workspacePackages = workspacePackages - s.resetActivePackagesLocked() s.mu.Unlock() diff --git a/gopls/internal/cache/session.go b/gopls/internal/cache/session.go index 65ba7e69d0a..2b45df9adb3 100644 --- a/gopls/internal/cache/session.go +++ b/gopls/internal/cache/session.go @@ -249,7 +249,6 @@ func (s *Session) createView(ctx context.Context, def *viewDefinition) (*View, * packages: new(persistent.Map[PackageID, *packageHandle]), meta: new(metadata.Graph), files: newFileMap(), - activePackages: new(persistent.Map[PackageID, *Package]), symbolizeHandles: new(persistent.Map[protocol.DocumentURI, *memoize.Promise]), shouldLoad: new(persistent.Map[PackageID, []PackagePath]), unloadableFiles: new(persistent.Set[protocol.DocumentURI]), diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go index 004dc5279c0..d9852ace1f6 100644 --- a/gopls/internal/cache/snapshot.go +++ b/gopls/internal/cache/snapshot.go @@ -144,12 +144,6 @@ type Snapshot struct { // be in packages, unless there is a missing import packages *persistent.Map[PackageID, *packageHandle] - // activePackages maps a package ID to a memoized active package, or nil if - // the package is known not to be open. - // - // IDs not contained in the map are not known to be open or not open. - activePackages *persistent.Map[PackageID, *Package] - // workspacePackages contains the workspace's packages, which are loaded // when the view is created. It does not contain intermediate test variants. workspacePackages immutable.Map[PackageID, PackagePath] @@ -182,14 +176,6 @@ type Snapshot struct { modWhyHandles *persistent.Map[protocol.DocumentURI, *memoize.Promise] // *memoize.Promise[modWhyResult] modVulnHandles *persistent.Map[protocol.DocumentURI, *memoize.Promise] // *memoize.Promise[modVulnResult] - // importGraph holds a shared import graph to use for type-checking. Adding - // more packages to this import graph can speed up type checking, at the - // expense of in-use memory. - // - // See getImportGraph for additional documentation. - importGraphDone chan struct{} // closed when importGraph is set; may be nil - importGraph *importGraph // copied from preceding snapshot and re-evaluated - // moduleUpgrades tracks known upgrades for module paths in each modfile. // Each modfile has a map of module name to upgrade version. moduleUpgrades *persistent.Map[protocol.DocumentURI, map[string]string] @@ -245,7 +231,6 @@ func (s *Snapshot) decref() { s.refcount-- if s.refcount == 0 { s.packages.Destroy() - s.activePackages.Destroy() s.files.destroy() s.symbolizeHandles.Destroy() s.parseModHandles.Destroy() @@ -844,50 +829,6 @@ func (s *Snapshot) ReverseDependencies(ctx context.Context, id PackageID, transi return rdeps, nil } -// -- Active package tracking -- -// -// We say a package is "active" if any of its files are open. -// This is an optimization: the "active" concept is an -// implementation detail of the cache and is not exposed -// in the source or Snapshot API. -// After type-checking we keep active packages in memory. -// The activePackages persistent map does bookkeeping for -// the set of active packages. - -// getActivePackage returns a the memoized active package for id, if it exists. -// If id is not active or has not yet been type-checked, it returns nil. -func (s *Snapshot) getActivePackage(id PackageID) *Package { - s.mu.Lock() - defer s.mu.Unlock() - - if value, ok := s.activePackages.Get(id); ok { - return value - } - return nil -} - -// setActivePackage checks if pkg is active, and if so either records it in -// the active packages map or returns the existing memoized active package for id. -func (s *Snapshot) setActivePackage(id PackageID, pkg *Package) { - s.mu.Lock() - defer s.mu.Unlock() - - if _, ok := s.activePackages.Get(id); ok { - return // already memoized - } - - if containsOpenFileLocked(s, pkg.Metadata()) { - s.activePackages.Set(id, pkg, nil) - } else { - s.activePackages.Set(id, (*Package)(nil), nil) // remember that pkg is not open - } -} - -func (s *Snapshot) resetActivePackagesLocked() { - s.activePackages.Destroy() - s.activePackages = new(persistent.Map[PackageID, *Package]) -} - // See Session.FileWatchingGlobPatterns for a description of gopls' file // watching heuristic. func (s *Snapshot) fileWatchingGlobPatterns() map[protocol.RelativePattern]unit { @@ -1670,7 +1611,6 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f initialized: s.initialized, initialErr: s.initialErr, packages: s.packages.Clone(), - activePackages: s.activePackages.Clone(), files: s.files.clone(changedFiles), symbolizeHandles: cloneWithout(s.symbolizeHandles, changedFiles, nil), workspacePackages: s.workspacePackages, @@ -1681,7 +1621,6 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f modTidyHandles: cloneWithout(s.modTidyHandles, changedFiles, &needsDiagnosis), modWhyHandles: cloneWithout(s.modWhyHandles, changedFiles, &needsDiagnosis), modVulnHandles: cloneWithout(s.modVulnHandles, changedFiles, &needsDiagnosis), - importGraph: s.importGraph, moduleUpgrades: cloneWith(s.moduleUpgrades, changed.ModuleUpgrades), vulns: cloneWith(s.vulns, changed.Vulns), } @@ -1963,9 +1902,6 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f result.packages.Set(id, ph, nil) } } - if result.activePackages.Delete(id) { - needsDiagnosis = true - } } // Compute which metadata updates are required. We only need to invalidate @@ -2006,7 +1942,6 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange, done f if result.meta != s.meta || anyFileOpenedOrClosed { needsDiagnosis = true result.workspacePackages = computeWorkspacePackagesLocked(ctx, result, result.meta) - result.resetActivePackagesLocked() } else { result.workspacePackages = s.workspacePackages } @@ -2190,8 +2125,8 @@ func metadataChanges(ctx context.Context, lockedSnapshot *Snapshot, oldFH, newFH // Check whether package imports have changed. Only consider potentially // valid imports paths. - oldImports := validImports(oldHead.File.Imports) - newImports := validImports(newHead.File.Imports) + oldImports := validImportPaths(oldHead.File.Imports) + newImports := validImportPaths(newHead.File.Imports) for path := range newImports { if _, ok := oldImports[path]; ok { @@ -2240,8 +2175,8 @@ func magicCommentsChanged(original *ast.File, current *ast.File) bool { return false } -// validImports extracts the set of valid import paths from imports. -func validImports(imports []*ast.ImportSpec) map[string]struct{} { +// validImportPaths extracts the set of valid import paths from imports. +func validImportPaths(imports []*ast.ImportSpec) map[string]struct{} { m := make(map[string]struct{}) for _, spec := range imports { if path := spec.Path.Value; validImportPath(path) { diff --git a/gopls/internal/test/integration/misc/definition_test.go b/gopls/internal/test/integration/misc/definition_test.go index 71f255b52e2..95054977e14 100644 --- a/gopls/internal/test/integration/misc/definition_test.go +++ b/gopls/internal/test/integration/misc/definition_test.go @@ -13,6 +13,7 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/test/compare" . "golang.org/x/tools/gopls/internal/test/integration" @@ -642,3 +643,48 @@ var _ = foo(123) // call } }) } + +func TestPackageKeyInvalidationAfterSave(t *testing.T) { + // This test is a little subtle, but catches a bug that slipped through + // testing of https://go.dev/cl/614165, which moved active packages to the + // packageHandle. + // + // The bug was that after a format-and-save operation, the save marks the + // package as dirty but doesn't change its identity. In other words, this is + // the sequence of change: + // + // S_0 --format--> S_1 --save--> S_2 + // + // A package is computed on S_0, invalidated in S_1 and immediately + // invalidated again in S_2. Due to an invalidation bug, the validity of the + // package from S_0 was checked by comparing the identical keys of S_1 and + // S_2, and so the stale package from S_0 was marked as valid. + const src = ` +-- go.mod -- +module mod.com + +-- a.go -- +package a + +func Foo() { +} +` + Run(t, src, func(t *testing.T, env *Env) { + env.OpenFile("a.go") + + fooLoc := env.RegexpSearch("a.go", "()Foo") + loc0 := env.GoToDefinition(fooLoc) + + // Insert a space that will be removed by formatting. + env.EditBuffer("a.go", protocol.TextEdit{ + Range: fooLoc.Range, + NewText: " ", + }) + env.SaveBuffer("a.go") // reformats the file before save + env.AfterChange() + loc1 := env.GoToDefinition(env.RegexpSearch("a.go", "Foo")) + if diff := cmp.Diff(loc0, loc1); diff != "" { + t.Errorf("mismatching locations (-want +got):\n%s", diff) + } + }) +} diff --git a/gopls/internal/util/persistent/map.go b/gopls/internal/util/persistent/map.go index b0e49f27d42..5cb556a482b 100644 --- a/gopls/internal/util/persistent/map.go +++ b/gopls/internal/util/persistent/map.go @@ -26,6 +26,9 @@ import ( // `foo(arg1:+n1, arg2:+n2) (ret1:+n3)`. // Each argument is followed by a delta change to its reference counter. // In case if no change is expected, the delta will be `-0`. +// +// TODO(rfindley): add Update(K, func(V, bool) V), as we have several instances +// of the Get--Set pattern that could be optimized. // Map is an associative mapping from keys to values. // From f21a1dce1e0a4622591e79c80f6dc70f0cf027ac Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 30 Sep 2024 18:10:30 +0000 Subject: [PATCH 07/95] gopls: add initial support for pull diagnostics Implement the scaffolding for pull diagnostics. For now, these are only supported for Go files, only return parse/type errors for the narrowest package in the default view, do not report related diagnostics, and do not run analysis. All of these limitations can be fixed, but this implementation should be sufficient for some end-to-end testing. Since the implementation is incomplete, guard the server capability behind a new internal "pullDiagnostics" setting. Wire in pull diagnostics to the marker tests: if the server supports it ("pullDiagnostics": true), use pull diagnostics rather than awaiting to collect the marker test diagnostics. For golang/go#53275 Change-Id: If6d1c0838d69e43f187863adeca6a3bd5d9bb45d Reviewed-on: https://go-review.googlesource.com/c/tools/+/616835 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan --- gopls/doc/features/diagnostics.md | 16 ++- gopls/doc/release/v0.17.0.md | 13 +- gopls/internal/cache/diagnostics.go | 26 ++++ gopls/internal/golang/diagnostics.go | 90 +++++++++++- gopls/internal/protocol/generate/tables.go | 2 - gopls/internal/protocol/tsprotocol.go | 10 ++ gopls/internal/protocol/tsserver.go | 8 +- gopls/internal/server/diagnostics.go | 136 +++++++----------- gopls/internal/server/general.go | 12 ++ gopls/internal/server/unimplemented.go | 4 - gopls/internal/settings/settings.go | 9 ++ .../diagnostics/diagnostics_test.go | 26 +++- .../internal/test/integration/fake/editor.go | 42 ++++++ gopls/internal/test/integration/wrappers.go | 16 ++- gopls/internal/test/marker/marker_test.go | 29 +++- .../marker/testdata/diagnostics/analyzers.txt | 5 + .../marker/testdata/diagnostics/generated.txt | 5 + .../testdata/diagnostics/issue56943.txt | 5 + 18 files changed, 339 insertions(+), 115 deletions(-) diff --git a/gopls/doc/features/diagnostics.md b/gopls/doc/features/diagnostics.md index b667f69a080..8c9305c94b6 100644 --- a/gopls/doc/features/diagnostics.md +++ b/gopls/doc/features/diagnostics.md @@ -11,7 +11,7 @@ common mistakes. Diagnostics come from two main sources: compilation errors and analysis findings. - **Compilation errors** are those that you would obtain from running `go - build`. Gopls doesn't actually run the compiler; that would be too +build`. Gopls doesn't actually run the compiler; that would be too slow. Instead it runs `go list` (when needed) to compute the metadata of the compilation, then processes those packages in a similar manner to the compiler front-end: reading, scanning, and parsing the @@ -51,7 +51,7 @@ Diagnostics come from two main sources: compilation errors and analysis findings ## Recomputation of diagnostics -Diagnostics are automatically recomputed each time the source files +By default, diagnostics are automatically recomputed each time the source files are edited. Compilation errors in open files are updated after a very short delay @@ -68,9 +68,12 @@ Alternatively, diagnostics may be triggered only after an edited file is saved, using the [`diagnosticsTrigger`](../settings.md#diagnosticsTrigger) setting. -Gopls does not currently support "pull-based" diagnostics, which are -computed synchronously when requested by the client; see golang/go#53275. - +When initialized with `"pullDiagnostics": true`, gopls also supports +["pull diagnostics"](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_pullDiagnostics), +an alternative mechanism for recomputing diagnostics in which the client +requests diagnostics from gopls explicitly using the `textDocument/diagnostic` +request. This feature is off by default until the performance of pull +diagnostics is comparable to push diagnostics. ## Quick fixes @@ -91,6 +94,7 @@ Suggested fixes that are indisputably safe are [code actions](transformation.md#code-actions) whose kind is `"source.fixAll"`. Many client editors have a shortcut to apply all such fixes. + diff --git a/gopls/doc/release/v0.17.0.md b/gopls/doc/release/v0.17.0.md index c57522973db..3f4f7458223 100644 --- a/gopls/doc/release/v0.17.0.md +++ b/gopls/doc/release/v0.17.0.md @@ -1,5 +1,3 @@ - - # Configuration Changes The `fieldalignment` analyzer, previously disabled by default, has @@ -30,6 +28,16 @@ or by selecting a whole declaration or multiple declrations. In order to avoid ambiguity and surprise about what to extract, some kinds of paritial selection of a declration cannot invoke this code action. +## Pull diagnostics + +When initialized with the option `"pullDiagnostics": true`, gopls will advertise support for the +`textDocument.diagnostic` +[client capability](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_pullDiagnostics), +which allows editors to request diagnostics directly from gopls using a +`textDocument/diagnostic` request, rather than wait for a +`textDocument/publishDiagnostics` notification. This feature is off by default +until the performance of pull diagnostics is comparable to push diagnostics. + ## Standard library version information in Hover Hovering over a standard library symbol now displays information about the first @@ -37,6 +45,7 @@ Go release containing the symbol. For example, hovering over `errors.As` shows "Added in go1.13". ## Semantic token modifiers of top-level constructor of types + The semantic tokens response now includes additional modifiers for the top-level constructor of the type of each symbol: `interface`, `struct`, `signature`, `pointer`, `array`, `map`, `slice`, `chan`, `string`, `number`, `bool`, and `invalid`. diff --git a/gopls/internal/cache/diagnostics.go b/gopls/internal/cache/diagnostics.go index 797ce961cd8..95b1b9f1c18 100644 --- a/gopls/internal/cache/diagnostics.go +++ b/gopls/internal/cache/diagnostics.go @@ -5,9 +5,11 @@ package cache import ( + "crypto/sha256" "encoding/json" "fmt" + "golang.org/x/tools/gopls/internal/file" "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/util/bug" ) @@ -69,6 +71,30 @@ func (d *Diagnostic) String() string { return fmt.Sprintf("%v: %s", d.Range, d.Message) } +// Hash computes a hash to identify the diagnostic. +// The hash is for deduplicating within a file, so does not incorporate d.URI. +func (d *Diagnostic) Hash() file.Hash { + h := sha256.New() + for _, t := range d.Tags { + fmt.Fprintf(h, "tag: %s\n", t) + } + for _, r := range d.Related { + fmt.Fprintf(h, "related: %s %s %s\n", r.Location.URI, r.Message, r.Location.Range) + } + fmt.Fprintf(h, "code: %s\n", d.Code) + fmt.Fprintf(h, "codeHref: %s\n", d.CodeHref) + fmt.Fprintf(h, "message: %s\n", d.Message) + fmt.Fprintf(h, "range: %s\n", d.Range) + fmt.Fprintf(h, "severity: %s\n", d.Severity) + fmt.Fprintf(h, "source: %s\n", d.Source) + if d.BundledFixes != nil { + fmt.Fprintf(h, "fixes: %s\n", *d.BundledFixes) + } + var hash [sha256.Size]byte + h.Sum(hash[:0]) + return hash +} + type DiagnosticSource string const ( diff --git a/gopls/internal/golang/diagnostics.go b/gopls/internal/golang/diagnostics.go index 1c6da2e9d4e..495bd877c4e 100644 --- a/gopls/internal/golang/diagnostics.go +++ b/gopls/internal/golang/diagnostics.go @@ -17,6 +17,36 @@ import ( "golang.org/x/tools/gopls/internal/util/moremaps" ) +// DiagnoseFile returns pull-based diagnostics for the given file. +func DiagnoseFile(ctx context.Context, snapshot *cache.Snapshot, uri protocol.DocumentURI) ([]*cache.Diagnostic, error) { + mp, err := NarrowestMetadataForFile(ctx, snapshot, uri) + if err != nil { + return nil, err + } + + // TODO(rfindley): consider analysing the package concurrently to package + // diagnostics. + + // Get package (list/parse/type check) diagnostics. + pkgDiags, err := snapshot.PackageDiagnostics(ctx, mp.ID) + if err != nil { + return nil, err + } + diags := pkgDiags[uri] + + // Get analysis diagnostics. + analyzers := analyzers(snapshot.Options().Staticcheck) + pkgAnalysisDiags, err := snapshot.Analyze(ctx, map[PackageID]*metadata.Package{mp.ID: mp}, analyzers, nil) + if err != nil { + return nil, err + } + analysisDiags := moremaps.Group(pkgAnalysisDiags, byURI)[uri] + + // Return the merged set of file diagnostics, combining type error analyses + // with type error diagnostics. + return CombineDiagnostics(diags, analysisDiags), nil +} + // Analyze reports go/analysis-framework diagnostics in the specified package. // // If the provided tracker is non-nil, it may be used to provide notifications @@ -30,15 +60,63 @@ func Analyze(ctx context.Context, snapshot *cache.Snapshot, pkgIDs map[PackageID return nil, ctx.Err() } - analyzers := slices.Collect(maps.Values(settings.DefaultAnalyzers)) - if snapshot.Options().Staticcheck { - analyzers = slices.AppendSeq(analyzers, maps.Values(settings.StaticcheckAnalyzers)) - } - + analyzers := analyzers(snapshot.Options().Staticcheck) analysisDiagnostics, err := snapshot.Analyze(ctx, pkgIDs, analyzers, tracker) if err != nil { return nil, err } - byURI := func(d *cache.Diagnostic) protocol.DocumentURI { return d.URI } return moremaps.Group(analysisDiagnostics, byURI), nil } + +// byURI is used for grouping diagnostics. +func byURI(d *cache.Diagnostic) protocol.DocumentURI { return d.URI } + +func analyzers(staticcheck bool) []*settings.Analyzer { + analyzers := slices.Collect(maps.Values(settings.DefaultAnalyzers)) + if staticcheck { + analyzers = slices.AppendSeq(analyzers, maps.Values(settings.StaticcheckAnalyzers)) + } + return analyzers +} + +// CombineDiagnostics combines and filters list/parse/type diagnostics from +// tdiags with the analysis adiags, returning the resulting combined set. +// +// Type-error analyzers produce diagnostics that are redundant with type +// checker diagnostics, but more detailed (e.g. fixes). Rather than report two +// diagnostics for the same problem, we combine them by augmenting the +// type-checker diagnostic and discarding the analyzer diagnostic. +// +// If an analysis diagnostic has the same range and message as a +// list/parse/type diagnostic, the suggested fix information (et al) of the +// latter is merged into a copy of the former. This handles the case where a +// type-error analyzer suggests a fix to a type error, and avoids duplication. +// +// The arguments are not modified. +func CombineDiagnostics(tdiags []*cache.Diagnostic, adiags []*cache.Diagnostic) []*cache.Diagnostic { + // Build index of (list+parse+)type errors. + type key struct { + Range protocol.Range + message string + } + combined := make([]*cache.Diagnostic, len(tdiags)) + index := make(map[key]int) // maps (Range,Message) to index in tdiags slice + for i, diag := range tdiags { + index[key{diag.Range, diag.Message}] = i + combined[i] = diag + } + + // Filter out analysis diagnostics that match type errors, + // retaining their suggested fix (etc) fields. + for _, diag := range adiags { + if i, ok := index[key{diag.Range, diag.Message}]; ok { + copy := *tdiags[i] + copy.SuggestedFixes = diag.SuggestedFixes + copy.Tags = diag.Tags + combined[i] = © + continue + } + combined = append(combined, diag) + } + return combined +} diff --git a/gopls/internal/protocol/generate/tables.go b/gopls/internal/protocol/generate/tables.go index 5ac5d473580..2036e701d48 100644 --- a/gopls/internal/protocol/generate/tables.go +++ b/gopls/internal/protocol/generate/tables.go @@ -120,8 +120,6 @@ var usedDisambiguate = make(map[string]bool) var goplsType = map[string]string{ "And_RegOpt_textDocument_colorPresentation": "WorkDoneProgressOptionsAndTextDocumentRegistrationOptions", "ConfigurationParams": "ParamConfiguration", - "DocumentDiagnosticParams": "string", - "DocumentDiagnosticReport": "string", "DocumentUri": "DocumentURI", "InitializeParams": "ParamInitialize", "LSPAny": "interface{}", diff --git a/gopls/internal/protocol/tsprotocol.go b/gopls/internal/protocol/tsprotocol.go index 65b97f5b164..b0b01a4b69a 100644 --- a/gopls/internal/protocol/tsprotocol.go +++ b/gopls/internal/protocol/tsprotocol.go @@ -1747,6 +1747,16 @@ type DocumentDiagnosticParams struct { WorkDoneProgressParams PartialResultParams } + +// The result of a document diagnostic pull request. A report can +// either be a full report containing all diagnostics for the +// requested document or an unchanged report indicating that nothing +// has changed in terms of diagnostics in comparison to the last +// pull request. +// +// @since 3.17.0 +// +// See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification#documentDiagnosticReport type DocumentDiagnosticReport = Or_DocumentDiagnosticReport // (alias) // The document diagnostic report kinds. // diff --git a/gopls/internal/protocol/tsserver.go b/gopls/internal/protocol/tsserver.go index b405aae1b89..4e7df50cae1 100644 --- a/gopls/internal/protocol/tsserver.go +++ b/gopls/internal/protocol/tsserver.go @@ -64,7 +64,7 @@ type Server interface { // See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification#textDocument_definition Definition(context.Context, *DefinitionParams) ([]Location, error) // See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification#textDocument_diagnostic - Diagnostic(context.Context, *string) (*string, error) + Diagnostic(context.Context, *DocumentDiagnosticParams) (*DocumentDiagnosticReport, error) // See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification#textDocument_didChange DidChange(context.Context, *DidChangeTextDocumentParams) error // See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification#textDocument_didClose @@ -387,7 +387,7 @@ func serverDispatch(ctx context.Context, server Server, reply jsonrpc2.Replier, return true, reply(ctx, resp, nil) case "textDocument/diagnostic": - var params string + var params DocumentDiagnosticParams if err := UnmarshalJSON(r.Params(), ¶ms); err != nil { return true, sendParseError(ctx, reply, err) } @@ -1030,8 +1030,8 @@ func (s *serverDispatcher) Definition(ctx context.Context, params *DefinitionPar } return result, nil } -func (s *serverDispatcher) Diagnostic(ctx context.Context, params *string) (*string, error) { - var result *string +func (s *serverDispatcher) Diagnostic(ctx context.Context, params *DocumentDiagnosticParams) (*DocumentDiagnosticReport, error) { + var result *DocumentDiagnosticReport if err := s.sender.Call(ctx, "textDocument/diagnostic", params, &result); err != nil { return nil, err } diff --git a/gopls/internal/server/diagnostics.go b/gopls/internal/server/diagnostics.go index f4a32d708e2..38759773cda 100644 --- a/gopls/internal/server/diagnostics.go +++ b/gopls/internal/server/diagnostics.go @@ -6,7 +6,6 @@ package server import ( "context" - "crypto/sha256" "errors" "fmt" "os" @@ -32,6 +31,46 @@ import ( "golang.org/x/tools/internal/event/keys" ) +// Diagnostic implements the textDocument/diagnostic LSP request, reporting +// diagnostics for the given file. +// +// This is a work in progress. +// TODO(rfindley): +// - support RelatedDocuments? If so, how? Maybe include other package diagnostics? +// - support resultID (=snapshot ID) +// - support multiple views +// - add orphaned file diagnostics +// - support go.mod, go.work files +func (s *server) Diagnostic(ctx context.Context, params *protocol.DocumentDiagnosticParams) (*protocol.DocumentDiagnosticReport, error) { + ctx, done := event.Start(ctx, "server.Diagnostic") + defer done() + + fh, snapshot, release, err := s.fileOf(ctx, params.TextDocument.URI) + if err != nil { + return nil, err + } + defer release() + uri := fh.URI() + kind := snapshot.FileKind(fh) + var diagnostics []*cache.Diagnostic + switch kind { + case file.Go: + diagnostics, err = golang.DiagnoseFile(ctx, snapshot, uri) + if err != nil { + return nil, err + } + default: + return nil, fmt.Errorf("pull diagnostics not supported for this file kind") + } + return &protocol.DocumentDiagnosticReport{ + Value: protocol.RelatedFullDocumentDiagnosticReport{ + FullDocumentDiagnosticReport: protocol.FullDocumentDiagnosticReport{ + Items: toProtocolDiagnostics(diagnostics), + }, + }, + }, nil +} + // fileDiagnostics holds the current state of published diagnostics for a file. type fileDiagnostics struct { publishedHash file.Hash // hash of the last set of diagnostics published for this URI @@ -62,31 +101,6 @@ type ( diagMap = map[protocol.DocumentURI][]*cache.Diagnostic ) -// hashDiagnostic computes a hash to identify a diagnostic. -// The hash is for deduplicating within a file, -// so it need not incorporate d.URI. -func hashDiagnostic(d *cache.Diagnostic) file.Hash { - h := sha256.New() - for _, t := range d.Tags { - fmt.Fprintf(h, "tag: %s\n", t) - } - for _, r := range d.Related { - fmt.Fprintf(h, "related: %s %s %s\n", r.Location.URI, r.Message, r.Location.Range) - } - fmt.Fprintf(h, "code: %s\n", d.Code) - fmt.Fprintf(h, "codeHref: %s\n", d.CodeHref) - fmt.Fprintf(h, "message: %s\n", d.Message) - fmt.Fprintf(h, "range: %s\n", d.Range) - fmt.Fprintf(h, "severity: %s\n", d.Severity) - fmt.Fprintf(h, "source: %s\n", d.Source) - if d.BundledFixes != nil { - fmt.Fprintf(h, "fixes: %s\n", *d.BundledFixes) - } - var hash [sha256.Size]byte - h.Sum(hash[:0]) - return hash -} - func sortDiagnostics(d []*cache.Diagnostic) { sort.Slice(d, func(i int, j int) bool { a, b := d[i], d[j] @@ -503,15 +517,17 @@ func (s *server) diagnose(ctx context.Context, snapshot *cache.Snapshot) (diagMa // Merge analysis diagnostics with package diagnostics, and store the // resulting analysis diagnostics. + combinedDiags := make(diagMap) for uri, adiags := range analysisDiags { tdiags := pkgDiags[uri] - var tdiags2, adiags2 []*cache.Diagnostic - combineDiagnostics(tdiags, adiags, &tdiags2, &adiags2) - pkgDiags[uri] = tdiags2 - analysisDiags[uri] = adiags2 + combinedDiags[uri] = golang.CombineDiagnostics(tdiags, adiags) + } + for uri, tdiags := range pkgDiags { + if _, ok := combinedDiags[uri]; !ok { + combinedDiags[uri] = tdiags + } } - store("type checking", pkgDiags, nil) // error reported above - store("analyzing packages", analysisDiags, nil) // error reported above + store("type checking and analysing", combinedDiags, nil) // error reported above return diagnostics, nil } @@ -546,55 +562,6 @@ func (s *server) gcDetailsDiagnostics(ctx context.Context, snapshot *cache.Snaps return diagnostics, nil } -// combineDiagnostics combines and filters list/parse/type diagnostics from -// tdiags with adiags, and appends the two lists to *outT and *outA, -// respectively. -// -// Type-error analyzers produce diagnostics that are redundant -// with type checker diagnostics, but more detailed (e.g. fixes). -// Rather than report two diagnostics for the same problem, -// we combine them by augmenting the type-checker diagnostic -// and discarding the analyzer diagnostic. -// -// If an analysis diagnostic has the same range and message as -// a list/parse/type diagnostic, the suggested fix information -// (et al) of the latter is merged into a copy of the former. -// This handles the case where a type-error analyzer suggests -// a fix to a type error, and avoids duplication. -// -// The use of out-slices, though irregular, allows the caller to -// easily choose whether to keep the results separate or combined. -// -// The arguments are not modified. -func combineDiagnostics(tdiags []*cache.Diagnostic, adiags []*cache.Diagnostic, outT, outA *[]*cache.Diagnostic) { - - // Build index of (list+parse+)type errors. - type key struct { - Range protocol.Range - message string - } - index := make(map[key]int) // maps (Range,Message) to index in tdiags slice - for i, diag := range tdiags { - index[key{diag.Range, diag.Message}] = i - } - - // Filter out analysis diagnostics that match type errors, - // retaining their suggested fix (etc) fields. - for _, diag := range adiags { - if i, ok := index[key{diag.Range, diag.Message}]; ok { - copy := *tdiags[i] - copy.SuggestedFixes = diag.SuggestedFixes - copy.Tags = diag.Tags - tdiags[i] = © - continue - } - - *outA = append(*outA, diag) - } - - *outT = append(*outT, tdiags...) -} - // mustPublishDiagnostics marks the uri as needing publication, independent of // whether the published contents have changed. // @@ -815,7 +782,7 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet // diagSuffixes records the set of view suffixes for a given diagnostic. diagSuffixes := make(map[file.Hash][]diagSuffix) add := func(diag *cache.Diagnostic, suffix string) { - h := hashDiagnostic(diag) + h := diag.Hash() diagSuffixes[h] = append(diagSuffixes[h], diagSuffix{diag, suffix}) } @@ -901,7 +868,7 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet diag2 := *first.diag // shallow copy diag2.Message += first.suffix first.diag = &diag2 - h = hashDiagnostic(&diag2) // update the hash + h = diag2.Hash() // update the hash } hash.XORWith(h) @@ -925,6 +892,9 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet } func toProtocolDiagnostics(diagnostics []*cache.Diagnostic) []protocol.Diagnostic { + // TODO(rfindley): support bundling edits, and bundle all suggested fixes here. + // (see cache.bundleLazyFixes). + reports := []protocol.Diagnostic{} for _, diag := range diagnostics { pdiag := protocol.Diagnostic{ diff --git a/gopls/internal/server/general.go b/gopls/internal/server/general.go index e330bd5bbc3..92e9729a0a6 100644 --- a/gopls/internal/server/general.go +++ b/gopls/internal/server/general.go @@ -108,6 +108,17 @@ func (s *server) Initialize(ctx context.Context, params *protocol.ParamInitializ ResolveProvider: true, } } + + var diagnosticProvider *protocol.Or_ServerCapabilities_diagnosticProvider + if options.PullDiagnostics { + diagnosticProvider = &protocol.Or_ServerCapabilities_diagnosticProvider{ + Value: protocol.DiagnosticOptions{ + InterFileDependencies: true, + WorkspaceDiagnostics: false, // we don't support workspace/diagnostic + }, + } + } + var renameOpts interface{} = true if r := params.Capabilities.TextDocument.Rename; r != nil && r.PrepareSupport { renameOpts = protocol.RenameOptions{ @@ -144,6 +155,7 @@ func (s *server) Initialize(ctx context.Context, params *protocol.ParamInitializ DocumentHighlightProvider: &protocol.Or_ServerCapabilities_documentHighlightProvider{Value: true}, DocumentLinkProvider: &protocol.DocumentLinkOptions{}, InlayHintProvider: protocol.InlayHintOptions{}, + DiagnosticProvider: diagnosticProvider, ReferencesProvider: &protocol.Or_ServerCapabilities_referencesProvider{Value: true}, RenameProvider: renameOpts, SelectionRangeProvider: &protocol.Or_ServerCapabilities_selectionRangeProvider{Value: true}, diff --git a/gopls/internal/server/unimplemented.go b/gopls/internal/server/unimplemented.go index c293ee167a7..9347f42c42e 100644 --- a/gopls/internal/server/unimplemented.go +++ b/gopls/internal/server/unimplemented.go @@ -22,10 +22,6 @@ func (s *server) Declaration(context.Context, *protocol.DeclarationParams) (*pro return nil, notImplemented("Declaration") } -func (s *server) Diagnostic(context.Context, *string) (*string, error) { - return nil, notImplemented("Diagnostic") -} - func (s *server) DiagnosticWorkspace(context.Context, *protocol.WorkspaceDiagnosticParams) (*protocol.WorkspaceDiagnosticReport, error) { return nil, notImplemented("DiagnosticWorkspace") } diff --git a/gopls/internal/settings/settings.go b/gopls/internal/settings/settings.go index 719d0690b5a..5d3d0cddceb 100644 --- a/gopls/internal/settings/settings.go +++ b/gopls/internal/settings/settings.go @@ -699,6 +699,12 @@ type InternalOptions struct { // dynamically creating build configurations for different modules, // directories, and GOOS/GOARCH combinations to cover open files. ZeroConfig bool + + // PullDiagnostics enables support for pull diagnostics. + // + // TODO(rfindley): make pull diagnostics robust, and remove this option, + // allowing pull diagnostics by default. + PullDiagnostics bool } type SubdirWatchPatterns string @@ -1161,6 +1167,9 @@ func (o *Options) setOne(name string, value any) error { case "zeroConfig": return setBool(&o.ZeroConfig, value) + case "pullDiagnostics": + return setBool(&o.PullDiagnostics, value) + // deprecated and renamed settings // // These should never be deleted: there is essentially no cost diff --git a/gopls/internal/test/integration/diagnostics/diagnostics_test.go b/gopls/internal/test/integration/diagnostics/diagnostics_test.go index 195089ffce3..2c9782a17ac 100644 --- a/gopls/internal/test/integration/diagnostics/diagnostics_test.go +++ b/gopls/internal/test/integration/diagnostics/diagnostics_test.go @@ -72,7 +72,11 @@ module mod.com go 1.12 ` - Run(t, onlyMod, func(t *testing.T, env *Env) { + WithOptions( + Settings{ + "pullDiagnostics": true, + }, + ).Run(t, onlyMod, func(t *testing.T, env *Env) { env.CreateBuffer("main.go", `package main func m() { @@ -81,6 +85,9 @@ func m() { `) env.AfterChange(Diagnostics(env.AtRegexp("main.go", "log"))) env.SaveBuffer("main.go") + if got := env.Diagnostics("main.go"); len(got) != 0 { + t.Errorf("got %d diagnostics, want 0", len(got)) + } env.AfterChange(NoDiagnostics(ForFile("main.go"))) }) } @@ -121,8 +128,18 @@ const a = 2 ` func TestDiagnosticClearingOnEdit(t *testing.T) { - Run(t, badPackage, func(t *testing.T, env *Env) { + WithOptions( + Settings{ + "pullDiagnostics": true, + }, + ).Run(t, badPackage, func(t *testing.T, env *Env) { env.OpenFile("b.go") + + for _, f := range []string{"a.go", "b.go"} { + if got := env.Diagnostics(f); len(got) != 1 { + t.Errorf("textDocument/diagnostic(%s) returned %d diagnostics, want 1. Got %v", f, len(got), got) + } + } env.AfterChange( Diagnostics(env.AtRegexp("a.go", "a = 1")), Diagnostics(env.AtRegexp("b.go", "a = 2")), @@ -130,6 +147,11 @@ func TestDiagnosticClearingOnEdit(t *testing.T) { // Fix the error by editing the const name in b.go to `b`. env.RegexpReplace("b.go", "(a) = 2", "b") + for _, f := range []string{"a.go", "b.go"} { + if got := env.Diagnostics(f); len(got) != 0 { + t.Errorf("textDocument/diagnostic(%s) returned %d diagnostics, want 0. Got %v", f, len(got), got) + } + } env.AfterChange( NoDiagnostics(ForFile("a.go")), NoDiagnostics(ForFile("b.go")), diff --git a/gopls/internal/test/integration/fake/editor.go b/gopls/internal/test/integration/fake/editor.go index 876d055da21..041891aaa5a 100644 --- a/gopls/internal/test/integration/fake/editor.go +++ b/gopls/internal/test/integration/fake/editor.go @@ -388,6 +388,12 @@ func clientCapabilities(cfg EditorConfig) (protocol.ClientCapabilities, error) { return capabilities, nil } +// Returns the connected LSP server's capabilities. +// Only populated after a call to [Editor.Connect]. +func (e *Editor) ServerCapabilities() protocol.ServerCapabilities { + return e.serverCapabilities +} + // marshalUnmarshal is a helper to json Marshal and then Unmarshal as a // different type. Used to work around cases where our protocol types are not // specific. @@ -1019,6 +1025,42 @@ func (e *Editor) ApplyCodeAction(ctx context.Context, action protocol.CodeAction return e.sandbox.Workdir.CheckForFileChanges(ctx) } +func (e *Editor) Diagnostics(ctx context.Context, path string) ([]protocol.Diagnostic, error) { + if e.Server == nil { + return nil, errors.New("not connected") + } + e.mu.Lock() + capabilities := e.serverCapabilities.DiagnosticProvider + e.mu.Unlock() + + if capabilities == nil { + return nil, errors.New("server does not support pull diagnostics") + } + switch capabilities.Value.(type) { + case nil: + return nil, errors.New("server does not support pull diagnostics") + case protocol.DiagnosticOptions: + case protocol.DiagnosticRegistrationOptions: + // We could optionally check TextDocumentRegistrationOptions here to + // see if any filters apply to path. + default: + panic(fmt.Sprintf("unknown DiagnosticsProvider type %T", capabilities.Value)) + } + + params := &protocol.DocumentDiagnosticParams{ + TextDocument: e.TextDocumentIdentifier(path), + } + result, err := e.Server.Diagnostic(ctx, params) + if err != nil { + return nil, err + } + report, ok := result.Value.(protocol.RelatedFullDocumentDiagnosticReport) + if !ok { + return nil, fmt.Errorf("unexpected diagnostics report type %T", result) + } + return report.Items, nil +} + // GetQuickFixes returns the available quick fix code actions. func (e *Editor) GetQuickFixes(ctx context.Context, loc protocol.Location, diagnostics []protocol.Diagnostic) ([]protocol.CodeAction, error) { return e.CodeActions(ctx, loc, diagnostics, protocol.QuickFix, protocol.SourceFixAll) diff --git a/gopls/internal/test/integration/wrappers.go b/gopls/internal/test/integration/wrappers.go index ddff4da979b..4e59f1a5bbd 100644 --- a/gopls/internal/test/integration/wrappers.go +++ b/gopls/internal/test/integration/wrappers.go @@ -238,7 +238,7 @@ func (e *Env) ApplyQuickFixes(path string, diagnostics []protocol.Diagnostic) { } } -// ApplyCodeAction applies the given code action. +// ApplyCodeAction applies the given code action, calling t.Fatal on any error. func (e *Env) ApplyCodeAction(action protocol.CodeAction) { e.T.Helper() if err := e.Editor.ApplyCodeAction(e.Ctx, action); err != nil { @@ -246,7 +246,19 @@ func (e *Env) ApplyCodeAction(action protocol.CodeAction) { } } -// GetQuickFixes returns the available quick fix code actions. +// Diagnostics returns diagnostics for the given file, calling t.Fatal on any +// error. +func (e *Env) Diagnostics(name string) []protocol.Diagnostic { + e.T.Helper() + diags, err := e.Editor.Diagnostics(e.Ctx, name) + if err != nil { + e.T.Fatal(err) + } + return diags +} + +// GetQuickFixes returns the available quick fix code actions, calling t.Fatal +// on any error. func (e *Env) GetQuickFixes(path string, diagnostics []protocol.Diagnostic) []protocol.CodeAction { e.T.Helper() loc := e.Sandbox.Workdir.EntireFile(path) diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index 1478fe631c7..e52f3807501 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/gopls/internal/test/marker/marker_test.go @@ -150,12 +150,14 @@ func Test(t *testing.T) { CapabilitiesJSON: test.capabilities, Env: test.env, } + if _, ok := config.Settings["diagnosticsDelay"]; !ok { if config.Settings == nil { config.Settings = make(map[string]any) } config.Settings["diagnosticsDelay"] = "10ms" } + // inv: config.Settings != nil run := &markerTestRun{ @@ -179,13 +181,32 @@ func Test(t *testing.T) { run.env.OpenFile(file) } + allDiags := make(map[string][]protocol.Diagnostic) + if run.env.Editor.ServerCapabilities().DiagnosticProvider != nil { + for name := range test.files { + // golang/go#53275: support pull diagnostics for go.mod and go.work + // files. + if strings.HasSuffix(name, ".go") { + allDiags[name] = run.env.Diagnostics(name) + } + } + } else { + // Wait for the didOpen notifications to be processed, then collect + // diagnostics. + + run.env.AfterChange() + var diags map[string]*protocol.PublishDiagnosticsParams + run.env.AfterChange(integration.ReadAllDiagnostics(&diags)) + for path, params := range diags { + allDiags[path] = params.Diagnostics + } + } + // Wait for the didOpen notifications to be processed, then collect // diagnostics. - var diags map[string]*protocol.PublishDiagnosticsParams - run.env.AfterChange(integration.ReadAllDiagnostics(&diags)) - for path, params := range diags { + for path, diags := range allDiags { uri := run.env.Sandbox.Workdir.URI(path) - for _, diag := range params.Diagnostics { + for _, diag := range diags { loc := protocol.Location{ URI: uri, Range: protocol.Range{ diff --git a/gopls/internal/test/marker/testdata/diagnostics/analyzers.txt b/gopls/internal/test/marker/testdata/diagnostics/analyzers.txt index 34488bec417..72da91ca7de 100644 --- a/gopls/internal/test/marker/testdata/diagnostics/analyzers.txt +++ b/gopls/internal/test/marker/testdata/diagnostics/analyzers.txt @@ -1,6 +1,11 @@ Test of warning diagnostics from various analyzers: copylocks, printf, slog, tests, timeformat, nilness, and cgocall. +-- settings.json -- +{ + "pullDiagnostics": true +} + -- go.mod -- module example.com go 1.12 diff --git a/gopls/internal/test/marker/testdata/diagnostics/generated.txt b/gopls/internal/test/marker/testdata/diagnostics/generated.txt index ea5886dae03..123602df3c3 100644 --- a/gopls/internal/test/marker/testdata/diagnostics/generated.txt +++ b/gopls/internal/test/marker/testdata/diagnostics/generated.txt @@ -1,5 +1,10 @@ Test of "undeclared" diagnostic in generated code. +-- settings.json -- +{ + "pullDiagnostics": true +} + -- go.mod -- module example.com go 1.12 diff --git a/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt b/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt index cd3ad6e9c63..22cff4315b5 100644 --- a/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt +++ b/gopls/internal/test/marker/testdata/diagnostics/issue56943.txt @@ -3,6 +3,11 @@ unexported interface methods in non-workspace packages. Previously, we would fail to produce a diagnostic because we trimmed the AST. See golang/go#56943. +-- settings.json -- +{ + "pullDiagnostics": true +} + -- main.go -- package main From f439874c29c661b1a48aa200b92ad86b5fb2b0b8 Mon Sep 17 00:00:00 2001 From: Peter Weinberger Date: Sun, 29 Sep 2024 16:36:05 -0400 Subject: [PATCH 08/95] internal/modindex: add symbol information This CL adds information about exported symbols to the module cache index, together with a minimal test. A future CL will add a Lookup function for finding completions for selectors. Change-Id: Ic36b5f30383187f9c5551ee2757258ed1445ea53 Reviewed-on: https://go-review.googlesource.com/c/tools/+/616416 LUCI-TryBot-Result: Go LUCI Reviewed-by: Robert Findley --- internal/modindex/dir_test.go | 16 ++- internal/modindex/directories.go | 1 + internal/modindex/index.go | 8 +- internal/modindex/modindex.go | 15 ++- internal/modindex/symbols.go | 184 +++++++++++++++++++++++++++++++ 5 files changed, 214 insertions(+), 10 deletions(-) create mode 100644 internal/modindex/symbols.go diff --git a/internal/modindex/dir_test.go b/internal/modindex/dir_test.go index 862d111ea42..47326a12352 100644 --- a/internal/modindex/dir_test.go +++ b/internal/modindex/dir_test.go @@ -37,7 +37,7 @@ var idtests = []id{ "cloud.google.com/go@v0.94.0/compute/metadata", }, }, - { //m test bizarre characters in directory name + { // test bizarre characters in directory name importPath: "bad,guy.com/go", best: 0, dirs: []string{"bad,guy.com/go@v0.1.0"}, @@ -51,17 +51,18 @@ func testModCache(t *testing.T) string { return dir } +// choose the semantically-latest version, with a single symbol func TestDirsSinglePath(t *testing.T) { for _, itest := range idtests { t.Run(itest.importPath, func(t *testing.T) { - // create a new fake GOMODCACHE + // create a new test GOMODCACHE dir := testModCache(t) for _, d := range itest.dirs { if err := os.MkdirAll(filepath.Join(dir, d), 0755); err != nil { t.Fatal(err) } - // gopathwalk wants to see .go files - err := os.WriteFile(filepath.Join(dir, d, "main.go"), []byte("package main\nfunc main() {}"), 0600) + err := os.WriteFile(filepath.Join(dir, d, "foo.go"), + []byte("package foo\nfunc Foo() {}"), 0600) if err != nil { t.Fatal(err) } @@ -83,6 +84,13 @@ func TestDirsSinglePath(t *testing.T) { if ix.Entries[0].Dir != Relpath(itest.dirs[itest.best]) { t.Fatalf("got dir %s, wanted %s", ix.Entries[0].Dir, itest.dirs[itest.best]) } + nms := ix.Entries[0].Names + if len(nms) != 1 { + t.Fatalf("got %d names, expected 1", len(nms)) + } + if nms[0] != "Foo F 0" { + t.Fatalf("got %q, expected Foo F 0", nms[0]) + } }) } } diff --git a/internal/modindex/directories.go b/internal/modindex/directories.go index b8aab3b736e..6f7bc16671f 100644 --- a/internal/modindex/directories.go +++ b/internal/modindex/directories.go @@ -23,6 +23,7 @@ type directory struct { path Relpath importPath string version string // semantic version + syms []symbol } // filterDirs groups the directories by import path, diff --git a/internal/modindex/index.go b/internal/modindex/index.go index eed8e41c21a..398484b1b73 100644 --- a/internal/modindex/index.go +++ b/internal/modindex/index.go @@ -207,11 +207,13 @@ func writeIndex(cachedir Abspath, ix *Index) error { } func writeIndexToFile(x *Index, fd *os.File) error { + cnt := 0 w := bufio.NewWriter(fd) fmt.Fprintf(w, "%d\n", x.Version) fmt.Fprintf(w, "%s\n", x.Cachedir) - // TODO(pjw): round the time down - fmt.Fprintf(w, "%s\n", x.Changed.Format(time.DateTime)) + // round the time down + tm := x.Changed.Add(-time.Second / 2) + fmt.Fprintf(w, "%s\n", tm.Format(time.DateTime)) for _, e := range x.Entries { if e.ImportPath == "" { continue // shouldn't happen @@ -227,11 +229,13 @@ func writeIndexToFile(x *Index, fd *os.File) error { } for _, x := range e.Names { fmt.Fprintf(w, "%s\n", x) + cnt++ } } if err := w.Flush(); err != nil { return err } + log.Printf("%d Entries %d names", len(x.Entries), cnt) return nil } diff --git a/internal/modindex/modindex.go b/internal/modindex/modindex.go index b6bfec43f98..fa18a1b6eb2 100644 --- a/internal/modindex/modindex.go +++ b/internal/modindex/modindex.go @@ -91,7 +91,7 @@ func (w *work) buildIndex() error { if err != nil { return err } - log.Printf("%d dirs, %d ips", len(dirs), len(newdirs)) + log.Printf("%d dirs, %d new", len(dirs), len(newdirs)) // for each import path it might occur only in newdirs, // only in w.oldIndex, or in both. // If it occurs in both, use the semantically later one @@ -111,7 +111,7 @@ func (w *work) buildIndex() error { delete(newdirs, e.ImportPath) } } - log.Printf("%d killed, %d ips", killed, len(newdirs)) + log.Printf("%d killed, %d new", killed, len(newdirs)) } // Build the skeleton of the new index using newdirs, // and include the surviving parts of the old index @@ -122,17 +122,24 @@ func (w *work) buildIndex() error { } } } + // get symbol information for all the new diredtories + getSymbols(w.cacheDir, newdirs) + // assemble the new index entries for k, v := range newdirs { d := v[0] + pkg, names := processSyms(d.syms) + if pkg == "" { + continue // PJW: does this ever happen? + } entry := Entry{ + PkgName: pkg, Dir: d.path, ImportPath: k, Version: d.version, + Names: names, } w.newIndex.Entries = append(w.newIndex.Entries, entry) } - // find symbols for the incomplete entries - log.Print("not finding any symbols yet") // sort the entries in the new index slices.SortFunc(w.newIndex.Entries, func(l, r Entry) int { if n := strings.Compare(l.PkgName, r.PkgName); n != 0 { diff --git a/internal/modindex/symbols.go b/internal/modindex/symbols.go new file mode 100644 index 00000000000..c15e803e77b --- /dev/null +++ b/internal/modindex/symbols.go @@ -0,0 +1,184 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package modindex + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "go/types" + "os" + "path/filepath" + "slices" + "strings" + + "golang.org/x/sync/errgroup" +) + +// The name of a symbol contains information about the symbol: +// T for types +// C for consts +// V for vars +// and for funcs: F ( )* +// any spaces in are replaced by $s so that the fields +// of the name are space separated +type symbol struct { + pkg string // name of the symbols's package + name string // declared name + kind string // T, C, V, or F + sig string // signature information, for F +} + +// find the symbols for the best directories +func getSymbols(cd Abspath, dirs map[string][]*directory) { + var g errgroup.Group + g.SetLimit(-1) // maybe throttle this some day + for _, vv := range dirs { + // throttling some day? + d := vv[0] + g.Go(func() error { + thedir := filepath.Join(string(cd), string(d.path)) + mode := parser.SkipObjectResolution + + fi, err := os.ReadDir(thedir) + if err != nil { + return nil // log this someday? + } + for _, fx := range fi { + if !strings.HasSuffix(fx.Name(), ".go") || strings.HasSuffix(fx.Name(), "_test.go") { + continue + } + fname := filepath.Join(thedir, fx.Name()) + tr, err := parser.ParseFile(token.NewFileSet(), fname, nil, mode) + if err != nil { + continue // ignore errors, someday log them? + } + d.syms = append(d.syms, getFileExports(tr)...) + } + return nil + }) + } + g.Wait() +} + +func getFileExports(f *ast.File) []symbol { + pkg := f.Name.Name + if pkg == "main" { + return nil + } + var ans []symbol + // should we look for //go:build ignore? + for _, decl := range f.Decls { + switch decl := decl.(type) { + case *ast.FuncDecl: + if decl.Recv != nil { + // ignore methods, as we are completing package selections + continue + } + name := decl.Name.Name + dtype := decl.Type + // not looking at dtype.TypeParams. That is, treating + // generic functions just like non-generic ones. + sig := dtype.Params + kind := "F" + result := []string{fmt.Sprintf("%d", dtype.Results.NumFields())} + for _, x := range sig.List { + // This code creates a string representing the type. + // TODO(pjw): it may be fragile: + // 1. x.Type could be nil, perhaps in ill-formed code + // 2. ExprString might someday change incompatibly to + // include struct tags, which can be arbitrary strings + if x.Type == nil { + // Can this happen without a parse error? (Files with parse + // errors are ignored in getSymbols) + continue // maybe report this someday + } + tp := types.ExprString(x.Type) + if len(tp) == 0 { + // Can this happen? + continue // maybe report this someday + } + // This is only safe if ExprString never returns anything with a $ + // The only place a $ can occur seems to be in a struct tag, which + // can be an arbitrary string literal, and ExprString does not presently + // print struct tags. So for this to happen the type of a formal parameter + // has to be a explict struct, e.g. foo(x struct{a int "$"}) and ExprString + // would have to show the struct tag. Even testing for this case seems + // a waste of effort, but let's not ignore such pathologies + if strings.Contains(tp, "$") { + continue + } + tp = strings.Replace(tp, " ", "$", -1) + for _, y := range x.Names { + result = append(result, y.Name) + result = append(result, tp) + } + } + sigs := strings.Join(result, " ") + if s := newsym(pkg, name, kind, sigs); s != nil { + ans = append(ans, *s) + } + case *ast.GenDecl: + switch decl.Tok { + case token.CONST, token.VAR: + tp := " V" + if decl.Tok == token.CONST { + tp = " C" + } + for _, sp := range decl.Specs { + for _, x := range sp.(*ast.ValueSpec).Names { + if s := newsym(pkg, x.Name, tp, ""); s != nil { + ans = append(ans, *s) + } + } + } + case token.TYPE: + for _, sp := range decl.Specs { + if s := newsym(pkg, sp.(*ast.TypeSpec).Name.Name, " T", ""); s != nil { + ans = append(ans, *s) + } + } + } + } + } + return ans +} + +func newsym(pkg, name, kind, sig string) *symbol { + if len(name) == 0 || !ast.IsExported(name) { + return nil + } + sym := symbol{pkg: pkg, name: name, kind: kind, sig: sig} + return &sym +} + +// return the package name and the value for the symbols. +// if there are multiple packages, choose one arbitrarily +// the returned slice is sorted lexicographically +func processSyms(syms []symbol) (string, []string) { + if len(syms) == 0 { + return "", nil + } + slices.SortFunc(syms, func(l, r symbol) int { + return strings.Compare(l.name, r.name) + }) + pkg := syms[0].pkg + var names []string + for _, s := range syms { + var nx string + if s.pkg == pkg { + if s.sig != "" { + nx = fmt.Sprintf("%s %s %s", s.name, s.kind, s.sig) + } else { + nx = fmt.Sprintf("%s %s", s.name, s.kind) + } + names = append(names, nx) + } else { + continue // PJW: do we want to keep track of these? + } + } + return pkg, names +} From 86219190c2d38b94bdb3d9a1b220da3ce7093487 Mon Sep 17 00:00:00 2001 From: Tim King Date: Tue, 8 Oct 2024 10:23:25 -0700 Subject: [PATCH 09/95] go/ssa/ssautil: disable fmt imports on wasm tests Fixes golang/go#69783 Change-Id: Iebdd90527de4efe8995d5b42d6e25089761135dc Reviewed-on: https://go-review.googlesource.com/c/tools/+/618555 Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI Auto-Submit: Tim King --- go/ssa/ssautil/deprecated_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/go/ssa/ssautil/deprecated_test.go b/go/ssa/ssautil/deprecated_test.go index 9bc39e7eebd..1793b06dcdb 100644 --- a/go/ssa/ssautil/deprecated_test.go +++ b/go/ssa/ssautil/deprecated_test.go @@ -15,10 +15,13 @@ import ( "golang.org/x/tools/go/loader" "golang.org/x/tools/go/ssa" "golang.org/x/tools/go/ssa/ssautil" + "golang.org/x/tools/internal/testenv" ) // TestCreateProgram tests CreateProgram which has an x/tools/go/loader.Program. func TestCreateProgram(t *testing.T) { + testenv.NeedsGoBuild(t) // for importer.Default() + conf := loader.Config{ParserMode: parser.ParseComments} f, err := conf.ParseFile("hello.go", hello) if err != nil { From cf8979b7fc8139138a18821b2f64bb80e8548ecd Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 8 Oct 2024 16:47:50 -0400 Subject: [PATCH 10/95] gopls/doc/features: add index of supported Code Actions Also: - add reminders to keep things in sync. - use explicit anchors instead of relying on GitHub's unspecified algorithm (which doesn't work in mdweb preview). Fixes golang/go#54115 Fixes golang/go#68791 Change-Id: I17c800075260e2e72dcd239fb7248a0cb3851d5d Reviewed-on: https://go-review.googlesource.com/c/tools/+/618616 Auto-Submit: Alan Donovan Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI --- gopls/doc/features/README.md | 8 ++--- gopls/doc/features/transformation.md | 44 +++++++++++++++++++---- gopls/internal/settings/codeactionkind.go | 9 +++-- 3 files changed, 46 insertions(+), 15 deletions(-) diff --git a/gopls/doc/features/README.md b/gopls/doc/features/README.md index 41449de7f20..92203ed677a 100644 --- a/gopls/doc/features/README.md +++ b/gopls/doc/features/README.md @@ -46,10 +46,10 @@ when making significant changes to existing features or when adding new ones. - [Code transformation](transformation.md): fixes and refactorings - [Formatting](transformation.md#formatting): format the source code - [Rename](transformation.md#rename): rename a symbol or package - - [Organize imports](transformation.md#organize-imports): organize the import declaration - - [Extract](transformation.md#extract): extract selection to a new file/function/variable - - [Inline](transformation.md#inline): inline a call to a function or method - - [Miscellaneous rewrites](transformation.md#miscellaneous-rewrites): various Go-specific refactorings + - [Organize imports](transformation.md#source.organizeImports): organize the import declaration + - [Extract](transformation.md#refactor.extract): extract selection to a new file/function/variable + - [Inline](transformation.md#refactor.inline.call): inline a call to a function or method + - [Miscellaneous rewrites](transformation.md#refactor.rewrite): various Go-specific refactorings - [Web-based queries](web.md): commands that open a browser page - [Package documentation](web.md#doc): browse documentation for current Go package - [Free symbols](web.md#freesymbols): show symbols used by a selected block of code diff --git a/gopls/doc/features/transformation.md b/gopls/doc/features/transformation.md index b65cd62424a..ac1bf5f8333 100644 --- a/gopls/doc/features/transformation.md +++ b/gopls/doc/features/transformation.md @@ -59,6 +59,28 @@ a lightbulb icon that triggers a menu of "quick fixes" (of kind `quickfix`); and a "Fix All" command that executes all code actions of kind `source.fixAll`, which are those deemed unambiguously safe to apply. +Gopls supports the following code actions: + +- `quickfix`, which applies unambiguously safe fixes +- [`source.organizeImports`](#source.organizeImports) +- [`source.assembly`](web.md#assembly) +- [`source.doc`](web.md#doc) +- [`source.freesymbols`](web.md#freesymbols) +- `source.test` (undocumented) +- [`gopls.doc.features`](README.md), which opens gopls' index of features in a browser +- [`refactor.extract.function`](#extract) +- [`refactor.extract.method`](#extract) +- [`refactor.extract.toNewFile`](#extract.toNewFile) +- [`refactor.extract.variable`](#extract) +- [`refactor.inline.call`](#refactor.inline.call) +- [`refactor.rewrite.changeQuote`](#refactor.rewrite.changeQuote) +- [`refactor.rewrite.fillStruct`](#refactor.rewrite.fillStruct) +- [`refactor.rewrite.fillSwitch`](#refactor.rewrite.fillSwitch) +- [`refactor.rewrite.invertIf`](#refactor.rewrite.invertIf) +- [`refactor.rewrite.joinLines`](#refactor.rewrite.joinLines) +- [`refactor.rewrite.removeUnusedParam`](#refactor.rewrite.removeUnusedParam) +- [`refactor.rewrite.splitLines`](#refactor.rewrite.splitLines) + Gopls reports some code actions twice, with two different kinds, so that they appear in multiple UI elements: simplifications, for example from `for _ = range m` to `for range m`, @@ -120,6 +142,7 @@ Client support for code actions: - **CLI**: `gopls codeaction -exec -kind k,... -diff file.go:#123-#456` executes code actions of the specified kinds (e.g. `refactor.inline`) on the selected range, specified using zero-based byte offsets, and displays the diff. + ## Formatting The LSP @@ -141,7 +164,8 @@ Client support: - **Emacs + eglot**: Use `M-x eglot-format-buffer` to format. Attach it to `before-save-hook` to format on save. For formatting combined with organize-imports, many users take the legacy approach of setting `"goimports"` as their `gofmt-command` using [go-mode](https://github.com/dominikh/go-mode.el), and adding `gofmt-before-save` to `before-save-hook`. An LSP-based solution requires code such as https://github.com/joaotavora/eglot/discussions/1409. - **CLI**: `gopls format file.go` -## Organize imports + +## `source.organizeImports`: Organize imports A `codeActions` request in a file whose imports are not organized will return an action of the standard kind `source.organizeImports`. @@ -187,7 +211,8 @@ Client support: - **CLI**: `gopls fix -a file.go:#offset source.organizeImports` -## `refactor.rename`: Rename + +## Rename The LSP [`textDocument/rename`](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_rename) @@ -268,7 +293,7 @@ Client support: - **CLI**: `gopls rename file.go:#offset newname` - + ## `refactor.extract`: Extract function/method/variable The `refactor.extract` family of code actions all return commands that @@ -326,7 +351,7 @@ The following Extract features are planned for 2024 but not yet supported: see golang/go#65721 and golang/go#46665. - + ## `refactor.extract.toNewFile`: Extract declarations to new file (Available from gopls/v0.17.0) @@ -343,7 +368,7 @@ first token of the declaration, such as `func` or `type`. ![After: the new file is based on the first symbol name](../assets/extract-to-new-file-after.png) - + ## `refactor.inline.call`: Inline call to function For a `codeActions` request where the selection is (or is within) a @@ -498,11 +523,13 @@ more detail. All of this is to say, it's a complex problem, and we aim for correctness first of all. We've already implemented a number of important "tidiness optimizations" and we expect more to follow. + ## `refactor.rewrite`: Miscellaneous rewrites This section covers a number of transformations that are accessible as code actions whose kinds are children of `refactor.rewrite`. + ### `refactor.rewrite.removeUnusedParam`: Remove unused parameter The [`unusedparams` analyzer](../analyzers.md#unusedparams) reports a @@ -538,6 +565,7 @@ Observe that in the first call, the argument `chargeCreditCard()` was not deleted because of potential side effects, whereas in the second call, the argument 2, a constant, was safely deleted. + ### `refactor.rewrite.changeQuote`: Convert string literal between raw and interpreted When the selection is a string literal, gopls offers a code action @@ -550,6 +578,7 @@ form (`"abc"`) where this is possible: Applying the code action a second time reverts back to the original form. + ### `refactor.rewrite.invertIf`: Invert 'if' condition When the selection is within an `if`/`else` statement that is not @@ -565,6 +594,8 @@ blocks. if the else block ends with a return statement; and thus applying the operation twice does not get you back to where you started. --> + + ### `refactor.rewrite.{split,join}Lines`: Split elements into separate lines When the selection is within a bracketed list of items such as: @@ -612,7 +643,7 @@ These code actions are not offered for lists containing `//`-style comments, which run to the end of the line. - + ### `refactor.rewrite.fillStruct`: Fill struct literal When the cursor is within a struct literal `S{}`, gopls offers the @@ -645,6 +676,7 @@ Caveats: or in other files in the package, are not considered; see golang/go#68224. + ### `refactor.rewrite.fillSwitch`: Fill switch When the cursor is within a switch statement whose operand type is an diff --git a/gopls/internal/settings/codeactionkind.go b/gopls/internal/settings/codeactionkind.go index fa06b90e7e3..177431b5f06 100644 --- a/gopls/internal/settings/codeactionkind.go +++ b/gopls/internal/settings/codeactionkind.go @@ -81,10 +81,6 @@ const ( GoTest protocol.CodeActionKind = "source.test" // gopls - // TODO(adonovan): we should not use this category as it will - // never be requested now that we no longer interpret "no kind - // restriction" as "quickfix" instead of "all kinds". - // We need another way to make docs discoverable. GoplsDocFeatures protocol.CodeActionKind = "gopls.doc.features" // refactor.rewrite @@ -105,5 +101,8 @@ const ( RefactorExtractVariable protocol.CodeActionKind = "refactor.extract.variable" RefactorExtractToNewFile protocol.CodeActionKind = "refactor.extract.toNewFile" - // Note: add new kinds to the SupportedCodeActions map in defaults.go too. + // Note: add new kinds to: + // - the SupportedCodeActions map in default.go + // - the codeActionProducers table in ../golang/codeaction.go + // - the docs in ../../doc/features/transformation.md ) From bbb979fff68ce3e1e12dbed1ea6718cc3222e389 Mon Sep 17 00:00:00 2001 From: John Dethridge Date: Wed, 18 Sep 2024 13:23:12 +0000 Subject: [PATCH 11/95] go/callgraph/vta: use node IDs for type flow graph The vtaGraph type stores a directed graph, where the nodes are interface values. This change maps the nodes to unique integers internally and stores the adjacency lists of the graph using the integers, which are smaller and faster to work with. This saves 40% time and peak memory usage when analyzing some very large projects. Change-Id: I45468ea9b478d2291f80140158cc8719e992b14e Reviewed-on: https://go-review.googlesource.com/c/tools/+/614095 Reviewed-by: Zvonimir Pavlinovic Auto-Submit: Alan Donovan Reviewed-by: Tim King LUCI-TryBot-Result: Go LUCI --- go/callgraph/vta/graph.go | 57 +++++++-- go/callgraph/vta/graph_test.go | 35 ++++-- go/callgraph/vta/propagation.go | 139 ++++++++++++---------- go/callgraph/vta/propagation_test.go | 172 ++++++++++++++++++--------- 4 files changed, 262 insertions(+), 141 deletions(-) diff --git a/go/callgraph/vta/graph.go b/go/callgraph/vta/graph.go index 61e841a39ae..c13b8a5e6cb 100644 --- a/go/callgraph/vta/graph.go +++ b/go/callgraph/vta/graph.go @@ -255,27 +255,64 @@ func (r recoverReturn) String() string { type empty = struct{} +// idx is an index representing a unique node in a vtaGraph. +type idx int + // vtaGraph remembers for each VTA node the set of its successors. // Tailored for VTA, hence does not support singleton (sub)graphs. -type vtaGraph map[node]map[node]empty +type vtaGraph struct { + m []map[idx]empty // m[i] has the successors for the node with index i. + idx map[node]idx // idx[n] is the index for the node n. + node []node // node[i] is the node with index i. +} + +func (g *vtaGraph) numNodes() int { + return len(g.idx) +} + +func (g *vtaGraph) successors(x idx) func(yield func(y idx) bool) { + return func(yield func(y idx) bool) { + for y := range g.m[x] { + if !yield(y) { + return + } + } + } +} // addEdge adds an edge x->y to the graph. -func (g vtaGraph) addEdge(x, y node) { - succs, ok := g[x] - if !ok { - succs = make(map[node]empty) - g[x] = succs +func (g *vtaGraph) addEdge(x, y node) { + if g.idx == nil { + g.idx = make(map[node]idx) + } + lookup := func(n node) idx { + i, ok := g.idx[n] + if !ok { + i = idx(len(g.idx)) + g.m = append(g.m, nil) + g.idx[n] = i + g.node = append(g.node, n) + } + return i + } + a := lookup(x) + b := lookup(y) + succs := g.m[a] + if succs == nil { + succs = make(map[idx]empty) + g.m[a] = succs } - succs[y] = empty{} + succs[b] = empty{} } // typePropGraph builds a VTA graph for a set of `funcs` and initial // `callgraph` needed to establish interprocedural edges. Returns the // graph and a map for unique type representatives. -func typePropGraph(funcs map[*ssa.Function]bool, callees calleesFunc) (vtaGraph, *typeutil.Map) { - b := builder{graph: make(vtaGraph), callees: callees} +func typePropGraph(funcs map[*ssa.Function]bool, callees calleesFunc) (*vtaGraph, *typeutil.Map) { + b := builder{callees: callees} b.visit(funcs) - return b.graph, &b.canon + b.callees = nil // ensure callees is not pinned by pointers to other fields of b. + return &b.graph, &b.canon } // Data structure responsible for linearly traversing the diff --git a/go/callgraph/vta/graph_test.go b/go/callgraph/vta/graph_test.go index ace80859e21..9e780c7e4e2 100644 --- a/go/callgraph/vta/graph_test.go +++ b/go/callgraph/vta/graph_test.go @@ -110,7 +110,7 @@ func TestVtaGraph(t *testing.T) { // n3 / // | / // n4 - g := make(vtaGraph) + var g vtaGraph g.addEdge(n1, n3) g.addEdge(n2, n3) g.addEdge(n3, n4) @@ -119,9 +119,19 @@ func TestVtaGraph(t *testing.T) { g.addEdge(n1, n3) want := vtaGraph{ - n1: map[node]empty{n3: empty{}}, - n2: map[node]empty{n3: empty{}, n4: empty{}}, - n3: map[node]empty{n4: empty{}}, + m: []map[idx]empty{ + map[idx]empty{1: empty{}}, + map[idx]empty{3: empty{}}, + map[idx]empty{1: empty{}, 3: empty{}}, + nil, + }, + idx: map[node]idx{ + n1: 0, + n3: 1, + n2: 2, + n4: 3, + }, + node: []node{n1, n3, n2, n4}, } if !reflect.DeepEqual(want, g) { @@ -137,7 +147,9 @@ func TestVtaGraph(t *testing.T) { {n3, 1}, {n4, 0}, } { - if sl := len(g[test.n]); sl != test.l { + sl := 0 + g.successors(g.idx[test.n])(func(_ idx) bool { sl++; return true }) + if sl != test.l { t.Errorf("want %d successors; got %d", test.l, sl) } } @@ -147,15 +159,16 @@ func TestVtaGraph(t *testing.T) { // where each string represents an edge set of the format // node -> succ_1, ..., succ_n. succ_1, ..., succ_n are // sorted in alphabetical order. -func vtaGraphStr(g vtaGraph) []string { +func vtaGraphStr(g *vtaGraph) []string { var vgs []string - for n, succ := range g { + for n := 0; n < g.numNodes(); n++ { var succStr []string - for s := range succ { - succStr = append(succStr, s.String()) - } + g.successors(idx(n))(func(s idx) bool { + succStr = append(succStr, g.node[s].String()) + return true + }) sort.Strings(succStr) - entry := fmt.Sprintf("%v -> %v", n.String(), strings.Join(succStr, ", ")) + entry := fmt.Sprintf("%v -> %v", g.node[n].String(), strings.Join(succStr, ", ")) vgs = append(vgs, removeModulePrefix(entry)) } return vgs diff --git a/go/callgraph/vta/propagation.go b/go/callgraph/vta/propagation.go index 4274f482d10..f448cde1135 100644 --- a/go/callgraph/vta/propagation.go +++ b/go/callgraph/vta/propagation.go @@ -6,6 +6,7 @@ package vta import ( "go/types" + "slices" "golang.org/x/tools/go/callgraph/vta/internal/trie" "golang.org/x/tools/go/ssa" @@ -14,63 +15,66 @@ import ( ) // scc computes strongly connected components (SCCs) of `g` using the -// classical Tarjan's algorithm for SCCs. The result is a pair -// where m is a map from nodes to unique id of their SCC in the range -// [0, id). The SCCs are sorted in reverse topological order: for SCCs +// classical Tarjan's algorithm for SCCs. The result is two slices: +// - sccs: the SCCs, each represented as a slice of node indices +// - idxToSccID: the inverse map, from node index to SCC number. +// +// The SCCs are sorted in reverse topological order: for SCCs // with ids X and Y s.t. X < Y, Y comes before X in the topological order. -func scc(g vtaGraph) (map[node]int, int) { +func scc(g *vtaGraph) (sccs [][]idx, idxToSccID []int) { // standard data structures used by Tarjan's algorithm. type state struct { - index int + pre int // preorder of the node (0 if unvisited) lowLink int onStack bool } - states := make(map[node]*state, len(g)) - var stack []node + states := make([]state, g.numNodes()) + var stack []idx - nodeToSccID := make(map[node]int, len(g)) - sccID := 0 + idxToSccID = make([]int, g.numNodes()) + nextPre := 0 - var doSCC func(node) - doSCC = func(n node) { - index := len(states) - ns := &state{index: index, lowLink: index, onStack: true} - states[n] = ns + var doSCC func(idx) + doSCC = func(n idx) { + nextPre++ + ns := &states[n] + *ns = state{pre: nextPre, lowLink: nextPre, onStack: true} stack = append(stack, n) - for s := range g[n] { - if ss, visited := states[s]; !visited { + g.successors(n)(func(s idx) bool { + if ss := &states[s]; ss.pre == 0 { // Analyze successor s that has not been visited yet. doSCC(s) - ss = states[s] ns.lowLink = min(ns.lowLink, ss.lowLink) } else if ss.onStack { // The successor is on the stack, meaning it has to be // in the current SCC. - ns.lowLink = min(ns.lowLink, ss.index) + ns.lowLink = min(ns.lowLink, ss.pre) } - } + return true + }) // if n is a root node, pop the stack and generate a new SCC. - if ns.lowLink == index { - var w node - for w != n { - w = stack[len(stack)-1] - stack = stack[:len(stack)-1] + if ns.lowLink == ns.pre { + sccStart := slicesLastIndex(stack, n) + scc := slices.Clone(stack[sccStart:]) + stack = stack[:sccStart] + sccID := len(sccs) + sccs = append(sccs, scc) + for _, w := range scc { states[w].onStack = false - nodeToSccID[w] = sccID + idxToSccID[w] = sccID } - sccID++ } } - for n := range g { - if _, visited := states[n]; !visited { - doSCC(n) + for n, nn := 0, g.numNodes(); n < nn; n++ { + if states[n].pre == 0 { + doSCC(idx(n)) } } - return nodeToSccID, sccID + return sccs, idxToSccID } func min(x, y int) int { @@ -80,6 +84,21 @@ func min(x, y int) int { return y } +// LastIndex returns the index of the last occurrence of v in s, or -1 if v is +// not present in s. +// +// LastIndex iterates backwards through the elements of s, stopping when the == +// operator determines an element is equal to v. +func slicesLastIndex[S ~[]E, E comparable](s S, v E) int { + // TODO: move to / dedup with slices.LastIndex + for i := len(s) - 1; i >= 0; i-- { + if s[i] == v { + return i + } + } + return -1 +} + // propType represents type information being propagated // over the vta graph. f != nil only for function nodes // and nodes reachable from function nodes. There, we also @@ -92,10 +111,7 @@ type propType struct { // propTypeMap is an auxiliary structure that serves // the role of a map from nodes to a set of propTypes. -type propTypeMap struct { - nodeToScc map[node]int - sccToTypes map[int]*trie.MutMap -} +type propTypeMap map[node]*trie.MutMap // propTypes returns a go1.23 iterator for the propTypes associated with // node `n` in map `ptm`. @@ -103,8 +119,8 @@ func (ptm propTypeMap) propTypes(n node) func(yield func(propType) bool) { // TODO: when x/tools uses go1.23, change callers to use range-over-func // (https://go.dev/issue/65237). return func(yield func(propType) bool) { - if id, ok := ptm.nodeToScc[n]; ok { - ptm.sccToTypes[id].M.Range(func(_ uint64, elem interface{}) bool { + if types := ptm[n]; types != nil { + types.M.Range(func(_ uint64, elem interface{}) bool { return yield(elem.(propType)) }) } @@ -116,14 +132,8 @@ func (ptm propTypeMap) propTypes(n node) func(yield func(propType) bool) { // graph. The result is a map from nodes to a set of types // and functions, stemming from higher-order data flow, // reaching the node. `canon` is used for type uniqueness. -func propagate(graph vtaGraph, canon *typeutil.Map) propTypeMap { - nodeToScc, sccID := scc(graph) - - // We also need the reverse map, from ids to SCCs. - sccs := make(map[int][]node, sccID) - for n, id := range nodeToScc { - sccs[id] = append(sccs[id], n) - } +func propagate(graph *vtaGraph, canon *typeutil.Map) propTypeMap { + sccs, idxToSccID := scc(graph) // propTypeIds are used to create unique ids for // propType, to be used for trie-based type sets. @@ -141,37 +151,40 @@ func propagate(graph vtaGraph, canon *typeutil.Map) propTypeMap { builder := trie.NewBuilder() // Initialize sccToTypes to avoid repeated check // for initialization later. - sccToTypes := make(map[int]*trie.MutMap, sccID) - for i := 0; i <= sccID; i++ { - sccToTypes[i] = nodeTypes(sccs[i], builder, propTypeId, canon) + sccToTypes := make([]*trie.MutMap, len(sccs)) + for sccID, scc := range sccs { + typeSet := builder.MutEmpty() + for _, idx := range scc { + if n := graph.node[idx]; hasInitialTypes(n) { + // add the propType for idx to typeSet. + pt := getPropType(n, canon) + typeSet.Update(propTypeId(pt), pt) + } + } + sccToTypes[sccID] = &typeSet } for i := len(sccs) - 1; i >= 0; i-- { nextSccs := make(map[int]empty) - for _, node := range sccs[i] { - for succ := range graph[node] { - nextSccs[nodeToScc[succ]] = empty{} - } + for _, n := range sccs[i] { + graph.successors(n)(func(succ idx) bool { + nextSccs[idxToSccID[succ]] = empty{} + return true + }) } // Propagate types to all successor SCCs. for nextScc := range nextSccs { sccToTypes[nextScc].Merge(sccToTypes[i].M) } } - return propTypeMap{nodeToScc: nodeToScc, sccToTypes: sccToTypes} -} - -// nodeTypes returns a set of propTypes for `nodes`. These are the -// propTypes stemming from the type of each node in `nodes` plus. -func nodeTypes(nodes []node, builder *trie.Builder, propTypeId func(p propType) uint64, canon *typeutil.Map) *trie.MutMap { - typeSet := builder.MutEmpty() - for _, n := range nodes { - if hasInitialTypes(n) { - pt := getPropType(n, canon) - typeSet.Update(propTypeId(pt), pt) + nodeToTypes := make(propTypeMap, graph.numNodes()) + for sccID, scc := range sccs { + types := sccToTypes[sccID] + for _, idx := range scc { + nodeToTypes[graph.node[idx]] = types } } - return &typeSet + return nodeToTypes } // hasInitialTypes check if a node can have initial types. diff --git a/go/callgraph/vta/propagation_test.go b/go/callgraph/vta/propagation_test.go index 87b80a20db7..1a274f38f84 100644 --- a/go/callgraph/vta/propagation_test.go +++ b/go/callgraph/vta/propagation_test.go @@ -7,7 +7,9 @@ package vta import ( "go/token" "go/types" + "math" "reflect" + "slices" "sort" "strings" "testing" @@ -64,17 +66,12 @@ func newNamedType(name string) *types.Named { // sccString is a utility for stringifying `nodeToScc`. Every // scc is represented as a string where string representation // of scc nodes are sorted and concatenated using `;`. -func sccString(nodeToScc map[node]int) []string { - sccs := make(map[int][]node) - for n, id := range nodeToScc { - sccs[id] = append(sccs[id], n) - } - +func sccString(sccs [][]idx, g *vtaGraph) []string { var sccsStr []string for _, scc := range sccs { var nodesStr []string - for _, node := range scc { - nodesStr = append(nodesStr, node.String()) + for _, idx := range scc { + nodesStr = append(nodesStr, g.node[idx].String()) } sort.Strings(nodesStr) sccsStr = append(sccsStr, strings.Join(nodesStr, ";")) @@ -99,7 +96,7 @@ func nodeToTypeString(pMap propTypeMap) map[string]string { } nodeToTypeStr := make(map[string]string) - for node := range pMap.nodeToScc { + for node := range pMap { var propStrings []string pMap.propTypes(node)(func(prop propType) bool { propStrings = append(propStrings, propTypeString(prop)) @@ -126,12 +123,31 @@ func sccEqual(sccs1 []string, sccs2 []string) bool { // topological order: // // for every edge x -> y in g, nodeToScc[x] > nodeToScc[y] -func isRevTopSorted(g vtaGraph, nodeToScc map[node]int) bool { - for n, succs := range g { - for s := range succs { - if nodeToScc[n] < nodeToScc[s] { +func isRevTopSorted(g *vtaGraph, idxToScc []int) bool { + result := true + for n := 0; n < len(idxToScc); n++ { + g.successors(idx(n))(func(s idx) bool { + if idxToScc[n] < idxToScc[s] { + result = false return false } + return true + }) + } + return result +} + +func sccMapsConsistent(sccs [][]idx, idxToSccID []int) bool { + for id, scc := range sccs { + for _, idx := range scc { + if idxToSccID[idx] != id { + return false + } + } + } + for i, id := range idxToSccID { + if !slices.Contains(sccs[id], idx(i)) { + return false } } return true @@ -183,7 +199,7 @@ func setName(f *ssa.Function, name string) { // t1 (A) -> t2 (B) -> F1 -> F2 -> F3 -> F4 // | | | | // <------- <------------ -func testSuite() map[string]vtaGraph { +func testSuite() map[string]*vtaGraph { a := newNamedType("A") b := newNamedType("B") c := newNamedType("C") @@ -198,44 +214,51 @@ func testSuite() map[string]vtaGraph { f4 := &ssa.Function{Signature: sig} setName(f4, "F4") - graphs := make(map[string]vtaGraph) - graphs["no-cycles"] = map[node]map[node]empty{ - newLocal("t0", a): {newLocal("t1", b): empty{}}, - newLocal("t1", b): {newLocal("t2", c): empty{}}, - } - - graphs["trivial-cycle"] = map[node]map[node]empty{ - newLocal("t0", a): {newLocal("t0", a): empty{}}, - newLocal("t1", b): {newLocal("t1", b): empty{}}, - } - - graphs["circle-cycle"] = map[node]map[node]empty{ - newLocal("t0", a): {newLocal("t1", a): empty{}}, - newLocal("t1", a): {newLocal("t2", b): empty{}}, - newLocal("t2", b): {newLocal("t0", a): empty{}}, - } - - graphs["fully-connected"] = map[node]map[node]empty{ - newLocal("t0", a): {newLocal("t1", b): empty{}, newLocal("t2", c): empty{}}, - newLocal("t1", b): {newLocal("t0", a): empty{}, newLocal("t2", c): empty{}}, - newLocal("t2", c): {newLocal("t0", a): empty{}, newLocal("t1", b): empty{}}, - } - - graphs["subsumed-scc"] = map[node]map[node]empty{ - newLocal("t0", a): {newLocal("t1", b): empty{}}, - newLocal("t1", b): {newLocal("t2", b): empty{}}, - newLocal("t2", b): {newLocal("t1", b): empty{}, newLocal("t3", a): empty{}}, - newLocal("t3", a): {newLocal("t0", a): empty{}}, - } - - graphs["more-realistic"] = map[node]map[node]empty{ - newLocal("t0", a): {newLocal("t0", a): empty{}}, - newLocal("t1", a): {newLocal("t2", b): empty{}}, - newLocal("t2", b): {newLocal("t1", a): empty{}, function{f1}: empty{}}, - function{f1}: {function{f2}: empty{}, function{f3}: empty{}}, - function{f2}: {function{f3}: empty{}}, - function{f3}: {function{f1}: empty{}, function{f4}: empty{}}, - } + graphs := make(map[string]*vtaGraph) + v := &vtaGraph{} + graphs["no-cycles"] = v + v.addEdge(newLocal("t0", a), newLocal("t1", b)) + v.addEdge(newLocal("t1", b), newLocal("t2", c)) + + v = &vtaGraph{} + graphs["trivial-cycle"] = v + v.addEdge(newLocal("t0", a), newLocal("t0", a)) + v.addEdge(newLocal("t1", b), newLocal("t1", b)) + + v = &vtaGraph{} + graphs["circle-cycle"] = v + v.addEdge(newLocal("t0", a), newLocal("t1", a)) + v.addEdge(newLocal("t1", a), newLocal("t2", b)) + v.addEdge(newLocal("t2", b), newLocal("t0", a)) + + v = &vtaGraph{} + graphs["fully-connected"] = v + v.addEdge(newLocal("t0", a), newLocal("t1", b)) + v.addEdge(newLocal("t0", a), newLocal("t2", c)) + v.addEdge(newLocal("t1", b), newLocal("t0", a)) + v.addEdge(newLocal("t1", b), newLocal("t2", c)) + v.addEdge(newLocal("t2", c), newLocal("t0", a)) + v.addEdge(newLocal("t2", c), newLocal("t1", b)) + + v = &vtaGraph{} + graphs["subsumed-scc"] = v + v.addEdge(newLocal("t0", a), newLocal("t1", b)) + v.addEdge(newLocal("t1", b), newLocal("t2", b)) + v.addEdge(newLocal("t2", b), newLocal("t1", b)) + v.addEdge(newLocal("t2", b), newLocal("t3", a)) + v.addEdge(newLocal("t3", a), newLocal("t0", a)) + + v = &vtaGraph{} + graphs["more-realistic"] = v + v.addEdge(newLocal("t0", a), newLocal("t0", a)) + v.addEdge(newLocal("t1", a), newLocal("t2", b)) + v.addEdge(newLocal("t2", b), newLocal("t1", a)) + v.addEdge(newLocal("t2", b), function{f1}) + v.addEdge(function{f1}, function{f2}) + v.addEdge(function{f1}, function{f3}) + v.addEdge(function{f2}, function{f3}) + v.addEdge(function{f3}, function{f1}) + v.addEdge(function{f3}, function{f4}) return graphs } @@ -244,7 +267,7 @@ func TestSCC(t *testing.T) { suite := testSuite() for _, test := range []struct { name string - graph vtaGraph + graph *vtaGraph want []string }{ // No cycles results in three separate SCCs: {t0} {t1} {t2} @@ -260,13 +283,17 @@ func TestSCC(t *testing.T) { // The more realistic example has the following SCCs: {t0} {t1, t2} {F1, F2, F3} {F4} {name: "more-realistic", graph: suite["more-realistic"], want: []string{"Local(t0)", "Local(t1);Local(t2)", "Function(F1);Function(F2);Function(F3)", "Function(F4)"}}, } { - sccs, _ := scc(test.graph) - if got := sccString(sccs); !sccEqual(test.want, got) { + sccs, idxToSccID := scc(test.graph) + if got := sccString(sccs, test.graph); !sccEqual(test.want, got) { t.Errorf("want %v for graph %v; got %v", test.want, test.name, got) } - if !isRevTopSorted(test.graph, sccs) { + if !isRevTopSorted(test.graph, idxToSccID) { t.Errorf("%v not topologically sorted", test.name) } + if !sccMapsConsistent(sccs, idxToSccID) { + t.Errorf("%v: scc maps not consistent", test.name) + } + break } } @@ -275,7 +302,7 @@ func TestPropagation(t *testing.T) { var canon typeutil.Map for _, test := range []struct { name string - graph vtaGraph + graph *vtaGraph want map[string]string }{ // No cycles graph pushes type information forward. @@ -336,3 +363,34 @@ func TestPropagation(t *testing.T) { } } } + +func testLastIndex[S ~[]E, E comparable](t *testing.T, s S, e E, want int) { + if got := slicesLastIndex(s, e); got != want { + t.Errorf("LastIndex(%v, %v): got %v want %v", s, e, got, want) + } +} + +func TestLastIndex(t *testing.T) { + testLastIndex(t, []int{10, 20, 30}, 10, 0) + testLastIndex(t, []int{10, 20, 30}, 20, 1) + testLastIndex(t, []int{10, 20, 30}, 30, 2) + testLastIndex(t, []int{10, 20, 30}, 42, -1) + testLastIndex(t, []int{10, 20, 10}, 10, 2) + testLastIndex(t, []int{20, 10, 10}, 10, 2) + testLastIndex(t, []int{10, 10, 20}, 10, 1) + type foo struct { + i int + s string + } + testLastIndex(t, []foo{{1, "abc"}, {2, "abc"}, {1, "xyz"}}, foo{1, "abc"}, 0) + // Test that LastIndex doesn't use bitwise comparisons for floats. + neg0 := 1 / math.Inf(-1) + nan := math.NaN() + testLastIndex(t, []float64{0, neg0}, 0, 1) + testLastIndex(t, []float64{0, neg0}, neg0, 1) + testLastIndex(t, []float64{neg0, 0}, 0, 1) + testLastIndex(t, []float64{neg0, 0}, neg0, 1) + testLastIndex(t, []float64{0, nan}, 0, 0) + testLastIndex(t, []float64{0, nan}, nan, -1) + testLastIndex(t, []float64{0, nan}, 1, -1) +} From f08b5c1e7c951d697592844d741d6712020410ef Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 9 Oct 2024 15:02:00 +0000 Subject: [PATCH 12/95] gopls/internal/test/integration/bench: add a pull diagnostics benchmark Add a benchmark that exercises a currently slow operation using pull diagnostics: requesting diagnostics for a large number of package files. For golang/go#53275 Change-Id: I233cfaeb4242ab44a7b46c870a195fdf7793c05c Reviewed-on: https://go-review.googlesource.com/c/tools/+/618995 LUCI-TryBot-Result: Go LUCI Auto-Submit: Robert Findley Reviewed-by: Alan Donovan --- .../test/integration/bench/diagnostic_test.go | 77 +++++++++++++++++++ .../test/integration/bench/didchange_test.go | 44 ++++++++--- 2 files changed, 109 insertions(+), 12 deletions(-) create mode 100644 gopls/internal/test/integration/bench/diagnostic_test.go diff --git a/gopls/internal/test/integration/bench/diagnostic_test.go b/gopls/internal/test/integration/bench/diagnostic_test.go new file mode 100644 index 00000000000..09e38642cb0 --- /dev/null +++ b/gopls/internal/test/integration/bench/diagnostic_test.go @@ -0,0 +1,77 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package bench + +import ( + "testing" + + "golang.org/x/tools/gopls/internal/protocol" + . "golang.org/x/tools/gopls/internal/test/integration" + "golang.org/x/tools/gopls/internal/test/integration/fake" +) + +// BenchmarkDiagnosePackageFiles measures how long it takes to request +// diagnostics for 10 files in a single package, following a change to that +// package. +// +// This can be used to measure the efficiency of pull diagnostics +// (golang/go#53275). +func BenchmarkDiagnosePackageFiles(b *testing.B) { + if testing.Short() { + b.Skip("pull diagnostics are not supported by the benchmark dashboard baseline") + } + + env := getRepo(b, "kubernetes").newEnv(b, fake.EditorConfig{ + Settings: map[string]any{ + "pullDiagnostics": true, // currently required for pull diagnostic support + }, + }, "diagnosePackageFiles", false) + + // 10 arbitrary files in a single package. + files := []string{ + "pkg/kubelet/active_deadline.go", // 98 lines + "pkg/kubelet/active_deadline_test.go", // 95 lines + "pkg/kubelet/kubelet.go", // 2439 lines + "pkg/kubelet/kubelet_pods.go", // 2061 lines + "pkg/kubelet/kubelet_network.go", // 70 lines + "pkg/kubelet/kubelet_network_test.go", // 46 lines + "pkg/kubelet/pod_workers.go", // 1323 lines + "pkg/kubelet/pod_workers_test.go", // 1758 lines + "pkg/kubelet/runonce.go", // 175 lines + "pkg/kubelet/volume_host.go", // 297 lines + } + + env.Await(InitialWorkspaceLoad) + + for _, file := range files { + env.OpenFile(file) + } + + env.AfterChange() + + edit := makeEditFunc(env, files[0]) + + if stopAndRecord := startProfileIfSupported(b, env, qualifiedName("kubernetes", "diagnosePackageFiles")); stopAndRecord != nil { + defer stopAndRecord() + } + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + edit() + var diags []protocol.Diagnostic + for _, file := range files { + fileDiags := env.Diagnostics(file) + for _, d := range fileDiags { + if d.Severity == protocol.SeverityError { + diags = append(diags, d) + } + } + } + if len(diags) != 0 { + b.Fatalf("got %d error diagnostics, want 0\ndiagnostics:\n%v", len(diags), diags) + } + } +} diff --git a/gopls/internal/test/integration/bench/didchange_test.go b/gopls/internal/test/integration/bench/didchange_test.go index 22e7ca2a11b..57ed01bbcd6 100644 --- a/gopls/internal/test/integration/bench/didchange_test.go +++ b/gopls/internal/test/integration/bench/didchange_test.go @@ -11,6 +11,7 @@ import ( "time" "golang.org/x/tools/gopls/internal/protocol" + . "golang.org/x/tools/gopls/internal/test/integration" "golang.org/x/tools/gopls/internal/test/integration/fake" ) @@ -48,30 +49,49 @@ func BenchmarkDidChange(b *testing.B) { defer closeBuffer(b, env, test.file) // Insert the text we'll be modifying at the top of the file. - env.EditBuffer(test.file, protocol.TextEdit{NewText: "// __TEST_PLACEHOLDER_0__\n"}) - env.AfterChange() - b.ResetTimer() + edit := makeEditFunc(env, test.file) if stopAndRecord := startProfileIfSupported(b, env, qualifiedName(test.repo, "didchange")); stopAndRecord != nil { defer stopAndRecord() } + b.ResetTimer() for i := 0; i < b.N; i++ { - edits := atomic.AddInt64(&editID, 1) - env.EditBuffer(test.file, protocol.TextEdit{ - Range: protocol.Range{ - Start: protocol.Position{Line: 0, Character: 0}, - End: protocol.Position{Line: 1, Character: 0}, - }, - // Increment the placeholder text, to ensure cache misses. - NewText: fmt.Sprintf("// __TEST_PLACEHOLDER_%d__\n", edits), - }) + edit() env.Await(env.StartedChange()) } }) } } +// makeEditFunc prepares the given file for incremental editing, by inserting a +// placeholder comment that will be overwritten with a new unique value by each +// call to the resulting function. While makeEditFunc awaits gopls to finish +// processing the initial edit, the callback for incremental edits does not +// await any gopls state. +// +// This is used for benchmarks that must repeatedly invalidate a file's +// contents. +// +// TODO(rfindley): use this throughout. +func makeEditFunc(env *Env, file string) func() { + // Insert the text we'll be modifying at the top of the file. + env.EditBuffer(file, protocol.TextEdit{NewText: "// __TEST_PLACEHOLDER_0__\n"}) + env.AfterChange() + + return func() { + edits := atomic.AddInt64(&editID, 1) + env.EditBuffer(file, protocol.TextEdit{ + Range: protocol.Range{ + Start: protocol.Position{Line: 0, Character: 0}, + End: protocol.Position{Line: 1, Character: 0}, + }, + // Increment the placeholder text, to ensure cache misses. + NewText: fmt.Sprintf("// __TEST_PLACEHOLDER_%d__\n", edits), + }) + } +} + func BenchmarkDiagnoseChange(b *testing.B) { for _, test := range didChangeTests { runChangeDiagnosticsBenchmark(b, test, false, "diagnoseChange") From db26c69117ee927d591047335ed031d31ecf6396 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 9 Oct 2024 15:55:54 +0000 Subject: [PATCH 13/95] cmd/stringer: fix test on android Fix a broken test build on android due to inconsistent build constraints in test files. Change-Id: I6350797fb65f2f2699e11b95b01a4aa3b3b84307 Reviewed-on: https://go-review.googlesource.com/c/tools/+/618996 Run-TryBot: Robert Findley LUCI-TryBot-Result: Go LUCI Reviewed-by: Hyang-Ah Hana Kim TryBot-Result: Gopher Robot --- cmd/stringer/endtoend_test.go | 1 - cmd/stringer/multifile_test.go | 5 +++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/stringer/endtoend_test.go b/cmd/stringer/endtoend_test.go index 2b7d6a786a5..5a56636be46 100644 --- a/cmd/stringer/endtoend_test.go +++ b/cmd/stringer/endtoend_test.go @@ -5,7 +5,6 @@ // go command is not available on android //go:build !android -// +build !android package main diff --git a/cmd/stringer/multifile_test.go b/cmd/stringer/multifile_test.go index 7a7ae669065..32914c5e825 100644 --- a/cmd/stringer/multifile_test.go +++ b/cmd/stringer/multifile_test.go @@ -2,8 +2,9 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// For os.CopyFS -//go:build go1.23 +// go1.23 is required for os.CopyFS. +// !android is required for compatibility with endtoend_test.go. +//go:build go1.23 && !android package main From a4e0a9e3d75a78c2b8c71a8750b2d79acf21975e Mon Sep 17 00:00:00 2001 From: Tim King Date: Tue, 8 Oct 2024 14:56:08 -0700 Subject: [PATCH 14/95] cmd/bundle: enable materialized aliases Change-Id: If402c12e2412890334c7188b6579964cb4b97454 Reviewed-on: https://go-review.googlesource.com/c/tools/+/618676 LUCI-TryBot-Result: Go LUCI Reviewed-by: Robert Findley --- cmd/bundle/gotypesalias.go | 12 ++++++++++++ cmd/bundle/main.go | 3 --- 2 files changed, 12 insertions(+), 3 deletions(-) create mode 100644 cmd/bundle/gotypesalias.go diff --git a/cmd/bundle/gotypesalias.go b/cmd/bundle/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/cmd/bundle/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/cmd/bundle/main.go b/cmd/bundle/main.go index a4831c78776..fa73eb83a0a 100644 --- a/cmd/bundle/main.go +++ b/cmd/bundle/main.go @@ -68,9 +68,6 @@ // Update all bundles in the standard library: // // go generate -run bundle std - -//go:debug gotypesalias=0 - package main import ( From bd86f8c28a06ee5c1ef4d233f30f5193aa26709a Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 9 Oct 2024 16:55:37 +0000 Subject: [PATCH 15/95] gopls/internal/cache/analysis: lazily resolve the import map MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In CL 613715, we made import lookup lazy to mitigate the quadratic construction of the import map during type checking. This CL makes the equivalent change for the analysis driver. Since analysis does not have fine-grained invalidation, it is even more susceptible to the performance cost of this quadratic operation: DiagnosePackageFiles-64 │ before.txt │ after.txt │ │ sec/op │ sec/op vs base │ 1691.6m ± ∞ ¹ 693.6m ± ∞ ¹ -59.00% (p=0.008 n=5) │ before.txt │ after.txt │ │ cpu_seconds/op │ cpu_seconds/op vs base │ 6.480 ± ∞ ¹ 3.260 ± ∞ ¹ -49.69% (p=0.008 n=5) For golang/go#53275 Change-Id: I8690bc85848fe1e36391d4622aa2e3d3f9878f57 Reviewed-on: https://go-review.googlesource.com/c/tools/+/619095 Auto-Submit: Robert Findley LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan --- gopls/internal/cache/analysis.go | 45 +++++++++++------------ gopls/internal/cache/check.go | 24 +++++++++--- gopls/internal/cache/metadata/graph.go | 5 +++ gopls/internal/cache/metadata/metadata.go | 3 -- 4 files changed, 45 insertions(+), 32 deletions(-) diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index b18c30fd7ee..e671aac15b1 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -233,6 +233,9 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac // File set for this batch (entire graph) of analysis. fset := token.NewFileSet() + // Get the metadata graph once for lock-free access during analysis. + meta := s.MetadataGraph() + // Starting from the root packages and following DepsByPkgPath, // build the DAG of packages we're going to analyze. // @@ -246,7 +249,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac makeNode = func(from *analysisNode, id PackageID) (*analysisNode, error) { an, ok := nodes[id] if !ok { - mp := s.Metadata(id) + mp := meta.Metadata(id) if mp == nil { return nil, bug.Errorf("no metadata for %s", id) } @@ -254,14 +257,15 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac // -- preorder -- an = &analysisNode{ - fset: fset, - fsource: struct{ file.Source }{s}, // expose only ReadFile - viewType: s.View().Type(), - mp: mp, - analyzers: facty, // all nodes run at least the facty analyzers - allDeps: make(map[PackagePath]*analysisNode), - exportDeps: make(map[PackagePath]*analysisNode), - stableNames: stableNames, + allNodes: nodes, + fset: fset, + fsource: struct{ file.Source }{s}, // expose only ReadFile + viewType: s.View().Type(), + mp: mp, + analyzers: facty, // all nodes run at least the facty analyzers + importLookup: importLookup(mp, meta), + exportDeps: make(map[PackagePath]*analysisNode), + stableNames: stableNames, } nodes[id] = an @@ -275,18 +279,10 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac return nil, err } an.succs[depID] = dep - - // Compute the union of all dependencies. - // (This step has quadratic complexity.) - for pkgPath, node := range dep.allDeps { - an.allDeps[pkgPath] = node - } } // -- postorder -- - an.allDeps[mp.PkgPath] = an // add self entry (reflexive transitive closure) - // Add leaf nodes (no successors) directly to queue. if len(an.succs) == 0 { leaves = append(leaves, an) @@ -520,6 +516,7 @@ func (an *analysisNode) decrefPreds() { // its summary field is populated, either from the cache (hit), or by // type-checking and analyzing syntax (miss). type analysisNode struct { + allNodes map[PackageID]*analysisNode // all nodes, for lazy lookup of transitive dependencies fset *token.FileSet // file set shared by entire batch (DAG) fsource file.Source // Snapshot.ReadFile, for use by Pass.ReadFile viewType ViewType // type of view @@ -530,8 +527,8 @@ type analysisNode struct { succs map[PackageID]*analysisNode // (preds -> self -> succs) unfinishedSuccs atomic.Int32 unfinishedPreds atomic.Int32 // effectively a summary.Actions refcount - allDeps map[PackagePath]*analysisNode // all dependencies including self - exportDeps map[PackagePath]*analysisNode // subset of allDeps ref'd by export data (+self) + importLookup func(PackagePath) PackageID // lazy lookup of path->id + exportDeps map[PackagePath]*analysisNode // nodes referenced by export data (+self) summary *analyzeSummary // serializable result of analyzing this package stableNames map[*analysis.Analyzer]string // cross-process stable names for Analyzers @@ -563,7 +560,9 @@ func (an *analysisNode) _import() (*types.Package, error) { var g errgroup.Group for i, item := range items { path := PackagePath(item.Path) - dep, ok := an.allDeps[path] + + id := an.importLookup(path) // possibly "" + dep, ok := an.allNodes[id] if !ok { // This early return bypasses Wait; that's ok. return fmt.Errorf("%s: unknown dependency %q", an.mp, path) @@ -879,9 +878,6 @@ func (an *analysisNode) run(ctx context.Context) (*analyzeSummary, error) { // by export data, it is safe to ignore it. // (In that case dep.types exists but may be unpopulated // or in the process of being populated from export data.) - if an.allDeps[PackagePath(path)] == nil { - log.Fatalf("fact package %q is not a dependency", path) - } return nil }) @@ -1094,7 +1090,8 @@ func (an *analysisNode) typeCheck(parsed []*parsego.File) *analysisPackage { log.Fatalf("internal error: bad export data: %v", err) } for _, path := range paths { - dep, ok := an.allDeps[path] + id := an.importLookup(path) // possibly "" + dep, ok := an.allNodes[id] if !ok { log.Fatalf("%s: missing dependency: %q", an, path) } diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index 99ca025eecd..dd122a6522a 100644 --- a/gopls/internal/cache/check.go +++ b/gopls/internal/cache/check.go @@ -447,13 +447,22 @@ func storePackageResults(ctx context.Context, ph *packageHandle, p *Package) { } } +// Metadata implements the [metadata.Source] interface. +func (b *typeCheckBatch) Metadata(id PackageID) *metadata.Package { + ph := b.getHandle(id) + if ph == nil { + return nil + } + return ph.mp +} + // importPackage loads the given package from its export data in p.exportData // (which must already be populated). func (b *typeCheckBatch) importPackage(ctx context.Context, mp *metadata.Package, data []byte) (*types.Package, error) { ctx, done := event.Start(ctx, "cache.typeCheckBatch.importPackage", label.Package.Of(string(mp.ID))) defer done() - importLookup := b.importLookup(mp) + importLookup := importLookup(mp, b) thisPackage := types.NewPackage(string(mp.PkgPath), string(mp.Name)) getPackages := func(items []gcimporter.GetPackagesItem) error { @@ -592,7 +601,9 @@ func (b *typeCheckBatch) checkPackageForImport(ctx context.Context, ph *packageH // package (id). // // The resulting function is not concurrency safe. -func (b *typeCheckBatch) importLookup(mp *metadata.Package) func(PackagePath) PackageID { +func importLookup(mp *metadata.Package, source metadata.Source) func(PackagePath) PackageID { + assert(mp != nil, "nil metadata") + // This function implements an incremental depth first scan through the // package imports. Previous implementations of import mapping built the // entire PackagePath->PackageID mapping eagerly, but that resulted in a @@ -603,6 +614,7 @@ func (b *typeCheckBatch) importLookup(mp *metadata.Package) func(PackagePath) Pa impMap := map[PackagePath]PackageID{ mp.PkgPath: mp.ID, } + // pending is a FIFO queue of package metadata that has yet to have its // dependencies fully scanned. // Invariant: all entries in pending are already mapped in impMap. @@ -623,9 +635,11 @@ func (b *typeCheckBatch) importLookup(mp *metadata.Package) func(PackagePath) Pa continue } impMap[depPath] = depID - // TODO(rfindley): express this as an operation on the import graph - // itself, rather than the set of package handles. - pending = append(pending, b.getHandle(depID).mp) + + dep := source.Metadata(depID) + assert(dep != nil, "missing dep metadata") + + pending = append(pending, dep) if depPath == pkgPath { // Don't return early; finish processing pkg's deps. id = depID diff --git a/gopls/internal/cache/metadata/graph.go b/gopls/internal/cache/metadata/graph.go index f09822d3575..4b846df53be 100644 --- a/gopls/internal/cache/metadata/graph.go +++ b/gopls/internal/cache/metadata/graph.go @@ -27,6 +27,11 @@ type Graph struct { IDs map[protocol.DocumentURI][]PackageID } +// Metadata implements the [Source] interface +func (g *Graph) Metadata(id PackageID) *Package { + return g.Packages[id] +} + // Update creates a new Graph containing the result of applying the given // updates to the receiver, though the receiver is not itself mutated. As a // special case, if updates is empty, Update just returns the receiver. diff --git a/gopls/internal/cache/metadata/metadata.go b/gopls/internal/cache/metadata/metadata.go index e42aac304f6..81b6dc57e1f 100644 --- a/gopls/internal/cache/metadata/metadata.go +++ b/gopls/internal/cache/metadata/metadata.go @@ -167,9 +167,6 @@ func (mp *Package) IsIntermediateTestVariant() bool { } // A Source maps package IDs to metadata for the packages. -// -// TODO(rfindley): replace this with a concrete metadata graph, once it is -// exposed from the snapshot. type Source interface { // Metadata returns the [Package] for the given package ID, or nil if it does // not exist. From 915132c3adc044a2640e5c50980a221b1905a7c9 Mon Sep 17 00:00:00 2001 From: Tim King Date: Tue, 8 Oct 2024 16:29:09 -0700 Subject: [PATCH 16/95] internal/typesinternal: add NamedOrAlias type A NamedOrAlias represents either a *types.Named or a *types.Alias. Used this generalization in gopls. This change is derived from https://go.dev/cl/603935. Change-Id: Ica1669784dec6bcdefafde02e9a6ce789db28814 Reviewed-on: https://go-review.googlesource.com/c/tools/+/618735 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan Commit-Queue: Tim King --- .../internal/golang/completion/completion.go | 6 +- gopls/internal/golang/completion/format.go | 11 ++-- gopls/internal/golang/stub.go | 21 ++++--- .../golang/stubmethods/stubmethods.go | 4 +- .../test/marker/testdata/completion/alias.txt | 33 +++++++++++ internal/typesinternal/types.go | 56 +++++++++++++++++++ 6 files changed, 113 insertions(+), 18 deletions(-) create mode 100644 gopls/internal/test/marker/testdata/completion/alias.txt diff --git a/gopls/internal/golang/completion/completion.go b/gopls/internal/golang/completion/completion.go index cf398693113..6bf8ad8acde 100644 --- a/gopls/internal/golang/completion/completion.go +++ b/gopls/internal/golang/completion/completion.go @@ -1713,7 +1713,7 @@ func (c *completer) injectType(ctx context.Context, t types.Type) { // considered via a lexical search, so we need to directly inject // them. Also allow generic types since lexical search does not // infer instantiated versions of them. - if named, ok := types.Unalias(t).(*types.Named); !ok || named.TypeParams().Len() > 0 { + if pnt, ok := t.(typesinternal.NamedOrAlias); !ok || typesinternal.TypeParams(pnt).Len() > 0 { // If our expected type is "[]int", this will add a literal // candidate of "[]int{}". c.literal(ctx, t, nil) @@ -2508,8 +2508,8 @@ func (c *completer) expectedCallParamType(inf candidateInference, node *ast.Call func expectedConstraint(t types.Type, idx int) types.Type { var tp *types.TypeParamList - if named, _ := t.(*types.Named); named != nil { - tp = named.TypeParams() + if pnt, ok := t.(typesinternal.NamedOrAlias); ok { + tp = typesinternal.TypeParams(pnt) } else if sig, _ := t.Underlying().(*types.Signature); sig != nil { tp = sig.TypeParams() } diff --git a/gopls/internal/golang/completion/format.go b/gopls/internal/golang/completion/format.go index c2b955ca7e9..c52ce94698e 100644 --- a/gopls/internal/golang/completion/format.go +++ b/gopls/internal/golang/completion/format.go @@ -19,6 +19,7 @@ import ( "golang.org/x/tools/gopls/internal/util/safetoken" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/imports" + "golang.org/x/tools/internal/typesinternal" ) var ( @@ -59,12 +60,10 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e detail = "" } if isTypeName(obj) && c.wantTypeParams() { - x := cand.obj.(*types.TypeName) - if named, ok := types.Unalias(x.Type()).(*types.Named); ok { - tp := named.TypeParams() - label += golang.FormatTypeParams(tp) - insert = label // maintain invariant above (label == insert) - } + // obj is a *types.TypeName, so its type must be Alias|Named. + tparams := typesinternal.TypeParams(obj.Type().(typesinternal.NamedOrAlias)) + label += golang.FormatTypeParams(tparams) + insert = label // maintain invariant above (label == insert) } snip.WriteText(insert) diff --git a/gopls/internal/golang/stub.go b/gopls/internal/golang/stub.go index ca5f0055c3b..1a4186cf372 100644 --- a/gopls/internal/golang/stub.go +++ b/gopls/internal/golang/stub.go @@ -26,6 +26,7 @@ import ( "golang.org/x/tools/gopls/internal/util/safetoken" "golang.org/x/tools/internal/diff" "golang.org/x/tools/internal/tokeninternal" + "golang.org/x/tools/internal/typesinternal" ) // stubMethodsFixer returns a suggested fix to declare the missing @@ -66,8 +67,10 @@ func stubMethodsFixer(ctx context.Context, snapshot *cache.Snapshot, pkg *cache. // Record all direct methods of the current object concreteFuncs := make(map[string]struct{}) - for i := 0; i < si.Concrete.NumMethods(); i++ { - concreteFuncs[si.Concrete.Method(i).Name()] = struct{}{} + if named, ok := types.Unalias(si.Concrete).(*types.Named); ok { + for i := 0; i < named.NumMethods(); i++ { + concreteFuncs[named.Method(i).Name()] = struct{}{} + } } // Find subset of interface methods that the concrete type lacks. @@ -80,7 +83,7 @@ func stubMethodsFixer(ctx context.Context, snapshot *cache.Snapshot, pkg *cache. var ( missing []missingFn - concreteStruct, isStruct = si.Concrete.Origin().Underlying().(*types.Struct) + concreteStruct, isStruct = typesinternal.Origin(si.Concrete).Underlying().(*types.Struct) ) for i := 0; i < ifaceType.NumMethods(); i++ { @@ -208,10 +211,12 @@ func stubMethodsFixer(ctx context.Context, snapshot *cache.Snapshot, pkg *cache. // If there are any that have named receiver, choose the first one. // Otherwise, use lowercase for the first letter of the object. rn := strings.ToLower(si.Concrete.Obj().Name()[0:1]) - for i := 0; i < si.Concrete.NumMethods(); i++ { - if recv := si.Concrete.Method(i).Signature().Recv(); recv.Name() != "" { - rn = recv.Name() - break + if named, ok := types.Unalias(si.Concrete).(*types.Named); ok { + for i := 0; i < named.NumMethods(); i++ { + if recv := named.Method(i).Type().(*types.Signature).Recv(); recv.Name() != "" { + rn = recv.Name() + break + } } } @@ -246,7 +251,7 @@ func stubMethodsFixer(ctx context.Context, snapshot *cache.Snapshot, pkg *cache. mrn, star, si.Concrete.Obj().Name(), - FormatTypeParams(si.Concrete.TypeParams()), + FormatTypeParams(typesinternal.TypeParams(si.Concrete)), missing[index].fn.Name(), strings.TrimPrefix(types.TypeString(missing[index].fn.Type(), qual), "func")) } diff --git a/gopls/internal/golang/stubmethods/stubmethods.go b/gopls/internal/golang/stubmethods/stubmethods.go index ee7b525a6a0..b5799861251 100644 --- a/gopls/internal/golang/stubmethods/stubmethods.go +++ b/gopls/internal/golang/stubmethods/stubmethods.go @@ -12,6 +12,8 @@ import ( "go/ast" "go/token" "go/types" + + "golang.org/x/tools/internal/typesinternal" ) // TODO(adonovan): eliminate the confusing Fset parameter; only the @@ -29,7 +31,7 @@ type StubInfo struct { // TODO(marwan-at-work): implement interface literals. Fset *token.FileSet // the FileSet used to type-check the types below Interface *types.TypeName - Concrete *types.Named + Concrete typesinternal.NamedOrAlias Pointer bool } diff --git a/gopls/internal/test/marker/testdata/completion/alias.txt b/gopls/internal/test/marker/testdata/completion/alias.txt new file mode 100644 index 00000000000..e4c340e3f1f --- /dev/null +++ b/gopls/internal/test/marker/testdata/completion/alias.txt @@ -0,0 +1,33 @@ +This test checks completion related to aliases. + +-- flags -- +-ignore_extra_diags +-min_go=go1.24 + +-- aliases.go -- +package aliases + +// Copied from the old builtins.go, which has been ported to the new marker tests. +/* string */ //@item(string, "string", "", "type") +/* int */ //@item(int, "int", "", "type") +/* float32 */ //@item(float32, "float32", "", "type") +/* float64 */ //@item(float64, "float64", "", "type") + +type p struct{} + +type s[a int | string] = p + +func _() { + s[]{} //@rank("]", int, float64) +} + +func takesGeneric[a int | string](s[a]) { + "s[a]{}" //@item(tpInScopeLit, "s[a]{}", "", "var") + takesGeneric() //@rank(")", tpInScopeLit),snippet(")", tpInScopeLit, "s[a]{\\}") +} + +type myType int //@item(flType, "myType", "int", "type") + +type myt[T int] myType //@item(aflType, "myt[T]", "int", "type") + +func (my myt) _() {} //@complete(") _", flType, aflType) diff --git a/internal/typesinternal/types.go b/internal/typesinternal/types.go index 83923286120..df3ea521254 100644 --- a/internal/typesinternal/types.go +++ b/internal/typesinternal/types.go @@ -11,6 +11,8 @@ import ( "go/types" "reflect" "unsafe" + + "golang.org/x/tools/internal/aliases" ) func SetUsesCgo(conf *types.Config) bool { @@ -63,3 +65,57 @@ func NameRelativeTo(pkg *types.Package) types.Qualifier { return other.Name() } } + +// A NamedOrAlias is a [types.Type] that is named (as +// defined by the spec) and capable of bearing type parameters: it +// abstracts aliases ([types.Alias]) and defined types +// ([types.Named]). +// +// Every type declared by an explicit "type" declaration is a +// NamedOrAlias. (Built-in type symbols may additionally +// have type [types.Basic], which is not a NamedOrAlias, +// though the spec regards them as "named".) +// +// NamedOrAlias cannot expose the Origin method, because +// [types.Alias.Origin] and [types.Named.Origin] have different +// (covariant) result types; use [Origin] instead. +type NamedOrAlias interface { + types.Type + Obj() *types.TypeName +} + +// TypeParams is a light shim around t.TypeParams(). +// (go/types.Alias).TypeParams requires >= 1.23. +func TypeParams(t NamedOrAlias) *types.TypeParamList { + switch t := t.(type) { + case *types.Alias: + return aliases.TypeParams(t) + case *types.Named: + return t.TypeParams() + } + return nil +} + +// TypeArgs is a light shim around t.TypeArgs(). +// (go/types.Alias).TypeArgs requires >= 1.23. +func TypeArgs(t NamedOrAlias) *types.TypeList { + switch t := t.(type) { + case *types.Alias: + return aliases.TypeArgs(t) + case *types.Named: + return t.TypeArgs() + } + return nil +} + +// Origin returns the generic type of the Named or Alias type t if it +// is instantiated, otherwise it returns t. +func Origin(t NamedOrAlias) NamedOrAlias { + switch t := t.(type) { + case *types.Alias: + return aliases.Origin(t) + case *types.Named: + return t.Origin() + } + return t +} From 4f6e118e23392b803fa0bc1f6b84ca12c5484826 Mon Sep 17 00:00:00 2001 From: Tim King Date: Thu, 10 Oct 2024 09:42:18 -0700 Subject: [PATCH 17/95] all: set gotypesalias=1 when using >=1.23 toolchain Set gotypesalias=1 when using >=1.23 toolchain on all of the main packages in x/tools that use go/types. Does not include packages that are ignored due to build tags. This effectively upgrades commit https://go.dev/cl/617095. For golang/go#69772 Change-Id: I434c280b928ad21e1fd9c7f880e1324c14741dc3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/619395 Reviewed-by: Robert Findley Commit-Queue: Tim King LUCI-TryBot-Result: Go LUCI --- cmd/callgraph/gotypesalias.go | 12 ++++++++++++ cmd/callgraph/main.go | 3 --- cmd/deadcode/deadcode.go | 2 -- cmd/deadcode/gotypesalias.go | 12 ++++++++++++ cmd/eg/eg.go | 3 --- cmd/eg/gotypesalias.go | 12 ++++++++++++ cmd/godex/godex.go | 2 -- cmd/godex/gotypesalias.go | 12 ++++++++++++ cmd/godoc/gotypesalias.go | 12 ++++++++++++ cmd/godoc/main.go | 2 -- cmd/goimports/goimports.go | 2 -- cmd/goimports/gotypesalias.go | 12 ++++++++++++ cmd/gomvpkg/gotypesalias.go | 12 ++++++++++++ cmd/gomvpkg/main.go | 3 --- cmd/gotype/gotype.go | 3 --- cmd/gotype/gotypesalias.go | 12 ++++++++++++ cmd/ssadump/gotypesalias.go | 12 ++++++++++++ cmd/ssadump/main.go | 3 --- cmd/stringer/gotypesalias.go | 12 ++++++++++++ cmd/stringer/stringer.go | 3 --- go/analysis/passes/defers/cmd/defers/gotypesalias.go | 12 ++++++++++++ go/analysis/passes/defers/cmd/defers/main.go | 3 --- .../cmd/fieldalignment/gotypesalias.go | 12 ++++++++++++ .../passes/fieldalignment/cmd/fieldalignment/main.go | 3 --- .../passes/findcall/cmd/findcall/gotypesalias.go | 12 ++++++++++++ go/analysis/passes/findcall/cmd/findcall/main.go | 3 --- .../passes/httpmux/cmd/httpmux/gotypesalias.go | 12 ++++++++++++ go/analysis/passes/httpmux/cmd/httpmux/main.go | 3 --- .../ifaceassert/cmd/ifaceassert/gotypesalias.go | 12 ++++++++++++ .../passes/ifaceassert/cmd/ifaceassert/main.go | 3 --- .../passes/lostcancel/cmd/lostcancel/gotypesalias.go | 12 ++++++++++++ go/analysis/passes/lostcancel/cmd/lostcancel/main.go | 3 --- .../passes/nilness/cmd/nilness/gotypesalias.go | 12 ++++++++++++ go/analysis/passes/nilness/cmd/nilness/main.go | 3 --- go/analysis/passes/shadow/cmd/shadow/gotypesalias.go | 12 ++++++++++++ go/analysis/passes/shadow/cmd/shadow/main.go | 3 --- .../stringintconv/cmd/stringintconv/gotypesalias.go | 12 ++++++++++++ .../passes/stringintconv/cmd/stringintconv/main.go | 3 --- .../passes/unmarshal/cmd/unmarshal/gotypesalias.go | 12 ++++++++++++ go/analysis/passes/unmarshal/cmd/unmarshal/main.go | 3 --- .../unusedresult/cmd/unusedresult/gotypesalias.go | 12 ++++++++++++ .../passes/unusedresult/cmd/unusedresult/main.go | 3 --- go/packages/gopackages/gotypesalias.go | 12 ++++++++++++ go/packages/gopackages/main.go | 3 --- go/packages/internal/nodecount/gotypesalias.go | 12 ++++++++++++ go/packages/internal/nodecount/nodecount.go | 3 --- go/types/internal/play/gotypesalias.go | 12 ++++++++++++ go/types/internal/play/play.go | 3 --- 48 files changed, 288 insertions(+), 68 deletions(-) create mode 100644 cmd/callgraph/gotypesalias.go create mode 100644 cmd/deadcode/gotypesalias.go create mode 100644 cmd/eg/gotypesalias.go create mode 100644 cmd/godex/gotypesalias.go create mode 100644 cmd/godoc/gotypesalias.go create mode 100644 cmd/goimports/gotypesalias.go create mode 100644 cmd/gomvpkg/gotypesalias.go create mode 100644 cmd/gotype/gotypesalias.go create mode 100644 cmd/ssadump/gotypesalias.go create mode 100644 cmd/stringer/gotypesalias.go create mode 100644 go/analysis/passes/defers/cmd/defers/gotypesalias.go create mode 100644 go/analysis/passes/fieldalignment/cmd/fieldalignment/gotypesalias.go create mode 100644 go/analysis/passes/findcall/cmd/findcall/gotypesalias.go create mode 100644 go/analysis/passes/httpmux/cmd/httpmux/gotypesalias.go create mode 100644 go/analysis/passes/ifaceassert/cmd/ifaceassert/gotypesalias.go create mode 100644 go/analysis/passes/lostcancel/cmd/lostcancel/gotypesalias.go create mode 100644 go/analysis/passes/nilness/cmd/nilness/gotypesalias.go create mode 100644 go/analysis/passes/shadow/cmd/shadow/gotypesalias.go create mode 100644 go/analysis/passes/stringintconv/cmd/stringintconv/gotypesalias.go create mode 100644 go/analysis/passes/unmarshal/cmd/unmarshal/gotypesalias.go create mode 100644 go/analysis/passes/unusedresult/cmd/unusedresult/gotypesalias.go create mode 100644 go/packages/gopackages/gotypesalias.go create mode 100644 go/packages/internal/nodecount/gotypesalias.go create mode 100644 go/types/internal/play/gotypesalias.go diff --git a/cmd/callgraph/gotypesalias.go b/cmd/callgraph/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/cmd/callgraph/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/cmd/callgraph/main.go b/cmd/callgraph/main.go index 1b5af1b52e1..9e440bbafb9 100644 --- a/cmd/callgraph/main.go +++ b/cmd/callgraph/main.go @@ -4,9 +4,6 @@ // callgraph: a tool for reporting the call graph of a Go program. // See Usage for details, or run with -help. - -//go:debug gotypesalias=0 - package main // import "golang.org/x/tools/cmd/callgraph" // TODO(adonovan): diff --git a/cmd/deadcode/deadcode.go b/cmd/deadcode/deadcode.go index e6f32bb9979..f129102cc4c 100644 --- a/cmd/deadcode/deadcode.go +++ b/cmd/deadcode/deadcode.go @@ -2,8 +2,6 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:debug gotypesalias=0 - package main import ( diff --git a/cmd/deadcode/gotypesalias.go b/cmd/deadcode/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/cmd/deadcode/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/cmd/eg/eg.go b/cmd/eg/eg.go index 07e73d2efe7..108b9e3009f 100644 --- a/cmd/eg/eg.go +++ b/cmd/eg/eg.go @@ -5,9 +5,6 @@ // The eg command performs example-based refactoring. // For documentation, run the command, or see Help in // golang.org/x/tools/refactor/eg. - -//go:debug gotypesalias=0 - package main // import "golang.org/x/tools/cmd/eg" import ( diff --git a/cmd/eg/gotypesalias.go b/cmd/eg/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/cmd/eg/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/cmd/godex/godex.go b/cmd/godex/godex.go index 4955600f2d6..e91dbfcea5f 100644 --- a/cmd/godex/godex.go +++ b/cmd/godex/godex.go @@ -2,8 +2,6 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:debug gotypesalias=0 - package main import ( diff --git a/cmd/godex/gotypesalias.go b/cmd/godex/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/cmd/godex/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/cmd/godoc/gotypesalias.go b/cmd/godoc/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/cmd/godoc/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/cmd/godoc/main.go b/cmd/godoc/main.go index 1c874cc0e15..a665be0769d 100644 --- a/cmd/godoc/main.go +++ b/cmd/godoc/main.go @@ -14,8 +14,6 @@ // http://godoc/pkg/compress/zlib) // -//go:debug gotypesalias=0 - package main import ( diff --git a/cmd/goimports/goimports.go b/cmd/goimports/goimports.go index 7463e641e95..dcb5023a2e7 100644 --- a/cmd/goimports/goimports.go +++ b/cmd/goimports/goimports.go @@ -2,8 +2,6 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:debug gotypesalias=0 - package main import ( diff --git a/cmd/goimports/gotypesalias.go b/cmd/goimports/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/cmd/goimports/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/cmd/gomvpkg/gotypesalias.go b/cmd/gomvpkg/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/cmd/gomvpkg/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/cmd/gomvpkg/main.go b/cmd/gomvpkg/main.go index d54b7070dec..5de1e44062d 100644 --- a/cmd/gomvpkg/main.go +++ b/cmd/gomvpkg/main.go @@ -4,9 +4,6 @@ // The gomvpkg command moves go packages, updating import declarations. // See the -help message or Usage constant for details. - -//go:debug gotypesalias=0 - package main import ( diff --git a/cmd/gotype/gotype.go b/cmd/gotype/gotype.go index 09b66207e63..4a731f26233 100644 --- a/cmd/gotype/gotype.go +++ b/cmd/gotype/gotype.go @@ -85,9 +85,6 @@ To verify the output of a pipe: echo "package foo" | gotype */ - -//go:debug gotypesalias=0 - package main import ( diff --git a/cmd/gotype/gotypesalias.go b/cmd/gotype/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/cmd/gotype/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/cmd/ssadump/gotypesalias.go b/cmd/ssadump/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/cmd/ssadump/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/cmd/ssadump/main.go b/cmd/ssadump/main.go index 2ecf04fba50..275e0a92aef 100644 --- a/cmd/ssadump/main.go +++ b/cmd/ssadump/main.go @@ -3,9 +3,6 @@ // license that can be found in the LICENSE file. // ssadump: a tool for displaying and interpreting the SSA form of Go programs. - -//go:debug gotypesalias=0 - package main // import "golang.org/x/tools/cmd/ssadump" import ( diff --git a/cmd/stringer/gotypesalias.go b/cmd/stringer/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/cmd/stringer/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/cmd/stringer/stringer.go b/cmd/stringer/stringer.go index 94eaee844a4..09be11ca58e 100644 --- a/cmd/stringer/stringer.go +++ b/cmd/stringer/stringer.go @@ -70,9 +70,6 @@ // PillAspirin // Aspirin // // to suppress it in the output. - -//go:debug gotypesalias=0 - package main // import "golang.org/x/tools/cmd/stringer" import ( diff --git a/go/analysis/passes/defers/cmd/defers/gotypesalias.go b/go/analysis/passes/defers/cmd/defers/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/analysis/passes/defers/cmd/defers/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/analysis/passes/defers/cmd/defers/main.go b/go/analysis/passes/defers/cmd/defers/main.go index ffa5ae2da9b..b3dc8b94eca 100644 --- a/go/analysis/passes/defers/cmd/defers/main.go +++ b/go/analysis/passes/defers/cmd/defers/main.go @@ -3,9 +3,6 @@ // license that can be found in the LICENSE file. // The defers command runs the defers analyzer. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/analysis/passes/fieldalignment/cmd/fieldalignment/gotypesalias.go b/go/analysis/passes/fieldalignment/cmd/fieldalignment/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/analysis/passes/fieldalignment/cmd/fieldalignment/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/analysis/passes/fieldalignment/cmd/fieldalignment/main.go b/go/analysis/passes/fieldalignment/cmd/fieldalignment/main.go index 9ec4b9b505e..47d383d2d0e 100644 --- a/go/analysis/passes/fieldalignment/cmd/fieldalignment/main.go +++ b/go/analysis/passes/fieldalignment/cmd/fieldalignment/main.go @@ -1,9 +1,6 @@ // Copyright 2021 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/analysis/passes/findcall/cmd/findcall/gotypesalias.go b/go/analysis/passes/findcall/cmd/findcall/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/analysis/passes/findcall/cmd/findcall/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/analysis/passes/findcall/cmd/findcall/main.go b/go/analysis/passes/findcall/cmd/findcall/main.go index 1ada9668313..e0ce9137d61 100644 --- a/go/analysis/passes/findcall/cmd/findcall/main.go +++ b/go/analysis/passes/findcall/cmd/findcall/main.go @@ -3,9 +3,6 @@ // license that can be found in the LICENSE file. // The findcall command runs the findcall analyzer. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/analysis/passes/httpmux/cmd/httpmux/gotypesalias.go b/go/analysis/passes/httpmux/cmd/httpmux/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/analysis/passes/httpmux/cmd/httpmux/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/analysis/passes/httpmux/cmd/httpmux/main.go b/go/analysis/passes/httpmux/cmd/httpmux/main.go index 5933df923da..e8a631157dc 100644 --- a/go/analysis/passes/httpmux/cmd/httpmux/main.go +++ b/go/analysis/passes/httpmux/cmd/httpmux/main.go @@ -3,9 +3,6 @@ // license that can be found in the LICENSE file. // The httpmux command runs the httpmux analyzer. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/analysis/passes/ifaceassert/cmd/ifaceassert/gotypesalias.go b/go/analysis/passes/ifaceassert/cmd/ifaceassert/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/analysis/passes/ifaceassert/cmd/ifaceassert/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/analysis/passes/ifaceassert/cmd/ifaceassert/main.go b/go/analysis/passes/ifaceassert/cmd/ifaceassert/main.go index 32390be1643..42250f93df8 100644 --- a/go/analysis/passes/ifaceassert/cmd/ifaceassert/main.go +++ b/go/analysis/passes/ifaceassert/cmd/ifaceassert/main.go @@ -3,9 +3,6 @@ // license that can be found in the LICENSE file. // The ifaceassert command runs the ifaceassert analyzer. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/analysis/passes/lostcancel/cmd/lostcancel/gotypesalias.go b/go/analysis/passes/lostcancel/cmd/lostcancel/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/analysis/passes/lostcancel/cmd/lostcancel/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/analysis/passes/lostcancel/cmd/lostcancel/main.go b/go/analysis/passes/lostcancel/cmd/lostcancel/main.go index 3f2ac7c38f5..0bba8465242 100644 --- a/go/analysis/passes/lostcancel/cmd/lostcancel/main.go +++ b/go/analysis/passes/lostcancel/cmd/lostcancel/main.go @@ -4,9 +4,6 @@ // The lostcancel command applies the golang.org/x/tools/go/analysis/passes/lostcancel // analysis to the specified packages of Go source code. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/analysis/passes/nilness/cmd/nilness/gotypesalias.go b/go/analysis/passes/nilness/cmd/nilness/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/analysis/passes/nilness/cmd/nilness/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/analysis/passes/nilness/cmd/nilness/main.go b/go/analysis/passes/nilness/cmd/nilness/main.go index 91b4d5c44b3..136ac254a45 100644 --- a/go/analysis/passes/nilness/cmd/nilness/main.go +++ b/go/analysis/passes/nilness/cmd/nilness/main.go @@ -4,9 +4,6 @@ // The nilness command applies the golang.org/x/tools/go/analysis/passes/nilness // analysis to the specified packages of Go source code. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/analysis/passes/shadow/cmd/shadow/gotypesalias.go b/go/analysis/passes/shadow/cmd/shadow/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/analysis/passes/shadow/cmd/shadow/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/analysis/passes/shadow/cmd/shadow/main.go b/go/analysis/passes/shadow/cmd/shadow/main.go index 38de46beb3e..f9e36ecee95 100644 --- a/go/analysis/passes/shadow/cmd/shadow/main.go +++ b/go/analysis/passes/shadow/cmd/shadow/main.go @@ -3,9 +3,6 @@ // license that can be found in the LICENSE file. // The shadow command runs the shadow analyzer. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/analysis/passes/stringintconv/cmd/stringintconv/gotypesalias.go b/go/analysis/passes/stringintconv/cmd/stringintconv/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/analysis/passes/stringintconv/cmd/stringintconv/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/analysis/passes/stringintconv/cmd/stringintconv/main.go b/go/analysis/passes/stringintconv/cmd/stringintconv/main.go index 8dfb9a2056d..118b9579a50 100644 --- a/go/analysis/passes/stringintconv/cmd/stringintconv/main.go +++ b/go/analysis/passes/stringintconv/cmd/stringintconv/main.go @@ -3,9 +3,6 @@ // license that can be found in the LICENSE file. // The stringintconv command runs the stringintconv analyzer. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/analysis/passes/unmarshal/cmd/unmarshal/gotypesalias.go b/go/analysis/passes/unmarshal/cmd/unmarshal/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/analysis/passes/unmarshal/cmd/unmarshal/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/analysis/passes/unmarshal/cmd/unmarshal/main.go b/go/analysis/passes/unmarshal/cmd/unmarshal/main.go index fd69013fa59..1a17cd64de3 100644 --- a/go/analysis/passes/unmarshal/cmd/unmarshal/main.go +++ b/go/analysis/passes/unmarshal/cmd/unmarshal/main.go @@ -3,9 +3,6 @@ // license that can be found in the LICENSE file. // The unmarshal command runs the unmarshal analyzer. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/analysis/passes/unusedresult/cmd/unusedresult/gotypesalias.go b/go/analysis/passes/unusedresult/cmd/unusedresult/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/analysis/passes/unusedresult/cmd/unusedresult/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/analysis/passes/unusedresult/cmd/unusedresult/main.go b/go/analysis/passes/unusedresult/cmd/unusedresult/main.go index 635883c4051..8116c6e06e9 100644 --- a/go/analysis/passes/unusedresult/cmd/unusedresult/main.go +++ b/go/analysis/passes/unusedresult/cmd/unusedresult/main.go @@ -4,9 +4,6 @@ // The unusedresult command applies the golang.org/x/tools/go/analysis/passes/unusedresult // analysis to the specified packages of Go source code. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/packages/gopackages/gotypesalias.go b/go/packages/gopackages/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/packages/gopackages/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/packages/gopackages/main.go b/go/packages/gopackages/main.go index 7387e7fd10e..aab3362dbfd 100644 --- a/go/packages/gopackages/main.go +++ b/go/packages/gopackages/main.go @@ -6,9 +6,6 @@ // how to use golang.org/x/tools/go/packages to load, parse, // type-check, and print one or more Go packages. // Its precise output is unspecified and may change. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/packages/internal/nodecount/gotypesalias.go b/go/packages/internal/nodecount/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/packages/internal/nodecount/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/packages/internal/nodecount/nodecount.go b/go/packages/internal/nodecount/nodecount.go index 4b36a579ac0..a9f25bfdc6c 100644 --- a/go/packages/internal/nodecount/nodecount.go +++ b/go/packages/internal/nodecount/nodecount.go @@ -13,9 +13,6 @@ // A typical distribution is 40% identifiers, 10% literals, 8% // selectors, and 6% calls; around 3% each of BinaryExpr, BlockStmt, // AssignStmt, Field, and Comment; and the rest accounting for 20%. - -//go:debug gotypesalias=0 - package main import ( diff --git a/go/types/internal/play/gotypesalias.go b/go/types/internal/play/gotypesalias.go new file mode 100644 index 00000000000..288c10c2d0a --- /dev/null +++ b/go/types/internal/play/gotypesalias.go @@ -0,0 +1,12 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build go1.23 + +//go:debug gotypesalias=1 + +package main + +// Materialize aliases whenever the go toolchain version is after 1.23 (#69772). +// Remove this file after go.mod >= 1.23 (which implies gotypesalias=1). diff --git a/go/types/internal/play/play.go b/go/types/internal/play/play.go index e8b8cb9bbbe..e8ab0540cee 100644 --- a/go/types/internal/play/play.go +++ b/go/types/internal/play/play.go @@ -9,9 +9,6 @@ // It is intended for convenient exploration and debugging of // go/types. The command and its web interface are not officially // supported and they may be changed arbitrarily in the future. - -//go:debug gotypesalias=0 - package main import ( From 50179f2225d65cd976b0d7ee584e087d7efd265a Mon Sep 17 00:00:00 2001 From: Tim King Date: Thu, 10 Oct 2024 16:49:59 +0000 Subject: [PATCH 18/95] Revert "internal/aliases: add a function to conditionally enable aliases" This reverts commit f8f3c13ff3cad44c8cf62bea13bf846e9d7e8b5a. Reason for revert: https://go.dev/cl/619395 appears to be a better solution for the same problem. Change-Id: Ie3f290fca74b2cc2627484504b6db780f89b3bb4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/619415 Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI Commit-Queue: Tim King --- internal/aliases/aliases.go | 54 ----------------------- internal/aliases/enable_test.go | 76 --------------------------------- 2 files changed, 130 deletions(-) delete mode 100644 internal/aliases/enable_test.go diff --git a/internal/aliases/aliases.go b/internal/aliases/aliases.go index 7598deae1b8..b9425f5a209 100644 --- a/internal/aliases/aliases.go +++ b/internal/aliases/aliases.go @@ -5,13 +5,8 @@ package aliases import ( - "go/build" "go/token" "go/types" - "os" - "slices" - "strings" - "sync" ) // Package aliases defines backward compatible shims @@ -41,52 +36,3 @@ func NewAlias(enabled bool, pos token.Pos, pkg *types.Package, name string, rhs } return types.NewTypeName(pos, pkg, name, rhs) } - -// ConditionallyEnableGoTypesAlias enables the gotypesalias GODEBUG setting if -// * the version of go used to compile the program is between 1.23 and 1.26, -// * gotypesalias are not already enabled, and -// * gotypesalias is not set in GODEBUG already -// exactly once. Otherwise it does nothing. -// -// Recommended usage is to do the following within a main package: -// -// func init() { ConditionallyEnableGoTypesAlias() } -// -// within a module with go version 1.22 or in GOPATH mode. -func ConditionallyEnableGoTypesAlias() { conditionallyEnableGoTypesAliasOnce() } - -var conditionallyEnableGoTypesAliasOnce = sync.OnceFunc(conditionallyEnableGoTypesAlias) - -func conditionallyEnableGoTypesAlias() { - // Let R be the version of go the program was compiled with. Then - // if R < 1.22, do nothing as gotypesalias is meaningless, - // if R == 1.22, do not turn on gotypesalias (latent bugs), - // if 1.23 <= R && R <= 1.26, turn on gotypesalias, and - // if R >= 1.27, this is a guaranteed no-op. - - if !slices.Contains(build.Default.ReleaseTags, "go1.23") { - return // R < 1.23 (do nothing) - } - if slices.Contains(build.Default.ReleaseTags, "go1.27") { - return // R >= 1.27 (do nothing) - } - // 1.23 <= R <= 1.26 - _, anyIsAlias := types.Universe.Lookup("any").Type().(*types.Alias) - if anyIsAlias { - return // gotypesalias are already enabled. - } - - // Does GODEBUG have gotypesalias set already? - godebugs := strings.Split(os.Getenv("GODEBUG"), ",") - for _, p := range godebugs { - if strings.HasPrefix(strings.TrimSpace(p), "gotypesalias=") { - // gotypesalias is set in GODEBUG already. - // Do not override this setting. - return - } - } - - // Enable gotypesalias. - godebugs = append(godebugs, "gotypesalias=1") - os.Setenv("GODEBUG", strings.Join(godebugs, ",")) -} diff --git a/internal/aliases/enable_test.go b/internal/aliases/enable_test.go deleted file mode 100644 index 3b05960e5f8..00000000000 --- a/internal/aliases/enable_test.go +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright 2024 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package aliases_test - -import ( - "fmt" - "os" - "os/exec" - "runtime" - "strings" - "testing" - - "golang.org/x/tools/internal/aliases" - "golang.org/x/tools/internal/testenv" -) - -func init() { - if os.Getenv("ConditionallyEnableGoTypesAlias_CHILD") == "1" { - go aliases.ConditionallyEnableGoTypesAlias() // Throw in an extra call. Should be fine. - aliases.ConditionallyEnableGoTypesAlias() - } -} - -func TestConditionallyEnableGoTypesAlias(t *testing.T) { - if !(runtime.GOOS == "linux" || runtime.GOOS == "darwin") { - t.Skipf("skipping fork/exec test on this platform") - } - - if os.Getenv("ConditionallyEnableGoTypesAlias_CHILD") == "1" { - // child process - enabled := aliases.Enabled() - fmt.Printf("gotypesalias is enabled %v", enabled) - return - } - - testenv.NeedsTool(t, "go") - - var wants map[string]string - const ( - enabled = "gotypesalias is enabled true" - disabled = "gotypesalias is enabled false" - ) - goversion := testenv.Go1Point() - if goversion <= 22 { - wants = map[string]string{ - "": disabled, - "0": disabled, - "1": enabled, - } - } else { - wants = map[string]string{ - "": enabled, - "0": disabled, - "1": enabled, - } - } - - for _, test := range []string{"", "0", "1"} { - cmd := exec.Command(os.Args[0], "-test.run=TestConditionallyEnableGoTypesAlias") - cmd.Env = append(os.Environ(), "ConditionallyEnableGoTypesAlias_CHILD=1") - if test != "" { - cmd.Env = append(cmd.Env, fmt.Sprintf("GODEBUG=gotypesalias=%s", test)) - } - out, err := cmd.CombinedOutput() - if err != nil { - t.Error(err) - } - want := wants[test] - if !strings.Contains(string(out), want) { - t.Errorf("gotypesalias=%q: want %s", test, want) - t.Logf("(go 1.%d) %q: out=<<%s>>", goversion, test, out) - } - } -} From feffeaabe71b01afd77c5b492cc28f2986b4107e Mon Sep 17 00:00:00 2001 From: xieyuschen Date: Tue, 24 Sep 2024 15:13:58 +0800 Subject: [PATCH 19/95] go/packages: report an error from Load when GOROOT is misconfigured Load should return an error when 'go list' command returns the following error: err: exit status 2: stderr: go: no such tool "compile" This occurs when internally running `go list` on a misconfigured GOROOT directory. Fixes golang/go#69606 Change-Id: Ib7bf58293612e26aa33d2cf8230378747ad01820 Reviewed-on: https://go-review.googlesource.com/c/tools/+/615396 Reviewed-by: Michael Podtserkovskii Reviewed-by: Cherry Mui Commit-Queue: Tim King LUCI-TryBot-Result: Go LUCI Reviewed-by: Tim King Reviewed-by: Michael Matloob --- go/packages/golist.go | 6 +++ go/packages/packages_test.go | 73 ++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/go/packages/golist.go b/go/packages/golist.go index 1a3a5b44f5c..3e2e3db0454 100644 --- a/go/packages/golist.go +++ b/go/packages/golist.go @@ -879,6 +879,12 @@ func (state *golistState) invokeGo(verb string, args ...string) (*bytes.Buffer, return nil, friendlyErr } + // Return an error if 'go list' failed due to missing tools in + // $GOROOT/pkg/tool/$GOOS_$GOARCH (#69606). + if len(stderr.String()) > 0 && strings.Contains(stderr.String(), `go: no such tool`) { + return nil, friendlyErr + } + // Is there an error running the C compiler in cgo? This will be reported in the "Error" field // and should be suppressed by go list -e. // diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go index e78d3cdb881..7534428ab76 100644 --- a/go/packages/packages_test.go +++ b/go/packages/packages_test.go @@ -23,12 +23,14 @@ import ( "sort" "strings" "testing" + "testing/fstest" "time" "golang.org/x/tools/go/packages" "golang.org/x/tools/go/packages/packagestest" "golang.org/x/tools/internal/packagesinternal" "golang.org/x/tools/internal/testenv" + "golang.org/x/tools/internal/testfiles" ) // testCtx is canceled when the test binary is about to time out. @@ -3103,3 +3105,74 @@ func TestLoadOverlayGoMod(t *testing.T) { t.Errorf("Load: got %s, want %v", got, want) } } + +func overlayFS(overlay map[string][]byte) fstest.MapFS { + fs := make(fstest.MapFS) + for name, data := range overlay { + fs[name] = &fstest.MapFile{Data: data} + } + return fs +} + +// TestIssue69606a tests when tools in $GOROOT/pkg/tool/$GOOS_$GOARCH are missing, +// Load should return an error. +func TestIssue69606a(t *testing.T) { + testenv.NeedsTool(t, "go") + overlay := overlayFS(map[string][]byte{ + "io/io.go": []byte("package io"), + "unsafe/unsafe.go": []byte("package unsafe"), + }) + goroot := testfiles.CopyToTmp(t, overlay) + + t.Logf("custom GOROOT: %s", goroot) + + // load the std packages under a custom GOROOT + _, err := packages.Load(&packages.Config{ + Mode: packages.NeedName | + packages.NeedFiles | + packages.NeedImports | + packages.NeedTypes, + Env: append( + os.Environ(), + "GO111MODULES=on", + "GOPATH=", + "GOWORK=off", + "GOPROXY=off", + fmt.Sprintf("GOROOT=%s", goroot)), + }, "std") + + if err == nil { + t.Fatal("Expected to get an error because missing tool 'compile' but got a nil error") + } +} + +// TestIssue69606b tests when loading std from a fake goroot without a unsafe package, +// Load should return an error. +func TestIssue69606b(t *testing.T) { + testenv.NeedsTool(t, "go") + overlay := overlayFS(map[string][]byte{ + "io/io.go": []byte("package io"), + }) + goroot := testfiles.CopyToTmp(t, overlay) + + t.Logf("custom GOROOT: %s", goroot) + + // load the std packages under a custom GOROOT + _, err := packages.Load(&packages.Config{ + Mode: packages.NeedName | + packages.NeedFiles | + packages.NeedImports | + packages.NeedTypes, + Env: append( + os.Environ(), + "GO111MODULES=on", + "GOPATH=", + "GOWORK=off", + "GOPROXY=off", + fmt.Sprintf("GOROOT=%s", goroot)), + }, "std") + + if err == nil { + t.Fatal("Expected to get an error because missing unsafe package but got a nil error") + } +} From aa87dcfff7e8eab062ec78033313460732e6e06b Mon Sep 17 00:00:00 2001 From: cuishuang Date: Thu, 10 Oct 2024 14:54:37 +0800 Subject: [PATCH 20/95] go/analysis/passes: execute gofmt Change-Id: I4f4c670f002272b3fa45e7f7eb43916413c975a4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/618896 Reviewed-by: Cherry Mui Reviewed-by: Tim King Commit-Queue: Tim King LUCI-TryBot-Result: Go LUCI --- go/analysis/passes/slog/slog.go | 6 +++--- go/analysis/passes/stdversion/main.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/go/analysis/passes/slog/slog.go b/go/analysis/passes/slog/slog.go index 0cade7bad7e..0129102a336 100644 --- a/go/analysis/passes/slog/slog.go +++ b/go/analysis/passes/slog/slog.go @@ -203,7 +203,7 @@ func kvFuncSkipArgs(fn *types.Func) (int, bool) { // order to get to the ones that match the ...any parameter. // The first key is the dereferenced receiver type name, or "" for a function. var kvFuncs = map[string]map[string]int{ - "": map[string]int{ + "": { "Debug": 1, "Info": 1, "Warn": 1, @@ -215,7 +215,7 @@ var kvFuncs = map[string]map[string]int{ "Log": 3, "Group": 1, }, - "Logger": map[string]int{ + "Logger": { "Debug": 1, "Info": 1, "Warn": 1, @@ -227,7 +227,7 @@ var kvFuncs = map[string]map[string]int{ "Log": 3, "With": 0, }, - "Record": map[string]int{ + "Record": { "Add": 0, }, } diff --git a/go/analysis/passes/stdversion/main.go b/go/analysis/passes/stdversion/main.go index a9efd0160eb..2156d41e4a9 100644 --- a/go/analysis/passes/stdversion/main.go +++ b/go/analysis/passes/stdversion/main.go @@ -8,8 +8,8 @@ package main import ( - "golang.org/x/tools/go/analysis/singlechecker" "golang.org/x/tools/go/analysis/passes/stdversion" + "golang.org/x/tools/go/analysis/singlechecker" ) func main() { singlechecker.Main(stdversion.Analyzer) } From 87d613117b4367083fa557b24f012ba635e5b1b6 Mon Sep 17 00:00:00 2001 From: Tim King Date: Wed, 9 Oct 2024 15:59:06 -0700 Subject: [PATCH 21/95] internal/typeparams: support parameterized aliases in Free Derived from https://go.dev/cl/603935. Change-Id: I1c5ab7cc180c33e69244fe2cfdc79fd9625eab95 Reviewed-on: https://go-review.googlesource.com/c/tools/+/619235 Commit-Queue: Tim King Reviewed-by: Robert Findley LUCI-TryBot-Result: Go LUCI --- internal/typeparams/free.go | 17 +++++++++-- internal/typeparams/free_test.go | 52 ++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/internal/typeparams/free.go b/internal/typeparams/free.go index 358108268b4..0ade5c2949e 100644 --- a/internal/typeparams/free.go +++ b/internal/typeparams/free.go @@ -6,6 +6,8 @@ package typeparams import ( "go/types" + + "golang.org/x/tools/internal/aliases" ) // Free is a memoization of the set of free type parameters within a @@ -36,6 +38,18 @@ func (w *Free) Has(typ types.Type) (res bool) { break case *types.Alias: + if aliases.TypeParams(t).Len() > aliases.TypeArgs(t).Len() { + return true // This is an uninstantiated Alias. + } + // The expansion of an alias can have free type parameters, + // whether or not the alias itself has type parameters: + // + // func _[K comparable]() { + // type Set = map[K]bool // free(Set) = {K} + // type MapTo[V] = map[K]V // free(Map[foo]) = {V} + // } + // + // So, we must Unalias. return w.Has(types.Unalias(t)) case *types.Array: @@ -96,9 +110,8 @@ func (w *Free) Has(typ types.Type) (res bool) { case *types.Named: args := t.TypeArgs() - // TODO(taking): this does not match go/types/infer.go. Check with rfindley. if params := t.TypeParams(); params.Len() > args.Len() { - return true + return true // this is an uninstantiated named type. } for i, n := 0, args.Len(); i < n; i++ { if w.Has(args.At(i)) { diff --git a/internal/typeparams/free_test.go b/internal/typeparams/free_test.go index b73a8238be3..5ba2779c6ba 100644 --- a/internal/typeparams/free_test.go +++ b/internal/typeparams/free_test.go @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. +//go:debug gotypesalias=1 + package typeparams import ( @@ -10,6 +12,8 @@ import ( "go/token" "go/types" "testing" + + "golang.org/x/tools/internal/testenv" ) func TestFree(t *testing.T) { @@ -71,3 +75,51 @@ func (v *V[T]) Push(x T) { *v = append(*v, x) } } } } + +func TestFree124(t *testing.T) { + testenv.NeedsGo1Point(t, 24) + const source = ` +package P + +func Within[T any]() { + type p[V []T] = int + + type q[V any] = T + + var end int // end provides a position to test at. + _ = end +} +` + + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "hello.go", source, 0) + if err != nil { + t.Fatal(err) + } + + var conf types.Config + pkg, err := conf.Check("P", fset, []*ast.File{f}, nil) + if err != nil { + t.Fatal(err) + } + + for _, test := range []struct { + expr string // type expression + want bool // expected value + }{ + {"p", true}, // not an instantiation + {"p[[]T]", false}, // is an instantiation + {"q[int]", true}, + } { + pos := pkg.Scope().Lookup("Within").(*types.Func).Scope().Lookup("end").Pos() + tv, err := types.Eval(fset, pkg, pos, test.expr) + if err != nil { + t.Errorf("Eval(%s) failed: %v", test.expr, err) + } + + if got := new(Free).Has(tv.Type); got != test.want { + t.Logf("Eval(%s) returned the type %s", test.expr, tv.Type) + t.Errorf("isParameterized(%s) = %v, want %v", test.expr, got, test.want) + } + } +} From 244a31e15e1ff254b3ec9681e1ac1c3438990d2a Mon Sep 17 00:00:00 2001 From: xzb <2598514867@qq.com> Date: Sat, 12 Oct 2024 19:14:54 +0000 Subject: [PATCH 22/95] gopls/internal: CodeAction: quickfix to generate missing method This change provides a new "quickfix" code action to repair a function call x.f() with a "type X has no field or method f" error by generating a stub declaration of the missing method. - extract common functionality about adjusting import, generating diffs to a shared function. - add new stubMissingCalledFunctionFixer (for missing method calls), change stubMethodsFixer to stubMissingInterfaceMethodsFixer (for interfaces) to make it not vague about which kind of stub it actually be. - algos to get type info: for arguments, get type directly, for return values, currently only implement inferring from variable assignment and parameter assignment. - remove a test in stub.txt that tests local types can't be stubbed, after this CL, this check has been moved to GetCallStubInfo and GetIfaceStubInfo, thus gopls will not offer a bad fix. There is a detailed test that demonstrated this pr. Fixes golang/go#69692 Change-Id: Ia7aecdaf895da2a9a33b706f26b7766e4d42c8a1 GitHub-Last-Rev: 7d9f4e0b46eb7a814b2fc6e2408ad72f1cde3452 GitHub-Pull-Request: golang/tools#528 Reviewed-on: https://go-review.googlesource.com/c/tools/+/617619 Reviewed-by: Robert Findley Reviewed-by: Alan Donovan Auto-Submit: Alan Donovan LUCI-TryBot-Result: Go LUCI Commit-Queue: Alan Donovan --- gopls/doc/features/diagnostics.md | 30 +- gopls/doc/release/v0.17.0.md | 9 + gopls/internal/golang/codeaction.go | 26 +- gopls/internal/golang/completion/format.go | 3 +- gopls/internal/golang/fix.go | 34 +- gopls/internal/golang/stub.go | 192 +++------- .../golang/stubmethods/stubcalledfunc.go | 352 ++++++++++++++++++ .../golang/stubmethods/stubmethods.go | 182 +++++++-- gopls/internal/golang/types_format.go | 19 - .../testdata/stubmethods/fromcall_basic.txt | 54 +++ .../testdata/stubmethods/fromcall_params.txt | 84 +++++ .../testdata/stubmethods/fromcall_returns.txt | 109 ++++++ .../marker/testdata/suggestedfix/stub.txt | 37 -- gopls/internal/util/typesutil/typesutil.go | 20 + 14 files changed, 903 insertions(+), 248 deletions(-) create mode 100644 gopls/internal/golang/stubmethods/stubcalledfunc.go create mode 100644 gopls/internal/test/marker/testdata/stubmethods/fromcall_basic.txt create mode 100644 gopls/internal/test/marker/testdata/stubmethods/fromcall_params.txt create mode 100644 gopls/internal/test/marker/testdata/stubmethods/fromcall_returns.txt diff --git a/gopls/doc/features/diagnostics.md b/gopls/doc/features/diagnostics.md index 8c9305c94b6..5955a55d8b3 100644 --- a/gopls/doc/features/diagnostics.md +++ b/gopls/doc/features/diagnostics.md @@ -127,7 +127,7 @@ Client support: -### `stubMethods`: Declare missing methods of type +### `stubMissingInterfaceMethods`: Declare missing methods of I When a value of a concrete type is assigned to a variable of an interface type, but the concrete type does not possess all the @@ -169,6 +169,34 @@ position. client there, or a progress notification indicating that something happened.) +### `StubMissingCalledFunction`: Declare missing method T.f + +When you attempt to call a method on a type that does not have that method, +the compiler will report an error such as "type X has no field or method Y". +In this scenario, gopls now offers a quick fix to generate a stub declaration of +the missing method, inferring its type from the call. + +Consider the following code where `Foo` does not have a method `bar`: + +```go +type Foo struct{} + +func main() { + var s string + f := Foo{} + s = f.bar("str", 42) // error: f.bar undefined (type Foo has no field or method bar) +} +``` + +Gopls will offer a quick fix, "Declare missing method Foo.bar". +When invoked, it creates the following declaration: + +```go +func (f Foo) bar(s string, i int) string { + panic("unimplemented") +} +``` +