diff --git a/cmd/digraph/digraph.go b/cmd/digraph/digraph.go index 9a8abca59fd..f3811fe80d4 100644 --- a/cmd/digraph/digraph.go +++ b/cmd/digraph/digraph.go @@ -67,6 +67,8 @@ func (l nodelist) println(sep string) { type nodeset map[string]bool +func singleton(x string) nodeset { return nodeset{x: true} } + func (s nodeset) sort() nodelist { nodes := make(nodelist, len(s)) var i int @@ -191,26 +193,17 @@ func (g graph) sccs() []nodeset { } func (g graph) allpaths(from, to string) error { - // Mark all nodes to "to". - seen := make(nodeset) // value of seen[x] indicates whether x is on some path to "to" - var visit func(node string) bool - visit = func(node string) bool { - reachesTo, ok := seen[node] - if !ok { - reachesTo = node == to - seen[node] = reachesTo - for e := range g[node] { - if visit(e) { - reachesTo = true - } - } - if reachesTo && node != to { - seen[node] = true - } + // We intersect the forward closure of 'from' with + // the reverse closure of 'to'. This is not the most + // efficient implementation, but it's the clearest, + // and the previous one had bugs. + seen := g.reachableFrom(singleton(from)) + rev := g.transpose().reachableFrom(singleton(to)) + for n := range seen { + if !rev[n] { + delete(seen, n) } - return reachesTo } - visit(from) // For each marked node, collect its marked successors. var edges []string @@ -241,7 +234,7 @@ func (g graph) somepath(from, to string) error { tail *path } - seen := nodeset{from: true} + seen := singleton(from) var queue []*path queue = append(queue, &path{node: from, tail: nil}) @@ -469,14 +462,14 @@ func digraph(cmd string, args []string) error { } edges := make(map[string]struct{}) - for from := range g.reachableFrom(nodeset{node: true}) { + for from := range g.reachableFrom(singleton(node)) { for to := range g[from] { edges[fmt.Sprintf("%s %s", from, to)] = struct{}{} } } gtrans := g.transpose() - for from := range gtrans.reachableFrom(nodeset{node: true}) { + for from := range gtrans.reachableFrom(singleton(node)) { for to := range gtrans[from] { edges[fmt.Sprintf("%s %s", to, from)] = struct{}{} } diff --git a/cmd/digraph/digraph_test.go b/cmd/digraph/digraph_test.go index c9527588f27..7c967027073 100644 --- a/cmd/digraph/digraph_test.go +++ b/cmd/digraph/digraph_test.go @@ -156,6 +156,33 @@ func TestAllpaths(t *testing.T) { to: "H", want: "A B\nA C\nB D\nC D\nD E\nE F\nE G\nF H\nG H\n", }, + { + // C <--> B --> A --> D <--> E + // ⋃ + name: "non-regression test for #74842", + in: "A D\nB A\nB B\nB C\nC B\nD E\nE D", + to: "D", + want: "A D\nD E\nE D\n", + }, + { + // A --> B --> D + // ^ + // v + // C[123] + name: "regression test for #74842", + in: "A B\nB C1\nB C2\nB C3\nB D\nC1 B\nC2 B\nC3 B\n", + to: "D", + want: "A B\nB C1\nB C2\nB C3\nB D\nC1 B\nC2 B\nC3 B\n", + }, + { + // A -------> B --> D + // \--> C ---^ | + // ^----------+ + name: "another regression test for #74842", + in: "A B\nA C\nB D\nC B\nD C\n", + to: "D", + want: "A B\nA C\nB D\nC B\nD C\n", + }, } { t.Run(test.name, func(t *testing.T) { stdin = strings.NewReader(test.in) diff --git a/cmd/digraph/doc.go b/cmd/digraph/doc.go index 55e3dd4ff97..fc9ce1d6309 100644 --- a/cmd/digraph/doc.go +++ b/cmd/digraph/doc.go @@ -28,9 +28,9 @@ The supported commands are: reverse ... the set of nodes that transitively reach the specified nodes somepath - the list of nodes on some arbitrary path from the first node to the second + the list of edges on some arbitrary path from the first node to the second allpaths - the set of nodes on all paths from the first node to the second + the set of edges on all paths from the first node to the second sccs all strongly connected components (one per line) scc diff --git a/go.mod b/go.mod index 40385508710..d99a50eb25e 100644 --- a/go.mod +++ b/go.mod @@ -5,10 +5,10 @@ go 1.23.0 require ( github.com/google/go-cmp v0.6.0 github.com/yuin/goldmark v1.4.13 - golang.org/x/mod v0.26.0 - golang.org/x/net v0.42.0 + golang.org/x/mod v0.27.0 + golang.org/x/net v0.43.0 golang.org/x/sync v0.16.0 - golang.org/x/telemetry v0.0.0-20250710130107-8d8967aff50b + golang.org/x/telemetry v0.0.0-20250807160809-1a19826ec488 ) -require golang.org/x/sys v0.34.0 // indirect +require golang.org/x/sys v0.35.0 // indirect diff --git a/go.sum b/go.sum index 3b247340551..fd1eae0a34a 100644 --- a/go.sum +++ b/go.sum @@ -2,13 +2,13 @@ github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/yuin/goldmark v1.4.13 h1:fVcFKWvrslecOb/tg+Cc05dkeYx540o0FuFt3nUVDoE= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= -golang.org/x/mod v0.26.0 h1:EGMPT//Ezu+ylkCijjPc+f4Aih7sZvaAr+O3EHBxvZg= -golang.org/x/mod v0.26.0/go.mod h1:/j6NAhSk8iQ723BGAUyoAcn7SlD7s15Dp9Nd/SfeaFQ= -golang.org/x/net v0.42.0 h1:jzkYrhi3YQWD6MLBJcsklgQsoAcw89EcZbJw8Z614hs= -golang.org/x/net v0.42.0/go.mod h1:FF1RA5d3u7nAYA4z2TkclSCKh68eSXtiFwcWQpPXdt8= +golang.org/x/mod v0.27.0 h1:kb+q2PyFnEADO2IEF935ehFUXlWiNjJWtRNgBLSfbxQ= +golang.org/x/mod v0.27.0/go.mod h1:rWI627Fq0DEoudcK+MBkNkCe0EetEaDSwJJkCcjpazc= +golang.org/x/net v0.43.0 h1:lat02VYK2j4aLzMzecihNvTlJNQUq316m2Mr9rnM6YE= +golang.org/x/net v0.43.0/go.mod h1:vhO1fvI4dGsIjh73sWfUVjj3N7CA9WkKJNQm2svM6Jg= golang.org/x/sync v0.16.0 h1:ycBJEhp9p4vXvUZNszeOq0kGTPghopOL8q0fq3vstxw= golang.org/x/sync v0.16.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= -golang.org/x/sys v0.34.0 h1:H5Y5sJ2L2JRdyv7ROF1he/lPdvFsd0mJHFw2ThKHxLA= -golang.org/x/sys v0.34.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= -golang.org/x/telemetry v0.0.0-20250710130107-8d8967aff50b h1:DU+gwOBXU+6bO0sEyO7o/NeMlxZxCZEvI7v+J4a1zRQ= -golang.org/x/telemetry v0.0.0-20250710130107-8d8967aff50b/go.mod h1:4ZwOYna0/zsOKwuR5X/m0QFOJpSZvAxFfkQT+Erd9D4= +golang.org/x/sys v0.35.0 h1:vz1N37gP5bs89s7He8XuIYXpyY0+QlsKmzipCbUtyxI= +golang.org/x/sys v0.35.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= +golang.org/x/telemetry v0.0.0-20250807160809-1a19826ec488 h1:3doPGa+Gg4snce233aCWnbZVFsyFMo/dR40KK/6skyE= +golang.org/x/telemetry v0.0.0-20250807160809-1a19826ec488/go.mod h1:fGb/2+tgXXjhjHsTNdVEEMZNWA0quBnfrO+AfoDSAKw= diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go index 20312345018..3f172159359 100644 --- a/go/analysis/analysistest/analysistest.go +++ b/go/analysis/analysistest/analysistest.go @@ -575,7 +575,7 @@ func check(t Testing, gopath string, act *checker.Action) { files := act.Package.OtherFiles // Hack: these two analyzers need to extract expectations from - // all configurations, so include the files are are usually + // all configurations, so include the files are usually // ignored. (This was previously a hack in the respective // analyzers' tests.) if act.Analyzer.Name == "buildtag" || act.Analyzer.Name == "directive" { diff --git a/go/analysis/internal/analysisflags/flags.go b/go/analysis/internal/analysisflags/flags.go index 6aefef25815..18e01c40def 100644 --- a/go/analysis/internal/analysisflags/flags.go +++ b/go/analysis/internal/analysisflags/flags.go @@ -318,24 +318,32 @@ var vetLegacyFlags = map[string]string{ // If contextLines is nonnegative, it also prints the // offending line plus this many lines of context. func PrintPlain(out io.Writer, fset *token.FileSet, contextLines int, diag analysis.Diagnostic) { - posn := fset.Position(diag.Pos) - fmt.Fprintf(out, "%s: %s\n", posn, diag.Message) - - // show offending line plus N lines of context. - if contextLines >= 0 { - posn := fset.Position(diag.Pos) - end := fset.Position(diag.End) - if !end.IsValid() { - end = posn - } - data, _ := os.ReadFile(posn.Filename) - lines := strings.Split(string(data), "\n") - for i := posn.Line - contextLines; i <= end.Line+contextLines; i++ { - if 1 <= i && i <= len(lines) { - fmt.Fprintf(out, "%d\t%s\n", i, lines[i-1]) + print := func(pos, end token.Pos, message string) { + posn := fset.Position(pos) + fmt.Fprintf(out, "%s: %s\n", posn, message) + + // show offending line plus N lines of context. + if contextLines >= 0 { + end := fset.Position(end) + if !end.IsValid() { + end = posn + } + // TODO(adonovan): highlight the portion of the line indicated + // by pos...end using ASCII art, terminal colors, etc? + data, _ := os.ReadFile(posn.Filename) + lines := strings.Split(string(data), "\n") + for i := posn.Line - contextLines; i <= end.Line+contextLines; i++ { + if 1 <= i && i <= len(lines) { + fmt.Fprintf(out, "%d\t%s\n", i, lines[i-1]) + } } } } + + print(diag.Pos, diag.End, diag.Message) + for _, rel := range diag.Related { + print(rel.Pos, rel.End, "\t"+rel.Message) + } } // A JSONTree is a mapping from package ID to analysis name to result. diff --git a/go/analysis/internal/checker/fix_test.go b/go/analysis/internal/checker/fix_test.go index 1f9d534017b..d144de40236 100644 --- a/go/analysis/internal/checker/fix_test.go +++ b/go/analysis/internal/checker/fix_test.go @@ -42,6 +42,7 @@ func TestMain(m *testing.M) { markerAnalyzer, noendAnalyzer, renameAnalyzer, + relatedAnalyzer, ) panic("unreachable") } @@ -587,6 +588,25 @@ var noendAnalyzer = &analysis.Analyzer{ }, } +var relatedAnalyzer = &analysis.Analyzer{ + Name: "related", + Doc: "reports a Diagnostic with RelatedInformaiton", + Run: func(pass *analysis.Pass) (any, error) { + decl := pass.Files[0].Decls[0] + pass.Report(analysis.Diagnostic{ + Pos: decl.Pos(), + End: decl.Pos() + 1, + Message: "decl starts here", + Related: []analysis.RelatedInformation{{ + Message: "decl ends here", + Pos: decl.End() - 1, + End: decl.End(), + }}, + }) + return nil, nil + }, +} + // panics asserts that f() panics with with a value whose printed form matches the regexp want. func panics(t *testing.T, want string, f func()) { defer func() { diff --git a/go/analysis/internal/checker/testdata/plain.txt b/go/analysis/internal/checker/testdata/plain.txt new file mode 100644 index 00000000000..562c75e3aea --- /dev/null +++ b/go/analysis/internal/checker/testdata/plain.txt @@ -0,0 +1,25 @@ +# Test plain output. +# +# File slashes assume non-Windows. +skip GOOS=windows + +checker -related example.com/p +stderr p/p.go:3:1: decl starts here +stderr p/p.go:4:1: decl ends here + +checker -related -c=0 example.com/p +stderr p/p.go:3:1: decl starts here +stderr 3 func f\(bar int\) { +stderr p/p.go:4:1: decl ends here +stderr 4 } +exit 3 + +-- go.mod -- +module example.com +go 1.22 + +-- p/p.go -- +package p + +func f(bar int) { +} diff --git a/go/ast/astutil/enclosing.go b/go/ast/astutil/enclosing.go index 89f5097be00..0fb4e7eea81 100644 --- a/go/ast/astutil/enclosing.go +++ b/go/ast/astutil/enclosing.go @@ -113,7 +113,7 @@ func PathEnclosingInterval(root *ast.File, start, end token.Pos) (path []ast.Nod // childrenOf elides the FuncType node beneath FuncDecl. // Add it back here for TypeParams, Params, Results, // all FieldLists). But we don't add it back for the "func" token - // even though it is is the tree at FuncDecl.Type.Func. + // even though it is the tree at FuncDecl.Type.Func. if decl, ok := node.(*ast.FuncDecl); ok { if fields, ok := child.(*ast.FieldList); ok && fields != decl.Recv { path = append(path, decl.Type) diff --git a/go/ast/inspector/inspector.go b/go/ast/inspector/inspector.go index bc44b2c8e7e..a703cdfcf90 100644 --- a/go/ast/inspector/inspector.go +++ b/go/ast/inspector/inspector.go @@ -85,6 +85,7 @@ type event struct { // TODO: Experiment with storing only the second word of event.node (unsafe.Pointer). // Type can be recovered from the sole bit in typ. +// [Tried this, wasn't faster. --adonovan] // Preorder visits all the nodes of the files supplied to New in // depth-first order. It calls f(n) for each node n before it visits diff --git a/go/ast/inspector/typeof.go b/go/ast/inspector/typeof.go index be0f990a295..9852331a3db 100644 --- a/go/ast/inspector/typeof.go +++ b/go/ast/inspector/typeof.go @@ -12,8 +12,6 @@ package inspector import ( "go/ast" "math" - - _ "unsafe" ) const ( diff --git a/go/packages/doc.go b/go/packages/doc.go index f1931d10eeb..366aab6b2ca 100644 --- a/go/packages/doc.go +++ b/go/packages/doc.go @@ -76,6 +76,8 @@ uninterpreted to Load, so that it can interpret them according to the conventions of the underlying build system. See the Example function for typical usage. +See also [golang.org/x/tools/go/packages/internal/linecount] +for an example application. # The driver protocol diff --git a/go/packages/internal/linecount/linecount.go b/go/packages/internal/linecount/linecount.go new file mode 100644 index 00000000000..f1fae384b77 --- /dev/null +++ b/go/packages/internal/linecount/linecount.go @@ -0,0 +1,189 @@ +// Copyright 2025 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. + +// The linecount command shows the number of lines of code in a set of +// Go packages plus their dependencies. It serves as a working +// illustration of the [packages.Load] operation. +// +// Example: show gopls' total source line count, and its breakdown +// between gopls, x/tools, and the std go/* packages. (The balance +// comes from other std packages.) +// +// $ linecount -mode=total ./gopls +// 752124 +// $ linecount -mode=total -module=golang.org/x/tools/gopls ./gopls +// 103519 +// $ linecount -mode=total -module=golang.org/x/tools ./gopls +// 99504 +// $ linecount -mode=total -prefix=go -module=std ./gopls +// 47502 +// +// Example: show the top 5 modules contributing to gopls' source line count: +// +// $ linecount -mode=module ./gopls | head -n 5 +// 440274 std +// 103519 golang.org/x/tools/gopls +// 99504 golang.org/x/tools +// 40220 honnef.co/go/tools +// 17707 golang.org/x/text +// +// Example: show the top 3 largest files in the gopls module: +// +// $ linecount -mode=file -module=golang.org/x/tools/gopls ./gopls | head -n 3 +// 6841 gopls/internal/protocol/tsprotocol.go +// 3769 gopls/internal/golang/completion/completion.go +// 2202 gopls/internal/cache/snapshot.go +package main + +import ( + "bytes" + "cmp" + "flag" + "fmt" + "log" + "os" + "path" + "slices" + "strings" + "sync" + + "golang.org/x/sync/errgroup" + "golang.org/x/tools/go/packages" +) + +// TODO(adonovan): filters: +// - exclude comment and blank lines (-nonblank) +// - exclude generated files (-generated=false) +// - exclude non-CompiledGoFiles +// - include OtherFiles (asm, etc) +// - include tests (needs care to avoid double counting) + +func usage() { + // See https://go.dev/issue/63659. + fmt.Fprintf(os.Stderr, "Usage: linecount [flags] packages...\n") + flag.PrintDefaults() + fmt.Fprintf(os.Stderr, ` +Docs: go doc golang.org/x/tools/go/packages/internal/linecount +https://pkg.go.dev/golang.org/x/tools/go/packages/internal/linecount +`) +} + +func main() { + // Parse command line. + log.SetPrefix("linecount: ") + log.SetFlags(0) + var ( + mode = flag.String("mode", "file", "group lines by 'module', 'package', or 'file', or show only 'total'") + prefix = flag.String("prefix", "", "only count files in packages whose path has the specified prefix") + onlyModule = flag.String("module", "", "only count files in the specified module") + ) + flag.Usage = usage + flag.Parse() + if len(flag.Args()) == 0 { + usage() + os.Exit(1) + } + + // Load packages. + cfg := &packages.Config{ + Mode: packages.NeedName | + packages.NeedFiles | + packages.NeedImports | + packages.NeedDeps | + packages.NeedModule, + } + pkgs, err := packages.Load(cfg, flag.Args()...) + if err != nil { + log.Fatal(err) + } + if packages.PrintErrors(pkgs) > 0 { + os.Exit(1) + } + + // Read files and count lines. + var ( + mu sync.Mutex + byFile = make(map[string]int) + byPackage = make(map[string]int) + byModule = make(map[string]int) + ) + var g errgroup.Group + g.SetLimit(20) // file system parallelism level + packages.Visit(pkgs, nil, func(p *packages.Package) { + pkgpath := p.PkgPath + module := "std" + if p.Module != nil { + module = p.Module.Path + } + if *prefix != "" && !within(pkgpath, path.Clean(*prefix)) { + return + } + if *onlyModule != "" && module != *onlyModule { + return + } + for _, f := range p.GoFiles { + g.Go(func() error { + data, err := os.ReadFile(f) + if err != nil { + return err + } + n := bytes.Count(data, []byte("\n")) + + mu.Lock() + byFile[f] = n + byPackage[pkgpath] += n + byModule[module] += n + mu.Unlock() + + return nil + }) + } + }) + if err := g.Wait(); err != nil { + log.Fatal(err) + } + + // Display the result. + switch *mode { + case "file", "package", "module": + var m map[string]int + switch *mode { + case "file": + m = byFile + case "package": + m = byPackage + case "module": + m = byModule + } + type item struct { + name string + count int + } + var items []item + for name, count := range m { + items = append(items, item{name, count}) + } + slices.SortFunc(items, func(x, y item) int { + return -cmp.Compare(x.count, y.count) + }) + for _, item := range items { + fmt.Printf("%d\t%s\n", item.count, item.name) + } + + case "total": + total := 0 + for _, n := range byFile { + total += n + } + fmt.Printf("%d\n", total) + + default: + log.Fatalf("invalid -mode %q (want file, package, module, or total)", *mode) + } +} + +func within(file, dir string) bool { + return file == dir || + strings.HasPrefix(file, dir) && file[len(dir)] == os.PathSeparator +} diff --git a/go/ssa/builder.go b/go/ssa/builder.go index fe713a77b61..a5ef8fb40d8 100644 --- a/go/ssa/builder.go +++ b/go/ssa/builder.go @@ -138,7 +138,7 @@ type builder struct { finished int // finished is the length of the prefix of fns containing built functions. // The task of building shared functions within the builder. - // Shared functions are ones the the builder may either create or lookup. + // Shared functions are ones the builder may either create or lookup. // These may be built by other builders in parallel. // The task is done when the builder has finished iterating, and it // waits for all shared functions to finish building. diff --git a/go/ssa/builder_test.go b/go/ssa/builder_test.go index be710ad66bf..2472b3eb13d 100644 --- a/go/ssa/builder_test.go +++ b/go/ssa/builder_test.go @@ -1115,8 +1115,10 @@ func TestGenericAliases(t *testing.T) { cmd.Env = append(os.Environ(), "GENERICALIASTEST_CHILD=1", "GODEBUG=gotypesalias=1", - "GOEXPERIMENT=aliastypeparams", ) + if testenv.Go1Point() == 23 { + cmd.Env = append(cmd.Env, "GOEXPERIMENT=aliastypeparams") + } out, err := cmd.CombinedOutput() if len(out) > 0 { t.Logf("out=<<%s>>", out) diff --git a/go/types/typeutil/map_test.go b/go/types/typeutil/map_test.go index 920c8131257..e89edd010bc 100644 --- a/go/types/typeutil/map_test.go +++ b/go/types/typeutil/map_test.go @@ -214,7 +214,7 @@ func Fb1[P any](x *P) { func Fb2[Q any](x *Q) { } -// G1 and G2 are mutally recursive, and have identical methods. +// G1 and G2 are mutually recursive, and have identical methods. type G1[P any] struct{ Field *G2[P] } diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md index 1deea3ed630..fcd81640125 100644 --- a/gopls/doc/analyzers.md +++ b/gopls/doc/analyzers.md @@ -4434,7 +4434,7 @@ package. The tool may report false positives in some situations, for example: - - For a declaration of an unexported function that is referenced + - for a declaration of an unexported function that is referenced from another package using the go:linkname mechanism, if the declaration's doc comment does not also have a go:linkname comment. @@ -4443,17 +4443,19 @@ example: annotations, if they must be used at all, should be used on both the declaration and the alias.) - - For compiler intrinsics in the "runtime" package that, though + - for compiler intrinsics in the "runtime" package that, though never referenced, are known to the compiler and are called indirectly by compiled object code. - - For functions called only from assembly. + - for functions called only from assembly. - - For functions called only from files whose build tags are not + - for functions called only from files whose build tags are not selected in the current build configuration. -See https://github.com/golang/go/issues/71686 for discussion of -these limitations. +Since these situations are relatively common in the low-level parts +of the runtime, this analyzer ignores the standard library. +See https://go.dev/issue/71686 and https://go.dev/issue/74130 for +further discussion of these limitations. The unusedfunc algorithm is not as precise as the golang.org/x/tools/cmd/deadcode tool, but it has the advantage that diff --git a/gopls/doc/contributing.md b/gopls/doc/contributing.md index 38644fc439e..47161acc89b 100644 --- a/gopls/doc/contributing.md +++ b/gopls/doc/contributing.md @@ -10,11 +10,9 @@ So, before sending a CL, please please please: exist already. This allows us to identify redundant requests, or to merge a specific problem into a more general one, and to assess the importance of the problem. - - **claim it for yourself** by commenting on the issue or, if you are able, by assigning the issue to yourself. This helps us avoid two people working on the same problem. - - **propose an implementation plan** in the issue tracker for CLs of any complexity. It is much more efficient to discuss the plan at a high level before we start getting bogged down in the details of @@ -41,7 +39,7 @@ for example by making the code self-explanatory. It's fine to disagree with a comment, point out a reviewer's mistake, or offer to address a comment in a follow-up change, -leaving a TODO comment in the current CL. +leaving a `TODO` comment in the current CL. But please don't dismiss or quietly ignore a comment without action, as it may lead reviewers to repeat themselves, or to serious problems being neglected. @@ -60,8 +58,10 @@ claiming it. ## Getting started -Most of the `gopls` logic is in the `golang.org/x/tools/gopls/internal` -directory. See [design/implementation.md](./design/implementation.md) for an overview of the code organization. +[![PkgGoDev](https://pkg.go.dev/badge/golang.org/x/tools/gopls/internal)](https://pkg.go.dev/golang.org/x/tools/gopls/internal) + +Most of the `gopls` logic is in the `golang.org/x/tools/gopls/internal` directory. +See [design/implementation.md](./design/implementation.md) for an overview of the code organization. ## Build diff --git a/gopls/doc/features/index.md b/gopls/doc/features/index.md index 7226509798e..56b66eb4e35 100644 --- a/gopls/doc/features/index.md +++ b/gopls/doc/features/index.md @@ -64,6 +64,7 @@ when making significant changes to existing features or when adding new ones. - [go.mod and go.work files](modfiles.md): Go module and workspace manifests - [Go *.s assembly files](assembly.md): Go assembly files - [Command-line interface](../command-line.md): CLI for debugging and scripting (unstable) +- [Model Context Protocol (MCP)](mcp.md): use some features in AI-assisted environments You can find this page from within your editor by executing the `gopls.doc.features` [code action](transformation.md#code-actions), diff --git a/gopls/doc/features/mcp.md b/gopls/doc/features/mcp.md new file mode 100644 index 00000000000..18f6fb30209 --- /dev/null +++ b/gopls/doc/features/mcp.md @@ -0,0 +1,74 @@ +--- +title: "Gopls: Model Context Protocol support" +--- + +Gopls includes an experimental built-in server for the [Model Context +Protocol](https://modelcontextprotocol.io/introduction) (MCP), allowing it to +expose a subset of its functionality to AI assistants in the form of MCP tools. + +## Running the MCP server + +There are two modes for running this server: 'attached' and 'detached'. In +attached mode, the MCP server operates in the context of an active gopls LSP +session, and so is able to share memory with your LSP session and observe the +current unsaved buffer state. In detached mode, gopls interacts with a headless +LSP session, and therefore only sees saved files on disk. + +### Attached mode + +To use the 'attached' mode, run gopls with the `-mcp.listen` flag. For +example: + +``` +gopls serve -mcp.listen=localhost:8092 +``` + +This exposes an HTTP based MCP server using the server-sent event transport +(SSE), available at `http://localhost:8092/sessions/1` (assuming you have only +one [session](../daemon.md) on your gopls instance). + +### Detached mode + +To use the 'detached' mode, run the `mcp` subcommand: + +``` +gopls mcp +``` + +This runs a standalone gopls instance that speaks MCP over stdin/stdout. + +## Instructions to the model + +This gopls MCP server includes model instructions for its usage, describing +workflows for interacting with Go code using its available tools. These +instructions are automatically published during the MCP server initialization, +but you may want to also load them as additional context in your AI-assisted +session, to emphasize their importance. The `-instructions` flag causes them to +be printed, so that you can do, for example: + +``` +gopls mcp -instructions > /path/to/contextFile.md +``` + +## Security considerations + +The gopls MCP server is a wrapper around the functionality ordinarily exposed +by gopls through the Language Server Protocol (LSP). As such, gopls' tools +may perform any of the operations gopls normally performs, including: + +- reading files from the file system, and returning their contents in tool + results (such as when providing context); +- executing the `go` command to load package information, which may result in + calls to https://proxy.golang.org to download Go modules, and writes to go + caches; +- writing to gopls' cache or persistant configuration files; and +- uploading weekly telemetry data **if you have opted in** to [Go telemetry](https://go.dev/doc/telemetry). + +The gopls MCP server does not perform any operations not already performed by +gopls in an ordinary IDE session. Like most LSP servers, gopls does not +generally write directly to your source tree, though it may instruct the client +to apply edits. Nor does it make arbitrary requests over the network, though it +may make narrowly scoped requests to certain services such as the Go module +mirror or the Go vulnerability database, which can't readily be exploited as a +vehicle for exfiltration by a confused agent. Nevertheless, these capabilities +may require additional consideration when used as part of an AI-enabled system. diff --git a/gopls/doc/index.md b/gopls/doc/index.md index e7908ff29f2..0e6f1c83cf6 100644 --- a/gopls/doc/index.md +++ b/gopls/doc/index.md @@ -9,8 +9,6 @@ title: "Gopls: The language server for Go" $ open http://localhost:6060/go.dev/gopls --> -[![PkgGoDev](https://pkg.go.dev/badge/golang.org/x/tools/gopls)](https://pkg.go.dev/golang.org/x/tools/gopls) - `gopls` (pronounced "Go please") is the official [language server](https://langserver.org) for Go, developed by the Go team. It provides a wide variety of [IDE features](features/) to any @@ -26,10 +24,13 @@ by editor, so we recommend that you proceed to the Also, the gopls documentation for each feature describes whether it is supported in each client editor. +This documentation (https://go.dev/gopls) describes the most recent release of gopls. +To preview documentation for the release under development, visit https://tip.golang.org/gopls. + ## Features Gopls supports a wide range of standard LSP features for navigation, -completion, diagnostics, analysis, and refactoring, and number of +completion, diagnostics, analysis, and refactoring, and a number of additional features not found in other language servers. See the [Index of features](features/) for complete @@ -67,12 +68,12 @@ ensure that Gopls is updated when a new stable version is released. After updating, you may need to restart running Gopls processes to observe the effect. Each client has its own way to restart the server. -(On a UNIX machine, you can use the commmand `killall gopls`.) +(On a UNIX machine, you can use the command `killall gopls`.) Learn more in the [advanced installation instructions](advanced.md#installing-unreleased-versions). -## Release policy +## Releases Gopls [releases](release/) follow [semantic versioning](http://semver.org), with major changes and new features introduced only in new minor versions @@ -117,7 +118,7 @@ full list of `gopls` settings can be found in the [settings documentation](setti variables you configure. Some editors, such as VS Code, allow users to selectively override the values of some environment variables. -## Support Policy +## Support policy Gopls is maintained by engineers on the [Go tools team](https://github.com/orgs/golang/teams/tools-team/members), diff --git a/gopls/doc/release/v0.20.0.md b/gopls/doc/release/v0.20.0.md index ad61f1c8b4c..4dfc22305f4 100644 --- a/gopls/doc/release/v0.20.0.md +++ b/gopls/doc/release/v0.20.0.md @@ -1,20 +1,24 @@ --- -title: "Gopls release v0.20.0 (forthcoming)" +title: "Gopls release v0.20.0" --- -Gopls' documentation is now available on the Go project's website at -https://go.dev/gopls. (Note: this link will not be accurate until the -completion of the release. Dueing the pre-release period, please use -https://tip.golang.org/gopls, which reflects the latest commit.) +This release contains a new experimental Model Context Protocol (MCP) +server for gopls, which may be used to integrate a subset of gopls' +features in AI-assisted environments. -Unlike markdown files in GitHub, these pages are crawled by Google's +Gopls' documentation is now available on the Go project's website at +https://go.dev/gopls. (This link reflects the latest gopls release; +use https://tip.golang.org/gopls to see docs for the latest commit.) +Unlike Markdown files in GitHub, these pages are crawled by Google's web search index. -## Configuration Changes +## Configuration changes -## Navigation features +This release enables by default the new persistent index of packages +in the module cache. This was first attempted in [v0.19](./v0.19.0.md) but reverted +due to problems that have since been fixed. -### Web-based features +## Web-based features ### "Split package" tool @@ -28,7 +32,7 @@ component, then visualize the dependencies among the components Refresh the page each time you edit your code to see the latest information. - +

The tool makes it easy to iterate over potential decompositions until you find one you are happy with. A future version of @@ -36,15 +40,21 @@ the tool will automate the code transformation, but for now you must do that step by hand. - +See the [documentation](../features/mcp.md) for more information. -## Editing features +**Caveats:** This is a brand new mode of operation for gopls, and so we're +still experimenting with the best set of tools and instructions to provide. +Please let us know how well it works for you. Also, please be aware that +allowing LLMs to execute operations in your workspace entails additional +security considerations, as discussed in the documentation above. ## Analysis features @@ -82,3 +92,8 @@ that are unreferenced within their declaring package. the analysis framework since it depends on the entire workspace.) ## Code transformation features + + +The Rename operation now allows you to rename an embedded field, such +as T in `struct{ T }`, so long as the operation is requested at the +field declaration (T). Both the field and its type will be renamed. diff --git a/gopls/doc/release/v0.21.0.md b/gopls/doc/release/v0.21.0.md new file mode 100644 index 00000000000..40fe02a6c70 --- /dev/null +++ b/gopls/doc/release/v0.21.0.md @@ -0,0 +1,21 @@ +--- +title: "Gopls release v0.21.0 (forthcoming)" +--- + +## Configuration changes +## Web-based features +## Editing features +## Analysis features +## Code transformation features + + +The Rename operation now treats [Doc Links](https://tip.golang.org/doc/comment#doclinks) +like identifiers, so you can initiate a renaming from a Doc Link. + + diff --git a/gopls/go.mod b/gopls/go.mod index 52d9f4cebc0..bd7df4b34a8 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -7,11 +7,11 @@ require ( github.com/fsnotify/fsnotify v1.9.0 github.com/google/go-cmp v0.7.0 github.com/jba/templatecheck v0.7.1 - golang.org/x/mod v0.26.0 + golang.org/x/mod v0.27.0 golang.org/x/sync v0.16.0 - golang.org/x/telemetry v0.0.0-20250710130107-8d8967aff50b - golang.org/x/text v0.27.0 - golang.org/x/tools v0.34.1-0.20250613162507-3f93fece84c7 + golang.org/x/telemetry v0.0.0-20250807160809-1a19826ec488 + golang.org/x/text v0.28.0 + golang.org/x/tools v0.35.0 golang.org/x/vuln v1.1.4 gopkg.in/yaml.v3 v3.0.1 honnef.co/go/tools v0.7.0-0.dev.0.20250523013057-bbc2f4dd71ea @@ -24,8 +24,8 @@ require ( github.com/fatih/camelcase v1.0.0 // indirect github.com/fatih/structtag v1.2.0 // indirect github.com/google/safehtml v0.1.0 // indirect - golang.org/x/exp/typeparams v0.0.0-20250620022241-b7579e27df2b // indirect - golang.org/x/sys v0.34.0 // indirect + golang.org/x/exp/typeparams v0.0.0-20250718183923-645b1fa84792 // indirect + golang.org/x/sys v0.35.0 // indirect golang.org/x/tools/go/expect v0.1.1-deprecated // indirect golang.org/x/tools/go/packages/packagestest v0.1.1-deprecated // indirect gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect diff --git a/gopls/go.sum b/gopls/go.sum index cbe0dcab1e3..9ada8f2615a 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -27,16 +27,19 @@ github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5t golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/crypto v0.39.0/go.mod h1:L+Xg3Wf6HoL4Bn4238Z6ft6KfEpN0tJGo53AAPC632U= golang.org/x/crypto v0.40.0/go.mod h1:Qr1vMER5WyS2dfPHAlsOj01wgLbsyWtFn/aY+5+ZdxY= -golang.org/x/exp/typeparams v0.0.0-20250620022241-b7579e27df2b h1:KdrhdYPDUvJTvrDK9gdjfFd6JTk8vA1WJoldYSi0kHo= -golang.org/x/exp/typeparams v0.0.0-20250620022241-b7579e27df2b/go.mod h1:LKZHyeOpPuZcMgxeHjJp4p5yvxrCX1xDvH10zYHhjjQ= +golang.org/x/crypto v0.41.0/go.mod h1:pO5AFd7FA68rFak7rOAGVuygIISepHftHnr8dr6+sUc= +golang.org/x/exp/typeparams v0.0.0-20250718183923-645b1fa84792 h1:54/e+WfmhvjR2Zuz8Q7dzLGxIBM+s5WZpvo1QfVDGB8= +golang.org/x/exp/typeparams v0.0.0-20250718183923-645b1fa84792/go.mod h1:LKZHyeOpPuZcMgxeHjJp4p5yvxrCX1xDvH10zYHhjjQ= golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.25.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww= -golang.org/x/mod v0.26.0 h1:EGMPT//Ezu+ylkCijjPc+f4Aih7sZvaAr+O3EHBxvZg= golang.org/x/mod v0.26.0/go.mod h1:/j6NAhSk8iQ723BGAUyoAcn7SlD7s15Dp9Nd/SfeaFQ= +golang.org/x/mod v0.27.0 h1:kb+q2PyFnEADO2IEF935ehFUXlWiNjJWtRNgBLSfbxQ= +golang.org/x/mod v0.27.0/go.mod h1:rWI627Fq0DEoudcK+MBkNkCe0EetEaDSwJJkCcjpazc= golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44= golang.org/x/net v0.41.0/go.mod h1:B/K4NNqkfmg07DQYrbwvSluqCJOOXwUjeb/5lOisjbA= golang.org/x/net v0.42.0/go.mod h1:FF1RA5d3u7nAYA4z2TkclSCKh68eSXtiFwcWQpPXdt8= +golang.org/x/net v0.43.0/go.mod h1:vhO1fvI4dGsIjh73sWfUVjj3N7CA9WkKJNQm2svM6Jg= golang.org/x/sync v0.15.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= golang.org/x/sync v0.16.0 h1:ycBJEhp9p4vXvUZNszeOq0kGTPghopOL8q0fq3vstxw= golang.org/x/sync v0.16.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= @@ -44,20 +47,23 @@ golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.33.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= -golang.org/x/sys v0.34.0 h1:H5Y5sJ2L2JRdyv7ROF1he/lPdvFsd0mJHFw2ThKHxLA= golang.org/x/sys v0.34.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= -golang.org/x/telemetry v0.0.0-20250710130107-8d8967aff50b h1:DU+gwOBXU+6bO0sEyO7o/NeMlxZxCZEvI7v+J4a1zRQ= -golang.org/x/telemetry v0.0.0-20250710130107-8d8967aff50b/go.mod h1:4ZwOYna0/zsOKwuR5X/m0QFOJpSZvAxFfkQT+Erd9D4= +golang.org/x/sys v0.35.0 h1:vz1N37gP5bs89s7He8XuIYXpyY0+QlsKmzipCbUtyxI= +golang.org/x/sys v0.35.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= +golang.org/x/telemetry v0.0.0-20250807160809-1a19826ec488 h1:3doPGa+Gg4snce233aCWnbZVFsyFMo/dR40KK/6skyE= +golang.org/x/telemetry v0.0.0-20250807160809-1a19826ec488/go.mod h1:fGb/2+tgXXjhjHsTNdVEEMZNWA0quBnfrO+AfoDSAKw= golang.org/x/term v0.8.0/go.mod h1:xPskH00ivmX89bAKVGSKKtLOWNx2+17Eiy94tnKShWo= golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= golang.org/x/term v0.32.0/go.mod h1:uZG1FhGx848Sqfsq4/DlJr3xGGsYMu/L5GW4abiaEPQ= golang.org/x/term v0.33.0/go.mod h1:s18+ql9tYWp1IfpV9DmCtQDDSRBUjKaw9M1eAv5UeF0= +golang.org/x/term v0.34.0/go.mod h1:5jC53AEywhIVebHgPVeg0mj8OD3VO9OzclacVrqpaAw= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.26.0/go.mod h1:QK15LZJUUQVJxhz7wXgxSy/CJaTFjd0G+YLonydOVQA= -golang.org/x/text v0.27.0 h1:4fGWRpyh641NLlecmyl4LOe6yDdfaYNrGb2zdfo4JV4= golang.org/x/text v0.27.0/go.mod h1:1D28KMCvyooCX9hBiosv5Tz/+YLxj0j7XhWjpSUF7CU= +golang.org/x/text v0.28.0 h1:rhazDwis8INMIwQ4tpjLDzUhx6RlXqZNPEM0huQojng= +golang.org/x/text v0.28.0/go.mod h1:U8nCwOR8jO/marOQ0QbDiOngZVEBB7MAiitBuMjXiNU= golang.org/x/tools/go/expect v0.1.1-deprecated h1:jpBZDwmgPhXsKZC6WhL20P4b/wmnpsEAGHaNy0n/rJM= golang.org/x/tools/go/expect v0.1.1-deprecated/go.mod h1:eihoPOH+FgIqa3FpoTwguz/bVUSGBlGQU67vpBeOrBY= golang.org/x/tools/go/packages/packagestest v0.1.1-deprecated h1:1h2MnaIAIXISqTFKdENegdpAgUXz6NrPEsbIeWaBRvM= diff --git a/gopls/internal/analysis/modernize/forvar.go b/gopls/internal/analysis/modernize/forvar.go index 6f88ab77ed9..91942211a89 100644 --- a/gopls/internal/analysis/modernize/forvar.go +++ b/gopls/internal/analysis/modernize/forvar.go @@ -30,66 +30,50 @@ import ( // is declared implicitly before executing the post statement and initialized to the // value of the previous iteration's variable at that moment.") func forvar(pass *analysis.Pass) { - info := pass.TypesInfo - inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - for curFile := range filesUsing(inspect, info, "go1.22") { + for curFile := range filesUsing(inspect, pass.TypesInfo, "go1.22") { + astFile := curFile.Node().(*ast.File) for curLoop := range curFile.Preorder((*ast.RangeStmt)(nil)) { - // in a range loop. Is the first statement var := var? - // if so, is var one of the range vars, and is it defined - // in the for statement? - // If so, decide how much to delete. loop := curLoop.Node().(*ast.RangeStmt) if loop.Tok != token.DEFINE { continue } - v, stmt := loopVarRedecl(loop.Body) - if v == nil { - continue // index is not redeclared - } - if (loop.Key == nil || !equalSyntax(loop.Key, v)) && - (loop.Value == nil || !equalSyntax(loop.Value, v)) { - continue + isLoopVarRedecl := func(assign *ast.AssignStmt) bool { + for i, lhs := range assign.Lhs { + if !(equalSyntax(lhs, assign.Rhs[i]) && + (equalSyntax(lhs, loop.Key) || equalSyntax(lhs, loop.Value))) { + return false + } + } + return true } - astFile := curFile.Node().(*ast.File) - edits := analysisinternal.DeleteStmt(pass.Fset, astFile, stmt, bug.Reportf) - if len(edits) == 0 { - bug.Reportf("forvar failed to delete statement") - continue - } - remove := edits[0] - diag := analysis.Diagnostic{ - Pos: remove.Pos, - End: remove.End, - Category: "forvar", - Message: "copying variable is unneeded", - SuggestedFixes: []analysis.SuggestedFix{{ - Message: "Remove unneeded redeclaration", - TextEdits: []analysis.TextEdit{remove}, - }}, + // Have: for k, v := range x { stmts } + // + // Delete the prefix of stmts that are + // of the form k := k; v := v; k, v := k, v; v, k := v, k. + for _, stmt := range loop.Body.List { + if assign, ok := stmt.(*ast.AssignStmt); ok && + assign.Tok == token.DEFINE && + len(assign.Lhs) == len(assign.Rhs) && + isLoopVarRedecl(assign) { + + edits := analysisinternal.DeleteStmt(pass.Fset, astFile, stmt, bug.Reportf) + if len(edits) > 0 { + pass.Report(analysis.Diagnostic{ + Pos: stmt.Pos(), + End: stmt.End(), + Category: "forvar", + Message: "copying variable is unneeded", + SuggestedFixes: []analysis.SuggestedFix{{ + Message: "Remove unneeded redeclaration", + TextEdits: edits, + }}, + }) + } + } else { + break // stop at first other statement + } } - pass.Report(diag) } } } - -// if the first statement is var := var, return var and the stmt -func loopVarRedecl(body *ast.BlockStmt) (*ast.Ident, *ast.AssignStmt) { - if len(body.List) < 1 { - return nil, nil - } - stmt, ok := body.List[0].(*ast.AssignStmt) - if !ok || !isSimpleAssign(stmt) || stmt.Tok != token.DEFINE { - return nil, nil - } - if _, ok := stmt.Lhs[0].(*ast.Ident); !ok { - return nil, nil - } - if _, ok := stmt.Rhs[0].(*ast.Ident); !ok { - return nil, nil - } - if stmt.Lhs[0].(*ast.Ident).Name == stmt.Rhs[0].(*ast.Ident).Name { - return stmt.Lhs[0].(*ast.Ident), stmt - } - return nil, nil -} diff --git a/gopls/internal/analysis/modernize/testdata/src/forvar/forvar.go b/gopls/internal/analysis/modernize/testdata/src/forvar/forvar.go index dd5ecd75e29..01852aae48e 100644 --- a/gopls/internal/analysis/modernize/testdata/src/forvar/forvar.go +++ b/gopls/internal/analysis/modernize/testdata/src/forvar/forvar.go @@ -12,12 +12,24 @@ func _(m map[int]int, s []int) { } for k, v := range m { k := k // want "copying variable is unneeded" - v := v // nope: report only the first redeclaration + v := v // want "copying variable is unneeded" go f(k) go f(v) } - for _, v := range m { + for k, v := range m { v := v // want "copying variable is unneeded" + k := k // want "copying variable is unneeded" + go f(k) + go f(v) + } + for k, v := range m { + k, v := k, v // want "copying variable is unneeded" + go f(k) + go f(v) + } + for k, v := range m { + v, k := v, k // want "copying variable is unneeded" + go f(k) go f(v) } for i := range s { @@ -53,10 +65,25 @@ func _(m map[int]int, s []int) { v := i go f(v) } - for i := range s { + for k, v := range m { // nope, LHS and RHS differ + v, k := k, v + go f(k) + go f(v) + } + for k, v := range m { // nope, not a simple redecl + k, v, x := k, v, 1 + go f(k) + go f(v) + go f(x) + } + for i := range s { // nope, not a simple redecl i := (i) go f(i) } + for i := range s { // nope, not a simple redecl + i := i + 1 + go f(i) + } } func f(n int) {} diff --git a/gopls/internal/analysis/modernize/testdata/src/forvar/forvar.go.golden b/gopls/internal/analysis/modernize/testdata/src/forvar/forvar.go.golden index 35f71404c35..6d4b0d14b92 100644 --- a/gopls/internal/analysis/modernize/testdata/src/forvar/forvar.go.golden +++ b/gopls/internal/analysis/modernize/testdata/src/forvar/forvar.go.golden @@ -12,12 +12,24 @@ func _(m map[int]int, s []int) { } for k, v := range m { // want "copying variable is unneeded" - v := v // nope: report only the first redeclaration + // want "copying variable is unneeded" + go f(k) + go f(v) + } + for k, v := range m { + // want "copying variable is unneeded" + // want "copying variable is unneeded" go f(k) go f(v) } - for _, v := range m { + for k, v := range m { // want "copying variable is unneeded" + go f(k) + go f(v) + } + for k, v := range m { + // want "copying variable is unneeded" + go f(k) go f(v) } for i := range s { @@ -53,10 +65,25 @@ func _(m map[int]int, s []int) { v := i go f(v) } - for i := range s { + for k, v := range m { // nope, LHS and RHS differ + v, k := k, v + go f(k) + go f(v) + } + for k, v := range m { // nope, not a simple redecl + k, v, x := k, v, 1 + go f(k) + go f(v) + go f(x) + } + for i := range s { // nope, not a simple redecl i := (i) go f(i) } + for i := range s { // nope, not a simple redecl + i := i + 1 + go f(i) + } } func f(n int) {} diff --git a/gopls/internal/analysis/unusedfunc/doc.go b/gopls/internal/analysis/unusedfunc/doc.go index c43d9a654be..515000282d2 100644 --- a/gopls/internal/analysis/unusedfunc/doc.go +++ b/gopls/internal/analysis/unusedfunc/doc.go @@ -23,7 +23,7 @@ // The tool may report false positives in some situations, for // example: // -// - For a declaration of an unexported function that is referenced +// - for a declaration of an unexported function that is referenced // from another package using the go:linkname mechanism, if the // declaration's doc comment does not also have a go:linkname // comment. @@ -32,17 +32,19 @@ // annotations, if they must be used at all, should be used on both // the declaration and the alias.) // -// - For compiler intrinsics in the "runtime" package that, though +// - for compiler intrinsics in the "runtime" package that, though // never referenced, are known to the compiler and are called // indirectly by compiled object code. // -// - For functions called only from assembly. +// - for functions called only from assembly. // -// - For functions called only from files whose build tags are not +// - for functions called only from files whose build tags are not // selected in the current build configuration. // -// See https://github.com/golang/go/issues/71686 for discussion of -// these limitations. +// Since these situations are relatively common in the low-level parts +// of the runtime, this analyzer ignores the standard library. +// See https://go.dev/issue/71686 and https://go.dev/issue/74130 for +// further discussion of these limitations. // // The unusedfunc algorithm is not as precise as the // golang.org/x/tools/cmd/deadcode tool, but it has the advantage that diff --git a/gopls/internal/analysis/unusedfunc/testdata/src/a/a.go b/gopls/internal/analysis/unusedfunc/testdata/basic.txtar similarity index 60% rename from gopls/internal/analysis/unusedfunc/testdata/src/a/a.go rename to gopls/internal/analysis/unusedfunc/testdata/basic.txtar index 45f5176deab..0d7506727b3 100644 --- a/gopls/internal/analysis/unusedfunc/testdata/src/a/a.go +++ b/gopls/internal/analysis/unusedfunc/testdata/basic.txtar @@ -1,3 +1,11 @@ +Basic test of unusedfunc. + +-- go.mod -- +module example.com + +go 1.21 + +-- a/a.go -- package a func main() { @@ -70,3 +78,60 @@ const ( constOne = 1 unusedConstTwo = constOne // want `const "unusedConstTwo" is unused` ) + +-- a/a.go.golden -- +package a + +func main() { + _ = live +} + +// -- functions -- + +func Exported() {} + +func live() {} + +//go:linkname foo +func apparentlyDeadButHasPrecedingLinknameComment() {} + +// -- methods -- + +type ExportedType int +type unexportedType int + +func (ExportedType) Exported() {} +func (unexportedType) Exported() {} + +func (x ExportedType) dynamic() {} // matches name of interface method => live + +type _ interface{ dynamic() } + + +// -- types without methods -- + +type ExportedType2 int + +// want `type "unusedUnexportedType2" is unused` + +type ( + one int +) + +// -- generic methods -- + +type g[T any] int + +// want `method "method" is unused` + +// -- constants -- + +// want `const "unusedConst" is unused` + +const ( + unusedEnum = iota +) + +const ( + constOne = 1 +) diff --git a/gopls/internal/analysis/unusedfunc/testdata/src/a/a.go.golden b/gopls/internal/analysis/unusedfunc/testdata/src/a/a.go.golden deleted file mode 100644 index 4e9c8fbfdc5..00000000000 --- a/gopls/internal/analysis/unusedfunc/testdata/src/a/a.go.golden +++ /dev/null @@ -1,55 +0,0 @@ -package a - -func main() { - _ = live -} - -// -- functions -- - -func Exported() {} - -func live() {} - -//go:linkname foo -func apparentlyDeadButHasPrecedingLinknameComment() {} - -// -- methods -- - -type ExportedType int -type unexportedType int - -func (ExportedType) Exported() {} -func (unexportedType) Exported() {} - -func (x ExportedType) dynamic() {} // matches name of interface method => live - -type _ interface{ dynamic() } - - -// -- types without methods -- - -type ExportedType2 int - -// want `type "unusedUnexportedType2" is unused` - -type ( - one int -) - -// -- generic methods -- - -type g[T any] int - -// want `method "method" is unused` - -// -- constants -- - -// want `const "unusedConst" is unused` - -const ( - unusedEnum = iota -) - -const ( - constOne = 1 -) diff --git a/gopls/internal/analysis/unusedfunc/unusedfunc.go b/gopls/internal/analysis/unusedfunc/unusedfunc.go index 02ab7c9fade..0bf738ee326 100644 --- a/gopls/internal/analysis/unusedfunc/unusedfunc.go +++ b/gopls/internal/analysis/unusedfunc/unusedfunc.go @@ -52,7 +52,7 @@ import ( // // Types (sans methods), constants, and vars are more straightforward. // For now we ignore enums (const decls using iota) since it is -// commmon for at least some values to be unused when they are added +// common for at least some values to be unused when they are added // for symmetry, future use, or to conform to some external pattern. //go:embed doc.go @@ -67,6 +67,12 @@ var Analyzer = &analysis.Analyzer{ } func run(pass *analysis.Pass) (any, error) { + // The standard library makes heavy use of intrinsics, linknames, etc, + // that confuse this algorithm; so skip it (#74130). + if analysisinternal.IsStdPackage(pass.Pkg.Path()) { + return nil, nil + } + var ( inspect = pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) index = pass.ResultOf[typeindexanalyzer.Analyzer].(*typeindex.Index) diff --git a/gopls/internal/analysis/unusedfunc/unusedfunc_test.go b/gopls/internal/analysis/unusedfunc/unusedfunc_test.go index 1bf73da3653..db117b1e801 100644 --- a/gopls/internal/analysis/unusedfunc/unusedfunc_test.go +++ b/gopls/internal/analysis/unusedfunc/unusedfunc_test.go @@ -5,13 +5,15 @@ package unusedfunc_test import ( + "path/filepath" "testing" "golang.org/x/tools/go/analysis/analysistest" "golang.org/x/tools/gopls/internal/analysis/unusedfunc" + "golang.org/x/tools/internal/testfiles" ) func Test(t *testing.T) { - testdata := analysistest.TestData() - analysistest.RunWithSuggestedFixes(t, testdata, unusedfunc.Analyzer, "a") + dir := testfiles.ExtractTxtarFileToTmp(t, filepath.Join(analysistest.TestData(), "basic.txtar")) + analysistest.RunWithSuggestedFixes(t, dir, unusedfunc.Analyzer, "example.com/a") } diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index b654833e08c..17ed8c9958e 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -1064,7 +1064,7 @@ func (act *action) exec(ctx context.Context) (any, *actionSummary, error) { // Read file from snapshot, to ensure reads are consistent. // // TODO(adonovan): make the dependency analysis sound by - // incorporating these additional files into the the analysis + // incorporating these additional files into the analysis // hash. This requires either (a) preemptively reading and // hashing a potentially large number of mostly irrelevant // files; or (b) some kind of dynamic dependency discovery diff --git a/gopls/internal/cache/imports.go b/gopls/internal/cache/imports.go index 735801f2345..fe8f853d09b 100644 --- a/gopls/internal/cache/imports.go +++ b/gopls/internal/cache/imports.go @@ -7,8 +7,8 @@ package cache import ( "context" "fmt" + "log" "sync" - "testing" "time" "golang.org/x/tools/gopls/internal/file" @@ -150,66 +150,55 @@ func newImportsState(backgroundCtx context.Context, modCache *sharedModCache, en // modcacheState holds a modindex.Index and controls its updates type modcacheState struct { - dir string // GOMODCACHE + gomodcache string refreshTimer *refreshTimer - mu sync.Mutex - index *modindex.Index + + // (index, indexErr) is zero in the initial state. + // Thereafter they hold the memoized result pair for getIndex. + mu sync.Mutex + index *modindex.Index + indexErr error } // newModcacheState constructs a new modcacheState for goimports. // The returned state is automatically updated until [modcacheState.stopTimer] is called. -func newModcacheState(dir string) *modcacheState { +func newModcacheState(gomodcache string) *modcacheState { s := &modcacheState{ - dir: dir, + gomodcache: gomodcache, } - s.index, _ = modindex.ReadIndex(dir) s.refreshTimer = newRefreshTimer(s.refreshIndex) go s.refreshIndex() return s } -// getIndex reads the module cache index. It might not exist yet -// inside tests. It might contain no Entries if the cache -// is empty. +// getIndex reads the module cache index. func (s *modcacheState) getIndex() (*modindex.Index, error) { s.mu.Lock() defer s.mu.Unlock() - ix := s.index - if ix == nil || len(ix.Entries) == 0 { - var err error - // this should only happen near the beginning of a session - // (or in tests) - ix, err = modindex.ReadIndex(s.dir) - if err != nil { - return nil, fmt.Errorf("ReadIndex %w", err) - } - if !testing.Testing() { - return ix, nil - } - if ix == nil || len(ix.Entries) == 0 { - err = modindex.Create(s.dir) - if err != nil { - return nil, fmt.Errorf("creating index %w", err) - } - ix, err = modindex.ReadIndex(s.dir) - if err != nil { - return nil, fmt.Errorf("read index after create %w", err) - } - s.index = ix - } + + if s.index == nil && s.indexErr == nil { + // getIndex was called before refreshIndex finished. + // Read, but don't update, whatever index is present. + s.index, s.indexErr = modindex.Read(s.gomodcache) } - return s.index, nil + + return s.index, s.indexErr } func (s *modcacheState) refreshIndex() { - ok, err := modindex.Update(s.dir) - if err != nil || !ok { - return - } - // read the new index + index, err := modindex.Update(s.gomodcache) s.mu.Lock() - defer s.mu.Unlock() - s.index, _ = modindex.ReadIndex(s.dir) + if err != nil { + if s.indexErr != nil { + s.indexErr = err // prefer most recent error + } else { + // Keep using stale s.index (if any). + log.Printf("modcacheState.refreshIndex: %v", err) + } + } else { + s.index, s.indexErr = index, nil // success + } + s.mu.Unlock() } func (s *modcacheState) stopTimer() { diff --git a/gopls/internal/cache/metadata/metadata.go b/gopls/internal/cache/metadata/metadata.go index 81b6dc57e1f..ae847d579be 100644 --- a/gopls/internal/cache/metadata/metadata.go +++ b/gopls/internal/cache/metadata/metadata.go @@ -151,7 +151,7 @@ func (mp *Package) String() string { return string(mp.ID) } // intermediate test variant of p, of which there could be many. // Good code doesn't rely on such trickery. // -// Most callers of MetadataForFile call RemoveIntermediateTestVariants +// Most callers of MetadataForFile set removeIntermediateTestVariants parameter // to discard them before requesting type checking, or the products of // type-checking such as the cross-reference index or method set index. // diff --git a/gopls/internal/cache/mod.go b/gopls/internal/cache/mod.go index aa4982489dd..35a796dc9e5 100644 --- a/gopls/internal/cache/mod.go +++ b/gopls/internal/cache/mod.go @@ -509,7 +509,7 @@ func ResolvedVersion(module *packages.Module) string { return module.Version } -// ResolvedPath returns the the module path, which considers replace directive. +// ResolvedPath returns the module path, which considers replace directive. func ResolvedPath(module *packages.Module) string { if module.Replace != nil { return module.Replace.Path diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go index dc5c8e932ca..9c8568e508e 100644 --- a/gopls/internal/cache/snapshot.go +++ b/gopls/internal/cache/snapshot.go @@ -6,6 +6,7 @@ package cache import ( "bytes" + "cmp" "context" "errors" "fmt" @@ -18,7 +19,6 @@ import ( "path/filepath" "regexp" "slices" - "sort" "strconv" "strings" "sync" @@ -652,11 +652,10 @@ func (s *Snapshot) Tests(ctx context.Context, ids ...PackageID) ([]*testfuncs.In // (the one with the fewest files) that encloses the specified file. // The result may be a test variant, but never an intermediate test variant. func (snapshot *Snapshot) NarrowestMetadataForFile(ctx context.Context, uri protocol.DocumentURI) (*metadata.Package, error) { - mps, err := snapshot.MetadataForFile(ctx, uri) + mps, err := snapshot.MetadataForFile(ctx, uri, true) if err != nil { return nil, err } - metadata.RemoveIntermediateTestVariants(&mps) if len(mps) == 0 { return nil, fmt.Errorf("no package metadata for file %s", uri) } @@ -668,13 +667,10 @@ func (snapshot *Snapshot) NarrowestMetadataForFile(ctx context.Context, uri prot // number of CompiledGoFiles (i.e. "narrowest" to "widest" package), // and secondarily by IsIntermediateTestVariant (false < true). // The result may include tests and intermediate test variants of -// importable packages. +// importable packages. If removeIntermediateTestVariants is provided, +// intermediate test variants will be excluded. // It returns an error if the context was cancelled. -// -// TODO(adonovan): in nearly all cases the caller must use -// [metadata.RemoveIntermediateTestVariants]. Make this a parameter to -// force the caller to consider it (and reduce code). -func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI) ([]*metadata.Package, error) { +func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI, removeIntermediateTestVariants bool) ([]*metadata.Package, error) { if s.view.typ == AdHocView { // As described in golang/go#57209, in ad-hoc workspaces (where we load ./ // rather than ./...), preempting the directory load with file loads can @@ -712,7 +708,6 @@ func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI scope := fileLoadScope(uri) err := s.load(ctx, NoNetwork, scope) - // // Return the context error here as the current operation is no longer // valid. if err != nil { @@ -752,23 +747,42 @@ func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI s.unloadableFiles.Add(uri) } + if removeIntermediateTestVariants { + metadata.RemoveIntermediateTestVariants(&metas) + } + // Sort packages "narrowest" to "widest" (in practice: // non-tests before tests), and regular packages before // their intermediate test variants (which have the same // files but different imports). - sort.Slice(metas, func(i, j int) bool { - x, y := metas[i], metas[j] - xfiles, yfiles := len(x.CompiledGoFiles), len(y.CompiledGoFiles) - if xfiles != yfiles { - return xfiles < yfiles + slices.SortFunc(metas, func(x, y *metadata.Package) int { + if sign := cmp.Compare(len(x.CompiledGoFiles), len(y.CompiledGoFiles)); sign != 0 { + return sign + } + // Skip ITV-specific ordering if they were removed. + if removeIntermediateTestVariants { + return 0 } - return boolLess(x.IsIntermediateTestVariant(), y.IsIntermediateTestVariant()) + return boolCompare(x.IsIntermediateTestVariant(), y.IsIntermediateTestVariant()) }) return metas, nil } -func boolLess(x, y bool) bool { return !x && y } // false < true +// btoi returns int(b) as proposed in #64825. +func btoi(b bool) int { + if b { + return 1 + } else { + return 0 + } +} + +// boolCompare is a comparison function for booleans, returning -1 if x < y, 0 +// if x == y, and 1 if x > y, where false < true. +func boolCompare(x, y bool) int { + return btoi(x) - btoi(y) +} // ReverseDependencies returns a new mapping whose entries are // the ID and Metadata of each package in the workspace that @@ -1252,7 +1266,7 @@ searchOverlays: if s.IsBuiltin(uri) || s.FileKind(o) != file.Go { continue } - mps, err := s.MetadataForFile(ctx, uri) + mps, err := s.MetadataForFile(ctx, uri, true) if err != nil { return nil, err } @@ -1261,7 +1275,6 @@ searchOverlays: continue searchOverlays } } - metadata.RemoveIntermediateTestVariants(&mps) // With zero-config gopls (golang/go#57979), orphaned file diagnostics // include diagnostics for orphaned files -- not just diagnostics relating @@ -1341,6 +1354,7 @@ searchOverlays: if s.view.folder.Env.GoVersion >= 18 { if s.view.gowork != "" { fix = fmt.Sprintf("To fix this problem, you can add this module to your go.work file (%s)", s.view.gowork) + cmd := command.NewRunGoWorkCommandCommand("Run `go work use`", command.RunGoWorkArgs{ ViewID: s.view.ID(), Args: []string{"use", modDir}, diff --git a/gopls/internal/cache/source.go b/gopls/internal/cache/source.go index 9d1ecdb440d..6dc43ffc459 100644 --- a/gopls/internal/cache/source.go +++ b/gopls/internal/cache/source.go @@ -21,7 +21,7 @@ import ( // goplsSource is an imports.Source that provides import information using // gopls and the module cache index. type goplsSource struct { - S *Snapshot + snapshot *Snapshot envSource *imports.ProcessEnvSource // set by each invocation of ResolveReferences @@ -30,7 +30,7 @@ type goplsSource struct { func (s *Snapshot) NewGoplsSource(is *imports.ProcessEnvSource) *goplsSource { return &goplsSource{ - S: s, + snapshot: s, envSource: is, } } @@ -110,7 +110,7 @@ func (s *goplsSource) ResolveReferences(ctx context.Context, filename string, mi dbgpr("fromWS", fromWS) dbgpr("old", old) - for k, v := range s.S.workspacePackages.All() { + for k, v := range s.snapshot.workspacePackages.All() { log.Printf("workspacePackages[%s]=%s", k, v) } // anything in ans with >1 matches? @@ -134,7 +134,7 @@ func (s *goplsSource) ResolveReferences(ctx context.Context, filename string, mi } func (s *goplsSource) resolveCacheReferences(missing imports.References) ([]*result, error) { - ix, err := s.S.view.ModcacheIndex() + ix, err := s.snapshot.view.ModcacheIndex() if err != nil { return nil, err } @@ -176,7 +176,7 @@ type found struct { func (s *goplsSource) resolveWorkspaceReferences(filename string, missing imports.References) ([]*imports.Result, error) { uri := protocol.URIFromPath(filename) - mypkgs, err := s.S.MetadataForFile(s.ctx, uri) + mypkgs, err := s.snapshot.MetadataForFile(s.ctx, uri, false) if err != nil { return nil, err } @@ -185,7 +185,7 @@ func (s *goplsSource) resolveWorkspaceReferences(filename string, missing import } mypkg := mypkgs[0] // narrowest package // search the metadata graph for package ids correstponding to missing - g := s.S.MetadataGraph() + g := s.snapshot.MetadataGraph() var ids []metadata.PackageID var pkgs []*metadata.Package for pid, pkg := range g.Packages { @@ -200,7 +200,7 @@ func (s *goplsSource) resolveWorkspaceReferences(filename string, missing import } // find the symbols in those packages // the syms occur in the same order as the ids and the pkgs - syms, err := s.S.Symbols(s.ctx, ids...) + syms, err := s.snapshot.Symbols(s.ctx, ids...) if err != nil { return nil, err } @@ -331,12 +331,12 @@ func (s *goplsSource) bestCache(nm string, got []*result) *imports.Result { func (s *goplsSource) fromGoMod(got []*result) *imports.Result { // should we use s.S.view.worsspaceModFiles, and the union of their requires? // (note that there are no tests where it contains more than one) - modURI := s.S.view.gomod - modfh, ok := s.S.files.get(modURI) + modURI := s.snapshot.view.gomod + modfh, ok := s.snapshot.files.get(modURI) if !ok { return nil } - parsed, err := s.S.ParseMod(s.ctx, modfh) + parsed, err := s.snapshot.ParseMod(s.ctx, modfh) if err != nil { return nil } diff --git a/gopls/internal/cmd/mcp.go b/gopls/internal/cmd/mcp.go index c97150ab052..468dbaaca43 100644 --- a/gopls/internal/cmd/mcp.go +++ b/gopls/internal/cmd/mcp.go @@ -23,9 +23,10 @@ import ( type headlessMCP struct { app *Application - Address string `flag:"listen" help:"the address on which to run the mcp server"` - Logfile string `flag:"logfile" help:"filename to log to; if unset, logs to stderr"` - RPCTrace bool `flag:"rpc.trace" help:"print MCP rpc traces; cannot be used with -listen"` + Address string `flag:"listen" help:"the address on which to run the mcp server"` + Logfile string `flag:"logfile" help:"filename to log to; if unset, logs to stderr"` + RPCTrace bool `flag:"rpc.trace" help:"print MCP rpc traces; cannot be used with -listen"` + Instructions bool `flag:"instructions" help:"if set, print gopls' MCP instructions and exit"` } func (m *headlessMCP) Name() string { return "mcp" } @@ -46,6 +47,10 @@ Examples: } func (m *headlessMCP) Run(ctx context.Context, args ...string) error { + if m.Instructions { + fmt.Println(mcp.Instructions) + return nil + } if m.Address != "" && m.RPCTrace { // There's currently no way to plumb logging instrumentation into the SSE // transport that is created on connections to the HTTP handler, so we must diff --git a/gopls/internal/cmd/mcp_test.go b/gopls/internal/cmd/mcp_test.go index 5fa3b48caa7..bdbc36ac951 100644 --- a/gopls/internal/cmd/mcp_test.go +++ b/gopls/internal/cmd/mcp_test.go @@ -12,6 +12,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strconv" "strings" "testing" @@ -23,6 +24,10 @@ import ( func TestMCPCommandStdio(t *testing.T) { // Test that the headless MCP subcommand works, and recognizes file changes. + if !(supportsFsnotify(runtime.GOOS)) { + // See golang/go#74580 + t.Skipf(`skipping on %s; fsnotify is not supported`, runtime.GOOS) + } testenv.NeedsExec(t) // stdio transport uses execve(2) tree := writeTree(t, ` -- go.mod -- @@ -95,6 +100,10 @@ const B = 2 func TestMCPCommandLogging(t *testing.T) { // Test that logging flags for headless MCP subcommand work as intended. + if !(supportsFsnotify(runtime.GOOS)) { + // See golang/go#74580 + t.Skipf(`skipping on %s; fsnotify is not supported`, runtime.GOOS) + } testenv.NeedsExec(t) // stdio transport uses execve(2) tests := []struct { @@ -155,6 +164,10 @@ package p } func TestMCPCommandHTTP(t *testing.T) { + if !(supportsFsnotify(runtime.GOOS)) { + // See golang/go#74580 + t.Skipf(`skipping on %s; fsnotify is not supported`, runtime.GOOS) + } testenv.NeedsExec(t) tree := writeTree(t, ` -- go.mod -- @@ -271,3 +284,8 @@ func getRandomPort() int { defer listener.Close() return listener.Addr().(*net.TCPAddr).Port } + +// supportsFsnotify returns true if fsnotify supports the os. +func supportsFsnotify(os string) bool { + return os == "darwin" || os == "linux" || os == "windows" +} diff --git a/gopls/internal/cmd/serve.go b/gopls/internal/cmd/serve.go index 4a56da179af..c31e225dd56 100644 --- a/gopls/internal/cmd/serve.go +++ b/gopls/internal/cmd/serve.go @@ -41,7 +41,7 @@ type Serve struct { RemoteLogfile string `flag:"remote.logfile" help:"when used with -remote=auto, the -logfile value used to start the daemon"` // MCP Server related configurations. - MCPAddress string `flag:"mcp-listen" help:"experimental: address on which to listen for model context protocol connections. If port is localhost:0, pick a random port in localhost instead."` + MCPAddress string `flag:"mcp.listen" help:"experimental: address on which to listen for model context protocol connections. If port is localhost:0, pick a random port in localhost instead."` app *Application } diff --git a/gopls/internal/cmd/usage/mcp.hlp b/gopls/internal/cmd/usage/mcp.hlp index 7d497d51dcb..8805d56c0a3 100644 --- a/gopls/internal/cmd/usage/mcp.hlp +++ b/gopls/internal/cmd/usage/mcp.hlp @@ -9,6 +9,8 @@ Starts the server over stdio or sse with http, depending on whether the listen f Examples: $ gopls mcp -listen=localhost:3000 $ gopls mcp //start over stdio + -instructions + if set, print gopls' MCP instructions and exit -listen=string the address on which to run the mcp server -logfile=string diff --git a/gopls/internal/cmd/usage/serve.hlp b/gopls/internal/cmd/usage/serve.hlp index 26c3d540ee0..c72852d0e7f 100644 --- a/gopls/internal/cmd/usage/serve.hlp +++ b/gopls/internal/cmd/usage/serve.hlp @@ -16,7 +16,7 @@ server-flags: when used with -listen, shut down the server when there are no connected clients for this duration -logfile=string filename to log to. if value is "auto", then logging to a default output file is enabled - -mcp-listen=string + -mcp.listen=string experimental: address on which to listen for model context protocol connections. If port is localhost:0, pick a random port in localhost instead. -mode=string no effect diff --git a/gopls/internal/cmd/usage/usage-v.hlp b/gopls/internal/cmd/usage/usage-v.hlp index 8587507b956..07f6737493e 100644 --- a/gopls/internal/cmd/usage/usage-v.hlp +++ b/gopls/internal/cmd/usage/usage-v.hlp @@ -60,7 +60,7 @@ flags: when used with -listen, shut down the server when there are no connected clients for this duration -logfile=string filename to log to. if value is "auto", then logging to a default output file is enabled - -mcp-listen=string + -mcp.listen=string experimental: address on which to listen for model context protocol connections. If port is localhost:0, pick a random port in localhost instead. -mode=string no effect diff --git a/gopls/internal/cmd/usage/usage.hlp b/gopls/internal/cmd/usage/usage.hlp index cd9b156cfbd..7a22b200371 100644 --- a/gopls/internal/cmd/usage/usage.hlp +++ b/gopls/internal/cmd/usage/usage.hlp @@ -57,7 +57,7 @@ flags: when used with -listen, shut down the server when there are no connected clients for this duration -logfile=string filename to log to. if value is "auto", then logging to a default output file is enabled - -mcp-listen=string + -mcp.listen=string experimental: address on which to listen for model context protocol connections. If port is localhost:0, pick a random port in localhost instead. -mode=string no effect diff --git a/gopls/internal/debug/flight.go b/gopls/internal/debug/flight.go index bec5f892991..ee3dec5a56c 100644 --- a/gopls/internal/debug/flight.go +++ b/gopls/internal/debug/flight.go @@ -17,9 +17,29 @@ import ( "runtime/trace" "strings" "sync" + "syscall" "time" ) +var ( + traceviewersMu sync.Mutex + traceviewers []*os.Process + + kill = (*os.Process).Kill // windows, plan9; UNIX impl kills whole process group + sysProcAttr syscall.SysProcAttr // UNIX configuration to create process group +) + +// KillTraceViewers kills all "go tool trace" processes started by +// /flightrecorder requests, for use in tests (see #74668). +func KillTraceViewers() { + traceviewersMu.Lock() + for _, p := range traceviewers { + kill(p) // ignore error + } + traceviewers = nil + traceviewersMu.Unlock() +} + // The FlightRecorder is a global resource, so create at most one per process. var getRecorder = sync.OnceValues(func() (*trace.FlightRecorder, error) { fr := trace.NewFlightRecorder(trace.FlightRecorderConfig{ @@ -53,7 +73,7 @@ func startFlightRecorder() (http.HandlerFunc, error) { return } if _, err := fr.WriteTo(f); err != nil { - f.Close() + f.Close() // ignore error errorf("failed to write flight record: %s", err) return } @@ -71,6 +91,7 @@ func startFlightRecorder() (http.HandlerFunc, error) { // web server process. It will run until gopls terminates. // (It would be nicer if we could just link it in; see #66843.) cmd := exec.Command("go", "tool", "trace", tracefile) + cmd.SysProcAttr = &sysProcAttr // Don't connect trace's std{out,err} to our os.Stderr directly, // otherwise the child may outlive the parent in tests, @@ -113,6 +134,11 @@ func startFlightRecorder() (http.HandlerFunc, error) { return } + // Save the process so we can kill it when tests finish. + traceviewersMu.Lock() + traceviewers = append(traceviewers, cmd.Process) + traceviewersMu.Unlock() + // Some of the CI builders can be quite heavily loaded. // Give them an extra grace period. timeout := 10 * time.Second diff --git a/gopls/internal/debug/flight_go124.go b/gopls/internal/debug/flight_go124.go index 807fa11093e..6b70dc971c7 100644 --- a/gopls/internal/debug/flight_go124.go +++ b/gopls/internal/debug/flight_go124.go @@ -14,3 +14,5 @@ import ( func startFlightRecorder() (http.HandlerFunc, error) { return nil, errors.ErrUnsupported } + +func KillTraceViewers() {} diff --git a/gopls/internal/debug/flight_unix.go b/gopls/internal/debug/flight_unix.go new file mode 100644 index 00000000000..e65fd442f95 --- /dev/null +++ b/gopls/internal/debug/flight_unix.go @@ -0,0 +1,23 @@ +// Copyright 2025 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.25 && unix + +package debug + +import ( + "os" + "syscall" +) + +func init() { + // UNIX: kill the whole process group, since + // "go tool trace" starts a cmd/trace child. + kill = killGroup + sysProcAttr.Setpgid = true +} + +func killGroup(p *os.Process) error { + return syscall.Kill(-p.Pid, syscall.SIGKILL) +} diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json index 65cbbd303b5..c0557632534 100644 --- a/gopls/internal/doc/api.json +++ b/gopls/internal/doc/api.json @@ -1648,7 +1648,7 @@ }, { "Name": "\"unusedfunc\"", - "Doc": "check for unused functions, methods, etc\n\nThe unusedfunc analyzer reports functions and methods that are\nnever referenced outside of their own declaration.\n\nA function is considered unused if it is unexported and not\nreferenced (except within its own declaration).\n\nA method is considered unused if it is unexported, not referenced\n(except within its own declaration), and its name does not match\nthat of any method of an interface type declared within the same\npackage.\n\nThe tool may report false positives in some situations, for\nexample:\n\n - For a declaration of an unexported function that is referenced\n from another package using the go:linkname mechanism, if the\n declaration's doc comment does not also have a go:linkname\n comment.\n\n (Such code is in any case strongly discouraged: linkname\n annotations, if they must be used at all, should be used on both\n the declaration and the alias.)\n\n - For compiler intrinsics in the \"runtime\" package that, though\n never referenced, are known to the compiler and are called\n indirectly by compiled object code.\n\n - For functions called only from assembly.\n\n - For functions called only from files whose build tags are not\n selected in the current build configuration.\n\nSee https://github.com/golang/go/issues/71686 for discussion of\nthese limitations.\n\nThe unusedfunc algorithm is not as precise as the\ngolang.org/x/tools/cmd/deadcode tool, but it has the advantage that\nit runs within the modular analysis framework, enabling near\nreal-time feedback within gopls.\n\nThe unusedfunc analyzer also reports unused types, vars, and\nconstants. Enums--constants defined with iota--are ignored since\neven the unused values must remain present to preserve the logical\nordering.", + "Doc": "check for unused functions, methods, etc\n\nThe unusedfunc analyzer reports functions and methods that are\nnever referenced outside of their own declaration.\n\nA function is considered unused if it is unexported and not\nreferenced (except within its own declaration).\n\nA method is considered unused if it is unexported, not referenced\n(except within its own declaration), and its name does not match\nthat of any method of an interface type declared within the same\npackage.\n\nThe tool may report false positives in some situations, for\nexample:\n\n - for a declaration of an unexported function that is referenced\n from another package using the go:linkname mechanism, if the\n declaration's doc comment does not also have a go:linkname\n comment.\n\n (Such code is in any case strongly discouraged: linkname\n annotations, if they must be used at all, should be used on both\n the declaration and the alias.)\n\n - for compiler intrinsics in the \"runtime\" package that, though\n never referenced, are known to the compiler and are called\n indirectly by compiled object code.\n\n - for functions called only from assembly.\n\n - for functions called only from files whose build tags are not\n selected in the current build configuration.\n\nSince these situations are relatively common in the low-level parts\nof the runtime, this analyzer ignores the standard library.\nSee https://go.dev/issue/71686 and https://go.dev/issue/74130 for\nfurther discussion of these limitations.\n\nThe unusedfunc algorithm is not as precise as the\ngolang.org/x/tools/cmd/deadcode tool, but it has the advantage that\nit runs within the modular analysis framework, enabling near\nreal-time feedback within gopls.\n\nThe unusedfunc analyzer also reports unused types, vars, and\nconstants. Enums--constants defined with iota--are ignored since\neven the unused values must remain present to preserve the logical\nordering.", "Default": "true", "Status": "" }, @@ -3386,7 +3386,7 @@ }, { "Name": "unusedfunc", - "Doc": "check for unused functions, methods, etc\n\nThe unusedfunc analyzer reports functions and methods that are\nnever referenced outside of their own declaration.\n\nA function is considered unused if it is unexported and not\nreferenced (except within its own declaration).\n\nA method is considered unused if it is unexported, not referenced\n(except within its own declaration), and its name does not match\nthat of any method of an interface type declared within the same\npackage.\n\nThe tool may report false positives in some situations, for\nexample:\n\n - For a declaration of an unexported function that is referenced\n from another package using the go:linkname mechanism, if the\n declaration's doc comment does not also have a go:linkname\n comment.\n\n (Such code is in any case strongly discouraged: linkname\n annotations, if they must be used at all, should be used on both\n the declaration and the alias.)\n\n - For compiler intrinsics in the \"runtime\" package that, though\n never referenced, are known to the compiler and are called\n indirectly by compiled object code.\n\n - For functions called only from assembly.\n\n - For functions called only from files whose build tags are not\n selected in the current build configuration.\n\nSee https://github.com/golang/go/issues/71686 for discussion of\nthese limitations.\n\nThe unusedfunc algorithm is not as precise as the\ngolang.org/x/tools/cmd/deadcode tool, but it has the advantage that\nit runs within the modular analysis framework, enabling near\nreal-time feedback within gopls.\n\nThe unusedfunc analyzer also reports unused types, vars, and\nconstants. Enums--constants defined with iota--are ignored since\neven the unused values must remain present to preserve the logical\nordering.", + "Doc": "check for unused functions, methods, etc\n\nThe unusedfunc analyzer reports functions and methods that are\nnever referenced outside of their own declaration.\n\nA function is considered unused if it is unexported and not\nreferenced (except within its own declaration).\n\nA method is considered unused if it is unexported, not referenced\n(except within its own declaration), and its name does not match\nthat of any method of an interface type declared within the same\npackage.\n\nThe tool may report false positives in some situations, for\nexample:\n\n - for a declaration of an unexported function that is referenced\n from another package using the go:linkname mechanism, if the\n declaration's doc comment does not also have a go:linkname\n comment.\n\n (Such code is in any case strongly discouraged: linkname\n annotations, if they must be used at all, should be used on both\n the declaration and the alias.)\n\n - for compiler intrinsics in the \"runtime\" package that, though\n never referenced, are known to the compiler and are called\n indirectly by compiled object code.\n\n - for functions called only from assembly.\n\n - for functions called only from files whose build tags are not\n selected in the current build configuration.\n\nSince these situations are relatively common in the low-level parts\nof the runtime, this analyzer ignores the standard library.\nSee https://go.dev/issue/71686 and https://go.dev/issue/74130 for\nfurther discussion of these limitations.\n\nThe unusedfunc algorithm is not as precise as the\ngolang.org/x/tools/cmd/deadcode tool, but it has the advantage that\nit runs within the modular analysis framework, enabling near\nreal-time feedback within gopls.\n\nThe unusedfunc analyzer also reports unused types, vars, and\nconstants. Enums--constants defined with iota--are ignored since\neven the unused values must remain present to preserve the logical\nordering.", "URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/unusedfunc", "Default": true }, diff --git a/gopls/internal/filewatcher/export_test.go b/gopls/internal/filewatcher/export_test.go new file mode 100644 index 00000000000..b3b8a0d7b30 --- /dev/null +++ b/gopls/internal/filewatcher/export_test.go @@ -0,0 +1,13 @@ +// Copyright 2025 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 filewatcher + +// This file defines things (and opens backdoors) needed only by tests. + +// SetAfterAddHook sets a hook to be called after a path is added to the watcher. +// This is used in tests to inspect the error returned by the underlying watcher. +func SetAfterAddHook(f func(string, error)) { + afterAddHook = f +} diff --git a/gopls/internal/filewatcher/filewatcher.go b/gopls/internal/filewatcher/filewatcher.go index a6ad08be87e..f35e0c65e42 100644 --- a/gopls/internal/filewatcher/filewatcher.go +++ b/gopls/internal/filewatcher/filewatcher.go @@ -5,6 +5,7 @@ package filewatcher import ( + "errors" "io/fs" "log/slog" "os" @@ -17,6 +18,9 @@ import ( "golang.org/x/tools/gopls/internal/protocol" ) +// ErrClosed is used when trying to operate on a closed Watcher. +var ErrClosed = errors.New("file watcher: watcher already closed") + // Watcher collects events from a [fsnotify.Watcher] and converts them into // batched LSP [protocol.FileEvent]s. type Watcher struct { @@ -24,15 +28,25 @@ type Watcher struct { stop chan struct{} // closed by Close to terminate run loop - wg sync.WaitGroup // counts number of active run goroutines (max 1) + // errs is an internal channel for surfacing errors from the file watcher, + // distinct from the fsnotify watcher's error channel. + errs chan error + + runners sync.WaitGroup // counts the number of active run goroutines (max 1) watcher *fsnotify.Watcher mu sync.Mutex // guards all fields below - // watchedDirs keeps track of which directories are being watched by the - // watcher, explicitly added via [fsnotify.Watcher.Add]. - watchedDirs map[string]bool + // watchers counts the number of active watch registration goroutines, + // including their error handling. + // After [Watcher.Close] called, watchers's counter will no longer increase. + watchers sync.WaitGroup + + // dirCancel maps a directory path to its cancellation channel. + // A nil map indicates the watcher is closing and prevents new directory + // watch registrations. + dirCancel map[string]chan struct{} // events is the current batch of unsent file events, which will be sent // when the timer expires. @@ -51,13 +65,14 @@ func New(delay time.Duration, logger *slog.Logger, handler func([]protocol.FileE return nil, err } w := &Watcher{ - logger: logger, - watcher: watcher, - watchedDirs: make(map[string]bool), - stop: make(chan struct{}), + logger: logger, + watcher: watcher, + dirCancel: make(map[string]chan struct{}), + errs: make(chan error), + stop: make(chan struct{}), } - w.wg.Add(1) + w.runners.Add(1) go w.run(delay, handler) return w, nil @@ -66,7 +81,7 @@ func New(delay time.Duration, logger *slog.Logger, handler func([]protocol.FileE // run is the main event-handling loop for the watcher. It should be run in a // separate goroutine. func (w *Watcher) run(delay time.Duration, handler func([]protocol.FileEvent, error)) { - defer w.wg.Done() + defer w.runners.Done() // timer is used to debounce events. timer := time.NewTimer(delay) @@ -78,13 +93,23 @@ func (w *Watcher) run(delay time.Duration, handler func([]protocol.FileEvent, er return case <-timer.C: - w.sendEvents(handler) + if events := w.drainEvents(); len(events) > 0 { + handler(events, nil) + } timer.Reset(delay) case err, ok := <-w.watcher.Errors: // When the watcher is closed, its Errors channel is closed, which // unblocks this case. We continue to the next loop iteration, - // allowing the <-w.closed case to handle the shutdown. + // allowing the <-w.stop case to handle the shutdown. + if !ok { + continue + } + if err != nil { + handler(nil, err) + } + + case err, ok := <-w.errs: if !ok { continue } @@ -96,10 +121,9 @@ func (w *Watcher) run(delay time.Duration, handler func([]protocol.FileEvent, er if !ok { continue } - // file watcher should not handle the fsnotify.Event concurrently, - // the original order should be preserved. E.g. if a file get - // deleted and recreated, running concurrently may result it in - // reverse order. + // fsnotify.Event should not be handled concurrently, to preserve their + // original order. For example, if a file is deleted and recreated, + // concurrent handling could process the events in reverse order. // // Only reset the timer if a relevant event happened. // https://github.com/fsnotify/fsnotify?tab=readme-ov-file#why-do-i-get-many-chmod-events @@ -134,18 +158,24 @@ func (w *Watcher) WatchDir(path string) error { if skipDir(d.Name()) { return filepath.SkipDir } - if err := w.watchDir(path); err != nil { - // TODO(hxjiang): retry on watch failures. - return filepath.SkipDir + + done, release := w.addWatchHandle(path) + if done == nil { // file watcher closing + return filepath.SkipAll } + defer release() + + return w.watchDir(path, done) } return nil }) } -// handleEvent converts a single [fsnotify.Event] to the corresponding -// [protocol.FileEvent] and updates the watcher state. -// Returns nil if the input event is not relevant. +// handleEvent converts an [fsnotify.Event] to the corresponding [protocol.FileEvent] +// and updates the watcher state, returning nil if the event is not relevant. +// +// To avoid blocking, any required watches for new subdirectories are registered +// asynchronously in a separate goroutine. func (w *Watcher) handleEvent(event fsnotify.Event) *protocol.FileEvent { // fsnotify does not guarantee clean filepaths. path := filepath.Clean(event.Name) @@ -156,8 +186,8 @@ func (w *Watcher) handleEvent(event fsnotify.Event) *protocol.FileEvent { } else if os.IsNotExist(err) { // Upon deletion, the file/dir has been removed. fsnotify // does not provide information regarding the deleted item. - // Use the watchedDirs to determine whether it's a dir. - isDir = w.isDir(path) + // Use watchHandles to determine if the deleted item was a directory. + isDir = w.isWatchedDir(path) } else { // If statting failed, something is wrong with the file system. // Log and move on. @@ -180,19 +210,31 @@ func (w *Watcher) handleEvent(event fsnotify.Event) *protocol.FileEvent { // directory is watched. fallthrough case event.Op.Has(fsnotify.Remove): - w.unwatchDir(path) + // Upon removal, we only need to remove the entries from the map. + // The [fsnotify.Watcher] removes the watch for us. + // fsnotify/fsnotify#268 + w.removeWatchHandle(path) // TODO(hxjiang): Directory removal events from some LSP clients may // not include corresponding removal events for child files and - // subdirectories. Should we do some filtering when add the dir + // subdirectories. Should we do some filtering when adding the dir // deletion event to the events slice. return &protocol.FileEvent{ URI: protocol.URIFromPath(path), Type: protocol.Deleted, } case event.Op.Has(fsnotify.Create): - // TODO(hxjiang): retry on watch failure. - _ = w.watchDir(path) + // This watch is added asynchronously to prevent a potential + // deadlock on Windows. See fsnotify/fsnotify#502. + // Error encountered will be sent to internal error channel. + if done, release := w.addWatchHandle(path); done != nil { + go func() { + w.errs <- w.watchDir(path, done) + + // Only release after the error is sent. + release() + }() + } return &protocol.FileEvent{ URI: protocol.URIFromPath(path), @@ -233,35 +275,93 @@ func (w *Watcher) handleEvent(event fsnotify.Event) *protocol.FileEvent { } } -func (w *Watcher) watchDir(path string) error { +// watchDir registers a watch for a directory, retrying with backoff if it fails. +// It can be canceled by calling removeWatchHandle. +// Returns nil on success or cancellation; otherwise, the last error after all +// retries. +func (w *Watcher) watchDir(path string, done chan struct{}) error { + // On darwin, watching a directory will fail if it contains broken symbolic + // links. This state can occur temporarily during operations like a git + // branch switch. To handle this, we retry multiple times with exponential + // backoff, allowing time for the symbolic link's target to be created. + + // TODO(hxjiang): Address a race condition where file or directory creations + // under current directory might be missed between the current directory + // creation and the establishment of the file watch. + // + // To fix this, we should: + // 1. Retrospectively check for and trigger creation events for any new + // files/directories. + // 2. Recursively add watches for any newly created subdirectories. + var ( + delay = 500 * time.Millisecond + err error + ) + + for i := range 5 { + if i > 0 { + select { + case <-time.After(delay): + delay *= 2 + case <-done: + return nil // cancelled + } + } + // This function may block due to fsnotify/fsnotify#502. + err = w.watcher.Add(path) + if afterAddHook != nil { + afterAddHook(path, err) + } + if err == nil { + break + } + } + + return err +} + +var afterAddHook func(path string, err error) + +// addWatchHandle registers a new directory watch. +// The returned 'done' channel should be used to signal cancellation of a +// pending watch, the release function should be called once watch registration +// is done. +// It returns nil if the watcher is already closing. +func (w *Watcher) addWatchHandle(path string) (done chan struct{}, release func()) { w.mu.Lock() defer w.mu.Unlock() - // Dir with broken symbolic link can not be watched. - // TODO(hxjiang): is it possible the files/dirs are - // created before the watch is successfully registered. - if err := w.watcher.Add(path); err != nil { - return err + if w.dirCancel == nil { // file watcher is closing. + return nil, nil } - w.watchedDirs[path] = true - return nil + + done = make(chan struct{}) + w.dirCancel[path] = done + + w.watchers.Add(1) + + return done, w.watchers.Done } -func (w *Watcher) unwatchDir(path string) { +// removeWatchHandle removes the handle for a directory watch and cancels any +// pending watch attempt for that path. +func (w *Watcher) removeWatchHandle(path string) { w.mu.Lock() defer w.mu.Unlock() - // Upon removal, we only need to remove the entries from the map. - // The [fsnotify.Watcher] remove the watch for us. - // fsnotify/fsnotify#268 - delete(w.watchedDirs, path) + if done, ok := w.dirCancel[path]; ok { + delete(w.dirCancel, path) + close(done) + } } -func (w *Watcher) isDir(path string) bool { +// isWatchedDir reports whether the given path has a watch handle, meaning it is +// a directory the watcher is managing. +func (w *Watcher) isWatchedDir(path string) bool { w.mu.Lock() defer w.mu.Unlock() - _, isDir := w.watchedDirs[path] + _, isDir := w.dirCancel[path] return isDir } @@ -283,26 +383,44 @@ func (w *Watcher) addEvent(event protocol.FileEvent) { } } -func (w *Watcher) sendEvents(handler func([]protocol.FileEvent, error)) { +func (w *Watcher) drainEvents() []protocol.FileEvent { w.mu.Lock() events := w.events w.events = nil w.mu.Unlock() - if len(events) != 0 { - handler(events, nil) - } + return events } // Close shuts down the watcher, waits for the internal goroutine to terminate, // and returns any final error. func (w *Watcher) Close() error { + // Set dirCancel to nil which prevent any future watch attempts. + w.mu.Lock() + dirCancel := w.dirCancel + w.dirCancel = nil + w.mu.Unlock() + + // Cancel any ongoing watch registration. + for _, ch := range dirCancel { + close(ch) + } + + // Wait for all watch registration goroutines to finish, including their + // error handling. This ensures that: + // - All [Watcher.watchDir] goroutines have exited and it's error is sent + // to the internal error channel. So it is safe to close the internal + // error channel. + // - There are no ongoing [fsnotify.Watcher.Add] calls, so it is safe to + // close the fsnotify watcher (see fsnotify/fsnotify#704). + w.watchers.Wait() + close(w.errs) + err := w.watcher.Close() - // Wait for the go routine to finish. So all the channels will be closed and - // all go routine will be terminated. - close(w.stop) - w.wg.Wait() + // Wait for the main run loop to terminate. + close(w.stop) + w.runners.Wait() return err } diff --git a/gopls/internal/filewatcher/filewatcher_test.go b/gopls/internal/filewatcher/filewatcher_test.go index 8ed97295e98..80abf95861a 100644 --- a/gopls/internal/filewatcher/filewatcher_test.go +++ b/gopls/internal/filewatcher/filewatcher_test.go @@ -5,6 +5,8 @@ package filewatcher_test import ( + "cmp" + "fmt" "os" "path/filepath" "runtime" @@ -12,8 +14,10 @@ import ( "testing" "time" + "golang.org/x/sync/errgroup" "golang.org/x/tools/gopls/internal/filewatcher" "golang.org/x/tools/gopls/internal/protocol" + "golang.org/x/tools/gopls/internal/util/moremaps" "golang.org/x/tools/txtar" ) @@ -25,10 +29,13 @@ func TestFileWatcher(t *testing.T) { } testCases := []struct { - name string - goos []string // if not empty, only run in these OS. + name string + goos []string // if not empty, only run in these OS. + // If set, sends watch errors for this path to an error channel + // passed to the 'changes' func. + watchErrorPath string initWorkspace string - changes func(t *testing.T, root string) + changes func(root string, errs chan error) error expectedEvents []protocol.FileEvent }{ { @@ -38,10 +45,8 @@ func TestFileWatcher(t *testing.T) { -- foo.go -- package foo `, - changes: func(t *testing.T, root string) { - if err := os.WriteFile(filepath.Join(root, "bar.go"), []byte("package main"), 0644); err != nil { - t.Fatal(err) - } + changes: func(root string, errs chan error) error { + return os.WriteFile(filepath.Join(root, "bar.go"), []byte("package main"), 0644) }, expectedEvents: []protocol.FileEvent{ {URI: "bar.go", Type: protocol.Created}, @@ -54,10 +59,8 @@ package foo -- foo.go -- package foo `, - changes: func(t *testing.T, root string) { - if err := os.WriteFile(filepath.Join(root, "bar.go"), []byte("package main"), 0644); err != nil { - t.Fatal(err) - } + changes: func(root string, errs chan error) error { + return os.WriteFile(filepath.Join(root, "bar.go"), []byte("package main"), 0644) }, expectedEvents: []protocol.FileEvent{ {URI: "bar.go", Type: protocol.Created}, @@ -70,10 +73,8 @@ package foo -- foo.go -- package foo `, - changes: func(t *testing.T, root string) { - if err := os.WriteFile(filepath.Join(root, "foo.go"), []byte("package main // modified"), 0644); err != nil { - t.Fatal(err) - } + changes: func(root string, errs chan error) error { + return os.WriteFile(filepath.Join(root, "foo.go"), []byte("package main // modified"), 0644) }, expectedEvents: []protocol.FileEvent{ {URI: "foo.go", Type: protocol.Changed}, @@ -87,10 +88,8 @@ package foo -- bar.go -- package bar `, - changes: func(t *testing.T, root string) { - if err := os.Remove(filepath.Join(root, "foo.go")); err != nil { - t.Fatal(err) - } + changes: func(root string, errs chan error) error { + return os.Remove(filepath.Join(root, "foo.go")) }, expectedEvents: []protocol.FileEvent{ {URI: "foo.go", Type: protocol.Deleted}, @@ -103,10 +102,8 @@ package bar -- foo.go -- package foo `, - changes: func(t *testing.T, root string) { - if err := os.Rename(filepath.Join(root, "foo.go"), filepath.Join(root, "bar.go")); err != nil { - t.Fatal(err) - } + changes: func(root string, errs chan error) error { + return os.Rename(filepath.Join(root, "foo.go"), filepath.Join(root, "bar.go")) }, expectedEvents: []protocol.FileEvent{ {URI: "foo.go", Type: protocol.Deleted}, @@ -120,10 +117,8 @@ package foo -- foo.go -- package foo `, - changes: func(t *testing.T, root string) { - if err := os.Rename(filepath.Join(root, "foo.go"), filepath.Join(root, "bar.go")); err != nil { - t.Fatal(err) - } + changes: func(root string, errs chan error) error { + return os.Rename(filepath.Join(root, "foo.go"), filepath.Join(root, "bar.go")) }, expectedEvents: []protocol.FileEvent{ {URI: "bar.go", Type: protocol.Created}, @@ -136,10 +131,8 @@ package foo -- foo.go -- package foo `, - changes: func(t *testing.T, root string) { - if err := os.Mkdir(filepath.Join(root, "bar"), 0755); err != nil { - t.Fatal(err) - } + changes: func(root string, errs chan error) error { + return os.Mkdir(filepath.Join(root, "bar"), 0755) }, expectedEvents: []protocol.FileEvent{ {URI: "bar", Type: protocol.Created}, @@ -151,10 +144,8 @@ package foo -- foo/bar.go -- package foo `, - changes: func(t *testing.T, root string) { - if err := os.RemoveAll(filepath.Join(root, "foo")); err != nil { - t.Fatal(err) - } + changes: func(root string, errs chan error) error { + return os.RemoveAll(filepath.Join(root, "foo")) }, expectedEvents: []protocol.FileEvent{ // We only assert that the directory deletion event exists, @@ -175,10 +166,8 @@ package foo -- foo/bar.go -- package foo `, - changes: func(t *testing.T, root string) { - if err := os.Rename(filepath.Join(root, "foo"), filepath.Join(root, "baz")); err != nil { - t.Fatal(err) - } + changes: func(root string, errs chan error) error { + return os.Rename(filepath.Join(root, "foo"), filepath.Join(root, "baz")) }, expectedEvents: []protocol.FileEvent{ {URI: "foo", Type: protocol.Deleted}, @@ -192,16 +181,93 @@ package foo -- foo/bar.go -- package foo `, - changes: func(t *testing.T, root string) { - if err := os.Rename(filepath.Join(root, "foo"), filepath.Join(root, "baz")); err != nil { - t.Fatal(err) - } + changes: func(root string, errs chan error) error { + return os.Rename(filepath.Join(root, "foo"), filepath.Join(root, "baz")) }, expectedEvents: []protocol.FileEvent{ {URI: "baz", Type: protocol.Created}, {URI: "foo", Type: protocol.Deleted}, }, }, + { + name: "broken symlink in darwin", + goos: []string{"darwin"}, + watchErrorPath: "foo", + changes: func(root string, errs chan error) error { + // Prepare a dir with with broken symbolic link. + // foo <- 1st + // └── from.go -> root/to.go <- 1st + tmp := filepath.Join(t.TempDir(), "foo") + if err := os.Mkdir(tmp, 0755); err != nil { + return err + } + from := filepath.Join(tmp, "from.go") + + to := filepath.Join(root, "to.go") + // Create the symbolic link to a non-existing file. This would + // cause the watch registration to fail. + if err := os.Symlink(to, from); err != nil { + return err + } + + // Move the directory containing the broken symlink into place + // to avoids a flaky test where the directory could be watched + // before the symlink is created. See golang/go#74782. + if err := os.Rename(tmp, filepath.Join(root, "foo")); err != nil { + return err + } + + // root + // ├── foo <- 2nd (Move) + // │ ├── from.go -> ../to.go <- 2nd (Move) + // │ └── foo.go <- 4th (Create) + // └── to.go <- 3rd (Create) + + // Should be able to capture an error from [fsnotify.Watcher.Add]. + err := <-errs + if err == nil { + return fmt.Errorf("did not capture watch registration failure") + } + + // The file watcher should retry watch registration and + // eventually succeed after the file got created. + if err := os.WriteFile(to, []byte("package main"), 0644); err != nil { + return err + } + + timer := time.NewTimer(30 * time.Second) + for { + var ( + err error + ok bool + ) + select { + case err, ok = <-errs: + if !ok { + return fmt.Errorf("can not register watch for foo") + } + case <-timer.C: + return fmt.Errorf("can not register watch for foo after 30 seconds") + } + + if err == nil { + break // watch registration success + } + } + + // Once the watch registration is done, file events under the + // dir should be captured. + return os.WriteFile(filepath.Join(root, "foo", "foo.go"), []byte("package main"), 0644) + }, + expectedEvents: []protocol.FileEvent{ + {URI: "foo", Type: protocol.Created}, + // TODO(hxjiang): enable this after implementing retrospectively + // generate create events. + // {URI: "foo/from.go", Type: protocol.Created}, + {URI: "to.go", Type: protocol.Created}, + {URI: "foo/foo.go", Type: protocol.Created}, + }, + }, } for _, tt := range testCases { @@ -209,9 +275,23 @@ package foo if len(tt.goos) > 0 && !slices.Contains(tt.goos, runtime.GOOS) { t.Skipf("skipping on %s", runtime.GOOS) } - t.Parallel() root := t.TempDir() + + var errs chan error + if tt.watchErrorPath != "" { + errs = make(chan error, 10) + filewatcher.SetAfterAddHook(func(path string, err error) { + if path == filepath.Join(root, tt.watchErrorPath) { + errs <- err + if err == nil { + close(errs) + } + } + }) + defer filewatcher.SetAfterAddHook(nil) + } + archive := txtar.Parse([]byte(tt.initWorkspace)) for _, f := range archive.Files { path := filepath.Join(root, f.Name) @@ -260,7 +340,9 @@ package foo } if tt.changes != nil { - tt.changes(t, root) + if err := tt.changes(root, errs); err != nil { + t.Fatal(err) + } } select { @@ -277,3 +359,121 @@ package foo }) } } + +func TestStress(t *testing.T) { + switch runtime.GOOS { + case "darwin", "linux", "windows": + default: + t.Skip("unsupported OS") + } + + const ( + delay = 50 * time.Millisecond + parallelism = 100 // number of parallel instances of each kind of operation + ) + + root := t.TempDir() + + mkdir := func(base string) func() error { + return func() error { + return os.Mkdir(filepath.Join(root, base), 0755) + } + } + write := func(base string) func() error { + return func() error { + return os.WriteFile(filepath.Join(root, base), []byte("package main"), 0644) + } + } + remove := func(base string) func() error { + return func() error { + return os.Remove(filepath.Join(root, base)) + } + } + rename := func(old, new string) func() error { + return func() error { + return os.Rename(filepath.Join(root, old), filepath.Join(root, new)) + } + } + + wants := make(map[protocol.FileEvent]bool) + want := func(base string, t protocol.FileChangeType) { + wants[protocol.FileEvent{URI: protocol.URIFromPath(filepath.Join(root, base)), Type: t}] = true + } + + for i := range parallelism { + // Create files and dirs that will be deleted or renamed later. + if err := cmp.Or( + mkdir(fmt.Sprintf("delete-dir-%d", i))(), + mkdir(fmt.Sprintf("old-dir-%d", i))(), + write(fmt.Sprintf("delete-file-%d.go", i))(), + write(fmt.Sprintf("old-file-%d.go", i))(), + ); err != nil { + t.Fatal(err) + } + + // Add expected notification events to the "wants" set. + want(fmt.Sprintf("file-%d.go", i), protocol.Created) + want(fmt.Sprintf("delete-file-%d.go", i), protocol.Deleted) + want(fmt.Sprintf("old-file-%d.go", i), protocol.Deleted) + want(fmt.Sprintf("new-file-%d.go", i), protocol.Created) + want(fmt.Sprintf("dir-%d", i), protocol.Created) + want(fmt.Sprintf("delete-dir-%d", i), protocol.Deleted) + want(fmt.Sprintf("old-dir-%d", i), protocol.Deleted) + want(fmt.Sprintf("new-dir-%d", i), protocol.Created) + } + + foundAll := make(chan struct{}) + w, err := filewatcher.New(delay, nil, func(events []protocol.FileEvent, err error) { + if err != nil { + t.Errorf("error from watcher: %v", err) + return + } + for _, e := range events { + delete(wants, e) + } + if len(wants) == 0 { + close(foundAll) + } + }) + if err != nil { + t.Fatal(err) + } + + if err := w.WatchDir(root); err != nil { + t.Fatal(err) + } + + // Spin up multiple goroutines, to perform 6 file system operations i.e. + // create, delete, rename of file or directory. For deletion and rename, + // the goroutine deletes / renames files or directories created before the + // watcher starts. + var g errgroup.Group + for id := range parallelism { + ops := []func() error{ + write(fmt.Sprintf("file-%d.go", id)), + remove(fmt.Sprintf("delete-file-%d.go", id)), + rename(fmt.Sprintf("old-file-%d.go", id), fmt.Sprintf("new-file-%d.go", id)), + mkdir(fmt.Sprintf("dir-%d", id)), + remove(fmt.Sprintf("delete-dir-%d", id)), + rename(fmt.Sprintf("old-dir-%d", id), fmt.Sprintf("new-dir-%d", id)), + } + for _, f := range ops { + g.Go(f) + } + } + if err := g.Wait(); err != nil { + t.Fatal(err) + } + + select { + case <-foundAll: + case <-time.After(30 * time.Second): + if len(wants) > 0 { + t.Errorf("missing expected events: %#v", moremaps.KeySlice(wants)) + } + } + + if err := w.Close(); err != nil { + t.Errorf("failed to close the file watcher: %v", err) + } +} diff --git a/gopls/internal/golang/addtest.go b/gopls/internal/golang/addtest.go index dfd78310f66..50a3b3edd45 100644 --- a/gopls/internal/golang/addtest.go +++ b/gopls/internal/golang/addtest.go @@ -212,21 +212,23 @@ var testTmpl = template.Must(template.New("test").Funcs(template.FuncMap{ // AddTestForFunc adds a test for the function enclosing the given input range. // It creates a _test.go file if one does not already exist. -func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol.Location) (changes []protocol.DocumentChange, _ error) { +// It returns the required text edits and the predicted location of the new test +// function, which is only valid after the edits have been successfully applied. +func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol.Location) (changes []protocol.DocumentChange, show *protocol.Location, _ error) { pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, loc.URI) if err != nil { - return nil, err + return nil, nil, err } if metadata.IsCommandLineArguments(pkg.Metadata().ID) { - return nil, fmt.Errorf("current file in command-line-arguments package") + return nil, nil, fmt.Errorf("current file in command-line-arguments package") } if errors := pkg.ParseErrors(); len(errors) > 0 { - return nil, fmt.Errorf("package has parse errors: %v", errors[0]) + return nil, nil, fmt.Errorf("package has parse errors: %v", errors[0]) } if errors := pkg.TypeErrors(); len(errors) > 0 { - return nil, fmt.Errorf("package has type errors: %v", errors[0]) + return nil, nil, fmt.Errorf("package has type errors: %v", errors[0]) } // All three maps map the path of an imported package to @@ -262,7 +264,7 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. // Collect all the imports from the x.go, keep track of the local package name. if fileImports, err = collectImports(pgf.File); err != nil { - return nil, err + return nil, nil, err } testBase := strings.TrimSuffix(loc.URI.Base(), ".go") + "_test.go" @@ -270,7 +272,7 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. testFH, err := snapshot.ReadFile(ctx, goTestFileURI) if err != nil { - return nil, err + return nil, nil, err } // TODO(hxjiang): use a fresh name if the same test function name already @@ -289,17 +291,17 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. start, end, err := pgf.RangePos(loc.Range) if err != nil { - return nil, err + return nil, nil, err } path, _ := astutil.PathEnclosingInterval(pgf.File, start, end) if len(path) < 2 { - return nil, fmt.Errorf("no enclosing function") + return nil, nil, fmt.Errorf("no enclosing function") } decl, ok := path[len(path)-2].(*ast.FuncDecl) if !ok { - return nil, fmt.Errorf("no enclosing function") + return nil, nil, fmt.Errorf("no enclosing function") } fn := pkg.TypesInfo().Defs[decl.Name].(*types.Func) @@ -308,7 +310,7 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. testPGF, err := snapshot.ParseGo(ctx, testFH, parsego.Header) if err != nil { if !errors.Is(err, os.ErrNotExist) { - return nil, err + return nil, nil, err } changes = append(changes, protocol.DocumentChangeCreate(goTestFileURI)) @@ -322,7 +324,7 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. if c := CopyrightComment(pgf.File); c != nil { text, err := pgf.NodeText(c) if err != nil { - return nil, err + return nil, nil, err } header.Write(text) // One empty line between copyright header and following. @@ -334,7 +336,7 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. if c := buildConstraintComment(pgf.File); c != nil { text, err := pgf.NodeText(c) if err != nil { - return nil, err + return nil, nil, err } header.Write(text) // One empty line between build constraint and following. @@ -397,7 +399,7 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. } else { // existing _test.go file. file := testPGF.File if !file.Name.NamePos.IsValid() { - return nil, fmt.Errorf("missing package declaration") + return nil, nil, fmt.Errorf("missing package declaration") } switch file.Name.Name { case pgf.File.Name.Name: @@ -405,17 +407,17 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. case pgf.File.Name.Name + "_test": xtest = true default: - return nil, fmt.Errorf("invalid package declaration %q in test file %q", file.Name, testPGF) + return nil, nil, fmt.Errorf("invalid package declaration %q in test file %q", file.Name, testPGF) } eofRange, err = testPGF.PosRange(file.FileEnd, file.FileEnd) if err != nil { - return nil, err + return nil, nil, err } // Collect all the imports from the foo_test.go. if testImports, err = collectImports(file); err != nil { - return nil, err + return nil, nil, err } } @@ -453,13 +455,13 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. if xtest { // Reject if function/method is unexported. if !fn.Exported() { - return nil, fmt.Errorf("cannot add test of unexported function %s to external test package %s_test", decl.Name, pgf.File.Name) + return nil, nil, fmt.Errorf("cannot add test of unexported function %s to external test package %s_test", decl.Name, pgf.File.Name) } // Reject if receiver is unexported. if sig.Recv() != nil { if _, ident, _ := goplsastutil.UnpackRecv(decl.Recv.List[0].Type); ident == nil || !ident.IsExported() { - return nil, fmt.Errorf("cannot add external test for method %s.%s as receiver type is not exported", ident.Name, decl.Name) + return nil, nil, fmt.Errorf("cannot add external test for method %s.%s as receiver type is not exported", ident.Name, decl.Name) } } // TODO(hxjiang): reject if the any input parameter type is unexported. @@ -469,7 +471,7 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. testName, err := testName(fn) if err != nil { - return nil, err + return nil, nil, err } data := testInfo{ @@ -525,7 +527,7 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. t, ok := recvType.(typesinternal.NamedOrAlias) if !ok { - return nil, fmt.Errorf("the receiver type is neither named type nor alias type") + return nil, nil, fmt.Errorf("the receiver type is neither named type nor alias type") } var varName string @@ -707,7 +709,7 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. } importEdits, err := ComputeImportFixEdits(snapshot.Options().Local, testPGF.Src, importFixes...) if err != nil { - return nil, fmt.Errorf("could not compute the import fix edits: %w", err) + return nil, nil, fmt.Errorf("could not compute the import fix edits: %w", err) } edits = append(edits, importEdits...) } else { @@ -740,21 +742,41 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol. var test bytes.Buffer if err := testTmpl.Execute(&test, data); err != nil { - return nil, err + return nil, nil, err } formatted, err := format.Source(test.Bytes()) if err != nil { - return nil, err + return nil, nil, err } edits = append(edits, protocol.TextEdit{ Range: eofRange, NewText: string(formatted), - }) + }, + ) + + // Show the line of generated test function. + { + line := eofRange.Start.Line + for i := range len(edits) - 1 { // last edits is the func decl + e := edits[i] + oldLines := e.Range.End.Line - e.Range.Start.Line + newLines := uint32(strings.Count(e.NewText, "\n")) + line += (newLines - oldLines) + } + show = &protocol.Location{ + URI: testFH.URI(), + Range: protocol.Range{ + // Test function template have a new line at beginning. + Start: protocol.Position{Line: line + 1}, + End: protocol.Position{Line: line + 1}, + }, + } + } - return append(changes, protocol.DocumentChangeEdit(testFH, edits)), nil + return append(changes, protocol.DocumentChangeEdit(testFH, edits)), show, nil } // testName returns the name of the function to use for the new function that diff --git a/gopls/internal/golang/codeaction.go b/gopls/internal/golang/codeaction.go index 377c012dfdf..7d25a1ae9ad 100644 --- a/gopls/internal/golang/codeaction.go +++ b/gopls/internal/golang/codeaction.go @@ -461,11 +461,9 @@ func goSplitPackage(ctx context.Context, req *codeActionsRequest) error { // goplsDocFeatures produces "Browse gopls feature documentation" code actions. // See [server.commandHandler.ClientOpenURL] for command implementation. func goplsDocFeatures(ctx context.Context, req *codeActionsRequest) error { - // TODO(adonovan): after the docs are published in gopls/v0.17.0, - // use the gopls release tag instead of master. cmd := command.NewClientOpenURLCommand( "Browse gopls feature documentation", - "https://github.com/golang/tools/blob/master/gopls/doc/features/index.md") + "https://go.dev/gopls/features") req.addCommandAction(cmd, false) return nil } diff --git a/gopls/internal/golang/completion/completion.go b/gopls/internal/golang/completion/completion.go index b48841500bd..31d58ce7cf7 100644 --- a/gopls/internal/golang/completion/completion.go +++ b/gopls/internal/golang/completion/completion.go @@ -2589,7 +2589,7 @@ func inferExpectedResultTypes(c *completer, callNodeIdx int) []types.Type { var expectedResults []types.Type // Check the parents of the call node to extract the expected result types of the call signature. - // Currently reverse inferences are only supported with the the following parent expressions, + // Currently reverse inferences are only supported with the following parent expressions, // however this list isn't exhaustive. switch node := c.path[callNodeIdx+1].(type) { case *ast.KeyValueExpr: @@ -2671,7 +2671,7 @@ func expectedValueSpecType(pkg *cache.Package, node *ast.ValueSpec, pos token.Po } // expectedAssignStmtTypes analyzes the provided assignStmt, and checks -// to see if the provided pos is within a RHS expresison. If so, it report +// to see if the provided pos is within a RHS expression. If so, it report // the expected type of that expression, and the LHS type(s) to which it // is being assigned. func expectedAssignStmtTypes(pkg *cache.Package, node *ast.AssignStmt, pos token.Pos) (objType types.Type, assignees []types.Type) { diff --git a/gopls/internal/golang/completion/package.go b/gopls/internal/golang/completion/package.go index ae421d821c0..ae38f8f56cc 100644 --- a/gopls/internal/golang/completion/package.go +++ b/gopls/internal/golang/completion/package.go @@ -215,13 +215,14 @@ func packageSuggestions(ctx context.Context, snapshot *cache.Snapshot, fileURI p return candidate{obj: obj, name: name, detail: name, score: score} } - matcher := fuzzy.NewMatcher(prefix) var currentPackageName string - if variants, err := snapshot.MetadataForFile(ctx, fileURI); err == nil && - len(variants) != 0 { - currentPackageName = string(variants[0].Name) + // TODO: consider propagating error. + if md, err := snapshot.NarrowestMetadataForFile(ctx, fileURI); err == nil { + currentPackageName = string(md.Name) } + matcher := fuzzy.NewMatcher(prefix) + // Always try to suggest a main package defer func() { mainScore := lowScore diff --git a/gopls/internal/golang/completion/unimported.go b/gopls/internal/golang/completion/unimported.go index 3627a5edaa1..108658e7204 100644 --- a/gopls/internal/golang/completion/unimported.go +++ b/gopls/internal/golang/completion/unimported.go @@ -224,9 +224,6 @@ func (c *completer) modcacheMatches(pkg metadata.PackageName, prefix string) ([] if err != nil { return nil, err } - if ix == nil || len(ix.Entries) == 0 { // in tests ix might always be nil - return nil, fmt.Errorf("no index %w", err) - } // retrieve everything and let usefulCompletion() and the matcher sort them out cands := ix.Lookup(string(pkg), "", true) lx := len(cands) diff --git a/gopls/internal/golang/extract.go b/gopls/internal/golang/extract.go index 179f8c328da..f898686d8f0 100644 --- a/gopls/internal/golang/extract.go +++ b/gopls/internal/golang/extract.go @@ -772,7 +772,9 @@ func extractFunctionMethod(cpkg *cache.Package, pgf *parsego.File, start, end to // cannot be its own reassignment or redefinition (objOverriden). vscope := v.obj.Parent() if vscope == nil { - return nil, nil, fmt.Errorf("parent nil") + // v.obj could be a field on an anonymous struct. We'll examine the + // struct in a different iteration so don't return an error here. + continue } isUsed, firstUseAfter := objUsed(info, end, vscope.End(), v.obj) if v.assigned && isUsed && !varOverridden(info, firstUseAfter, v.obj, v.free, outer) { @@ -1566,6 +1568,12 @@ func collectFreeVars(info *types.Info, file *ast.File, start, end token.Pos, nod if !ok { return nil, fmt.Errorf("no seen types.Object for %v", obj) } + if named, ok := v.obj.Type().(typesinternal.NamedOrAlias); ok { + namedPos := named.Obj().Pos() + if isLocal(named.Obj()) && !(start <= namedPos && namedPos <= end) { + return nil, fmt.Errorf("Cannot extract selection: the code refers to a local type whose definition lies outside the extracted block") + } + } variables = append(variables, v) } return variables, nil diff --git a/gopls/internal/golang/format.go b/gopls/internal/golang/format.go index 9d2ccc94d48..f17ce8b4ffc 100644 --- a/gopls/internal/golang/format.go +++ b/gopls/internal/golang/format.go @@ -149,8 +149,8 @@ func computeImportEdits(ctx context.Context, pgf *parsego.File, snapshot *cache. source = isource } // imports require a current metadata graph - // TODO(rfindlay) improve the API - snapshot.WorkspaceMetadata(ctx) + // TODO(rfindley): improve the API + snapshot.WorkspaceMetadata(ctx) // ignore error allFixes, err := imports.FixImports(ctx, filename, pgf.Src, goroot, options.Env.Logf, source) if err != nil { return nil, nil, err diff --git a/gopls/internal/golang/hover.go b/gopls/internal/golang/hover.go index 4b0e12b314e..0fdabcd396f 100644 --- a/gopls/internal/golang/hover.go +++ b/gopls/internal/golang/hover.go @@ -556,10 +556,15 @@ func hover(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, pp pro } } - // realTypeDecl is defined to store the underlying definition of an alias. - realTypeDecl, _ := findRhsTypeDecl(ctx, snapshot, pkg, obj) // tolerate the error - if realTypeDecl != "" { - typeDecl += fmt.Sprintf("\n\n%s", realTypeDecl) + if isTypeName { + // get the real type decl only if current object is a type, + // for non-types, we'd better hide the real type decl to avoid possible confusion. + // + // realTypeDecl is defined to store the underlying definition of an alias. + realTypeDecl, _ := findRhsTypeDecl(ctx, snapshot, pkg, obj) // tolerate the error + if realTypeDecl != "" { + typeDecl += fmt.Sprintf("\n\n%s", realTypeDecl) + } } // Compute link data (on pkg.go.dev or other documentation host). diff --git a/gopls/internal/golang/implementation.go b/gopls/internal/golang/implementation.go index 8a70a05160c..a0ac931968f 100644 --- a/gopls/internal/golang/implementation.go +++ b/gopls/internal/golang/implementation.go @@ -143,11 +143,10 @@ func implementationsMsets(ctx context.Context, snapshot *cache.Snapshot, pkg *ca // enumerate all types within the package that satisfy the // query type, even those defined local to a function. declURI = protocol.URIFromPath(declPosn.Filename) - declMPs, err := snapshot.MetadataForFile(ctx, declURI) + declMPs, err := snapshot.MetadataForFile(ctx, declURI, true) if err != nil { return err } - metadata.RemoveIntermediateTestVariants(&declMPs) if len(declMPs) == 0 { return fmt.Errorf("no packages for file %s", declURI) } @@ -372,7 +371,7 @@ func implementsObj(info *types.Info, file *ast.File, pos token.Pos) (types.Objec // a function body. The global search index excludes such types // because reliably naming such types is hard.) // -// Results are reported via the the yield function. +// Results are reported via the yield function. func localImplementations(ctx context.Context, snapshot *cache.Snapshot, pkg *cache.Package, id *ast.Ident, rel methodsets.TypeRelation, yield implYieldFunc) error { queryType, queryMethod := typeOrMethod(pkg.TypesInfo().Defs[id]) if queryType == nil { diff --git a/gopls/internal/golang/inlay_hint.go b/gopls/internal/golang/inlay_hint.go index 019bcf8eeb0..e14cfb99c03 100644 --- a/gopls/internal/golang/inlay_hint.go +++ b/gopls/internal/golang/inlay_hint.go @@ -165,7 +165,8 @@ outer: obj := typeutil.Callee(info, call) if analysisinternal.IsFunctionNamed(obj, "fmt", "Print", "Printf", "Println", "Fprint", "Fprintf", "Fprintln") || analysisinternal.IsMethodNamed(obj, "bytes", "Buffer", "Write", "WriteByte", "WriteRune", "WriteString") || - analysisinternal.IsMethodNamed(obj, "strings", "Builder", "Write", "WriteByte", "WriteRune", "WriteString") { + analysisinternal.IsMethodNamed(obj, "strings", "Builder", "Write", "WriteByte", "WriteRune", "WriteString") || + analysisinternal.IsFunctionNamed(obj, "io", "WriteString") { continue } diff --git a/gopls/internal/golang/references.go b/gopls/internal/golang/references.go index 7fe054a5a7d..76ec05bb25e 100644 --- a/gopls/internal/golang/references.go +++ b/gopls/internal/golang/references.go @@ -110,7 +110,7 @@ func references(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, pp // import declarations of all packages that directly import the target // package. func packageReferences(ctx context.Context, snapshot *cache.Snapshot, uri protocol.DocumentURI) ([]reference, error) { - metas, err := snapshot.MetadataForFile(ctx, uri) + metas, err := snapshot.MetadataForFile(ctx, uri, false) if err != nil { return nil, err } @@ -260,7 +260,7 @@ func ordinaryReferences(ctx context.Context, snapshot *cache.Snapshot, uri proto // This may include the query pkg, and possibly other variants. declPosn := safetoken.StartPosition(pkg.FileSet(), obj.Pos()) declURI := protocol.URIFromPath(declPosn.Filename) - variants, err := snapshot.MetadataForFile(ctx, declURI) + variants, err := snapshot.MetadataForFile(ctx, declURI, false) if err != nil { return nil, err } diff --git a/gopls/internal/golang/rename.go b/gopls/internal/golang/rename.go index 6336a115b41..8c0defd2ec1 100644 --- a/gopls/internal/golang/rename.go +++ b/gopls/internal/golang/rename.go @@ -62,6 +62,7 @@ import ( "golang.org/x/mod/modfile" "golang.org/x/tools/go/ast/astutil" + "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/types/objectpath" "golang.org/x/tools/go/types/typeutil" "golang.org/x/tools/gopls/internal/cache" @@ -135,9 +136,14 @@ func PrepareRename(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, return nil, nil, err } + cur, ok := pgf.Cursor.FindByPos(pos, pos) + if !ok { + return nil, nil, fmt.Errorf("can't find cursor for selection") + } + // Check if we're in a 'func' keyword. If so, we hijack the renaming to // change the function signature. - if item, err := prepareRenameFuncSignature(pgf, pos); err != nil { + if item, err := prepareRenameFuncSignature(pgf, pos, cur); err != nil { return nil, nil, err } else if item != nil { return item, nil, nil @@ -145,7 +151,19 @@ func PrepareRename(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, targets, node, err := objectsAt(pkg.TypesInfo(), pgf.File, pos) if err != nil { - return nil, nil, err + // Check if we are renaming an ident inside its doc comment. The call to + // objectsAt will have returned an error in this case. + id := docCommentPosToIdent(pgf, pos, cur) + if id == nil { + return nil, nil, err + } + obj := pkg.TypesInfo().Defs[id] + if obj == nil { + return nil, nil, fmt.Errorf("error fetching Object for ident %q", id.Name) + } + // Change rename target to the ident. + targets = map[types.Object]ast.Node{obj: id} + node = id } var obj types.Object for obj = range targets { @@ -209,8 +227,8 @@ func prepareRenamePackageName(ctx context.Context, snapshot *cache.Snapshot, pgf // // The resulting text is the signature of the function, which may be edited to // the new signature. -func prepareRenameFuncSignature(pgf *parsego.File, pos token.Pos) (*PrepareItem, error) { - fdecl := funcKeywordDecl(pgf, pos) +func prepareRenameFuncSignature(pgf *parsego.File, pos token.Pos, cursor inspector.Cursor) (*PrepareItem, error) { + fdecl := funcKeywordDecl(pos, cursor) if fdecl == nil { return nil, nil } @@ -264,17 +282,8 @@ func nameBlankParams(ftype *ast.FuncType) *ast.FuncType { // renameFuncSignature computes and applies the effective change signature // operation resulting from a 'renamed' (=rewritten) signature. -func renameFuncSignature(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, pp protocol.Position, newName string) (map[protocol.DocumentURI][]protocol.TextEdit, error) { - // Find the renamed signature. - pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, f.URI()) - if err != nil { - return nil, err - } - pos, err := pgf.PositionPos(pp) - if err != nil { - return nil, err - } - fdecl := funcKeywordDecl(pgf, pos) +func renameFuncSignature(ctx context.Context, pkg *cache.Package, pgf *parsego.File, pos token.Pos, snapshot *cache.Snapshot, cursor inspector.Cursor, f file.Handle, pp protocol.Position, newName string) (map[protocol.DocumentURI][]protocol.TextEdit, error) { + fdecl := funcKeywordDecl(pos, cursor) if fdecl == nil { return nil, nil } @@ -350,15 +359,12 @@ func renameFuncSignature(ctx context.Context, snapshot *cache.Snapshot, f file.H // funcKeywordDecl returns the FuncDecl for which pos is in the 'func' keyword, // if any. -func funcKeywordDecl(pgf *parsego.File, pos token.Pos) *ast.FuncDecl { - path, _ := astutil.PathEnclosingInterval(pgf.File, pos, pos) - if len(path) < 1 { - return nil - } - fdecl, _ := path[0].(*ast.FuncDecl) - if fdecl == nil { +func funcKeywordDecl(pos token.Pos, cursor inspector.Cursor) *ast.FuncDecl { + curDecl, ok := moreiters.First(cursor.Enclosing((*ast.FuncDecl)(nil))) + if !ok { return nil } + fdecl := curDecl.Node().(*ast.FuncDecl) ftyp := fdecl.Type if pos < ftyp.Func || pos > ftyp.Func+token.Pos(len("func")) { // tolerate renaming immediately after 'func' return nil @@ -396,7 +402,21 @@ func Rename(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, pp pro ctx, done := event.Start(ctx, "golang.Rename") defer done() - if edits, err := renameFuncSignature(ctx, snapshot, f, pp, newName); err != nil { + pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, f.URI()) + if err != nil { + return nil, false, err + } + pos, err := pgf.PositionPos(pp) + if err != nil { + return nil, false, err + } + + cur, ok := pgf.Cursor.FindByPos(pos, pos) + if !ok { + return nil, false, fmt.Errorf("can't find cursor for selection") + } + + if edits, err := renameFuncSignature(ctx, pkg, pgf, pos, snapshot, cur, f, pp, newName); err != nil { return nil, false, err } else if edits != nil { return edits, false, nil @@ -474,11 +494,10 @@ func renameOrdinary(ctx context.Context, snapshot *cache.Snapshot, uri protocol. // only package we need. (In case you're wondering why // 'references' doesn't also want the widest variant: it // computes the union across all variants.) - mps, err := snapshot.MetadataForFile(ctx, uri) + mps, err := snapshot.MetadataForFile(ctx, uri, true) if err != nil { return nil, err } - metadata.RemoveIntermediateTestVariants(&mps) if len(mps) == 0 { return nil, fmt.Errorf("no package metadata for file %s", uri) } @@ -503,7 +522,18 @@ func renameOrdinary(ctx context.Context, snapshot *cache.Snapshot, uri protocol. } targets, node, err := objectsAt(pkg.TypesInfo(), pgf.File, pos) if err != nil { - return nil, err + // Check if we are renaming an ident inside its doc comment. The call to + // objectsAt will have returned an error in this case. + id := docCommentPosToIdent(pgf, pos, cur) + if id == nil { + return nil, err + } + obj := pkg.TypesInfo().Defs[id] + if obj == nil { + return nil, fmt.Errorf("error fetching types.Object for ident %q", id.Name) + } + // Change rename target to the ident. + targets = map[types.Object]ast.Node{obj: id} } // Pick a representative object arbitrarily. @@ -714,7 +744,7 @@ func renameReceivers(pkg *cache.Package, recv *types.Var, newName string, editMa // selectors used only in an ITV, but life is short. Also sin must be // punished.) func typeCheckReverseDependencies(ctx context.Context, snapshot *cache.Snapshot, declURI protocol.DocumentURI, transitive bool) ([]*cache.Package, error) { - variants, err := snapshot.MetadataForFile(ctx, declURI) + variants, err := snapshot.MetadataForFile(ctx, declURI, false) if err != nil { return nil, err } @@ -1633,6 +1663,41 @@ func docComment(pgf *parsego.File, id *ast.Ident) *ast.CommentGroup { return nil } +// docCommentPosToIdent returns the node whose doc comment contains pos, if any. +// The pos must be within an occurrence of the identifier's name, otherwise it returns nil. +func docCommentPosToIdent(pgf *parsego.File, pos token.Pos, cur inspector.Cursor) *ast.Ident { + for curId := range cur.Preorder((*ast.Ident)(nil)) { + id := curId.Node().(*ast.Ident) + if pos > id.Pos() { + continue // Doc comments are not located after an ident. + } + doc := docComment(pgf, id) + if doc == nil || !(doc.Pos() <= pos && pos < doc.End()) { + continue + } + + docRegexp := regexp.MustCompile(`\b` + id.Name + `\b`) + for _, comment := range doc.List { + if isDirective(comment.Text) || !(comment.Pos() <= pos && pos < comment.End()) { + continue + } + start := comment.Pos() + text, err := pgf.NodeText(comment) + if err != nil { + return nil + } + for _, locs := range docRegexp.FindAllIndex(text, -1) { + matchStart := start + token.Pos(locs[0]) + matchEnd := start + token.Pos(locs[1]) + if matchStart <= pos && pos <= matchEnd { + return id + } + } + } + } + return nil +} + // updatePkgName returns the updates to rename a pkgName in the import spec by // only modifying the package name portion of the import declaration. func (r *renamer) updatePkgName(pgf *parsego.File, pkgName *types.PkgName) (diff.Edit, error) { diff --git a/gopls/internal/golang/signature_help.go b/gopls/internal/golang/signature_help.go index 94803f6f116..260bd0f0bcc 100644 --- a/gopls/internal/golang/signature_help.go +++ b/gopls/internal/golang/signature_help.go @@ -23,7 +23,7 @@ import ( // SignatureHelp returns information about the signature of the innermost // function call enclosing the position, or nil if there is none. -func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, position protocol.Position) (*protocol.SignatureInformation, error) { +func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, params *protocol.SignatureHelpParams) (*protocol.SignatureInformation, error) { ctx, done := event.Start(ctx, "golang.SignatureHelp") defer done() @@ -33,7 +33,7 @@ func SignatureHelp(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle if err != nil { return nil, fmt.Errorf("getting file for SignatureHelp: %w", err) } - pos, err := pgf.PositionPos(position) + pos, err := pgf.PositionPos(params.Position) if err != nil { return nil, err } @@ -78,12 +78,10 @@ loop: // Don't show signature help in this case. return nil, nil case *ast.BasicLit: - if node.Kind == token.STRING { - // golang/go#43397: don't offer signature help when the user is typing - // in a string literal. Most LSP clients use ( or , as trigger - // characters, but within a string literal these should not trigger - // signature help (and it can be annoying when this happens after - // you've already dismissed the help!). + // golang/go#43397: don't offer signature help when the user is typing + // in a string literal unless it was manually invoked or help is already active. + if node.Kind == token.STRING && + (params.Context == nil || (params.Context.TriggerKind != protocol.SigInvoked && !params.Context.IsRetrigger)) { return nil, nil } } @@ -119,7 +117,7 @@ loop: // Special handling for error.Error, which is the only builtin method. if obj.Name() == "Error" { return &protocol.SignatureInformation{ - Label: "Error()", + Label: "Error() string", // TODO(skewb1k): move the docstring for error.Error to builtin.go and reuse it across all relevant LSP methods. Documentation: stringToSigInfoDocumentation("Error returns the error message.", snapshot.Options()), Parameters: nil, diff --git a/gopls/internal/golang/snapshot.go b/gopls/internal/golang/snapshot.go index 53b2b872e6c..0eb17227bdd 100644 --- a/gopls/internal/golang/snapshot.go +++ b/gopls/internal/golang/snapshot.go @@ -52,11 +52,10 @@ func WidestPackageForFile(ctx context.Context, snapshot *cache.Snapshot, uri pro } func selectPackageForFile(ctx context.Context, snapshot *cache.Snapshot, uri protocol.DocumentURI, selector func([]*metadata.Package) *metadata.Package) (*cache.Package, *parsego.File, error) { - mps, err := snapshot.MetadataForFile(ctx, uri) + mps, err := snapshot.MetadataForFile(ctx, uri, true) if err != nil { return nil, nil, err } - metadata.RemoveIntermediateTestVariants(&mps) if len(mps) == 0 { return nil, nil, fmt.Errorf("no package metadata for file %s", uri) } diff --git a/gopls/internal/mcp/instructions.md b/gopls/internal/mcp/instructions.md index 25b8245a976..6a528696768 100644 --- a/gopls/internal/mcp/instructions.md +++ b/gopls/internal/mcp/instructions.md @@ -33,14 +33,15 @@ The editing workflow is iterative. You should cycle through these steps until th 1. **Read first**: Before making any edits, follow the Read Workflow to understand the user's request and the relevant code. -2. **Find references**: Before modifying the definition of any exported symbol, use the `go_symbol_references` tool to find all references to that identifier. This is critical for understanding the impact of your change. Read the files containing references to evaluate if any further edits are required. +2. **Find references**: Before modifying the definition of any symbol, use the `go_symbol_references` tool to find all references to that identifier. This is critical for understanding the impact of your change. Read the files containing references to evaluate if any further edits are required. EXAMPLE: `go_symbol_references({"file":"/path/to/server.go","symbol":"Server.Run"})` -3. **Make edits**: Make the primary edit, as well as any edits to references you identified in the previous step. +3. **Make edits**: Make the required edits, including edits to references you identified in the previous step. Don't proceed to the next step until all planned edits are complete. 4. **Check for errors**: After every code modification, you MUST call the `go_diagnostics` tool. Pass the paths of the files you have edited. This tool will report any build or analysis errors. EXAMPLE: `go_diagnostics({"files":["/path/to/server.go"]})` -5. **Fix errors**: If `go_diagnostics` reports any errors, fix them. The tool may provide suggested quick fixes in the form of diffs. You should review these diffs and apply them if they are correct. Once you've applied a fix, re-run `go_diagnostics` to confirm that the issue is resolved. It is OK to ignore 'hint' or 'info' diagnostics if they are not relevant to the current task. +5. **Fix errors**: If `go_diagnostics` reports any errors, fix them. The tool may provide suggested quick fixes in the form of diffs. You should review these diffs and apply them if they are correct. Once you've applied a fix, re-run `go_diagnostics` to confirm that the issue is resolved. It is OK to ignore 'hint' or 'info' diagnostics if they are not relevant to the current task. Note that Go diagnostic messages may contain a summary of the source code, which may not match its exact text. 6. **Run tests**: Once `go_diagnostics` reports no errors (and ONLY once there are no errors), run the tests for the packages you have changed. You can do this with `go test [packagePath...]`. Don't run `go test ./...` unless the user explicitly requests it, as doing so may slow down the iteration loop. + diff --git a/gopls/internal/mcp/mcp.go b/gopls/internal/mcp/mcp.go index a396d159339..9de7dfbb71f 100644 --- a/gopls/internal/mcp/mcp.go +++ b/gopls/internal/mcp/mcp.go @@ -25,7 +25,7 @@ import ( ) //go:embed instructions.md -var instructions string +var Instructions string // A handler implements various MCP tools for an LSP session. type handler struct { @@ -156,7 +156,7 @@ func newServer(session *cache.Session, lspServer protocol.Server) *mcp.Server { lspServer: lspServer, } mcpServer := mcp.NewServer("gopls", "v0.1.0", &mcp.ServerOptions{ - Instructions: instructions, + Instructions: Instructions, }) defaultTools := []*mcp.ServerTool{ diff --git a/gopls/internal/mod/hover.go b/gopls/internal/mod/hover.go index b9b026674fa..4c79f1cef53 100644 --- a/gopls/internal/mod/hover.go +++ b/gopls/internal/mod/hover.go @@ -343,7 +343,7 @@ func formatExplanation(text string, replaceMap map[module.Version]module.Version target := imp if strings.ToLower(options.LinkTarget) == "pkg.go.dev" { mod := req.Mod - // respect the repalcement when constructing a module link. + // respect the replacement when constructing a module link. if m, ok := replaceMap[req.Mod]; ok { // Have: 'replace A v1.2.3 => A vx.x.x' or 'replace A v1.2.3 => B vx.x.x'. mod = m diff --git a/gopls/internal/protocol/generate/main.go b/gopls/internal/protocol/generate/main.go index 57c6aa44c2b..e25cd4b6376 100644 --- a/gopls/internal/protocol/generate/main.go +++ b/gopls/internal/protocol/generate/main.go @@ -105,7 +105,6 @@ func writeclient() { } out.WriteString("}\n\n") out.WriteString(`func clientDispatch(ctx context.Context, client Client, reply jsonrpc2.Replier, r jsonrpc2.Request) (bool, error) { - defer recoverHandlerPanic(r.Method()) switch r.Method() { `) for _, k := range ccases.keys() { @@ -136,7 +135,6 @@ func writeserver() { } func serverDispatch(ctx context.Context, server Server, reply jsonrpc2.Replier, r jsonrpc2.Request) (bool, error) { - defer recoverHandlerPanic(r.Method()) switch r.Method() { `) for _, k := range scases.keys() { diff --git a/gopls/internal/protocol/protocol.go b/gopls/internal/protocol/protocol.go index 2d6d8173523..d92e4c52d44 100644 --- a/gopls/internal/protocol/protocol.go +++ b/gopls/internal/protocol/protocol.go @@ -11,7 +11,6 @@ import ( "fmt" "io" - "golang.org/x/tools/gopls/internal/util/bug" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/jsonrpc2" jsonrpc2_v2 "golang.org/x/tools/internal/jsonrpc2_v2" @@ -296,17 +295,3 @@ func NonNilSlice[T comparable](x []T) []T { } return x } - -func recoverHandlerPanic(method string) { - // Report panics in the handler goroutine, - // unless we have enabled the monitor, - // which reports all crashes. - if !true { - defer func() { - if x := recover(); x != nil { - bug.Reportf("panic in %s request", method) - panic(x) - } - }() - } -} diff --git a/gopls/internal/protocol/semtok/semtok.go b/gopls/internal/protocol/semtok/semtok.go index 86332d37e1a..99228391e6a 100644 --- a/gopls/internal/protocol/semtok/semtok.go +++ b/gopls/internal/protocol/semtok/semtok.go @@ -9,8 +9,8 @@ import "sort" // A Token provides the extent and semantics of a token. type Token struct { - Line, Start uint32 - Len uint32 + Line, Start uint32 // 0-based UTF-16 index + Len uint32 // in UTF-16 codes Type Type Modifiers []Modifier } diff --git a/gopls/internal/protocol/tsclient.go b/gopls/internal/protocol/tsclient.go index 51eef36b4bf..0dbb8261d22 100644 --- a/gopls/internal/protocol/tsclient.go +++ b/gopls/internal/protocol/tsclient.go @@ -62,7 +62,6 @@ type Client interface { } func clientDispatch(ctx context.Context, client Client, reply jsonrpc2.Replier, r jsonrpc2.Request) (bool, error) { - defer recoverHandlerPanic(r.Method()) switch r.Method() { case "$/logTrace": var params LogTraceParams diff --git a/gopls/internal/protocol/tsserver.go b/gopls/internal/protocol/tsserver.go index d09f118c171..e98553b68f0 100644 --- a/gopls/internal/protocol/tsserver.go +++ b/gopls/internal/protocol/tsserver.go @@ -168,7 +168,6 @@ type Server interface { } func serverDispatch(ctx context.Context, server Server, reply jsonrpc2.Replier, r jsonrpc2.Request) (bool, error) { - defer recoverHandlerPanic(r.Method()) switch r.Method() { case "$/progress": var params ProgressParams diff --git a/gopls/internal/server/command.go b/gopls/internal/server/command.go index f16d7545cc3..e7588b21760 100644 --- a/gopls/internal/server/command.go +++ b/gopls/internal/server/command.go @@ -292,13 +292,19 @@ func (c *commandHandler) AddTest(ctx context.Context, loc protocol.Location) (*p if deps.snapshot.FileKind(deps.fh) != file.Go { return fmt.Errorf("can't add test for non-Go file") } - docedits, err := golang.AddTestForFunc(ctx, deps.snapshot, loc) + docedits, show, err := golang.AddTestForFunc(ctx, deps.snapshot, loc) if err != nil { return err } - return applyChanges(ctx, c.s.client, docedits) + if err := applyChanges(ctx, c.s.client, docedits); err != nil { + return err + } + + if show != nil { + showDocumentImpl(ctx, c.s.client, protocol.URI(show.URI), &show.Range, c.s.options) + } + return nil }) - // TODO(hxjiang): move the cursor to the new test once edits applied. return result, err } @@ -709,7 +715,7 @@ func (c *commandHandler) Doc(ctx context.Context, args command.DocArgs) (protoco } // Compute package path and optional symbol fragment - // (e.g. "#Buffer.Len") from the the selection. + // (e.g. "#Buffer.Len") from the selection. pkgpath, fragment, _ := golang.DocFragment(pkg, pgf, start, end) // Direct the client to open the /pkg page. @@ -1800,8 +1806,13 @@ func (c *commandHandler) ModifyTags(ctx context.Context, args command.ModifyTags } m.Transform = transform + // Each command involves either adding or removing tags, depending on + // whether Add or Clear is set. if args.Add != "" { + countAddStructTags.Inc() m.Add = strings.Split(args.Add, ",") + } else if args.Clear { + countRemoveStructTags.Inc() } if args.AddOptions != "" { if options, err := optionsStringToMap(args.AddOptions); err != nil { diff --git a/gopls/internal/server/counters.go b/gopls/internal/server/counters.go index 1c5f47baa24..72095d71e37 100644 --- a/gopls/internal/server/counters.go +++ b/gopls/internal/server/counters.go @@ -35,3 +35,11 @@ var ( countRename = counter.New("gopls/rename") ) + +// Proposed counters for evaluating gopls refactoring codeactions add struct +// tags and remove struct tags. +var ( + countAddStructTags = counter.New("gopls/structtags:add") + + countRemoveStructTags = counter.New("gopls/structtags:remove") +) diff --git a/gopls/internal/server/general.go b/gopls/internal/server/general.go index 0464f4ca3b7..d00636c80e0 100644 --- a/gopls/internal/server/general.go +++ b/gopls/internal/server/general.go @@ -161,6 +161,9 @@ func (s *server) Initialize(ctx context.Context, params *protocol.ParamInitializ }, SignatureHelpProvider: &protocol.SignatureHelpOptions{ TriggerCharacters: []string{"(", ","}, + // Used to update or dismiss signature help when it's already active, + // typically after a call expression is closed. + RetriggerCharacters: []string{")"}, }, TextDocumentSync: &protocol.TextDocumentSyncOptions{ Change: protocol.Incremental, @@ -678,6 +681,10 @@ func recordClientInfo(clientName string) { case "Sublime Text LSP": // https://github.com/sublimelsp/LSP/blob/e608f878e7e9dd34aabe4ff0462540fadcd88fcc/plugin/core/sessions.py#L493 key = "gopls/client:sublimetext" + case "Windsurf": + key = "gopls/client:windsurf" + case "Cursor": + key = "gopls/client:cursor" default: // Accumulate at least a local counter for an unknown // client name, but also fall through to count it as diff --git a/gopls/internal/server/link.go b/gopls/internal/server/link.go index a98e2bc2688..e8092795fe7 100644 --- a/gopls/internal/server/link.go +++ b/gopls/internal/server/link.go @@ -101,7 +101,7 @@ func modLinks(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle) ([] } mod := req.Mod - // respect the repalcement when constructing a module link. + // respect the replacement when constructing a module link. if m, ok := pm.ReplaceMap[req.Mod]; ok { // Have: 'replace A v1.2.3 => A vx.x.x' or 'replace A v1.2.3 => B vx.x.x'. mod = m diff --git a/gopls/internal/server/signature_help.go b/gopls/internal/server/signature_help.go index e71d1f4706f..c8b6cb615c4 100644 --- a/gopls/internal/server/signature_help.go +++ b/gopls/internal/server/signature_help.go @@ -28,7 +28,7 @@ func (s *server) SignatureHelp(ctx context.Context, params *protocol.SignatureHe return nil, nil // empty result } - info, err := golang.SignatureHelp(ctx, snapshot, fh, params.Position) + info, err := golang.SignatureHelp(ctx, snapshot, fh, params) if err != nil { // TODO(rfindley): is this correct? Apparently, returning an error from // signatureHelp is distracting in some editors, though I haven't confirmed diff --git a/gopls/internal/template/completion.go b/gopls/internal/template/completion.go index dbb80cf2e3a..fb09ba4ba01 100644 --- a/gopls/internal/template/completion.go +++ b/gopls/internal/template/completion.go @@ -9,7 +9,7 @@ import ( "context" "fmt" "go/scanner" - "go/token" + gotoken "go/token" "strings" "golang.org/x/tools/gopls/internal/cache" @@ -19,7 +19,7 @@ import ( // information needed for completion type completer struct { - p *Parsed + p *parsed pos protocol.Position offset int // offset of the start of the Token ctx protocol.CompletionContext @@ -27,19 +27,21 @@ type completer struct { } func Completion(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, pos protocol.Position, context protocol.CompletionContext) (*protocol.CompletionList, error) { - all := New(snapshot.Templates()) + all := parseSet(snapshot.Templates()) var start int // the beginning of the Token (completed or not) syms := make(map[string]symbol) - var p *Parsed - for fn, fc := range all.files { + var p *parsed + for uri, fc := range all.files { // collect symbols from all template files filterSyms(syms, fc.symbols) - if fn.Path() != fh.URI().Path() { + if uri.Path() != fh.URI().Path() { continue } - if start = inTemplate(fc, pos); start == -1 { - return nil, nil + offset, err := enclosingTokenStart(fc, pos) + if err != nil { + return nil, err } + start = offset p = fc } if p == nil { @@ -49,7 +51,7 @@ func Completion(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p c := completer{ p: p, pos: pos, - offset: start + len(Left), + offset: start + len(lbraces), ctx: context, syms: syms, } @@ -74,22 +76,28 @@ func filterSyms(syms map[string]symbol, ns []symbol) { } } -// return the starting position of the enclosing token, or -1 if none -func inTemplate(fc *Parsed, pos protocol.Position) int { +// enclosingTokenStart returns the start offset of the enclosing token. +// A (-1, non-nil) result indicates "no enclosing token". +func enclosingTokenStart(fc *parsed, pos protocol.Position) (int, error) { // pos is the pos-th character. if the cursor is at the beginning // of the file, pos is 0. That is, we've only seen characters before pos // 1. pos might be in a Token, return tk.Start // 2. pos might be after an elided but before a Token, return elided // 3. return -1 for false - offset := fc.FromPosition(pos) - // this could be a binary search, as the tokens are ordered + offset, err := fc.mapper.PositionOffset(pos) + if err != nil { + return 0, err + } + + // TODO: opt: this could be a binary search, as the tokens are ordered for _, tk := range fc.tokens { - if tk.Start+len(Left) <= offset && offset+len(Right) <= tk.End { - return tk.Start + if tk.start+len(lbraces) <= offset && offset+len(rbraces) <= tk.end { + return tk.start, nil } } + for _, x := range fc.elided { - if x+len(Left) > offset { + if x+len(lbraces) > offset { // fc.elided is sorted, and x is the position where a '{{' was replaced // by ' '. We consider only cases where the replaced {{ is to the left // of the cursor. @@ -97,11 +105,11 @@ func inTemplate(fc *Parsed, pos protocol.Position) int { } // If the interval [x,offset] does not contain Left or Right // then provide completions. (do we need the test for Right?) - if !bytes.Contains(fc.buf[x:offset], Left) && !bytes.Contains(fc.buf[x:offset], Right) { - return x + if !bytes.Contains(fc.buf[x:offset], lbraces) && !bytes.Contains(fc.buf[x:offset], rbraces) { + return x, nil } } - return -1 + return -1, fmt.Errorf("no token enclosing %d", pos) } var ( @@ -115,7 +123,10 @@ var ( // The error return is always nil. func (c *completer) complete() (*protocol.CompletionList, error) { ans := &protocol.CompletionList{IsIncomplete: true, Items: []protocol.CompletionItem{}} - start := c.p.FromPosition(c.pos) + start, err := c.p.mapper.PositionOffset(c.pos) + if err != nil { + return ans, err + } sofar := c.p.buf[c.offset:start] if len(sofar) == 0 || sofar[len(sofar)-1] == ' ' || sofar[len(sofar)-1] == '\t' { return ans, nil @@ -194,22 +205,22 @@ func (c *completer) complete() (*protocol.CompletionList, error) { // version of c.analyze that uses go/scanner. func scan(buf []byte) []string { - fset := token.NewFileSet() + fset := gotoken.NewFileSet() fp := fset.AddFile("", -1, len(buf)) var sc scanner.Scanner - sc.Init(fp, buf, func(pos token.Position, msg string) {}, scanner.ScanComments) + sc.Init(fp, buf, func(pos gotoken.Position, msg string) {}, scanner.ScanComments) ans := make([]string, 0, 10) // preallocating gives a measurable savings for { _, tok, lit := sc.Scan() // tok is an int - if tok == token.EOF { + if tok == gotoken.EOF { break // done - } else if tok == token.SEMICOLON && lit == "\n" { + } else if tok == gotoken.SEMICOLON && lit == "\n" { continue // don't care, but probably can't happen - } else if tok == token.PERIOD { + } else if tok == gotoken.PERIOD { ans = append(ans, ".") // lit is empty - } else if tok == token.IDENT && len(ans) > 0 && ans[len(ans)-1] == "." { + } else if tok == gotoken.IDENT && len(ans) > 0 && ans[len(ans)-1] == "." { ans[len(ans)-1] = "." + lit - } else if tok == token.IDENT && len(ans) > 0 && ans[len(ans)-1] == "$" { + } else if tok == gotoken.IDENT && len(ans) > 0 && ans[len(ans)-1] == "$" { ans[len(ans)-1] = "$" + lit } else if lit != "" { ans = append(ans, lit) diff --git a/gopls/internal/template/completion_test.go b/gopls/internal/template/completion_test.go index 8e1bdbf0535..279864ab80d 100644 --- a/gopls/internal/template/completion_test.go +++ b/gopls/internal/template/completion_test.go @@ -19,13 +19,13 @@ func init() { type tparse struct { marked string // ^ shows where to ask for completions. (The user just typed the following character.) - wanted []string // expected completions + wanted []string // expected completions; nil => no enclosing token } // Test completions in templates that parse enough (if completion needs symbols) // Seen characters up to the ^ func TestParsed(t *testing.T) { - var tests = []tparse{ + for _, test := range []tparse{ {"{{x}}{{12. xx^", nil}, // https://github.com/golang/go/issues/50430 {``, nil}, {"{{i^f}}", []string{"index", "if"}}, @@ -50,53 +50,56 @@ func TestParsed(t *testing.T) { {"{{`e^", []string{}}, {"{{`No i^", []string{}}, // example of why go/scanner is used {"{{xavier}}{{12. x^", []string{"xavier"}}, - } - for _, tx := range tests { - c := testCompleter(t, tx) - var v []string - if c != nil { - ans, _ := c.complete() - for _, a := range ans.Items { - v = append(v, a.Label) + } { + t.Run("", func(t *testing.T) { + var got []string + if c := testCompleter(t, test); c != nil { + ans, _ := c.complete() + for _, a := range ans.Items { + got = append(got, a.Label) + } } - } - if len(v) != len(tx.wanted) { - t.Errorf("%q: got %q, wanted %q %d,%d", tx.marked, v, tx.wanted, len(v), len(tx.wanted)) - continue - } - sort.Strings(tx.wanted) - sort.Strings(v) - for i := 0; i < len(v); i++ { - if tx.wanted[i] != v[i] { - t.Errorf("%q at %d: got %v, wanted %v", tx.marked, i, v, tx.wanted) - break + if len(got) != len(test.wanted) { + t.Fatalf("%q: got %q, wanted %q %d,%d", test.marked, got, test.wanted, len(got), len(test.wanted)) } - } + sort.Strings(test.wanted) + sort.Strings(got) + for i := 0; i < len(got); i++ { + if test.wanted[i] != got[i] { + t.Fatalf("%q at %d: got %v, wanted %v", test.marked, i, got, test.wanted) + } + } + }) } } func testCompleter(t *testing.T, tx tparse) *completer { - t.Helper() // seen chars up to ^ - col := strings.Index(tx.marked, "^") + offset := strings.Index(tx.marked, "^") buf := strings.Replace(tx.marked, "^", "", 1) - p := parseBuffer([]byte(buf)) - pos := protocol.Position{Line: 0, Character: uint32(col)} - if p.ParseErr != nil { - log.Printf("%q: %v", tx.marked, p.ParseErr) + p := parseBuffer("", []byte(buf)) + if p.parseErr != nil { + t.Logf("%q: %v", tx.marked, p.parseErr) + } + pos, err := p.mapper.OffsetPosition(offset) + if err != nil { + t.Fatal(err) } - offset := inTemplate(p, pos) - if offset == -1 { - return nil + + start, err := enclosingTokenStart(p, pos) + if err != nil { + if start == -1 { + return nil // no enclosing token + } + t.Fatal(err) } syms := make(map[string]symbol) filterSyms(syms, p.symbols) - c := &completer{ + return &completer{ p: p, - pos: protocol.Position{Line: 0, Character: uint32(col)}, - offset: offset + len(Left), + pos: pos, + offset: start + len(lbraces), ctx: protocol.CompletionContext{TriggerKind: protocol.Invoked}, syms: syms, } - return c } diff --git a/gopls/internal/template/highlight.go b/gopls/internal/template/highlight.go index c6b0c0f778e..8a8244d4c36 100644 --- a/gopls/internal/template/highlight.go +++ b/gopls/internal/template/highlight.go @@ -19,32 +19,37 @@ func Highlight(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, lo if err != nil { return nil, err } - p := parseBuffer(buf) - pos := p.FromPosition(loc) - var ans []protocol.DocumentHighlight - if p.ParseErr == nil { + p := parseBuffer(fh.URI(), buf) + pos, err := p.mapper.PositionOffset(loc) + if err != nil { + return nil, err + } + + if p.parseErr == nil { for _, s := range p.symbols { - if s.start <= pos && pos < s.start+s.length { + if s.start <= pos && pos < s.start+s.len { return markSymbols(p, s) } } } + // these tokens exist whether or not there was a parse error // (symbols require a successful parse) for _, tok := range p.tokens { - if tok.Start <= pos && pos < tok.End { - wordAt := findWordAt(p, pos) + if tok.start <= pos && pos < tok.end { + wordAt := wordAt(p.buf, pos) if len(wordAt) > 0 { return markWordInToken(p, wordAt) } } } - // find the 'word' at pos, etc: someday + + // TODO: find the 'word' at pos, etc: someday // until then we get the default action, which doesn't respect word boundaries - return ans, nil + return nil, nil } -func markSymbols(p *Parsed, sym symbol) ([]protocol.DocumentHighlight, error) { +func markSymbols(p *parsed, sym symbol) ([]protocol.DocumentHighlight, error) { var ans []protocol.DocumentHighlight for _, s := range p.symbols { if s.name == sym.name { @@ -52,8 +57,12 @@ func markSymbols(p *Parsed, sym symbol) ([]protocol.DocumentHighlight, error) { if s.vardef { kind = protocol.Write } + rng, err := p.mapper.OffsetRange(s.offsets()) + if err != nil { + return nil, err + } ans = append(ans, protocol.DocumentHighlight{ - Range: p.Range(s.start, s.length), + Range: rng, Kind: kind, }) } @@ -62,17 +71,21 @@ func markSymbols(p *Parsed, sym symbol) ([]protocol.DocumentHighlight, error) { } // A token is {{...}}, and this marks words in the token that equal the give word -func markWordInToken(p *Parsed, wordAt string) ([]protocol.DocumentHighlight, error) { +func markWordInToken(p *parsed, wordAt string) ([]protocol.DocumentHighlight, error) { var ans []protocol.DocumentHighlight pat, err := regexp.Compile(fmt.Sprintf(`\b%s\b`, wordAt)) if err != nil { return nil, fmt.Errorf("%q: unmatchable word (%v)", wordAt, err) } for _, tok := range p.tokens { - got := pat.FindAllIndex(p.buf[tok.Start:tok.End], -1) - for i := range got { + matches := pat.FindAllIndex(p.buf[tok.start:tok.end], -1) + for _, match := range matches { + rng, err := p.mapper.OffsetRange(match[0], match[1]) + if err != nil { + return nil, err + } ans = append(ans, protocol.DocumentHighlight{ - Range: p.Range(got[i][0], got[i][1]-got[i][0]), + Range: rng, Kind: protocol.Text, }) } @@ -80,18 +93,20 @@ func markWordInToken(p *Parsed, wordAt string) ([]protocol.DocumentHighlight, er return ans, nil } -var wordRe = regexp.MustCompile(`[$]?\w+$`) -var moreRe = regexp.MustCompile(`^[$]?\w+`) - -// findWordAt finds the word the cursor is in (meaning in or just before) -func findWordAt(p *Parsed, pos int) string { - if pos >= len(p.buf) { - return "" // can't happen, as we are called with pos < tok.End +// wordAt returns the word the cursor is in (meaning in or just before) +func wordAt(buf []byte, pos int) string { + if pos >= len(buf) { + return "" } - after := moreRe.Find(p.buf[pos:]) + after := moreRe.Find(buf[pos:]) if len(after) == 0 { return "" // end of the word } - got := wordRe.Find(p.buf[:pos+len(after)]) + got := wordRe.Find(buf[:pos+len(after)]) return string(got) } + +var ( + wordRe = regexp.MustCompile(`[$]?\w+$`) + moreRe = regexp.MustCompile(`^[$]?\w+`) +) diff --git a/gopls/internal/template/implementations.go b/gopls/internal/template/implementations.go index 5ae4bf2a182..7c69c01c184 100644 --- a/gopls/internal/template/implementations.go +++ b/gopls/internal/template/implementations.go @@ -36,46 +36,61 @@ func diagnoseOne(fh file.Handle) []*cache.Diagnostic { // snapshot's template files buf, err := fh.Content() if err != nil { - // Is a Diagnostic with no Range useful? event.Error also? + // TODO: Is a Diagnostic with no Range useful? event.Error also? msg := fmt.Sprintf("failed to read %s (%v)", fh.URI().Path(), err) - d := cache.Diagnostic{Message: msg, Severity: protocol.SeverityError, URI: fh.URI(), - Source: cache.TemplateError} - return []*cache.Diagnostic{&d} + return []*cache.Diagnostic{{ + Message: msg, + Severity: protocol.SeverityError, + URI: fh.URI(), + Source: cache.TemplateError, + }} } - p := parseBuffer(buf) - if p.ParseErr == nil { + p := parseBuffer(fh.URI(), buf) + if p.parseErr == nil { return nil } - unknownError := func(msg string) []*cache.Diagnostic { - s := fmt.Sprintf("malformed template error %q: %s", p.ParseErr.Error(), msg) - d := cache.Diagnostic{ - Message: s, Severity: protocol.SeverityError, Range: p.Range(p.nls[0], 1), - URI: fh.URI(), Source: cache.TemplateError} - return []*cache.Diagnostic{&d} + + errorf := func(format string, args ...any) []*cache.Diagnostic { + msg := fmt.Sprintf("malformed template error %q: %s", + p.parseErr.Error(), + fmt.Sprintf(format, args)) + rng, err := p.mapper.OffsetRange(0, 1) // first UTF-16 code + if err != nil { + rng = protocol.Range{} // start of file + } + return []*cache.Diagnostic{{ + Message: msg, + Severity: protocol.SeverityError, + Range: rng, + URI: fh.URI(), + Source: cache.TemplateError, + }} } + // errors look like `template: :40: unexpected "}" in operand` // so the string needs to be parsed - matches := errRe.FindStringSubmatch(p.ParseErr.Error()) + matches := errRe.FindStringSubmatch(p.parseErr.Error()) if len(matches) != 3 { - msg := fmt.Sprintf("expected 3 matches, got %d (%v)", len(matches), matches) - return unknownError(msg) + return errorf("expected 3 matches, got %d (%v)", len(matches), matches) } lineno, err := strconv.Atoi(matches[1]) if err != nil { - msg := fmt.Sprintf("couldn't convert %q to int, %v", matches[1], err) - return unknownError(msg) + return errorf("couldn't convert %q to int, %v", matches[1], err) } msg := matches[2] - d := cache.Diagnostic{Message: msg, Severity: protocol.SeverityError, - Source: cache.TemplateError} - start := p.nls[lineno-1] - if lineno < len(p.nls) { - size := p.nls[lineno] - start - d.Range = p.Range(start, size) - } else { - d.Range = p.Range(start, 1) - } - return []*cache.Diagnostic{&d} + + // Compute the range for the whole (1-based) line. + rng, err := lineRange(p.mapper, lineno) + if err != nil { + return errorf("invalid position: %v", err) + } + + return []*cache.Diagnostic{{ + Message: msg, + Severity: protocol.SeverityError, + Range: rng, + Source: cache.TemplateError, + }} } // Definition finds the definitions of the symbol at loc. It @@ -90,13 +105,17 @@ func Definition(snapshot *cache.Snapshot, fh file.Handle, loc protocol.Position) sym := x.name ans := []protocol.Location{} // PJW: this is probably a pattern to abstract - a := New(snapshot.Templates()) - for k, p := range a.files { + a := parseSet(snapshot.Templates()) + for _, p := range a.files { for _, s := range p.symbols { if !s.vardef || s.name != sym { continue } - ans = append(ans, k.Location(p.Range(s.start, s.length))) + loc, err := p.mapper.OffsetLocation(s.offsets()) + if err != nil { + return nil, err + } + ans = append(ans, loc) } } return ans, nil @@ -104,44 +123,60 @@ func Definition(snapshot *cache.Snapshot, fh file.Handle, loc protocol.Position) func Hover(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, position protocol.Position) (*protocol.Hover, error) { sym, p, err := symAtPosition(fh, position) - if sym == nil || err != nil { + if err != nil { return nil, err } - ans := protocol.Hover{Range: p.Range(sym.start, sym.length), Contents: protocol.MarkupContent{Kind: protocol.Markdown}} + + var value string switch sym.kind { case protocol.Function: - ans.Contents.Value = fmt.Sprintf("function: %s", sym.name) + value = fmt.Sprintf("function: %s", sym.name) case protocol.Variable: - ans.Contents.Value = fmt.Sprintf("variable: %s", sym.name) + value = fmt.Sprintf("variable: %s", sym.name) case protocol.Constant: - ans.Contents.Value = fmt.Sprintf("constant %s", sym.name) + value = fmt.Sprintf("constant %s", sym.name) case protocol.Method: // field or method - ans.Contents.Value = fmt.Sprintf("%s: field or method", sym.name) + value = fmt.Sprintf("%s: field or method", sym.name) case protocol.Package: // template use, template def (PJW: do we want two?) - ans.Contents.Value = fmt.Sprintf("template %s\n(add definition)", sym.name) + value = fmt.Sprintf("template %s\n(add definition)", sym.name) case protocol.Namespace: - ans.Contents.Value = fmt.Sprintf("template %s defined", sym.name) + value = fmt.Sprintf("template %s defined", sym.name) case protocol.Number: - ans.Contents.Value = "number" + value = "number" case protocol.String: - ans.Contents.Value = "string" + value = "string" case protocol.Boolean: - ans.Contents.Value = "boolean" + value = "boolean" default: - ans.Contents.Value = fmt.Sprintf("oops, sym=%#v", sym) + value = fmt.Sprintf("oops, sym=%#v", sym) } - return &ans, nil + + rng, err := p.mapper.OffsetRange(sym.offsets()) + if err != nil { + return nil, err + } + + return &protocol.Hover{ + Range: rng, + Contents: protocol.MarkupContent{ + Kind: protocol.Markdown, + Value: value, + }, + }, nil } func References(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, params *protocol.ReferenceParams) ([]protocol.Location, error) { sym, _, err := symAtPosition(fh, params.Position) - if sym == nil || err != nil || sym.name == "" { + if err != nil { return nil, err } + if sym.name == "" { + return nil, fmt.Errorf("no symbol at position") + } ans := []protocol.Location{} - a := New(snapshot.Templates()) - for k, p := range a.files { + a := parseSet(snapshot.Templates()) + for _, p := range a.files { for _, s := range p.symbols { if s.name != sym.name { continue @@ -149,10 +184,14 @@ func References(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p if s.vardef && !params.Context.IncludeDeclaration { continue } - ans = append(ans, k.Location(p.Range(s.start, s.length))) + loc, err := p.mapper.OffsetLocation(s.offsets()) + if err != nil { + return nil, err + } + ans = append(ans, loc) } } - // do these need to be sorted? (a.files is a map) + // TODO: do these need to be sorted? (a.files is a map) return ans, nil } @@ -165,47 +204,54 @@ func SemanticTokens(ctx context.Context, snapshot *cache.Snapshot, spn protocol. if err != nil { return nil, err } - p := parseBuffer(buf) + p := parseBuffer(fh.URI(), buf) var items []semtok.Token - add := func(line, start, len uint32) { - if len == 0 { - return // vscode doesn't like 0-length Tokens + for _, t := range p.tokens { + if t.start == t.end { + continue // vscode doesn't like 0-length tokens + } + pos, err := p.mapper.OffsetPosition(t.start) + if err != nil { + return nil, err } // TODO(adonovan): don't ignore the rng restriction, if any. items = append(items, semtok.Token{ - Line: line, - Start: start, - Len: len, + Line: pos.Line, + Start: pos.Character, + Len: uint32(protocol.UTF16Len(p.buf[t.start:t.end])), Type: semtok.TokMacro, }) } - - for _, t := range p.Tokens() { - if t.Multiline { - la, ca := p.LineCol(t.Start) - lb, cb := p.LineCol(t.End) - add(la, ca, p.RuneCount(la, ca, 0)) - for l := la + 1; l < lb; l++ { - add(l, 0, p.RuneCount(l, 0, 0)) - } - add(lb, 0, p.RuneCount(lb, 0, cb)) - continue - } - sz, err := p.TokenSize(t) - if err != nil { - return nil, err - } - line, col := p.LineCol(t.Start) - add(line, col, uint32(sz)) - } - ans := &protocol.SemanticTokens{ + return &protocol.SemanticTokens{ Data: semtok.Encode(items, nil, nil), // for small cache, some day. for now, the LSP client ignores this // (that is, when the LSP client starts returning these, we can cache) ResultID: fmt.Sprintf("%v", time.Now()), - } - return ans, nil + }, nil } -// still need to do rename, etc +// TODO: still need to do rename, etc + +func symAtPosition(fh file.Handle, posn protocol.Position) (*symbol, *parsed, error) { + buf, err := fh.Content() + if err != nil { + return nil, nil, err + } + p := parseBuffer(fh.URI(), buf) + offset, err := p.mapper.PositionOffset(posn) + if err != nil { + return nil, nil, err + } + var syms []symbol + for _, s := range p.symbols { + if s.start <= offset && offset < s.start+s.len { + syms = append(syms, s) + } + } + if len(syms) == 0 { + return nil, p, fmt.Errorf("no symbol found") + } + sym := syms[0] + return &sym, p, nil +} diff --git a/gopls/internal/template/parse.go b/gopls/internal/template/parse.go index f1b26bbb14f..2050b32431d 100644 --- a/gopls/internal/template/parse.go +++ b/gopls/internal/template/parse.go @@ -10,105 +10,79 @@ package template import ( "bytes" - "context" "fmt" "io" "log" "regexp" - "runtime" "sort" "text/template" "text/template/parse" - "unicode/utf8" "golang.org/x/tools/gopls/internal/file" "golang.org/x/tools/gopls/internal/protocol" - "golang.org/x/tools/internal/event" ) var ( - Left = []byte("{{") - Right = []byte("}}") + lbraces = []byte("{{") + rbraces = []byte("}}") ) -type Parsed struct { - buf []byte //contents - lines [][]byte // needed?, other than for debugging? - elided []int // offsets where Left was replaced by blanks +type parsed struct { + buf []byte // contents + mapper *protocol.Mapper + elided []int // offsets where lbraces was replaced by blanks - // tokens are matched Left-Right pairs, computed before trying to parse - tokens []Token + // tokens are matched lbraces-rbraces pairs, computed before trying to parse + tokens []token // result of parsing named []*template.Template // the template and embedded templates - ParseErr error + parseErr error symbols []symbol stack []parse.Node // used while computing symbols - - // for mapping from offsets in buf to LSP coordinates - // See FromPosition() and LineCol() - nls []int // offset of newlines before each line (nls[0]==-1) - lastnl int // last line seen - check int // used to decide whether to use lastnl or search through nls - nonASCII bool // are there any non-ascii runes in buf? } -// Token is a single {{...}}. More precisely, Left...Right -type Token struct { - Start, End int // offset from start of template - Multiline bool +// A token is a single {{...}}. +type token struct { + start, end int // 0-based byte offset from start of template } -// All contains the Parse of all the template files -type All struct { - files map[protocol.DocumentURI]*Parsed +// set contains the Parse of all the template files +type set struct { + files map[protocol.DocumentURI]*parsed } -// New returns the Parses of the snapshot's tmpl files +// parseSet returns the set of the snapshot's tmpl files // (maybe cache these, but then avoiding import cycles needs code rearrangements) -func New(tmpls map[protocol.DocumentURI]file.Handle) *All { - all := make(map[protocol.DocumentURI]*Parsed) - for k, v := range tmpls { - buf, err := v.Content() - if err != nil { // PJW: decide what to do with these errors - log.Printf("failed to read %s (%v)", v.URI().Path(), err) +// +// TODO(adonovan): why doesn't parseSet return an error? +func parseSet(tmpls map[protocol.DocumentURI]file.Handle) *set { + all := make(map[protocol.DocumentURI]*parsed) + for uri, fh := range tmpls { + buf, err := fh.Content() + if err != nil { + // TODO(pjw): decide what to do with these errors + log.Printf("failed to read %s (%v)", fh.URI().Path(), err) continue } - all[k] = parseBuffer(buf) + all[uri] = parseBuffer(uri, buf) } - return &All{files: all} + return &set{files: all} } -func parseBuffer(buf []byte) *Parsed { - ans := &Parsed{ - buf: buf, - check: -1, - nls: []int{-1}, +func parseBuffer(uri protocol.DocumentURI, buf []byte) *parsed { + ans := &parsed{ + buf: buf, + mapper: protocol.NewMapper(uri, buf), } if len(buf) == 0 { return ans } - // how to compute allAscii... - for _, b := range buf { - if b >= utf8.RuneSelf { - ans.nonASCII = true - break - } - } - if buf[len(buf)-1] != '\n' { - ans.buf = append(buf, '\n') - } - for i, p := range ans.buf { - if p == '\n' { - ans.nls = append(ans.nls, i) - } - } ans.setTokens() // ans.buf may be a new []byte - ans.lines = bytes.Split(ans.buf, []byte{'\n'}) t, err := template.New("").Parse(string(ans.buf)) if err != nil { funcs := make(template.FuncMap) - for t == nil && ans.ParseErr == nil { + for t == nil && ans.parseErr == nil { // in 1.17 it may be possible to avoid getting this error // template: :2: function "foo" not defined matches := parseErrR.FindStringSubmatch(err.Error()) @@ -118,7 +92,7 @@ func parseBuffer(buf []byte) *Parsed { t, err = template.New("").Funcs(funcs).Parse(string(ans.buf)) continue } - ans.ParseErr = err // unfixed error + ans.parseErr = err // unfixed error return ans } } @@ -129,8 +103,8 @@ func parseBuffer(buf []byte) *Parsed { ans.findSymbols() if t.Name() != "" { // defining a template. The pos is just after {{define...}} (or {{block...}}?) - at, sz := ans.FindLiteralBefore(int(t.Root.Pos)) - s := symbol{start: at, length: sz, name: t.Name(), kind: protocol.Namespace, vardef: true} + at, sz := ans.findLiteralBefore(int(t.Root.Pos)) + s := symbol{start: at, len: sz, name: t.Name(), kind: protocol.Namespace, vardef: true} ans.symbols = append(ans.symbols, s) } } @@ -148,11 +122,10 @@ func parseBuffer(buf []byte) *Parsed { return ans } -// FindLiteralBefore locates the first preceding string literal -// returning its position and length in buf -// or returns -1 if there is none. +// findLiteralBefore locates the first preceding string literal +// returning its offset and length in buf or (-1, 0) if there is none. // Assume double-quoted string rather than backquoted string for now. -func (p *Parsed) FindLiteralBefore(pos int) (int, int) { +func (p *parsed) findLiteralBefore(pos int) (int, int) { left, right := -1, -1 for i := pos - 1; i >= 0; i-- { if p.buf[i] != '"' { @@ -175,7 +148,7 @@ var ( parseErrR = regexp.MustCompile(`template:.*function "([^"]+)" not defined`) ) -func (p *Parsed) setTokens() { +func (p *parsed) setTokens() { const ( // InRaw and InString only occur inside an action (SeenLeft) Start = iota @@ -207,46 +180,41 @@ func (p *Parsed) setTokens() { state = InString continue } - if bytes.HasPrefix(p.buf[n:], Right) { - right := n + len(Right) - tok := Token{Start: left, - End: right, - Multiline: bytes.Contains(p.buf[left:right], []byte{'\n'}), - } + if bytes.HasPrefix(p.buf[n:], rbraces) { + right := n + len(rbraces) + tok := token{start: left, end: right} p.tokens = append(p.tokens, tok) state = Start } - // If we see (unquoted) Left then the original left is probably the user + // If we see (unquoted) lbraces then the original left is probably the user // typing. Suppress the original left - if bytes.HasPrefix(p.buf[n:], Left) { + if bytes.HasPrefix(p.buf[n:], lbraces) { p.elideAt(left) left = n - n += len(Left) - 1 // skip the rest + n += len(lbraces) - 1 // skip the rest } case Start: - if bytes.HasPrefix(p.buf[n:], Left) { + if bytes.HasPrefix(p.buf[n:], lbraces) { left = n state = SeenLeft - n += len(Left) - 1 // skip the rest (avoids {{{ bug) + n += len(lbraces) - 1 // skip the rest (avoids {{{ bug) } } } // this error occurs after typing {{ at the end of the file if state != Start { - // Unclosed Left. remove the Left at left + // Unclosed lbraces. remove the lbraces at left p.elideAt(left) } } -func (p *Parsed) elideAt(left int) { +func (p *parsed) elideAt(left int) { if p.elided == nil { // p.buf is the same buffer that v.Read() returns, so copy it. // (otherwise the next time it's parsed, elided information is lost) - b := make([]byte, len(p.buf)) - copy(b, p.buf) - p.buf = b + p.buf = bytes.Clone(p.buf) } - for i := 0; i < len(Left); i++ { + for i := range lbraces { p.buf[left+i] = ' ' } p.elided = append(p.elided, left) @@ -261,164 +229,49 @@ func isEscaped(buf []byte) bool { return backSlashes%2 == 1 } -func (p *Parsed) Tokens() []Token { - return p.tokens -} - -// TODO(adonovan): the next 100 lines could perhaps replaced by use of protocol.Mapper. - -func (p *Parsed) utf16len(buf []byte) int { - cnt := 0 - if !p.nonASCII { - return len(buf) - } - // we need a utf16len(rune), but we don't have it - for _, r := range string(buf) { - cnt++ - if r >= 1<<16 { - cnt++ - } - } - return cnt -} - -func (p *Parsed) TokenSize(t Token) (int, error) { - if t.Multiline { - return -1, fmt.Errorf("TokenSize called with Multiline token %#v", t) - } - ans := p.utf16len(p.buf[t.Start:t.End]) - return ans, nil -} +// lineRange returns the range for the entire specified (1-based) line. +func lineRange(m *protocol.Mapper, line int) (protocol.Range, error) { + posn := protocol.Position{Line: uint32(line - 1)} -// RuneCount counts runes in line l, from col s to e -// (e==0 for end of line. called only for multiline tokens) -func (p *Parsed) RuneCount(l, s, e uint32) uint32 { - start := p.nls[l] + 1 + int(s) - end := p.nls[l] + 1 + int(e) - if e == 0 || end > p.nls[l+1] { - end = p.nls[l+1] + // start of line + start, err := m.PositionOffset(posn) + if err != nil { + return protocol.Range{}, err } - return uint32(utf8.RuneCount(p.buf[start:end])) -} -// LineCol converts from a 0-based byte offset to 0-based line, col. col in runes -func (p *Parsed) LineCol(x int) (uint32, uint32) { - if x < p.check { - p.lastnl = 0 - } - p.check = x - for i := p.lastnl; i < len(p.nls); i++ { - if p.nls[i] <= x { - continue - } - p.lastnl = i - var count int - if i > 0 && x == p.nls[i-1] { // \n - count = 0 - } else { - count = p.utf16len(p.buf[p.nls[i-1]+1 : x]) - } - return uint32(i - 1), uint32(count) - } - if x == len(p.buf)-1 { // trailing \n - return uint32(len(p.nls) - 1), 0 - } - // shouldn't happen - for i := 1; i < 4; i++ { - _, f, l, ok := runtime.Caller(i) - if !ok { - break - } - log.Printf("%d: %s:%d", i, f, l) + // end of line (or file) + posn.Line++ + end := len(m.Content) // EOF + if offset, err := m.PositionOffset(posn); err != nil { + end = offset - len("\n") } - msg := fmt.Errorf("LineCol off the end, %d of %d, nls=%v, %q", x, len(p.buf), p.nls, p.buf[x:]) - event.Error(context.Background(), "internal error", msg) - return 0, 0 + return m.OffsetRange(start, end) } -// Position produces a protocol.Position from an offset in the template -func (p *Parsed) Position(pos int) protocol.Position { - line, col := p.LineCol(pos) - return protocol.Position{Line: line, Character: col} -} +// -- debugging -- -func (p *Parsed) Range(x, length int) protocol.Range { - line, col := p.LineCol(x) - ans := protocol.Range{ - Start: protocol.Position{Line: line, Character: col}, - End: protocol.Position{Line: line, Character: col + uint32(length)}, - } - return ans -} - -// FromPosition translates a protocol.Position into an offset into the template -func (p *Parsed) FromPosition(x protocol.Position) int { - l, c := int(x.Line), int(x.Character) - if l >= len(p.nls) || p.nls[l]+1 >= len(p.buf) { - // paranoia to avoid panic. return the largest offset - return len(p.buf) - } - line := p.buf[p.nls[l]+1:] - cnt := 0 - for w := range string(line) { - if cnt >= c { - return w + p.nls[l] + 1 - } - cnt++ - } - // do we get here? NO - pos := int(x.Character) + p.nls[int(x.Line)] + 1 - event.Error(context.Background(), "internal error", fmt.Errorf("surprise %#v", x)) - return pos -} - -func symAtPosition(fh file.Handle, loc protocol.Position) (*symbol, *Parsed, error) { - buf, err := fh.Content() - if err != nil { - return nil, nil, err - } - p := parseBuffer(buf) - pos := p.FromPosition(loc) - syms := p.SymsAtPos(pos) - if len(syms) == 0 { - return nil, p, fmt.Errorf("no symbol found") - } - if len(syms) > 1 { - log.Printf("Hover: %d syms, not 1 %v", len(syms), syms) - } - sym := syms[0] - return &sym, p, nil -} - -func (p *Parsed) SymsAtPos(pos int) []symbol { - ans := []symbol{} - for _, s := range p.symbols { - if s.start <= pos && pos < s.start+s.length { - ans = append(ans, s) - } - } - return ans +func (p *parsed) writeNode(w io.Writer, n parse.Node) { + wr := wrNode{p: p, w: w} + wr.writeNode(n, "") } type wrNode struct { - p *Parsed + p *parsed w io.Writer } -// WriteNode is for debugging -func (p *Parsed) WriteNode(w io.Writer, n parse.Node) { - wr := wrNode{p: p, w: w} - wr.writeNode(n, "") -} - func (wr wrNode) writeNode(n parse.Node, indent string) { if n == nil { return } at := func(pos parse.Pos) string { - line, col := wr.p.LineCol(int(pos)) - return fmt.Sprintf("(%d)%v:%v", pos, line, col) + offset := int(pos) + posn, err := wr.p.mapper.OffsetPosition(offset) + if err != nil { + return fmt.Sprintf("", pos, err) + } + return fmt.Sprintf("(%d)%v:%v", pos, posn.Line, posn.Character) } switch x := n.(type) { case *parse.ActionNode: @@ -489,16 +342,3 @@ func (wr wrNode) writeNode(n parse.Node, indent string) { wr.writeNode(&x.BranchNode, indent+". ") } } - -var kindNames = []string{"", "File", "Module", "Namespace", "Package", "Class", "Method", "Property", - "Field", "Constructor", "Enum", "Interface", "Function", "Variable", "Constant", "String", - "Number", "Boolean", "Array", "Object", "Key", "Null", "EnumMember", "Struct", "Event", - "Operator", "TypeParameter"} - -func kindStr(k protocol.SymbolKind) string { - n := int(k) - if n < 1 || n >= len(kindNames) { - return fmt.Sprintf("?SymbolKind %d?", n) - } - return kindNames[n] -} diff --git a/gopls/internal/template/parse_test.go b/gopls/internal/template/parse_test.go index 345f52347fa..dc023f34bd9 100644 --- a/gopls/internal/template/parse_test.go +++ b/gopls/internal/template/parse_test.go @@ -4,51 +4,64 @@ package template -import ( - "strings" - "testing" -) +import "testing" -type datum struct { - buf string - cnt int - syms []string // the symbols in the parse of buf -} - -var tmpl = []datum{{` +func TestSymbols(t *testing.T) { + for i, test := range []struct { + buf string + wantNamed int // expected number of named templates + syms []string // expected symbols (start, len, name, kind, def?) + }{ + {` {{if (foo .X.Y)}}{{$A := "hi"}}{{.Z $A}}{{else}} {{$A.X 12}} {{foo (.X.Y) 23 ($A.Zü)}} -{{end}}`, 1, []string{"{7,3,foo,Function,false}", "{12,1,X,Method,false}", - "{14,1,Y,Method,false}", "{21,2,$A,Variable,true}", "{26,2,,String,false}", - "{35,1,Z,Method,false}", "{38,2,$A,Variable,false}", - "{53,2,$A,Variable,false}", "{56,1,X,Method,false}", "{57,2,,Number,false}", - "{64,3,foo,Function,false}", "{70,1,X,Method,false}", - "{72,1,Y,Method,false}", "{75,2,,Number,false}", "{80,2,$A,Variable,false}", - "{83,2,Zü,Method,false}", "{94,3,,Constant,false}"}}, - - {`{{define "zzz"}}{{.}}{{end}} -{{template "zzz"}}`, 2, []string{"{10,3,zzz,Namespace,true}", "{18,1,dot,Variable,false}", - "{41,3,zzz,Package,false}"}}, - - {`{{block "aaa" foo}}b{{end}}`, 2, []string{"{9,3,aaa,Namespace,true}", - "{9,3,aaa,Package,false}", "{14,3,foo,Function,false}", "{19,1,,Constant,false}"}}, - {"", 0, nil}, -} - -func TestSymbols(t *testing.T) { - for i, x := range tmpl { - got := parseBuffer([]byte(x.buf)) - if got.ParseErr != nil { - t.Errorf("error:%v", got.ParseErr) +{{end}}`, 1, []string{ + "{7,3,foo,Function,false}", + "{12,1,X,Method,false}", + "{14,1,Y,Method,false}", + "{21,2,$A,Variable,true}", + "{26,4,,String,false}", + "{35,1,Z,Method,false}", + "{38,2,$A,Variable,false}", + "{53,2,$A,Variable,false}", + "{56,1,X,Method,false}", + "{57,2,,Number,false}", + "{64,3,foo,Function,false}", + "{70,1,X,Method,false}", + "{72,1,Y,Method,false}", + "{75,2,,Number,false}", + "{80,2,$A,Variable,false}", + "{83,3,Zü,Method,false}", + "{94,3,,Constant,false}", + }}, + {`{{define "zzz"}}{{.}}{{end}} +{{template "zzz"}}`, 2, []string{ + "{10,3,zzz,Namespace,true}", + "{18,1,dot,Variable,false}", + "{41,3,zzz,Package,false}", + }}, + {`{{block "aaa" foo}}b{{end}}`, 2, []string{ + "{9,3,aaa,Namespace,true}", + "{9,3,aaa,Package,false}", + "{14,3,foo,Function,false}", + "{19,1,,Constant,false}", + }}, + {"", 0, nil}, + {`{{/* this is +a comment */}}`, 1, nil}, // https://go.dev/issue/74635 + } { + got := parseBuffer("", []byte(test.buf)) + if got.parseErr != nil { + t.Error(got.parseErr) continue } - if len(got.named) != x.cnt { - t.Errorf("%d: got %d, expected %d", i, len(got.named), x.cnt) + if len(got.named) != test.wantNamed { + t.Errorf("%d: got %d, expected %d", i, len(got.named), test.wantNamed) } for n, s := range got.symbols { - if s.String() != x.syms[n] { - t.Errorf("%d: got %s, expected %s", i, s.String(), x.syms[n]) + if s.String() != test.syms[n] { + t.Errorf("%d: got %s, expected %s", i, s.String(), test.syms[n]) } } } @@ -58,178 +71,34 @@ func TestWordAt(t *testing.T) { want := []string{"", "", "$A", "$A", "", "", "", "", "", "", "", "", "", "if", "if", "", "$A", "$A", "", "", "B", "", "", "end", "end", "end", "", "", ""} - p := parseBuffer([]byte("{{$A := .}}{{if $A}}B{{end}}")) - for i := 0; i < len(p.buf); i++ { - got := findWordAt(p, i) + buf := []byte("{{$A := .}}{{if $A}}B{{end}}") + for i := 0; i < len(buf); i++ { + got := wordAt(buf, i) if got != want[i] { t.Errorf("for %d, got %q, wanted %q", i, got, want[i]) } } } -func TestNLS(t *testing.T) { - buf := `{{if (foÜx .X.Y)}}{{$A := "hi"}}{{.Z $A}}{{else}} - {{$A.X 12}} - {{foo (.X.Y) 23 ($A.Z)}} - {{end}} - ` - p := parseBuffer([]byte(buf)) - if p.ParseErr != nil { - t.Fatal(p.ParseErr) - } - // line 0 doesn't have a \n in front of it - for i := 1; i < len(p.nls)-1; i++ { - if buf[p.nls[i]] != '\n' { - t.Errorf("line %d got %c", i, buf[p.nls[i]]) - } - } - // fake line at end of file - if p.nls[len(p.nls)-1] != len(buf) { - t.Errorf("got %d expected %d", p.nls[len(p.nls)-1], len(buf)) - } -} - -func TestLineCol(t *testing.T) { - buf := `{{if (foÜx .X.Y)}}{{$A := "hi"}}{{.Z $A}}{{else}} - {{$A.X 12}} - {{foo (.X.Y) 23 ($A.Z)}} - {{end}}` - if false { - t.Error(buf) - } - for n, cx := range tmpl { - buf := cx.buf - p := parseBuffer([]byte(buf)) - if p.ParseErr != nil { - t.Fatal(p.ParseErr) - } - type loc struct { - offset int - l, c uint32 - } - saved := []loc{} - // forwards - var lastl, lastc uint32 - for offset := range buf { - l, c := p.LineCol(offset) - saved = append(saved, loc{offset, l, c}) - if l > lastl { - lastl = l - if c != 0 { - t.Errorf("line %d, got %d instead of 0", l, c) - } - } - if c > lastc { - lastc = c - } - } - lines := strings.Split(buf, "\n") - mxlen := -1 - for _, l := range lines { - if len(l) > mxlen { - mxlen = len(l) - } - } - if int(lastl) != len(lines)-1 && int(lastc) != mxlen { - // lastl is 0 if there is only 1 line(?) - t.Errorf("expected %d, %d, got %d, %d for case %d", len(lines)-1, mxlen, lastl, lastc, n) - } - // backwards - for j := len(saved) - 1; j >= 0; j-- { - s := saved[j] - xl, xc := p.LineCol(s.offset) - if xl != s.l || xc != s.c { - t.Errorf("at offset %d(%d), got (%d,%d), expected (%d,%d)", s.offset, j, xl, xc, s.l, s.c) - } - } - } -} - -func TestLineColNL(t *testing.T) { - buf := "\n\n\n\n\n" - p := parseBuffer([]byte(buf)) - if p.ParseErr != nil { - t.Fatal(p.ParseErr) - } - for i := 0; i < len(buf); i++ { - l, c := p.LineCol(i) - if c != 0 || int(l) != i+1 { - t.Errorf("got (%d,%d), expected (%d,0)", l, c, i) - } - } -} - -func TestPos(t *testing.T) { - buf := ` - {{if (foÜx .X.Y)}}{{$A := "hi"}}{{.Z $A}}{{else}} - {{$A.X 12}} - {{foo (.X.Y) 23 ($A.Z)}} - {{end}}` - p := parseBuffer([]byte(buf)) - if p.ParseErr != nil { - t.Fatal(p.ParseErr) - } - for pos, r := range buf { - if r == '\n' { - continue - } - x := p.Position(pos) - n := p.FromPosition(x) - if n != pos { - // once it's wrong, it will be wrong forever - t.Fatalf("at pos %d (rune %c) got %d {%#v]", pos, r, n, x) - } - - } -} -func TestLen(t *testing.T) { - data := []struct { - cnt int - v string - }{{1, "a"}, {1, "膈"}, {4, "😆🥸"}, {7, "3😀4567"}} - p := &Parsed{nonASCII: true} - for _, d := range data { - got := p.utf16len([]byte(d.v)) - if got != d.cnt { - t.Errorf("%v, got %d wanted %d", d, got, d.cnt) - } - } -} - -func TestUtf16(t *testing.T) { - buf := ` - {{if (foÜx .X.Y)}}😀{{$A := "hi"}}{{.Z $A}}{{else}} - {{$A.X 12}} - {{foo (.X.Y) 23 ($A.Z)}} - {{end}}` - p := parseBuffer([]byte(buf)) - if p.nonASCII == false { - t.Error("expected nonASCII to be true") - } -} - -type ttest struct { - tmpl string - tokCnt int - elidedCnt int8 -} - func TestQuotes(t *testing.T) { - tsts := []ttest{ + for _, s := range []struct { + tmpl string + tokCnt int + elidedCnt int8 + }{ {"{{- /*comment*/ -}}", 1, 0}, {"{{/*`\ncomment\n`*/}}", 1, 0}, //{"{{foo\nbar}}\n", 1, 0}, // this action spanning lines parses in 1.16 {"{{\"{{foo}}{{\"}}", 1, 0}, {"{{\n{{- when}}", 1, 1}, // corrected {"{{{{if .}}xx{{\n{{end}}", 2, 2}, // corrected - } - for _, s := range tsts { - p := parseBuffer([]byte(s.tmpl)) + } { + p := parseBuffer("", []byte(s.tmpl)) if len(p.tokens) != s.tokCnt { t.Errorf("%q: got %d tokens, expected %d", s, len(p.tokens), s.tokCnt) } - if p.ParseErr != nil { - t.Errorf("%q: %v", string(p.buf), p.ParseErr) + if p.parseErr != nil { + t.Errorf("%q: %v", string(p.buf), p.parseErr) } if len(p.elided) != int(s.elidedCnt) { t.Errorf("%q: elided %d, expected %d", s, len(p.elided), s.elidedCnt) diff --git a/gopls/internal/template/symbols.go b/gopls/internal/template/symbols.go index fcbaec43c54..00745c29dc5 100644 --- a/gopls/internal/template/symbols.go +++ b/gopls/internal/template/symbols.go @@ -9,7 +9,6 @@ import ( "context" "fmt" "text/template/parse" - "unicode/utf8" "golang.org/x/tools/gopls/internal/cache" "golang.org/x/tools/gopls/internal/file" @@ -19,8 +18,8 @@ import ( // in local coordinates, to be translated to protocol.DocumentSymbol type symbol struct { - start int // for sorting - length int // in runes (unicode code points) + start int // 0-based byte offset, for sorting + len int // of source, in bytes name string kind protocol.SymbolKind vardef bool // is this a variable definition? @@ -28,12 +27,16 @@ type symbol struct { // no children yet, and selection range is the same as range } +func (s symbol) offsets() (start, end int) { + return s.start, s.start + s.len +} + func (s symbol) String() string { - return fmt.Sprintf("{%d,%d,%s,%s,%v}", s.start, s.length, s.name, s.kind, s.vardef) + return fmt.Sprintf("{%d,%d,%s,%s,%v}", s.start, s.len, s.name, s.kind, s.vardef) } // for FieldNode or VariableNode (or ChainNode?) -func (p *Parsed) fields(flds []string, x parse.Node) []symbol { +func (p *parsed) fields(flds []string, x parse.Node) []symbol { ans := []symbol{} // guessing that there are no embedded blanks allowed. The doc is unclear lookfor := "" @@ -80,7 +83,7 @@ func (p *Parsed) fields(flds []string, x parse.Node) []symbol { if f[0] == '$' { kind = protocol.Variable } - sym := symbol{name: f, kind: kind, start: at, length: utf8.RuneCount([]byte(f))} + sym := symbol{name: f, kind: kind, start: at, len: len(f)} if kind == protocol.Variable && len(p.stack) > 1 { if pipe, ok := p.stack[len(p.stack)-2].(*parse.PipeNode); ok { for _, y := range pipe.Decl { @@ -96,7 +99,7 @@ func (p *Parsed) fields(flds []string, x parse.Node) []symbol { return ans } -func (p *Parsed) findSymbols() { +func (p *parsed) findSymbols() { if len(p.stack) == 0 { return } @@ -118,7 +121,7 @@ func (p *Parsed) findSymbols() { case *parse.BoolNode: // need to compute the length from the value msg := fmt.Sprintf("%v", x.True) - p.symbols = append(p.symbols, symbol{start: int(x.Pos), length: len(msg), kind: protocol.Boolean}) + p.symbols = append(p.symbols, symbol{start: int(x.Pos), len: len(msg), kind: protocol.Boolean}) case *parse.BranchNode: nxt(x.Pipe) nxt(x.List) @@ -133,13 +136,12 @@ func (p *Parsed) findSymbols() { //case *parse.CommentNode: // go 1.16 // log.Printf("implement %d", x.Type()) case *parse.DotNode: - sym := symbol{name: "dot", kind: protocol.Variable, start: int(x.Pos), length: 1} + sym := symbol{name: "dot", kind: protocol.Variable, start: int(x.Pos), len: 1} p.symbols = append(p.symbols, sym) case *parse.FieldNode: p.symbols = append(p.symbols, p.fields(x.Ident, x)...) case *parse.IdentifierNode: - sym := symbol{name: x.Ident, kind: protocol.Function, start: int(x.Pos), - length: utf8.RuneCount([]byte(x.Ident))} + sym := symbol{name: x.Ident, kind: protocol.Function, start: int(x.Pos), len: len(x.Ident)} p.symbols = append(p.symbols, sym) case *parse.IfNode: nxt(&x.BranchNode) @@ -150,11 +152,11 @@ func (p *Parsed) findSymbols() { } } case *parse.NilNode: - sym := symbol{name: "nil", kind: protocol.Constant, start: int(x.Pos), length: 3} + sym := symbol{name: "nil", kind: protocol.Constant, start: int(x.Pos), len: 3} p.symbols = append(p.symbols, sym) case *parse.NumberNode: // no name; ascii - p.symbols = append(p.symbols, symbol{start: int(x.Pos), length: len(x.Text), kind: protocol.Number}) + p.symbols = append(p.symbols, symbol{start: int(x.Pos), len: len(x.Text), kind: protocol.Number}) case *parse.PipeNode: if x == nil { // {{template "foo"}} return @@ -169,25 +171,23 @@ func (p *Parsed) findSymbols() { nxt(&x.BranchNode) case *parse.StringNode: // no name - sz := utf8.RuneCount([]byte(x.Text)) - p.symbols = append(p.symbols, symbol{start: int(x.Pos), length: sz, kind: protocol.String}) - case *parse.TemplateNode: // invoking a template - // x.Pos points to the quote before the name - p.symbols = append(p.symbols, symbol{name: x.Name, kind: protocol.Package, start: int(x.Pos) + 1, - length: utf8.RuneCount([]byte(x.Name))}) + p.symbols = append(p.symbols, symbol{start: int(x.Pos), len: len(x.Quoted), kind: protocol.String}) + case *parse.TemplateNode: + // invoking a template, e.g. {{define "foo"}} + // x.Pos is the index of "foo". + // The logic below assumes that the literal is trivial. + p.symbols = append(p.symbols, symbol{name: x.Name, kind: protocol.Package, start: int(x.Pos) + len(`"`), len: len(x.Name)}) nxt(x.Pipe) case *parse.TextNode: if len(x.Text) == 1 && x.Text[0] == '\n' { break } // nothing to report, but build one for hover - sz := utf8.RuneCount(x.Text) - p.symbols = append(p.symbols, symbol{start: int(x.Pos), length: sz, kind: protocol.Constant}) + p.symbols = append(p.symbols, symbol{start: int(x.Pos), len: len(x.Text), kind: protocol.Constant}) case *parse.VariableNode: p.symbols = append(p.symbols, p.fields(x.Ident, x)...) case *parse.WithNode: nxt(&x.BranchNode) - } pop() } @@ -199,33 +199,73 @@ func DocumentSymbols(snapshot *cache.Snapshot, fh file.Handle) ([]protocol.Docum if err != nil { return nil, err } - p := parseBuffer(buf) - if p.ParseErr != nil { - return nil, p.ParseErr + p := parseBuffer(fh.URI(), buf) + if p.parseErr != nil { + return nil, p.parseErr } var ans []protocol.DocumentSymbol - for _, s := range p.symbols { - if s.kind == protocol.Constant { + for _, sym := range p.symbols { + if sym.kind == protocol.Constant { continue } - d := kindStr(s.kind) - if d == "Namespace" { - d = "Template" + detail := kindStr(sym.kind) + if detail == "Namespace" { + detail = "Template" } - if s.vardef { - d += "(def)" + if sym.vardef { + detail += "(def)" } else { - d += "(use)" + detail += "(use)" } - r := p.Range(s.start, s.length) - y := protocol.DocumentSymbol{ - Name: s.name, - Detail: d, - Kind: s.kind, - Range: r, - SelectionRange: r, // or should this be the entire {{...}}? + rng, err := p.mapper.OffsetRange(sym.offsets()) + if err != nil { + return nil, err } - ans = append(ans, y) + ans = append(ans, protocol.DocumentSymbol{ + Name: sym.name, + Detail: detail, + Kind: sym.kind, + Range: rng, + SelectionRange: rng, // or should this be the entire {{...}}? + }) } return ans, nil } + +func kindStr(k protocol.SymbolKind) string { + n := int(k) + if n < 1 || n >= len(kindNames) { + return fmt.Sprintf("?SymbolKind %d?", n) + } + return kindNames[n] +} + +var kindNames = []string{ + "", + "File", + "Module", + "Namespace", + "Package", + "Class", + "Method", + "Property", + "Field", + "Constructor", + "Enum", + "Interface", + "Function", + "Variable", + "Constant", + "String", + "Number", + "Boolean", + "Array", + "Object", + "Key", + "Null", + "EnumMember", + "Struct", + "Event", + "Operator", + "TypeParameter", +} diff --git a/gopls/internal/test/integration/bench/unimported_test.go b/gopls/internal/test/integration/bench/unimported_test.go index 9d7139b0bce..baa1d3c0c55 100644 --- a/gopls/internal/test/integration/bench/unimported_test.go +++ b/gopls/internal/test/integration/bench/unimported_test.go @@ -123,15 +123,12 @@ func findSym(t testing.TB) (pkg, name, gomodcache string) { t.Fatal(err) } modcache := strings.TrimSpace(string(out)) - ix, err := modindex.ReadIndex(modcache) + ix, err := modindex.Read(modcache) if err != nil { t.Fatal(err) } if ix == nil { - t.Fatal("no index") - } - if len(ix.Entries) == 0 { - t.Fatal("no entries") + t.Fatal("nil index") } nth := 100 // or something for _, e := range ix.Entries { diff --git a/gopls/internal/test/integration/diagnostics/diagnostics_test.go b/gopls/internal/test/integration/diagnostics/diagnostics_test.go index 017def82373..58d6b371cf4 100644 --- a/gopls/internal/test/integration/diagnostics/diagnostics_test.go +++ b/gopls/internal/test/integration/diagnostics/diagnostics_test.go @@ -681,7 +681,7 @@ var ErrHelpWanted error ` // Test for golang/go#38211. -func Test_Issue38211(t *testing.T) { +func Test_issue38211(t *testing.T) { const ardanLabs = ` -- go.mod -- module mod.com @@ -696,46 +696,89 @@ func main() { _ = conf.ErrHelpWanted } ` - WithOptions( + + modcache := t.TempDir() + defer CleanModCache(t, modcache) + + opts := []RunOption{ + EnvVars{"GOMODCACHE": modcache}, ProxyFiles(ardanLabsProxy), - ).Run(t, ardanLabs, func(t *testing.T, env *Env) { - // Expect a diagnostic with a suggested fix to add - // "github.com/ardanlabs/conf" to the go.mod file. + //WriteGoSum("."), // TODO(golang/go#74594): uncommenting this causes mysterious failure; investigate and make the error clearer (go list?) + } + + t.Run("setup", func(t *testing.T) { + // Forcibly populate GOMODCACHE + // so OrganizeImports can later rely on it. + WithOptions(opts...).Run(t, ardanLabs, func(t *testing.T, env *Env) { + // TODO(adonovan): why doesn't RunGoCommand respect EnvVars?? + // (That was the motivation to use Sandbox.RunGoCommand + // rather than execute go mod download directly!) + // See comment at CleanModCache and golang/go#74595. + environ := []string{"GOMODCACHE=" + modcache} + _, err := env.Sandbox.RunGoCommand(env.Ctx, "", "get", []string{"github.com/ardanlabs/conf@v1.2.3"}, environ, false) + if err != nil { + t.Error(err) + } + }) + }) + + WithOptions(opts...).Run(t, ardanLabs, func(t *testing.T, env *Env) { + // Expect a "no module provides package" diagnostic. env.OpenFile("go.mod") env.OpenFile("main.go") var d protocol.PublishDiagnosticsParams env.AfterChange( - Diagnostics(env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`)), + Diagnostics( + env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`), + WithMessage("no required module provides package")), ReadDiagnostics("main.go", &d), ) + + // Apply the suggested fix to make the go.mod file + // require "github.com/ardanlabs/conf". env.ApplyQuickFixes("main.go", d.Diagnostics) env.SaveBuffer("go.mod") env.AfterChange( NoDiagnostics(ForFile("main.go")), ) - // Comment out the line that depends on conf and expect a - // diagnostic and a fix to remove the import. + + // Comment out the sole use of conf, + // causing an "unused import" diagnostic. env.RegexpReplace("main.go", "_ = conf.ErrHelpWanted", "//_ = conf.ErrHelpWanted") env.AfterChange( - Diagnostics(env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`)), + Diagnostics( + env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`), + WithMessage("imported and not used")), ) - env.SaveBuffer("main.go") - // Expect a diagnostic and fix to remove the dependency in the go.mod. + + // Remove the import using OrganizeImports, leading + // to an "unused require" diagnostic in the go.mod. + env.SaveBuffer("main.go") // => OrganizeImports env.AfterChange( NoDiagnostics(ForFile("main.go")), - Diagnostics(env.AtRegexp("go.mod", "require github.com/ardanlabs/conf"), WithMessage("not used in this module")), + Diagnostics( + env.AtRegexp("go.mod", "require github.com/ardanlabs/conf"), + WithMessage("not used in this module")), ReadDiagnostics("go.mod", &d), ) + + // Apply the suggested fix to remove the "require" directive. env.ApplyQuickFixes("go.mod", d.Diagnostics) env.SaveBuffer("go.mod") env.AfterChange( NoDiagnostics(ForFile("go.mod")), ) - // Uncomment the lines and expect a new diagnostic for the import. + + // Uncomment the use of the import. + // OrganizeImports should add the import. + // Expect another "no required module provides package" + // diagnostic, bringing us full circle. env.RegexpReplace("main.go", "//_ = conf.ErrHelpWanted", "_ = conf.ErrHelpWanted") - env.SaveBuffer("main.go") + env.SaveBuffer("main.go") // => OrganizeImports env.AfterChange( - Diagnostics(env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`)), + Diagnostics( + env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`), + WithMessage("no required module provides package")), ) }) } diff --git a/gopls/internal/test/integration/env.go b/gopls/internal/test/integration/env.go index 67417169f9b..537c2c12180 100644 --- a/gopls/internal/test/integration/env.go +++ b/gopls/internal/test/integration/env.go @@ -8,6 +8,8 @@ import ( "context" "fmt" "net/http/httptest" + "os" + "os/exec" "strings" "sync" "sync/atomic" @@ -378,3 +380,20 @@ func indent(msg string) string { const prefix = " " return prefix + strings.ReplaceAll(msg, "\n", "\n"+prefix) } + +// CleanModCache cleans the specified GOMODCACHE. +// +// TODO(golang/go#74595): this is only necessary as the module cache cleaning of the +// sandbox does not respect GOMODCACHE set via EnvVars. We should fix this, but +// that is probably part of a larger refactoring of the sandbox that I'm not +// inclined to undertake. --rfindley. +// +// (For similar problems caused by the same bug, see Test_issue38211; see also +// comment in Sandbox.Env.) +func CleanModCache(t *testing.T, modcache string) { + cmd := exec.Command("go", "clean", "-modcache") + cmd.Env = append(os.Environ(), "GOMODCACHE="+modcache, "GOTOOLCHAIN=local") + if output, err := cmd.CombinedOutput(); err != nil { + t.Errorf("cleaning modcache: %v\noutput:\n%s", err, string(output)) + } +} diff --git a/gopls/internal/test/integration/fake/proxy.go b/gopls/internal/test/integration/fake/proxy.go index 9e56efeb17f..cd83ee72c54 100644 --- a/gopls/internal/test/integration/fake/proxy.go +++ b/gopls/internal/test/integration/fake/proxy.go @@ -6,6 +6,7 @@ package fake import ( "fmt" + "strings" "golang.org/x/tools/internal/proxydir" ) @@ -27,6 +28,11 @@ func WriteProxy(tmpdir string, files map[string][]byte) (string, error) { filesByModule[mv][suffix] = data } for mv, files := range filesByModule { + // Don't hoist this check out of the loop: + // the problem is benign if filesByModule is empty. + if strings.Contains(tmpdir, "#") { + return "", fmt.Errorf("WriteProxy's tmpdir contains '#', which is unsuitable for GOPROXY. (If tmpdir was derived from testing.T.Name, use t.Run to ensure that each subtest has a unique name.)") + } if err := proxydir.WriteModuleVersion(tmpdir, mv.modulePath, mv.version, files); err != nil { return "", fmt.Errorf("error writing %s@%s: %v", mv.modulePath, mv.version, err) } diff --git a/gopls/internal/test/integration/fake/sandbox.go b/gopls/internal/test/integration/fake/sandbox.go index 22352dbda9c..12ce516f3b7 100644 --- a/gopls/internal/test/integration/fake/sandbox.go +++ b/gopls/internal/test/integration/fake/sandbox.go @@ -14,7 +14,6 @@ import ( "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/robustio" - "golang.org/x/tools/internal/testenv" "golang.org/x/tools/txtar" ) @@ -202,18 +201,17 @@ func (sb *Sandbox) GOPATH() string { // GoEnv returns the default environment variables that can be used for // invoking Go commands in the sandbox. func (sb *Sandbox) GoEnv() map[string]string { - vars := map[string]string{ + return map[string]string{ "GOPATH": sb.GOPATH(), "GOPROXY": sb.goproxy, "GO111MODULE": "", "GOSUMDB": "off", "GOPACKAGESDRIVER": "off", "GOTOOLCHAIN": "local", // tests should not download toolchains + // TODO(golang/go#74595): Why don't we respect GOMODCACHE in the + // settings.env? See comment at env.CleanModCache. + "GOMODCACHE": "", } - if testenv.Go1Point() >= 5 { - vars["GOMODCACHE"] = "" - } - return vars } // goCommandInvocation returns a new gocommand.Invocation initialized with the @@ -273,7 +271,8 @@ func (sb *Sandbox) GoVersion(ctx context.Context) (int, error) { // Close removes all state associated with the sandbox. func (sb *Sandbox) Close() error { var goCleanErr error - if sb.gopath != "" { + // Careful: sb may not be fully initialized. + if sb.gopath != "" && sb.Workdir != nil { // Important: run this command in RootDir so that it doesn't interact with // any toolchain downloads that may occur _, goCleanErr = sb.RunGoCommand(context.Background(), sb.RootDir(), "clean", []string{"-modcache"}, nil, false) diff --git a/gopls/internal/test/integration/misc/addtest_test.go b/gopls/internal/test/integration/misc/addtest_test.go new file mode 100644 index 00000000000..9ad888f891b --- /dev/null +++ b/gopls/internal/test/integration/misc/addtest_test.go @@ -0,0 +1,120 @@ +// Copyright 2025 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 misc + +import ( + "testing" + + "golang.org/x/tools/gopls/internal/protocol" + "golang.org/x/tools/gopls/internal/settings" + "golang.org/x/tools/gopls/internal/test/compare" + . "golang.org/x/tools/gopls/internal/test/integration" +) + +// TestAddTest is a basic test of interaction with the "gopls.add_test" code action. +func TestAddTest(t *testing.T) { + const files = ` +-- go.mod -- +module example.com + +-- a/a.go -- +package a + +import( + "context" +) + +func Foo(ctx context.Context, in string) string {return in} + +-- a/a_test.go -- +package a_test + +import( + "testing" +) + +func TestExisting(t *testing.T) {} +` + const want = `package a_test + +import ( + "context" + "testing" + + "example.com/a" +) + +func TestExisting(t *testing.T) {} + +func TestFoo(t *testing.T) { + tests := []struct { + name string // description of this test case + // Named input parameters for target function. + in string + want string + }{ + // TODO: Add test cases. + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := a.Foo(context.Background(), tt.in) + // TODO: update the condition below to compare got with tt.want. + if true { + t.Errorf("Foo() = %v, want %v", got, tt.want) + } + }) + } +} +` + Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("a/a.go") + + loc := env.RegexpSearch("a/a.go", "Foo") + actions, err := env.Editor.CodeAction(env.Ctx, loc, nil, protocol.CodeActionUnknownTrigger) + if err != nil { + t.Fatalf("CodeAction: %v", err) + } + action, err := codeActionByKind(actions, settings.AddTest) + if err != nil { + t.Fatal(err) + } + + // Execute the command. + // Its side effect should be a single showDocument request. + params := &protocol.ExecuteCommandParams{ + Command: action.Command.Command, + Arguments: action.Command.Arguments, + } + + listen := env.Awaiter.ListenToShownDocuments() + env.ExecuteCommand(params, nil) + // Wait until we finish writing to the file. + env.AfterChange() + if got := env.BufferText("a/a_test.go"); got != want { + t.Errorf("gopls.add_test returned unexpected diff (-want +got):\n%s", compare.Text(want, got)) + } + + got := listen() + if len(got) != 1 { + t.Errorf("gopls.add_test: got %d showDocument requests, want 1: %v", len(got), got) + } else { + if want := protocol.URI(env.Sandbox.Workdir.URI("a/a_test.go")); got[0].URI != want { + t.Errorf("gopls.add_test: got showDocument requests for %v, want %v", got[0].URI, want) + } + + // Pointing to the line of test function declaration. + if want := (protocol.Range{ + Start: protocol.Position{ + Line: 11, + }, + End: protocol.Position{ + Line: 11, + }, + }); *got[0].Selection != want { + t.Errorf("gopls.add_test: got showDocument requests selection for %v, want %v", *got[0].Selection, want) + } + } + }) +} diff --git a/gopls/internal/test/integration/misc/imports_test.go b/gopls/internal/test/integration/misc/imports_test.go index d83e91002b2..1662c34bf8c 100644 --- a/gopls/internal/test/integration/misc/imports_test.go +++ b/gopls/internal/test/integration/misc/imports_test.go @@ -6,7 +6,6 @@ package misc import ( "os" - "os/exec" "path/filepath" "runtime" "strings" @@ -16,6 +15,7 @@ import ( "golang.org/x/tools/gopls/internal/test/compare" . "golang.org/x/tools/gopls/internal/test/integration" "golang.org/x/tools/gopls/internal/test/integration/fake" + "golang.org/x/tools/internal/modindex" "golang.org/x/tools/gopls/internal/protocol" ) @@ -230,17 +230,34 @@ import "example.com/x" var _, _ = x.X, y.Y ` modcache := t.TempDir() - defer cleanModCache(t, modcache) // see doc comment of cleanModCache + defer CleanModCache(t, modcache) - WithOptions( + opts := []RunOption{ EnvVars{"GOMODCACHE": modcache}, ProxyFiles(exampleProxy), WriteGoSum("."), - ).Run(t, files, func(t *testing.T, env *Env) { + } + + // Force go list to populate GOMODCACHE + // so OrganizeImports can later rely on it. + t.Run("setup", func(t *testing.T) { + WithOptions(opts...).Run(t, files, func(t *testing.T, env *Env) {}) + }) + + WithOptions(opts...).Run(t, files, func(t *testing.T, env *Env) { + // Expect y is undefined. env.OpenFile("main.go") - env.AfterChange(Diagnostics(env.AtRegexp("main.go", `y.Y`))) - env.SaveBuffer("main.go") + env.AfterChange( + Diagnostics( + env.AtRegexp("main.go", `y.Y`), + WithMessage("undefined")), + ) + + // Apply suggested fix via OrganizeImports. + env.SaveBuffer("main.go") // => OrganizeImports env.AfterChange(NoDiagnostics(ForFile("main.go"))) + + // Verify that y.Y is defined within the module cache. loc := env.FirstDefinition(env.RegexpSearch("main.go", `y.(Y)`)) path := env.Sandbox.Workdir.URIToPath(loc.URI) if !strings.HasPrefix(path, filepath.ToSlash(modcache)) { @@ -278,7 +295,7 @@ return nil } ` modcache := t.TempDir() - defer cleanModCache(t, modcache) + defer CleanModCache(t, modcache) mx := fake.UnpackTxt(cache) for k, v := range mx { fname := filepath.Join(modcache, k) @@ -328,7 +345,7 @@ return nil } ` modcache := t.TempDir() - defer cleanModCache(t, modcache) + defer CleanModCache(t, modcache) mx := fake.UnpackTxt(cache) for k, v := range mx { fname := filepath.Join(modcache, k) @@ -379,7 +396,7 @@ return nil } ` modcache := t.TempDir() - defer cleanModCache(t, modcache) + defer CleanModCache(t, modcache) mx := fake.UnpackTxt(cache) for k, v := range mx { fname := filepath.Join(modcache, k) @@ -426,18 +443,16 @@ var _ = strs.Builder } }) } -func TestRelativeReplace(t *testing.T) { + +func TestIssue67156(t *testing.T) { const files = ` -- go.mod -- module mod.com/a go 1.20 -require ( - example.com v1.2.3 -) +require example.com v1.2.3 -replace example.com/b => ../b -- main.go -- package main @@ -447,37 +462,39 @@ var _, _ = x.X, y.Y ` modcache := t.TempDir() base := filepath.Base(modcache) - defer cleanModCache(t, modcache) // see doc comment of cleanModCache + defer CleanModCache(t, modcache) // Construct a very unclean module cache whose length exceeds the length of // the clean directory path, to reproduce the crash in golang/go#67156 const sep = string(filepath.Separator) modcache += strings.Repeat(sep+".."+sep+base, 10) - WithOptions( + opts := []RunOption{ EnvVars{"GOMODCACHE": modcache}, ProxyFiles(exampleProxy), WriteGoSum("."), - ).Run(t, files, func(t *testing.T, env *Env) { + } + + t.Run("setup", func(t *testing.T) { + // Force go list to populate GOMODCACHE. + WithOptions(opts...).Run(t, files, func(t *testing.T, env *Env) {}) + + // Update module index. + if ix, err := modindex.Update(modcache); err != nil { + t.Fatalf("failed to obtain updated module index: %v", err) + } else if len(ix.Entries) != 2 { + t.Fatalf("got %v, want 2 entries", ix) + } + }) + + WithOptions(opts...).Run(t, files, func(t *testing.T, env *Env) { env.OpenFile("main.go") env.AfterChange(Diagnostics(env.AtRegexp("main.go", `y.Y`))) - env.SaveBuffer("main.go") + env.SaveBuffer("main.go") // => OrganizeImports env.AfterChange(NoDiagnostics(ForFile("main.go"))) }) } -// TODO(rfindley): this is only necessary as the module cache cleaning of the -// sandbox does not respect GOMODCACHE set via EnvVars. We should fix this, but -// that is probably part of a larger refactoring of the sandbox that I'm not -// inclined to undertake. -func cleanModCache(t *testing.T, modcache string) { - cmd := exec.Command("go", "clean", "-modcache") - cmd.Env = append(os.Environ(), "GOMODCACHE="+modcache, "GOTOOLCHAIN=local") - if output, err := cmd.CombinedOutput(); err != nil { - t.Errorf("cleaning modcache: %v\noutput:\n%s", err, string(output)) - } -} - // Tests golang/go#40685. func TestAcceptImportsQuickFixTestVariant(t *testing.T) { const pkg = ` @@ -631,7 +648,7 @@ return nil var A int ` modcache := t.TempDir() - defer cleanModCache(t, modcache) + defer CleanModCache(t, modcache) mx := fake.UnpackTxt(cache) for k, v := range mx { fname := filepath.Join(modcache, k) diff --git a/gopls/internal/test/integration/misc/vuln_test.go b/gopls/internal/test/integration/misc/vuln_test.go index 47f4c6a77b7..59de3b0f1c9 100644 --- a/gopls/internal/test/integration/misc/vuln_test.go +++ b/gopls/internal/test/integration/misc/vuln_test.go @@ -299,7 +299,7 @@ type fetchVulncheckResult struct { // testFetchVulncheckResult checks that calling gopls.fetch_vulncheck_result // returns the expected summarized results contained in the want argument. // -// If fromRun is non-nil, is is the result of running running vulncheck for +// If fromRun is non-nil, it is the result of running running vulncheck for // runPath, and testFetchVulncheckResult also checks that the fetched result // for runPath matches fromRun. // diff --git a/gopls/internal/test/integration/template/template_test.go b/gopls/internal/test/integration/template/template_test.go index 796fe5e0a57..0cf35922154 100644 --- a/gopls/internal/test/integration/template/template_test.go +++ b/gopls/internal/test/integration/template/template_test.go @@ -52,6 +52,35 @@ go 1.17 }) } +func TestMultilineTokensAgain(t *testing.T) { + // Regression tests for a crash; see go.dev/issue/74635. + const files = ` +-- go.mod -- +module mod.com + +go 1.17 +-- hi.tmpl -- +{{/* this is +a comment */}} +` + WithOptions( + Settings{ + "templateExtensions": []string{"tmpl"}, + "semanticTokens": true, + }, + ).Run(t, files, func(t *testing.T, env *Env) { + var p protocol.SemanticTokensParams + p.TextDocument.URI = env.Sandbox.Workdir.URI("hi.tmpl") + toks, err := env.Editor.Server.SemanticTokensFull(env.Ctx, &p) + if err != nil { + t.Errorf("semantic token failed: %v", err) + } + if toks == nil || len(toks.Data) == 0 { + t.Errorf("got no semantic tokens") + } + }) +} + func TestTemplatesFromExtensions(t *testing.T) { const files = ` -- go.mod -- diff --git a/gopls/internal/test/integration/web/flight_test.go b/gopls/internal/test/integration/web/flight_test.go index 0aba411aab0..5d839c8284c 100644 --- a/gopls/internal/test/integration/web/flight_test.go +++ b/gopls/internal/test/integration/web/flight_test.go @@ -9,6 +9,7 @@ import ( "runtime" "testing" + "golang.org/x/tools/gopls/internal/debug" "golang.org/x/tools/gopls/internal/protocol" "golang.org/x/tools/gopls/internal/protocol/command" . "golang.org/x/tools/gopls/internal/test/integration" @@ -28,6 +29,10 @@ func TestFlightRecorder(t *testing.T) { } testenv.NeedsGo1Point(t, 25) + // This is a global hammer; it won't play nicely with + // multiple concurrent tests of Flight Recorder. + t.Cleanup(debug.KillTraceViewers) + const files = ` -- go.mod -- module example.com diff --git a/gopls/internal/test/integration/workspace/quickfix_test.go b/gopls/internal/test/integration/workspace/quickfix_test.go index 3f6b8e8dc32..61e5da2ddc2 100644 --- a/gopls/internal/test/integration/workspace/quickfix_test.go +++ b/gopls/internal/test/integration/workspace/quickfix_test.go @@ -92,7 +92,7 @@ use ( ` got := env.ReadWorkspaceFile("go.work") if diff := compare.Text(want, got); diff != "" { - t.Errorf("unexpeced go.work content:\n%s", diff) + t.Errorf("unexpected go.work content:\n%s", diff) } }) }) diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index a6086efa989..a22edbfc6c3 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/gopls/internal/test/marker/marker_test.go @@ -2142,7 +2142,8 @@ func codeActionMarker(mark marker, loc protocol.Location, kind string) { if end := namedArgFunc(mark, "end", convertNamedArgLocation, protocol.Location{}); end.URI != "" { if end.URI != loc.URI { - panic("unreachable") + mark.errorf("end marker is in a different file (%s)", filepath.Base(loc.URI.Path())) + return } loc.Range.End = end.Range.End } diff --git a/gopls/internal/test/marker/testdata/codeaction/extract_anonymous_struct.txt b/gopls/internal/test/marker/testdata/codeaction/extract_anonymous_struct.txt new file mode 100644 index 00000000000..f606fe2dc3a --- /dev/null +++ b/gopls/internal/test/marker/testdata/codeaction/extract_anonymous_struct.txt @@ -0,0 +1,222 @@ +This test checks of the behavior of extract function when the extracted block includes anonymous structs. +-- go.mod -- +module mod.com + +go 1.12 +-- a/a.go -- +package a + +func _() { + var x struct{ y int } //@codeaction("var", "refactor.extract.function", end=endA, result=anonA) + println(x.y) //@loc(endA, ")") +} + +-- b/b.go -- +package b + +func _() { + type T struct { + y int + } + var x T //@codeaction("var", "refactor.extract.function", end=endB, err="the code refers to a local type") + println(x.y) //@loc(endB, ")") +} + +-- @anonA/a/a.go -- +package a + +func _() { + newFunction() //@loc(endA, ")") +} + +func newFunction() { + var x struct{ y int } //@codeaction("var", "refactor.extract.function", end=endA, result=anonA) + println(x.y) +} + +-- d/d.go -- +package d + +func _() { + s := []struct{ y int }{ + {y: 1}, + {y: 2}, + } + for _, v := range s { //@codeaction("for", "refactor.extract.function", end=endD, result=anonD) + println(v.y) + } //@loc(endD, "}") +} + +-- @anonD/d/d.go -- +package d + +func _() { + s := []struct{ y int }{ + {y: 1}, + {y: 2}, + } + newFunction(s) //@loc(endD, "}") +} + +func newFunction(s []struct{y int}) { + for _, v := range s { //@codeaction("for", "refactor.extract.function", end=endD, result=anonD) + println(v.y) + } +} + +-- e/e.go -- +package e + +func _() { + var x int + s := []struct { //@codeaction("s", "refactor.extract.function", end=endE, result=anonE) + y int + }{ + {y: 1}, + {y: 2}, + } + x = s[0].y //@loc(endE, "x = s[0].y") + println(x) +} + +-- @anonE/e/e.go -- +package e + +func _() { + var x int + x = newFunction(x) //@loc(endE, "x = s[0].y") + println(x) +} + +func newFunction(x int) int { + s := []struct { //@codeaction("s", "refactor.extract.function", end=endE, result=anonE) + y int + }{ + {y: 1}, + {y: 2}, + } + x = s[0].y + return x +} + +-- f/f.go -- +package f +func _() int { + x := struct{ y int } { y: 1 } //@codeaction("x", "refactor.extract.function", end=endF, result=anonF) + return x.y //@loc(endF, "y") +} + +-- @anonF/f/f.go -- +package f +func _() int { + return newFunction() //@loc(endF, "y") +} + +func newFunction() int { + x := struct{ y int }{y: 1} //@codeaction("x", "refactor.extract.function", end=endF, result=anonF) + return x.y +} + +-- g/g.go -- +package g + +import "fmt" + +func _() error { + x := struct{ y error }{fmt.Errorf("test error")} + return x.y //@ loc(endG, "y"), codeaction("return", "refactor.extract.function", end=endG, result=anonG) +} + +-- @anonG/g/g.go -- +package g + +import "fmt" + +func _() error { + x := struct{ y error }{fmt.Errorf("test error")} + return newFunction(x) //@ loc(endG, "y"), codeaction("return", "refactor.extract.function", end=endG, result=anonG) +} + +func newFunction(x struct{y error}) error { + return x.y +} + +-- h/h.go -- +package h + +import "fmt" + +func _() string { + type A error + type B struct { + A + } + a := B{A: fmt.Errorf("test error")} //@codeaction("a", "refactor.extract.function", end=endH, err="the code refers to a local type") + return a.Error() //@loc(endH, "Error()") +} + +-- i/i.go -- +package i + +import "fmt" + +func _() string { + var a struct{ e error } //@codeaction("var", "refactor.extract.function", end=endI, result=anonI) + a.e = fmt.Errorf("test error") + return a.e.Error() //@loc(endI, "Error()") +} + +-- @anonI/i/i.go -- +package i + +import "fmt" + +func _() string { + return newFunction() //@loc(endI, "Error()") +} + +func newFunction() string { + var a struct{ e error } //@codeaction("var", "refactor.extract.function", end=endI, result=anonI) + a.e = fmt.Errorf("test error") + return a.e.Error() +} + +-- j/j.go -- +package j + +import "unsafe" + +func _() uintptr { + var x struct{ p unsafe.Pointer } + y := uintptr(x.p) //@codeaction("y", "refactor.extract.function", end=endJ, result=anonJ) + return y //@loc(endJ, "y") +} + +-- @anonJ/j/j.go -- +package j + +import "unsafe" + +func _() uintptr { + var x struct{ p unsafe.Pointer } + return newFunction(x) //@loc(endJ, "y") +} + +func newFunction(x struct{p unsafe.Pointer}) uintptr { + y := uintptr(x.p) //@codeaction("y", "refactor.extract.function", end=endJ, result=anonJ) + return y +} + +-- k/k.go -- +package k + +import "unsafe" + +func _(x int) unsafe.Pointer { + type A struct { + p unsafe.Pointer + } + c := A{p: unsafe.Pointer(&x)} //@codeaction("c", "refactor.extract.function", end=endK, err="the code refers to a local type") + return c.p //@loc(endK, "c.p") +} + diff --git a/gopls/internal/test/marker/testdata/completion/testy.txt b/gopls/internal/test/marker/testdata/completion/testy.txt index 25adbbe7f97..50d235e83d5 100644 --- a/gopls/internal/test/marker/testdata/completion/testy.txt +++ b/gopls/internal/test/marker/testdata/completion/testy.txt @@ -57,5 +57,5 @@ func _() { } func issue63578(err error) { - err.Error() //@signature(")", "Error()", -1) + err.Error() //@signature(")", "Error() string", -1) } diff --git a/gopls/internal/test/marker/testdata/hover/hover_alias.txt b/gopls/internal/test/marker/testdata/hover/hover_alias.txt index 886a175981c..b177b90f120 100644 --- a/gopls/internal/test/marker/testdata/hover/hover_alias.txt +++ b/gopls/internal/test/marker/testdata/hover/hover_alias.txt @@ -6,6 +6,8 @@ This test checks gopls behavior when hovering over alias type. -- go.mod -- module mod.com +go 1.18 + -- main.go -- package main @@ -35,6 +37,17 @@ type RealType struct { Age int } +-- generic/a.go -- +package generic +func generic[T any]() {} + +type Named string +type Alias = Named + +func _(){ + generic[Alias]() //@hover("Alias", "Alias", Alias) +} + -- @ToTypeDecl -- ```go type ToTypeDecl = b.RealType // size=24 (0x18) @@ -79,3 +92,13 @@ type ToAliasWithComment = a.AliasWithComment // size=24 (0x18) --- [`main.ToAliasWithComment` on pkg.go.dev](https://pkg.go.dev/mod.com#ToAliasWithComment) +-- @Alias -- +```go +type Alias = Named + +type Named string +``` + +--- + +[`generic.Alias` on pkg.go.dev](https://pkg.go.dev/mod.com/generic#Alias) diff --git a/gopls/internal/test/marker/testdata/hover/issue74361.txt b/gopls/internal/test/marker/testdata/hover/issue74361.txt new file mode 100644 index 00000000000..9fe3ac83c95 --- /dev/null +++ b/gopls/internal/test/marker/testdata/hover/issue74361.txt @@ -0,0 +1,40 @@ +-- flags -- +-skip_goarch=386,arm + +-- settings.json -- +{"analyses": {"unusedfunc": false}} + +-- go.mod -- +module mod.com + +-- a/a.go -- +package a + +type ( + Named int + Alias = Named + Alias2 = Alias +) + +var ( + named Named + alias Alias //@hover("alias", "alias", alias) + alias2 Alias2 //@hover("alias2", "alias2", alias2) +) + +-- @alias -- +```go +var alias Alias +``` + +--- + +@hover("alias", "alias", alias) +-- @alias2 -- +```go +var alias2 Alias2 +``` + +--- + +@hover("alias2", "alias2", alias2) diff --git a/gopls/internal/test/marker/testdata/rename/issue42301.txt b/gopls/internal/test/marker/testdata/rename/issue42301.txt new file mode 100644 index 00000000000..9f26fb9a198 --- /dev/null +++ b/gopls/internal/test/marker/testdata/rename/issue42301.txt @@ -0,0 +1,63 @@ +This test verifies the fix for golang/go#42301: renaming an ident inside its doc +comment should also rename the ident. + +-- go.mod -- +module example.com + +go 1.21 +-- a/a.go -- +package a + +// Foo doesn't do anything, Foo is just an empty function. //@rename("Foo", "Bar", fooToBar), renameerr("anything", "Bar", "no identifier found") +func Foo() {} + +func _() { + Foo() +} + +-- b/b.go -- +package b + +import "example.com/a" + +func _() { + a.Foo() +} + +-- c/c.go -- +package c + +// A is an empty struct. //@rename("A", "B", aToB) +type A struct {} + +-- d/d.go -- +package d + +// Bar doesn't do anything, Bar is just an empty function. //@loc(Bar, re`^.*?\bBar\b.*?\b(Bar)\b.*`), rename(Bar, "Foo", barToFoo) +func Bar() {} + +-- @aToB/c/c.go -- +@@ -3,2 +3,2 @@ +-// A is an empty struct. //@rename("A", "B", aToB) +-type A struct {} ++// B is an empty struct. //@rename("B", "B", aToB) ++type B struct {} +-- @barToFoo/d/d.go -- +@@ -3,2 +3,2 @@ +-// Bar doesn't do anything, Bar is just an empty function. //@loc(Bar, re`^.*?\bBar\b.*?\b(Bar)\b.*`), rename(Bar, "Foo", barToFoo) +-func Bar() {} ++// Foo doesn't do anything, Foo is just an empty function. //@loc(Foo, re`^.*?\bBar\b.*?\b(Foo)\b.*`), rename(Foo, "Foo", barToFoo) ++func Foo() {} +-- @fooToBar/a/a.go -- +@@ -3,2 +3,2 @@ +-// Foo doesn't do anything, Foo is just an empty function. //@rename("Foo", "Bar", fooToBar), renameerr("anything", "Bar", "no identifier found") +-func Foo() {} ++// Bar doesn't do anything, Bar is just an empty function. //@rename("Bar", "Bar", fooToBar), renameerr("anything", "Bar", "no identifier found") ++func Bar() {} +@@ -7 +7 @@ +- Foo() ++ Bar() +-- @fooToBar/b/b.go -- +@@ -6 +6 @@ +- a.Foo() ++ a.Bar() diff --git a/gopls/main.go b/gopls/main.go index 4ef58df77fd..b439f3562aa 100644 --- a/gopls/main.go +++ b/gopls/main.go @@ -7,9 +7,8 @@ // to be extended with IDE-like features; // see https://langserver.org/ for details. // -// See https://github.com/golang/tools/blob/master/gopls/doc/index.md -// for the most up-to-date documentation. -package main // import "golang.org/x/tools/gopls" +// See https://go.dev/gopls for comprehensive documentation on Gopls. +package main import ( "context" diff --git a/internal/diff/lcs/doc.go b/internal/diff/lcs/doc.go index 9029dd20b3d..aa4b0fb5910 100644 --- a/internal/diff/lcs/doc.go +++ b/internal/diff/lcs/doc.go @@ -139,7 +139,7 @@ computed labels. That is the worst case. Had the code noticed (x,y)=(u,v)=(3,3) from the edgegraph. The implementation looks for a number of special cases to try to avoid computing an extra forward path. If the two-sided algorithm has stop early (because D has become too large) it will have found a forward LCS and a -backwards LCS. Ideally these go with disjoint prefixes and suffixes of A and B, but disjointness may fail and the two +backwards LCS. Ideally these go with disjoint prefixes and suffixes of A and B, but disjointedness may fail and the two computed LCS may conflict. (An easy example is where A is a suffix of B, and shares a short prefix. The backwards LCS is all of A, and the forward LCS is a prefix of A.) The algorithm combines the two to form a best-effort LCS. In the worst case the forward partial LCS may have to diff --git a/internal/gocommand/invoke_test.go b/internal/gocommand/invoke_test.go index 0d4dbb1eb13..cc8cc9c6f6d 100644 --- a/internal/gocommand/invoke_test.go +++ b/internal/gocommand/invoke_test.go @@ -13,6 +13,7 @@ import ( "path/filepath" "strings" "testing" + "time" "golang.org/x/sync/errgroup" "golang.org/x/tools/internal/gocommand" @@ -64,23 +65,34 @@ func TestRmdirAfterGoList_Runner(t *testing.T) { // TestRmdirAfterGoList_Runner that executes go list directly, to // control for the substantial logic of the gocommand package. // -// If this test ever fails, the go command itself has a bug; as of May -// 2025 this has never been observed. +// It has two variants: the first does not set WaitDelay; the second +// sets it to 30s. If the first variant ever fails, the go command +// itself has a bug; as of May 2025 this has never been observed. +// +// If the second variant fails, it indicates that the WaitDelay +// mechanism is responsible for causing Wait to return before the +// child process has naturally finished. This is to confirm the +// hypothesis at https://github.com/golang/go/issues/73736#issuecomment-2885407104. func TestRmdirAfterGoList_Direct(t *testing.T) { - testRmdirAfterGoList(t, func(ctx context.Context, dir string) { - cmd := exec.Command("go", "list", "-json", "example.com/p") - cmd.Dir = dir - cmd.Stdout = new(strings.Builder) - cmd.Stderr = new(strings.Builder) - err := cmd.Run() - if ctx.Err() != nil { - return // don't report error if canceled - } - if err != nil { - t.Fatalf("go list failed: %v (stdout=%s stderr=%s)", - err, cmd.Stdout, cmd.Stderr) - } - }) + for _, delay := range []time.Duration{0, 30 * time.Second} { + t.Run(delay.String(), func(t *testing.T) { + testRmdirAfterGoList(t, func(ctx context.Context, dir string) { + cmd := exec.Command("go", "list", "-json", "example.com/p") + cmd.Dir = dir + cmd.Stdout = new(strings.Builder) + cmd.Stderr = new(strings.Builder) + cmd.WaitDelay = delay + err := cmd.Run() + if ctx.Err() != nil { + return // don't report error if canceled + } + if err != nil { + t.Fatalf("go list failed: %v (stdout=%s stderr=%s)", + err, cmd.Stdout, cmd.Stderr) + } + }) + }) + } } func testRmdirAfterGoList(t *testing.T, f func(ctx context.Context, dir string)) { @@ -102,6 +114,7 @@ func testRmdirAfterGoList(t *testing.T, f func(ctx context.Context, dir string)) } } + t0 := time.Now() g, ctx := errgroup.WithContext(context.Background()) for range 10 { g.Go(func() error { @@ -110,7 +123,9 @@ func testRmdirAfterGoList(t *testing.T, f func(ctx context.Context, dir string)) return fmt.Errorf("oops") }) } - g.Wait() // ignore expected error + g.Wait() // ignore error (expected) + + t.Logf("10 concurrent executions (9 canceled) took %v", time.Since(t0)) // This is the critical operation. if err := os.RemoveAll(dir); err != nil { @@ -119,6 +134,6 @@ func testRmdirAfterGoList(t *testing.T, f func(ctx context.Context, dir string)) filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { t.Log(path, d, err) return nil - }) + }) // ignore error } } diff --git a/internal/imports/source_modindex.go b/internal/imports/source_modindex.go index 05229f06ce6..ca745d4a1bb 100644 --- a/internal/imports/source_modindex.go +++ b/internal/imports/source_modindex.go @@ -15,6 +15,10 @@ import ( // This code is here rather than in the modindex package // to avoid import loops +// TODO(adonovan): this code is only used by a test in this package. +// Can we delete it? Or is there a plan to call NewIndexSource from +// cmd/goimports? + // implements Source using modindex, so only for module cache. // // this is perhaps over-engineered. A new Index is read at first use. @@ -22,8 +26,8 @@ import ( // is read if the index changed. It is not clear the Mutex is needed. type IndexSource struct { modcachedir string - mutex sync.Mutex - ix *modindex.Index + mu sync.Mutex + index *modindex.Index // (access via getIndex) expires time.Time } @@ -39,13 +43,14 @@ func (s *IndexSource) LoadPackageNames(ctx context.Context, srcDir string, paths } func (s *IndexSource) ResolveReferences(ctx context.Context, filename string, missing References) ([]*Result, error) { - if err := s.maybeReadIndex(); err != nil { + index, err := s.getIndex() + if err != nil { return nil, err } var cs []modindex.Candidate for pkg, nms := range missing { for nm := range nms { - x := s.ix.Lookup(pkg, nm, false) + x := index.Lookup(pkg, nm, false) cs = append(cs, x...) } } @@ -74,30 +79,22 @@ func (s *IndexSource) ResolveReferences(ctx context.Context, filename string, mi return ans, nil } -func (s *IndexSource) maybeReadIndex() error { - s.mutex.Lock() - defer s.mutex.Unlock() - - var readIndex bool - if time.Now().After(s.expires) { - ok, err := modindex.Update(s.modcachedir) - if err != nil { - return err - } - if ok { - readIndex = true - } - } +func (s *IndexSource) getIndex() (*modindex.Index, error) { + s.mu.Lock() + defer s.mu.Unlock() - if readIndex || s.ix == nil { - ix, err := modindex.ReadIndex(s.modcachedir) + // (s.index = nil => s.expires is zero, + // so the first condition is strictly redundant. + // But it makes the postcondition very clear.) + if s.index == nil || time.Now().After(s.expires) { + index, err := modindex.Update(s.modcachedir) if err != nil { - return err + return nil, err } - s.ix = ix - // for now refresh every 15 minutes - s.expires = time.Now().Add(time.Minute * 15) + s.index = index + s.expires = index.ValidAt.Add(15 * time.Minute) // (refresh period) } + // Inv: s.index != nil - return nil + return s.index, nil } diff --git a/internal/imports/sourcex_test.go b/internal/imports/sourcex_test.go index 0a2327ca300..ed3e8f3418c 100644 --- a/internal/imports/sourcex_test.go +++ b/internal/imports/sourcex_test.go @@ -57,13 +57,14 @@ func newpkgs(cachedir string, pks ...*tpkg) error { for _, s := range p.syms { fmt.Fprintf(fd, "func %s() {}\n", s) } - fd.Close() + if err := fd.Close(); err != nil { + return err + } } return nil } func TestSource(t *testing.T) { - dirs := testDirs(t) if err := newpkgs(dirs.cachedir, &foo, &foobar); err != nil { t.Fatal(err) @@ -102,6 +103,8 @@ func testDirs(t *testing.T) dirs { if err := os.MkdirAll(x.cachedir, 0755); err != nil { t.Fatal(err) } - os.MkdirAll(x.rootdir, 0755) + if err := os.MkdirAll(x.rootdir, 0755); err != nil { + t.Fatal(err) + } return x } diff --git a/internal/mcp/CONTRIBUTING.md b/internal/mcp/CONTRIBUTING.md index 75f31b2dc9f..7104a29f0ae 100644 --- a/internal/mcp/CONTRIBUTING.md +++ b/internal/mcp/CONTRIBUTING.md @@ -1,7 +1,7 @@ # Contributing to the Go MCP SDK Thank you for your interest in contributing! The Go SDK needs active -contributions to keep up with changes in the MCP spec, fix bugs, and accomodate +contributions to keep up with changes in the MCP spec, fix bugs, and accommodate new and emerging use-cases. We welcome all forms of contribution, from filing and reviewing issues, to contributing fixes, to proposing and implementing new features. diff --git a/internal/mcp/design/design.md b/internal/mcp/design/design.md index 6049c1c0717..a2c62c88c66 100644 --- a/internal/mcp/design/design.md +++ b/internal/mcp/design/design.md @@ -933,7 +933,7 @@ In addition to the `List` methods, the SDK provides an iterator method for each # Governance and Community -While the sections above propose an initial implementation of the Go SDK, MCP is evolving rapidly. SDKs need to keep pace, by implementing changes to the spec, fixing bugs, and accomodating new and emerging use-cases. This section proposes how the SDK project can be managed so that it can change safely and transparently. +While the sections above propose an initial implementation of the Go SDK, MCP is evolving rapidly. SDKs need to keep pace, by implementing changes to the spec, fixing bugs, and accommodating new and emerging use-cases. This section proposes how the SDK project can be managed so that it can change safely and transparently. Initially, the Go SDK repository will be administered by the Go team and Anthropic, and they will be the Approvers (the set of people able to merge PRs to the SDK). The policies here are also intended to satisfy necessary constraints of the Go team's participation in the project. @@ -961,7 +961,7 @@ A proposal is an issue that proposes a new API for the SDK, or a change to the s Proposals that are straightforward and uncontroversial may be approved based on GitHub discussion. However, proposals that are deemed to be sufficiently unclear or complicated will be deferred to a regular steering meeting (see below). -This process is similar to the [Go proposal process](https://github.com/golang/proposal), but is necessarily lighter weight to accomodate the greater rate of change expected for the SDK. +This process is similar to the [Go proposal process](https://github.com/golang/proposal), but is necessarily lighter weight to accommodate the greater rate of change expected for the SDK. ### Steering meetings diff --git a/internal/mcp/mcp.go b/internal/mcp/mcp.go index 4363c73bf67..38a0742de60 100644 --- a/internal/mcp/mcp.go +++ b/internal/mcp/mcp.go @@ -25,7 +25,7 @@ // [ServerSession]. // // A [Transport] connects a bidirectional [Connection] of jsonrpc2 messages. In -// practice, transports in the MCP spec are are either client transports or +// practice, transports in the MCP spec are either client transports or // server transports. For example, the [StdioTransport] is a server transport // that communicates over stdin/stdout, and its counterpart is a // [CommandTransport] that communicates with a subprocess over its diff --git a/internal/modindex/dir_test.go b/internal/modindex/dir_test.go index e0919e4c4bf..a0a9357b4b9 100644 --- a/internal/modindex/dir_test.go +++ b/internal/modindex/dir_test.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -package modindex +package modindex_test import ( "os" @@ -10,6 +10,8 @@ import ( "testing" "github.com/google/go-cmp/cmp" + + . "golang.org/x/tools/internal/modindex" ) type id struct { @@ -52,11 +54,11 @@ func testModCache(t *testing.T) string { } // add a trivial package to the test module cache -func addPkg(cachedir, dir string) error { - if err := os.MkdirAll(filepath.Join(cachedir, dir), 0755); err != nil { +func addPkg(gomodcache, dir string) error { + if err := os.MkdirAll(filepath.Join(gomodcache, dir), 0755); err != nil { return err } - return os.WriteFile(filepath.Join(cachedir, dir, "foo.go"), + return os.WriteFile(filepath.Join(gomodcache, dir, "foo.go"), []byte("package foo\nfunc Foo() {}"), 0644) } @@ -74,7 +76,8 @@ func TestIncremental(t *testing.T) { } } } - if err := Create(dir); err != nil { + index0, err := Create(dir) + if err != nil { t.Fatal(err) } // add new stuff to the module cache @@ -88,25 +91,25 @@ func TestIncremental(t *testing.T) { } } } - if ok, err := Update(dir); err != nil { - t.Fatal(err) - } else if !ok { - t.Error("failed to write updated index") + if index1, err := Update(dir); err != nil { + t.Fatalf("failed to update index: %v", err) + } else if len(index1.Entries) <= len(index0.Entries) { + t.Fatalf("updated index is not larger: %v", err) } - index2, err := ReadIndex(dir) + index2, err := Read(dir) if err != nil { t.Fatal(err) } // build a fresh index - if err := Create(dir); err != nil { + if _, err := Create(dir); err != nil { t.Fatal(err) } - index1, err := ReadIndex(dir) + index1, err := Read(dir) if err != nil { t.Fatal(err) } // they should be the same except maybe for the time - index1.Changed = index2.Changed + index1.ValidAt = index2.ValidAt if diff := cmp.Diff(index1, index2); diff != "" { t.Errorf("mismatching indexes (-updated +cleared):\n%s", diff) } @@ -126,7 +129,7 @@ func TestIncrementalNope(t *testing.T) { } } } - if err := Create(dir); err != nil { + if _, err := Create(dir); err != nil { t.Fatal(err) } // add new stuff to the module cache @@ -140,25 +143,20 @@ func TestIncrementalNope(t *testing.T) { } } } - if ok, err := Update(dir); err != nil { - t.Fatal(err) - } else if !ok { - t.Error("failed to write updated index") - } - index2, err := ReadIndex(dir) + index2, err := Update(dir) if err != nil { t.Fatal(err) } // build a fresh index - if err := Create(dir); err != nil { + if _, err := Create(dir); err != nil { t.Fatal(err) } - index1, err := ReadIndex(dir) + index1, err := Read(dir) if err != nil { t.Fatal(err) } // they should be the same except maybe for the time - index1.Changed = index2.Changed + index1.ValidAt = index2.ValidAt if diff := cmp.Diff(index1, index2); diff != "" { t.Errorf("mismatching indexes (-updated +cleared):\n%s", diff) } @@ -176,10 +174,10 @@ func TestDirsSinglePath(t *testing.T) { } } // build and check the index - if err := Create(dir); err != nil { + if _, err := Create(dir); err != nil { t.Fatal(err) } - ix, err := ReadIndex(dir) + ix, err := Read(dir) if err != nil { t.Fatal(err) } @@ -189,8 +187,9 @@ func TestDirsSinglePath(t *testing.T) { if ix.Entries[0].ImportPath != itest.importPath { t.Fatalf("got %s import path, wanted %s", ix.Entries[0].ImportPath, itest.importPath) } - 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]) + gotDir := filepath.ToSlash(ix.Entries[0].Dir) + if gotDir != itest.dirs[itest.best] { + t.Fatalf("got dir %s, wanted %s", gotDir, itest.dirs[itest.best]) } nms := ix.Entries[0].Names if len(nms) != 1 { @@ -203,10 +202,10 @@ func TestDirsSinglePath(t *testing.T) { } } -func TestMissingCachedir(t *testing.T) { +func TestMissingGOMODCACHE(t *testing.T) { // behave properly if the cached dir is empty dir := testModCache(t) - if err := Create(dir); err != nil { + if _, err := Create(dir); err != nil { t.Fatal(err) } des, err := os.ReadDir(IndexDir) @@ -214,23 +213,21 @@ func TestMissingCachedir(t *testing.T) { t.Fatal(err) } if len(des) != 2 { - t.Errorf("got %d, butexpected two entries in index dir", len(des)) + t.Errorf("got %d, but expected two entries in index dir", len(des)) } } func TestMissingIndex(t *testing.T) { // behave properly if there is no existing index dir := testModCache(t) - if ok, err := Update(dir); err != nil { + if _, err := Update(dir); err != nil { t.Fatal(err) - } else if !ok { - t.Error("Update returned !ok") } des, err := os.ReadDir(IndexDir) if err != nil { t.Fatal(err) } if len(des) != 2 { - t.Errorf("got %d, butexpected two entries in index dir", len(des)) + t.Errorf("got %d, but expected two entries in index dir", len(des)) } } diff --git a/internal/modindex/directories.go b/internal/modindex/directories.go index 2faa6ce0b8a..9a963744b50 100644 --- a/internal/modindex/directories.go +++ b/internal/modindex/directories.go @@ -10,7 +10,6 @@ import ( "os" "path/filepath" "regexp" - "slices" "strings" "sync" "time" @@ -20,50 +19,48 @@ import ( ) type directory struct { - path Relpath + path string // relative to GOMODCACHE importPath string version string // semantic version - syms []symbol } -// byImportPath groups the directories by import path, -// sorting the ones with the same import path by semantic version, -// most recent first. -func byImportPath(dirs []Relpath) (map[string][]*directory, error) { - ans := make(map[string][]*directory) // key is import path - for _, d := range dirs { - ip, sv, err := DirToImportPathVersion(d) +// bestDirByImportPath returns the best directory for each import +// path, where "best" means most recent semantic version. These import +// paths are inferred from the GOMODCACHE-relative dir names in dirs. +func bestDirByImportPath(dirs []string) (map[string]directory, error) { + dirsByPath := make(map[string]directory) + for _, dir := range dirs { + importPath, version, err := dirToImportPathVersion(dir) if err != nil { return nil, err } - ans[ip] = append(ans[ip], &directory{ - path: d, - importPath: ip, - version: sv, - }) - } - for k, v := range ans { - semanticSort(v) - ans[k] = v + new := directory{ + path: dir, + importPath: importPath, + version: version, + } + if old, ok := dirsByPath[importPath]; !ok || compareDirectory(new, old) < 0 { + dirsByPath[importPath] = new + } } - return ans, nil + return dirsByPath, nil } -// sort the directories by semantic version, latest first -func semanticSort(v []*directory) { - slices.SortFunc(v, func(l, r *directory) int { - if n := semver.Compare(l.version, r.version); n != 0 { - return -n // latest first - } - return strings.Compare(string(l.path), string(r.path)) - }) +// compareDirectory defines an ordering of path@version directories, +// by descending version, then by ascending path. +func compareDirectory(x, y directory) int { + if sign := -semver.Compare(x.version, y.version); sign != 0 { + return sign // latest first + } + return strings.Compare(string(x.path), string(y.path)) } // modCacheRegexp splits a relpathpath into module, module version, and package. var modCacheRegexp = regexp.MustCompile(`(.*)@([^/\\]*)(.*)`) -// DirToImportPathVersion computes import path and semantic version -func DirToImportPathVersion(dir Relpath) (string, string, error) { +// dirToImportPathVersion computes import path and semantic version +// from a GOMODCACHE-relative directory name. +func dirToImportPathVersion(dir string) (string, string, error) { m := modCacheRegexp.FindStringSubmatch(string(dir)) // m[1] is the module path // m[2] is the version major.minor.patch(-
  
-// The commands are 'create' which builds a new index,
+// The commands are:
 // 'update', which attempts to update an existing index,
 // 'query', which looks up things in the index.
 // 'clean', which remove obsolete index files.
-// If the command is invoked with no arguments, it defaults to 'create'.
+// If the command is invoked with no arguments, it defaults to 'update'.
 package main
 
 import (
@@ -34,7 +34,6 @@ type cmd struct {
 }
 
 var cmds = []cmd{
-	{"create", index, "create a clean index of GOMODCACHE"},
 	{"update", update, "if there is an existing index of GOMODCACHE, update it. Otherwise create one."},
 	{"clean", clean, "removed unreferenced indexes more than an hour old"},
 	{"query", query, "not yet implemented"},
@@ -52,17 +51,17 @@ func goEnv(s string) string {
 func main() {
 	flag.Parse()
 	log.SetFlags(log.Lshortfile)
-	cachedir := goEnv("GOMODCACHE")
-	if cachedir == "" {
+	gomodcache := goEnv("GOMODCACHE")
+	if gomodcache == "" {
 		log.Fatal("can't find GOMODCACHE")
 	}
 	if flag.NArg() == 0 {
-		index(cachedir)
+		update(gomodcache)
 		return
 	}
 	for _, c := range cmds {
 		if flag.Arg(0) == c.name {
-			c.f(cachedir)
+			c.f(gomodcache)
 			return
 		}
 	}
@@ -81,17 +80,16 @@ func init() {
 	}
 }
 
-func index(dir string) {
-	modindex.Create(dir)
-}
-
 func update(dir string) {
-	modindex.Update(dir)
+	if _, err := modindex.Update(dir); err != nil {
+		log.Print(err)
+	}
 }
 
 func query(dir string) {
 	panic("implement")
 }
+
 func clean(_ string) {
 	des := modindex.IndexDir
 	// look at the files starting with 'index'
diff --git a/internal/modindex/index.go b/internal/modindex/index.go
index 9665356c01b..c41d1dd9035 100644
--- a/internal/modindex/index.go
+++ b/internal/modindex/index.go
@@ -6,12 +6,10 @@ package modindex
 
 import (
 	"bufio"
+	"crypto/sha256"
 	"encoding/csv"
-	"errors"
 	"fmt"
-	"hash/crc64"
 	"io"
-	"io/fs"
 	"log"
 	"os"
 	"path/filepath"
@@ -22,7 +20,7 @@ import (
 )
 
 /*
-The on-disk index is a text file.
+The on-disk index ("payload") is a text file.
 The first 3 lines are header information containing CurrentVersion,
 the value of GOMODCACHE, and the validity date of the index.
 (This is when the code started building the index.)
@@ -68,34 +66,45 @@ whose types are []byte and interface{}.
 // CurrentVersion tells readers about the format of the index.
 const CurrentVersion int = 0
 
-// Index is returned by ReadIndex().
+// Index is returned by [Read].
 type Index struct {
-	Version  int
-	Cachedir Abspath   // The directory containing the module cache
-	Changed  time.Time // The index is up to date as of Changed
-	Entries  []Entry
+	Version    int
+	GOMODCACHE string    // absolute path of Go module cache dir
+	ValidAt    time.Time // moment at which the index was up to date
+	Entries    []Entry
+}
+
+func (ix *Index) String() string {
+	return fmt.Sprintf("Index(%s v%d has %d entries at %v)",
+		ix.GOMODCACHE, ix.Version, len(ix.Entries), ix.ValidAt)
 }
 
 // An Entry contains information for an import path.
 type Entry struct {
-	Dir        Relpath // directory in modcache
+	Dir        string // package directory relative to GOMODCACHE; uses OS path separator
 	ImportPath string
 	PkgName    string
 	Version    string
-	//ModTime    STime    // is this useful?
-	Names []string // exported names and information
+	Names      []string // exported names and information
 }
 
 // IndexDir is where the module index is stored.
-var IndexDir string
-
-// Set IndexDir
-func init() {
+// Each logical index entry consists of a pair of files:
+//
+//   - the "payload" (index-VERSION-XXX), whose name is
+//     randomized, holds the actual index; and
+//   - the "link" (index-name-VERSION-HASH),
+//     whose name is predictable, contains the
+//     name of the payload file.
+//
+// Since the link file is small (<512B),
+// reads and writes to it may be assumed atomic.
+var IndexDir string = func() string {
 	var dir string
-	var err error
 	if testing.Testing() {
 		dir = os.TempDir()
 	} else {
+		var err error
 		dir, err = os.UserCacheDir()
 		// shouldn't happen, but TempDir is better than
 		// creating ./go/imports
@@ -103,81 +112,83 @@ func init() {
 			dir = os.TempDir()
 		}
 	}
-	dir = filepath.Join(dir, "go", "imports")
-	os.MkdirAll(dir, 0777)
-	IndexDir = dir
-}
+	dir = filepath.Join(dir, "goimports")
+	if err := os.MkdirAll(dir, 0777); err != nil {
+		log.Printf("failed to create modcache index dir: %v", err)
+	}
+	return dir
+}()
 
-// ReadIndex reads the latest version of the on-disk index
-// for the cache directory cd.
-// It returns (nil, nil) if there is no index, but returns
-// a non-nil error if the index exists but could not be read.
-func ReadIndex(cachedir string) (*Index, error) {
-	cachedir, err := filepath.Abs(cachedir)
+// Read reads the latest version of the on-disk index
+// for the specified Go module cache directory.
+// If there is no index, it returns a nil Index and an fs.ErrNotExist error.
+func Read(gomodcache string) (*Index, error) {
+	gomodcache, err := filepath.Abs(gomodcache)
 	if err != nil {
 		return nil, err
 	}
-	cd := Abspath(cachedir)
-	dir := IndexDir
-	base := indexNameBase(cd)
-	iname := filepath.Join(dir, base)
-	buf, err := os.ReadFile(iname)
-	if err != nil {
-		if errors.Is(err, fs.ErrNotExist) {
-			return nil, nil
-		}
-		return nil, fmt.Errorf("cannot read %s: %w", iname, err)
-	}
-	fname := filepath.Join(dir, string(buf))
-	fd, err := os.Open(fname)
+
+	// Read the "link" file for the specified gomodcache directory.
+	// It names the payload file.
+	content, err := os.ReadFile(filepath.Join(IndexDir, linkFileBasename(gomodcache)))
 	if err != nil {
 		return nil, err
 	}
-	defer fd.Close()
-	r := bufio.NewReader(fd)
-	ix, err := readIndexFrom(cd, r)
+	payloadFile := filepath.Join(IndexDir, string(content))
+
+	// Read the index out of the payload file.
+	f, err := os.Open(payloadFile)
 	if err != nil {
 		return nil, err
 	}
-	return ix, nil
+	defer f.Close()
+	return readIndexFrom(gomodcache, bufio.NewReader(f))
 }
 
-func readIndexFrom(cd Abspath, bx io.Reader) (*Index, error) {
-	b := bufio.NewScanner(bx)
-	var ans Index
-	// header
-	ok := b.Scan()
-	if !ok {
-		return nil, fmt.Errorf("unexpected scan error")
+func readIndexFrom(gomodcache string, r io.Reader) (*Index, error) {
+	scan := bufio.NewScanner(r)
+
+	// version
+	if !scan.Scan() {
+		return nil, fmt.Errorf("unexpected scan error: %v", scan.Err())
 	}
-	l := b.Text()
-	var err error
-	ans.Version, err = strconv.Atoi(l)
+	version, err := strconv.Atoi(scan.Text())
 	if err != nil {
 		return nil, err
 	}
-	if ans.Version != CurrentVersion {
-		return nil, fmt.Errorf("got version %d, expected %d", ans.Version, CurrentVersion)
+	if version != CurrentVersion {
+		return nil, fmt.Errorf("got version %d, expected %d", version, CurrentVersion)
 	}
-	if ok := b.Scan(); !ok {
-		return nil, fmt.Errorf("scanner error reading cachedir")
-	}
-	ans.Cachedir = Abspath(b.Text())
-	if ok := b.Scan(); !ok {
-		return nil, fmt.Errorf("scanner error reading index creation time")
+
+	// gomodcache
+	if !scan.Scan() {
+		return nil, fmt.Errorf("scanner error reading module cache dir: %v", scan.Err())
 	}
-	// TODO(pjw): need to check that this is the expected cachedir
+	// TODO(pjw): need to check that this is the expected cache dir
 	// so the tag should be passed in to this function
-	ans.Changed, err = time.ParseInLocation(time.DateTime, b.Text(), time.Local)
+	if dir := string(scan.Text()); dir != gomodcache {
+		return nil, fmt.Errorf("index file GOMODCACHE mismatch: got %q, want %q", dir, gomodcache)
+	}
+
+	// changed
+	if !scan.Scan() {
+		return nil, fmt.Errorf("scanner error reading index creation time: %v", scan.Err())
+	}
+	changed, err := time.ParseInLocation(time.DateTime, scan.Text(), time.Local)
 	if err != nil {
 		return nil, err
 	}
-	var curEntry *Entry
-	for b.Scan() {
-		v := b.Text()
+
+	// entries
+	var (
+		curEntry *Entry
+		entries  []Entry
+	)
+	for scan.Scan() {
+		v := scan.Text()
 		if v[0] == ':' {
 			if curEntry != nil {
-				ans.Entries = append(ans.Entries, *curEntry)
+				entries = append(entries, *curEntry)
 			}
 			// as directories may contain commas and quotes, they need to be read as csv.
 			rdr := strings.NewReader(v[1:])
@@ -189,49 +200,56 @@ func readIndexFrom(cd Abspath, bx io.Reader) (*Index, error) {
 			if len(flds) != 4 {
 				return nil, fmt.Errorf("header contains %d fields, not 4: %q", len(v), v)
 			}
-			curEntry = &Entry{PkgName: flds[0], ImportPath: flds[1], Dir: toRelpath(cd, flds[2]), Version: flds[3]}
+			curEntry = &Entry{
+				PkgName:    flds[0],
+				ImportPath: flds[1],
+				Dir:        relative(gomodcache, flds[2]),
+				Version:    flds[3],
+			}
 			continue
 		}
 		curEntry.Names = append(curEntry.Names, v)
 	}
-	if curEntry != nil {
-		ans.Entries = append(ans.Entries, *curEntry)
+	if err := scan.Err(); err != nil {
+		return nil, fmt.Errorf("scanner failed while reading modindex entry: %v", err)
 	}
-	if err := b.Err(); err != nil {
-		return nil, fmt.Errorf("scanner failed %v", err)
+	if curEntry != nil {
+		entries = append(entries, *curEntry)
 	}
-	return &ans, nil
+
+	return &Index{
+		Version:    version,
+		GOMODCACHE: gomodcache,
+		ValidAt:    changed,
+		Entries:    entries,
+	}, nil
 }
 
-// write the index as a text file
-func writeIndex(cachedir Abspath, ix *Index) error {
-	ipat := fmt.Sprintf("index-%d-*", CurrentVersion)
-	fd, err := os.CreateTemp(IndexDir, ipat)
+// write writes the index file and updates the index directory to refer to it.
+func write(gomodcache string, ix *Index) error {
+	// Write the index into a payload file with a fresh name.
+	f, err := os.CreateTemp(IndexDir, fmt.Sprintf("index-%d-*", CurrentVersion))
 	if err != nil {
-		return err // can this happen?
+		return err // e.g. disk full, or index dir deleted
 	}
-	defer fd.Close()
-	if err := writeIndexToFile(ix, fd); err != nil {
+	if err := writeIndexToFile(ix, bufio.NewWriter(f)); err != nil {
+		_ = f.Close() // ignore error
 		return err
 	}
-	content := fd.Name()
-	content = filepath.Base(content)
-	base := indexNameBase(cachedir)
-	nm := filepath.Join(IndexDir, base)
-	err = os.WriteFile(nm, []byte(content), 0666)
-	if err != nil {
+	if err := f.Close(); err != nil {
 		return err
 	}
-	return nil
+
+	// Write the name of the payload file into a link file.
+	indexDirFile := filepath.Join(IndexDir, linkFileBasename(gomodcache))
+	content := []byte(filepath.Base(f.Name()))
+	return os.WriteFile(indexDirFile, content, 0666)
 }
 
-func writeIndexToFile(x *Index, fd *os.File) error {
-	cnt := 0
-	w := bufio.NewWriter(fd)
+func writeIndexToFile(x *Index, w *bufio.Writer) error {
 	fmt.Fprintf(w, "%d\n", x.Version)
-	fmt.Fprintf(w, "%s\n", x.Cachedir)
-	// round the time down
-	tm := x.Changed.Add(-time.Second / 2)
+	fmt.Fprintf(w, "%s\n", x.GOMODCACHE)
+	tm := x.ValidAt.Truncate(time.Second) // round the time down
 	fmt.Fprintf(w, "%s\n", tm.Format(time.DateTime))
 	for _, e := range x.Entries {
 		if e.ImportPath == "" {
@@ -239,7 +257,6 @@ func writeIndexToFile(x *Index, fd *os.File) error {
 		}
 		// PJW: maybe always write these headers as csv?
 		if strings.ContainsAny(string(e.Dir), ",\"") {
-			log.Printf("DIR: %s", e.Dir)
 			cw := csv.NewWriter(w)
 			cw.Write([]string{":" + e.PkgName, e.ImportPath, string(e.Dir), e.Version})
 			cw.Flush()
@@ -248,19 +265,23 @@ 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
-	}
-	return nil
+	return w.Flush()
 }
 
-// return the base name of the file containing the name of the current index
-func indexNameBase(cachedir Abspath) string {
-	// crc64 is a way to convert path names into 16 hex digits.
-	h := crc64.Checksum([]byte(cachedir), crc64.MakeTable(crc64.ECMA))
-	fname := fmt.Sprintf("index-name-%d-%016x", CurrentVersion, h)
-	return fname
+// linkFileBasename returns the base name of the link file in the
+// index directory that holds the name of the payload file for the
+// specified (absolute) Go module cache dir.
+func linkFileBasename(gomodcache string) string {
+	// Note: coupled to logic in ./gomodindex/cmd.go. TODO: factor.
+	h := sha256.Sum256([]byte(gomodcache)) // collision-resistant hash
+	return fmt.Sprintf("index-name-%d-%032x", CurrentVersion, h)
+}
+
+func relative(base, file string) string {
+	if rel, err := filepath.Rel(base, file); err == nil {
+		return rel
+	}
+	return file
 }
diff --git a/internal/modindex/lookup_test.go b/internal/modindex/lookup_test.go
index 265b26767dd..a0876ec3c6c 100644
--- a/internal/modindex/lookup_test.go
+++ b/internal/modindex/lookup_test.go
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-package modindex
+package modindex_test
 
 import (
 	"fmt"
@@ -11,6 +11,8 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+
+	. "golang.org/x/tools/internal/modindex"
 )
 
 type tdata struct {
@@ -70,10 +72,10 @@ func okresult(r result, p Candidate) bool {
 func TestLookup(t *testing.T) {
 	dir := testModCache(t)
 	wrtData(t, dir, thedata)
-	if _, err := indexModCache(dir, true); err != nil {
+	if _, err := Create(dir); err != nil {
 		t.Fatal(err)
 	}
-	ix, err := ReadIndex(dir)
+	ix, err := Read(dir)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -127,7 +129,7 @@ func wrtData(t *testing.T, dir string, data tdata) {
 		t.Fatal(err)
 	}
 	defer fd.Close()
-	fd.WriteString(fmt.Sprintf("package %s\n", data.pkg))
+	fmt.Fprintf(fd, "package %s\n", data.pkg)
 	for _, item := range data.items {
 		fd.WriteString(item.code + "\n")
 	}
@@ -158,10 +160,10 @@ func TestLookupAll(t *testing.T) {
 	wrtModule("b.com/go/x3@v1.2.1", "A", "B", "C")
 	wrtModule("c.com/go/x5@v1.3.1", "A", "B", "C", "D", "E")
 
-	if _, err := indexModCache(dir, true); err != nil {
+	if _, err := Create(dir); err != nil {
 		t.Fatal(err)
 	}
-	ix, err := ReadIndex(dir)
+	ix, err := Read(dir)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -183,14 +185,14 @@ func TestUniquify(t *testing.T) {
 	var v []string
 	for i := 1; i < 4; i++ {
 		v = append(v, "A")
-		w := uniquify(v)
+		w := Uniquify(v)
 		if len(w) != 1 {
 			t.Errorf("got %d, expected 1", len(w))
 		}
 	}
 	for i := 1; i < 3; i++ {
 		v = append(v, "B", "C")
-		w := uniquify(v)
+		w := Uniquify(v)
 		if len(w) != 3 {
 			t.Errorf("got %d, expected 3", len(w))
 		}
diff --git a/internal/modindex/modindex.go b/internal/modindex/modindex.go
index 355a53e71aa..5fa285d98e4 100644
--- a/internal/modindex/modindex.go
+++ b/internal/modindex/modindex.go
@@ -2,17 +2,21 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-// Package modindex contains code for building and searching an index to
-// the Go module cache. The directory containing the index, returned by
-// IndexDir(), contains a file index-name- that contains the name
+// Package modindex contains code for building and searching an
+// [Index] of the Go module cache.
+package modindex
+
+// The directory containing the index, returned by
+// [IndexDir], contains a file index-name- that contains the name
 // of the current index. We believe writing that short file is atomic.
-// ReadIndex reads that file to get the file name of the index.
+// [Read] reads that file to get the file name of the index.
 // WriteIndex writes an index with a unique name and then
 // writes that name into a new version of index-name-.
 // ( stands for the CurrentVersion of the index format.)
-package modindex
 
 import (
+	"maps"
+	"os"
 	"path/filepath"
 	"slices"
 	"strings"
@@ -21,144 +25,95 @@ import (
 	"golang.org/x/mod/semver"
 )
 
-// Create always creates a new index for the go module cache that is in cachedir.
-func Create(cachedir string) error {
-	_, err := indexModCache(cachedir, true)
-	return err
-}
-
-// Update the index for the go module cache that is in cachedir,
-// If there is no existing index it will build one.
-// If there are changed directories since the last index, it will
-// write a new one and return true. Otherwise it returns false.
-func Update(cachedir string) (bool, error) {
-	return indexModCache(cachedir, false)
+// Update updates the index for the specified Go
+// module cache directory, creating it as needed.
+// On success it returns the current index.
+func Update(gomodcache string) (*Index, error) {
+	prev, err := Read(gomodcache)
+	if err != nil {
+		if !os.IsNotExist(err) {
+			return nil, err
+		}
+		prev = nil
+	}
+	return update(gomodcache, prev)
 }
 
-// indexModCache writes an index current as of when it is called.
-// If clear is true the index is constructed from all of GOMODCACHE
-// otherwise the index is constructed from the last previous index
-// and the updates to the cache. It returns true if it wrote an index,
-// false otherwise.
-func indexModCache(cachedir string, clear bool) (bool, error) {
-	cachedir, err := filepath.Abs(cachedir)
+// update builds, writes, and returns the current index.
+//
+// If old is nil, the new index is built from all of GOMODCACHE;
+// otherwise it is built from the old index plus cache updates
+// since the previous index's time.
+func update(gomodcache string, old *Index) (*Index, error) {
+	gomodcache, err := filepath.Abs(gomodcache)
 	if err != nil {
-		return false, err
+		return nil, err
 	}
-	cd := Abspath(cachedir)
-	future := time.Now().Add(24 * time.Hour) // safely in the future
-	ok, err := modindexTimed(future, cd, clear)
+	new, changed, err := build(gomodcache, old)
 	if err != nil {
-		return false, err
+		return nil, err
 	}
-	return ok, nil
-}
-
-// modindexTimed writes an index current as of onlyBefore.
-// If clear is true the index is constructed from all of GOMODCACHE
-// otherwise the index is constructed from the last previous index
-// and all the updates to the cache before onlyBefore.
-// It returns true if it wrote a new index, false if it wrote nothing.
-func modindexTimed(onlyBefore time.Time, cachedir Abspath, clear bool) (bool, error) {
-	var curIndex *Index
-	if !clear {
-		var err error
-		curIndex, err = ReadIndex(string(cachedir))
-		if clear && err != nil {
-			return false, err
+	if old == nil || changed {
+		if err := write(gomodcache, new); err != nil {
+			return nil, err
 		}
-		// TODO(pjw): check that most of those directories still exist
-	}
-	cfg := &work{
-		onlyBefore: onlyBefore,
-		oldIndex:   curIndex,
-		cacheDir:   cachedir,
-	}
-	if curIndex != nil {
-		cfg.onlyAfter = curIndex.Changed
-	}
-	if err := cfg.buildIndex(); err != nil {
-		return false, err
 	}
-	if len(cfg.newIndex.Entries) == 0 && curIndex != nil {
-		// no changes from existing curIndex, don't write a new index
-		return false, nil
-	}
-	if err := cfg.writeIndex(); err != nil {
-		return false, err
-	}
-	return true, nil
-}
-
-type work struct {
-	onlyBefore time.Time // do not use directories later than this
-	onlyAfter  time.Time // only interested in directories after this
-	// directories from before onlyAfter come from oldIndex
-	oldIndex *Index
-	newIndex *Index
-	cacheDir Abspath
+	return new, nil
 }
 
-func (w *work) buildIndex() error {
-	// The effective date of the new index should be at least
-	// slightly earlier than when the directories are scanned
-	// so set it now.
-	w.newIndex = &Index{Changed: time.Now(), Cachedir: w.cacheDir}
-	dirs := findDirs(string(w.cacheDir), w.onlyAfter, w.onlyBefore)
-	if len(dirs) == 0 {
-		return nil
+// build returns a new index for the specified Go module cache (an
+// absolute path).
+//
+// If an old index is provided, only directories more recent than it
+// that it are scanned; older directories are provided by the old
+// Index.
+//
+// The boolean result indicates whether new entries were found.
+func build(gomodcache string, old *Index) (*Index, bool, error) {
+	// Set the time window.
+	var start time.Time // = dawn of time
+	if old != nil {
+		start = old.ValidAt
 	}
-	newdirs, err := byImportPath(dirs)
+	now := time.Now()
+	end := now.Add(24 * time.Hour) // safely in the future
+
+	// Enumerate GOMODCACHE package directories.
+	// Choose the best (latest) package for each import path.
+	pkgDirs := findDirs(gomodcache, start, end)
+	dirByPath, err := bestDirByImportPath(pkgDirs)
 	if err != nil {
-		return err
+		return nil, false, err
 	}
-	// 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
-	if w.oldIndex != nil {
-		for _, e := range w.oldIndex.Entries {
-			found, ok := newdirs[e.ImportPath]
-			if !ok {
-				w.newIndex.Entries = append(w.newIndex.Entries, e)
-				continue // use this one, there is no new one
-			}
-			if semver.Compare(found[0].version, e.Version) > 0 {
-				// use the new one
-			} else {
-				// use the old one, forget the new one
-				w.newIndex.Entries = append(w.newIndex.Entries, e)
-				delete(newdirs, e.ImportPath)
+
+	// For each import path it might occur only in
+	// dirByPath, only in old, or in both.
+	// If both, use the semantically later one.
+	var entries []Entry
+	if old != nil {
+		for _, entry := range old.Entries {
+			dir, ok := dirByPath[entry.ImportPath]
+			if !ok || semver.Compare(dir.version, entry.Version) <= 0 {
+				// New dir is missing or not more recent; use old entry.
+				entries = append(entries, entry)
+				delete(dirByPath, entry.ImportPath)
 			}
 		}
 	}
-	// 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)
-	}
-	// 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 {
+
+	// Extract symbol information for all the new directories.
+	newEntries := extractSymbols(gomodcache, maps.Values(dirByPath))
+	entries = append(entries, newEntries...)
+	slices.SortFunc(entries, func(x, y Entry) int {
+		if n := strings.Compare(x.PkgName, y.PkgName); n != 0 {
 			return n
 		}
-		return strings.Compare(l.ImportPath, r.ImportPath)
+		return strings.Compare(x.ImportPath, y.ImportPath)
 	})
-	return nil
-}
 
-func (w *work) writeIndex() error {
-	return writeIndex(w.cacheDir, w.newIndex)
+	return &Index{
+		GOMODCACHE: gomodcache,
+		ValidAt:    now, // time before the directories were scanned
+		Entries:    entries,
+	}, len(newEntries) > 0, nil
 }
diff --git a/internal/modindex/symbols.go b/internal/modindex/symbols.go
index 31a502c5891..fe24db9b13f 100644
--- a/internal/modindex/symbols.go
+++ b/internal/modindex/symbols.go
@@ -10,11 +10,13 @@ import (
 	"go/parser"
 	"go/token"
 	"go/types"
+	"iter"
 	"os"
 	"path/filepath"
 	"runtime"
 	"slices"
 	"strings"
+	"sync"
 
 	"golang.org/x/sync/errgroup"
 )
@@ -34,41 +36,65 @@ type symbol struct {
 	sig  string // signature information, for F
 }
 
-// find the symbols for the best directories
-func getSymbols(cd Abspath, dirs map[string][]*directory) {
+// extractSymbols returns a (new, unordered) array of Entries, one for
+// each provided package directory, describing its exported symbols.
+func extractSymbols(cwd string, dirs iter.Seq[directory]) []Entry {
+	var (
+		mu      sync.Mutex
+		entries []Entry
+	)
+
 	var g errgroup.Group
 	g.SetLimit(max(2, runtime.GOMAXPROCS(0)/2))
-	for _, vv := range dirs {
-		// throttling some day?
-		d := vv[0]
+	for dir := range dirs {
 		g.Go(func() error {
-			thedir := filepath.Join(string(cd), string(d.path))
+			thedir := filepath.Join(cwd, string(dir.path))
 			mode := parser.SkipObjectResolution | parser.ParseComments
 
-			fi, err := os.ReadDir(thedir)
+			// Parse all Go files in dir and extract symbols.
+			dirents, 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") {
+			var syms []symbol
+			for _, dirent := range dirents {
+				if !strings.HasSuffix(dirent.Name(), ".go") ||
+					strings.HasSuffix(dirent.Name(), "_test.go") {
 					continue
 				}
-				fname := filepath.Join(thedir, fx.Name())
+				fname := filepath.Join(thedir, dirent.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)...)
+				syms = append(syms, getFileExports(tr)...)
+			}
+
+			// Create an entry for the package.
+			pkg, names := processSyms(syms)
+			if pkg != "" {
+				mu.Lock()
+				defer mu.Unlock()
+				entries = append(entries, Entry{
+					PkgName:    pkg,
+					Dir:        dir.path,
+					ImportPath: dir.importPath,
+					Version:    dir.version,
+					Names:      names,
+				})
 			}
+
 			return nil
 		})
 	}
-	g.Wait()
+	g.Wait() // ignore error
+
+	return entries
 }
 
 func getFileExports(f *ast.File) []symbol {
 	pkg := f.Name.Name
-	if pkg == "main" {
+	if pkg == "main" || pkg == "" {
 		return nil
 	}
 	var ans []symbol
@@ -202,17 +228,18 @@ func processSyms(syms []symbol) (string, []string) {
 	pkg := syms[0].pkg
 	var names []string
 	for _, s := range syms {
+		if s.pkg != pkg {
+			// Symbols came from two files in same dir
+			// with different package declarations.
+			continue
+		}
 		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)
+		if s.sig != "" {
+			nx = fmt.Sprintf("%s %s %s", s.name, s.kind, s.sig)
 		} else {
-			continue // PJW: do we want to keep track of these?
+			nx = fmt.Sprintf("%s %s", s.name, s.kind)
 		}
+		names = append(names, nx)
 	}
 	return pkg, names
 }
diff --git a/internal/modindex/types.go b/internal/modindex/types.go
deleted file mode 100644
index ece44886309..00000000000
--- a/internal/modindex/types.go
+++ /dev/null
@@ -1,25 +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 modindex
-
-import (
-	"strings"
-)
-
-// some special types to avoid confusions
-
-// distinguish various types of directory names. It's easy to get confused.
-type Abspath string // absolute paths
-type Relpath string // paths with GOMODCACHE prefix removed
-
-func toRelpath(cachedir Abspath, s string) Relpath {
-	if strings.HasPrefix(s, string(cachedir)) {
-		if s == string(cachedir) {
-			return Relpath("")
-		}
-		return Relpath(s[len(cachedir)+1:])
-	}
-	return Relpath(s)
-}
diff --git a/internal/refactor/inline/callee.go b/internal/refactor/inline/callee.go
index 41deebb8228..b46340c66a8 100644
--- a/internal/refactor/inline/callee.go
+++ b/internal/refactor/inline/callee.go
@@ -539,7 +539,7 @@ func signature(fset *token.FileSet, info *types.Info, decl *ast.FuncDecl) *types
 
 // -- callee helpers --
 
-// analyzeAssignment looks at the the given stack, and analyzes certain
+// analyzeAssignment looks at the given stack, and analyzes certain
 // attributes of the innermost expression.
 //
 // In all cases we 'fail closed' when we cannot detect (or for simplicity