From 2a900561e78a69afe5828ff8388aeb0dfc6220dc Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Tue, 28 Jun 2022 09:12:07 -0400 Subject: [PATCH 01/12] go/gcexportdata: fix Find for Go modules Find needs to invoke the go command to find the package export data. It cannot rely on GOPATH-based file location heuristics. This has the nice side effect of automatically compiling the code, removing the possibility of stale export data. Ideally Find would use go/packages, but go/packages imports this package for the export data parser (not for Find itself), so we have to make do with an explicit go command invocation. Marked both Find and NewImporter deprecated: using go/packages will be faster for nearly all use cases, because it can gather info about multiple packages in a single go command invocation. They are essentially unused anyway. Removed the file name print from the example because the file may be in the cache, in which case it will not be named "fmt.a". Change-Id: I7940c90e230b22df9dcbfc8103a69a2d18df3bb0 Reviewed-on: https://go-review.googlesource.com/c/tools/+/310515 Reviewed-by: Alan Donovan TryBot-Result: Gopher Robot gopls-CI: kokoro Run-TryBot: Russ Cox Auto-Submit: Russ Cox --- go/gcexportdata/example_test.go | 2 -- go/gcexportdata/gcexportdata.go | 20 ++++++++++++++++++-- go/gcexportdata/importer.go | 3 +++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/go/gcexportdata/example_test.go b/go/gcexportdata/example_test.go index e81e705b1c4..cdd68f49c53 100644 --- a/go/gcexportdata/example_test.go +++ b/go/gcexportdata/example_test.go @@ -30,7 +30,6 @@ func ExampleRead() { log.Fatalf("can't find export data for fmt") } fmt.Printf("Package path: %s\n", path) - fmt.Printf("Export data: %s\n", filepath.Base(filename)) // Open and read the file. f, err := os.Open(filename) @@ -80,7 +79,6 @@ func ExampleRead() { // Output: // // Package path: fmt - // Export data: fmt.a // Package members: Println found // Println type: func(a ...any) (n int, err error) // Println location: $GOROOT/src/fmt/print.go:123:1 diff --git a/go/gcexportdata/gcexportdata.go b/go/gcexportdata/gcexportdata.go index d50826dbf7e..ddc276cfbcb 100644 --- a/go/gcexportdata/gcexportdata.go +++ b/go/gcexportdata/gcexportdata.go @@ -22,26 +22,42 @@ package gcexportdata // import "golang.org/x/tools/go/gcexportdata" import ( "bufio" "bytes" + "encoding/json" "fmt" "go/token" "go/types" "io" "io/ioutil" + "os/exec" "golang.org/x/tools/go/internal/gcimporter" ) // Find returns the name of an object (.o) or archive (.a) file // containing type information for the specified import path, -// using the workspace layout conventions of go/build. +// using the go command. // If no file was found, an empty filename is returned. // // A relative srcDir is interpreted relative to the current working directory. // // Find also returns the package's resolved (canonical) import path, // reflecting the effects of srcDir and vendoring on importPath. +// +// Deprecated: Use the higher-level API in golang.org/x/tools/go/packages, +// which is more efficient. func Find(importPath, srcDir string) (filename, path string) { - return gcimporter.FindPkg(importPath, srcDir) + cmd := exec.Command("go", "list", "-json", "-export", "--", importPath) + cmd.Dir = srcDir + out, err := cmd.CombinedOutput() + if err != nil { + return "", "" + } + var data struct { + ImportPath string + Export string + } + json.Unmarshal(out, &data) + return data.Export, data.ImportPath } // NewReader returns a reader for the export data section of an object diff --git a/go/gcexportdata/importer.go b/go/gcexportdata/importer.go index fe6ed93215c..37a7247e268 100644 --- a/go/gcexportdata/importer.go +++ b/go/gcexportdata/importer.go @@ -22,6 +22,9 @@ import ( // version-skew problems described in the documentation of this package, // or to control the FileSet or access the imports map populated during // package loading. +// +// Deprecated: Use the higher-level API in golang.org/x/tools/go/packages, +// which is more efficient. func NewImporter(fset *token.FileSet, imports map[string]*types.Package) types.ImporterFrom { return importer{fset, imports} } From e5b33249972ac93113ee6a34ee08f8096d06271c Mon Sep 17 00:00:00 2001 From: Suzy Mueller Date: Tue, 14 Jun 2022 15:18:07 -0400 Subject: [PATCH 02/12] internal/lsp: add InlayHint regtests Add regtests for inlay hints to verify we can turn on and off different inlay hints. Change-Id: Id88450c40c048b6c2544d22a0d3eadb57b70a723 Reviewed-on: https://go-review.googlesource.com/c/tools/+/411911 Reviewed-by: Robert Findley Reviewed-by: Jamal Carvalho gopls-CI: kokoro TryBot-Result: Gopher Robot Run-TryBot: Suzy Mueller --- .../regtest/inlayHints/inlayHints_test.go | 72 +++++++++++++++++++ internal/lsp/fake/editor.go | 21 ++++++ internal/lsp/regtest/wrappers.go | 11 +++ 3 files changed, 104 insertions(+) create mode 100644 gopls/internal/regtest/inlayHints/inlayHints_test.go diff --git a/gopls/internal/regtest/inlayHints/inlayHints_test.go b/gopls/internal/regtest/inlayHints/inlayHints_test.go new file mode 100644 index 00000000000..67931fbdc83 --- /dev/null +++ b/gopls/internal/regtest/inlayHints/inlayHints_test.go @@ -0,0 +1,72 @@ +// Copyright 2022 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 inlayHint + +import ( + "testing" + + "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/internal/lsp/bug" + . "golang.org/x/tools/internal/lsp/regtest" + "golang.org/x/tools/internal/lsp/source" + "golang.org/x/tools/internal/testenv" +) + +func TestMain(m *testing.M) { + bug.PanicOnBugs = true + Main(m, hooks.Options) +} +func TestEnablingInlayHints(t *testing.T) { + testenv.NeedsGo1Point(t, 14) // Test fails on 1.13. + const workspace = ` +-- go.mod -- +module inlayHint.test +go 1.12 +-- lib.go -- +package lib +type Number int +const ( + Zero Number = iota + One + Two +) +` + tests := []struct { + label string + enabled map[string]bool + wantInlayHint bool + }{ + { + label: "default", + wantInlayHint: false, + }, + { + label: "enable const", + enabled: map[string]bool{source.ConstantValues: true}, + wantInlayHint: true, + }, + { + label: "enable parameter names", + enabled: map[string]bool{source.ParameterNames: true}, + wantInlayHint: false, + }, + } + for _, test := range tests { + t.Run(test.label, func(t *testing.T) { + WithOptions( + EditorConfig{ + Settings: map[string]interface{}{ + "hints": test.enabled, + }, + }, + ).Run(t, workspace, func(t *testing.T, env *Env) { + env.OpenFile("lib.go") + lens := env.InlayHints("lib.go") + if gotInlayHint := len(lens) > 0; gotInlayHint != test.wantInlayHint { + t.Errorf("got inlayHint: %t, want %t", gotInlayHint, test.wantInlayHint) + } + }) + }) + } +} diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go index 06b90bb84e5..0fc99a04982 100644 --- a/internal/lsp/fake/editor.go +++ b/internal/lsp/fake/editor.go @@ -1114,6 +1114,27 @@ func (e *Editor) Symbols(ctx context.Context, sym string) ([]protocol.SymbolInfo return ans, err } +// CodeLens executes a codelens request on the server. +func (e *Editor) InlayHint(ctx context.Context, path string) ([]protocol.InlayHint, error) { + if e.Server == nil { + return nil, nil + } + e.mu.Lock() + _, ok := e.buffers[path] + e.mu.Unlock() + if !ok { + return nil, fmt.Errorf("buffer %q is not open", path) + } + params := &protocol.InlayHintParams{ + TextDocument: e.textDocumentIdentifier(path), + } + hints, err := e.Server.InlayHint(ctx, params) + if err != nil { + return nil, err + } + return hints, nil +} + // References executes a reference request on the server. func (e *Editor) References(ctx context.Context, path string, pos Pos) ([]protocol.Location, error) { if e.Server == nil { diff --git a/internal/lsp/regtest/wrappers.go b/internal/lsp/regtest/wrappers.go index 9031e71f1f1..96e2de96271 100644 --- a/internal/lsp/regtest/wrappers.go +++ b/internal/lsp/regtest/wrappers.go @@ -358,6 +358,17 @@ func (e *Env) ExecuteCommand(params *protocol.ExecuteCommandParams, result inter } } +// InlayHints calls textDocument/inlayHints for the given path, calling t.Fatal on +// any error. +func (e *Env) InlayHints(path string) []protocol.InlayHint { + e.T.Helper() + hints, err := e.Editor.InlayHint(e.Ctx, path) + if err != nil { + e.T.Fatal(err) + } + return hints +} + // WorkspaceSymbol calls workspace/symbol func (e *Env) WorkspaceSymbol(sym string) []protocol.SymbolInformation { e.T.Helper() From 0248714391a4231dea84e35fc04f8b65a609821e Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Tue, 28 Jun 2022 14:50:14 -0400 Subject: [PATCH 03/12] internal/lsp: add additional instrumentation around package loading Add some additional logging to help debug golang/go#53586 For golang/go#53586 Change-Id: I0574fb01be47b265cd6e412855794bc2cb836dff Reviewed-on: https://go-review.googlesource.com/c/tools/+/414854 gopls-CI: kokoro Reviewed-by: Alan Donovan TryBot-Result: Gopher Robot Run-TryBot: Robert Findley Auto-Submit: Robert Findley --- internal/lsp/cache/load.go | 14 +++++++++++--- internal/lsp/source/completion/completion.go | 2 +- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index db9a06d4dee..da0b246c54f 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -15,6 +15,7 @@ import ( "path/filepath" "sort" "strings" + "sync/atomic" "time" "golang.org/x/tools/go/packages" @@ -28,12 +29,17 @@ import ( "golang.org/x/tools/internal/span" ) +var loadID uint64 // atomic identifier for loads + // load calls packages.Load for the given scopes, updating package metadata, // import graph, and mapped files with the result. // // The resulting error may wrap the moduleErrorMap error type, representing // errors associated with specific modules. func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interface{}) (err error) { + id := atomic.AddUint64(&loadID, 1) + eventName := fmt.Sprintf("go/packages.Load #%d", id) // unique name for logging + var query []string var containsDir bool // for logging @@ -138,9 +144,9 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf return ctx.Err() } if err != nil { - event.Error(ctx, "go/packages.Load", err, tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs))) + event.Error(ctx, eventName, err, tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs))) } else { - event.Log(ctx, "go/packages.Load", tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs))) + event.Log(ctx, eventName, tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs))) } if len(pkgs) == 0 { if err == nil { @@ -168,7 +174,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf } if !containsDir || s.view.Options().VerboseOutput { - event.Log(ctx, "go/packages.Load", + event.Log(ctx, eventName, tag.Snapshot.Of(s.ID()), tag.Package.Of(pkg.ID), tag.Files.Of(pkg.CompiledGoFiles)) @@ -209,6 +215,8 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf loadedIDs = append(loadedIDs, id) } + event.Log(ctx, fmt.Sprintf("%s: updating metadata for %d packages", eventName, len(updates))) + s.mu.Lock() // invalidate the reverse transitive closure of packages that have changed. diff --git a/internal/lsp/source/completion/completion.go b/internal/lsp/source/completion/completion.go index bb1c68d2238..0c1ff3f21b0 100644 --- a/internal/lsp/source/completion/completion.go +++ b/internal/lsp/source/completion/completion.go @@ -441,7 +441,7 @@ func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHan items, surrounding, innerErr := packageClauseCompletions(ctx, snapshot, fh, protoPos) if innerErr != nil { // return the error for GetParsedFile since it's more relevant in this situation. - return nil, nil, fmt.Errorf("getting file for Completion: %w (package completions: %v)", err, innerErr) + return nil, nil, fmt.Errorf("getting file %s for Completion: %w (package completions: %v)", fh.URI(), err, innerErr) } return items, surrounding, nil } From 7743d1d949f1006ad12b190d68996acafc84d1d6 Mon Sep 17 00:00:00 2001 From: Suzy Mueller Date: Wed, 15 Jun 2022 15:43:22 -0400 Subject: [PATCH 04/12] internal/lsp: respect range for inlay hints This is an optimization to avoid calculating inlayhints that are not in the requested range. Change-Id: I311f297d2998ae7d0db822eac540b1c12cae6e23 Reviewed-on: https://go-review.googlesource.com/c/tools/+/412455 gopls-CI: kokoro TryBot-Result: Gopher Robot Run-TryBot: Suzy Mueller Reviewed-by: Jamal Carvalho --- internal/lsp/source/inlay_hint.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/internal/lsp/source/inlay_hint.go b/internal/lsp/source/inlay_hint.go index 0c147283532..009cc52fdd5 100644 --- a/internal/lsp/source/inlay_hint.go +++ b/internal/lsp/source/inlay_hint.go @@ -104,7 +104,7 @@ var AllInlayHints = map[string]*Hint{ }, } -func InlayHint(ctx context.Context, snapshot Snapshot, fh FileHandle, _ protocol.Range) ([]protocol.InlayHint, error) { +func InlayHint(ctx context.Context, snapshot Snapshot, fh FileHandle, pRng protocol.Range) ([]protocol.InlayHint, error) { ctx, done := event.Start(ctx, "source.InlayHint") defer done() @@ -132,8 +132,23 @@ func InlayHint(ctx context.Context, snapshot Snapshot, fh FileHandle, _ protocol info := pkg.GetTypesInfo() q := Qualifier(pgf.File, pkg.GetTypes(), info) + // Set the range to the full file if the range is not valid. + start, end := pgf.File.Pos(), pgf.File.End() + if pRng.Start.Line < pRng.End.Line || pRng.Start.Character < pRng.End.Character { + // Adjust start and end for the specified range. + rng, err := pgf.Mapper.RangeToSpanRange(pRng) + if err != nil { + return nil, err + } + start, end = rng.Start, rng.End + } + var hints []protocol.InlayHint ast.Inspect(pgf.File, func(node ast.Node) bool { + // If not in range, we can stop looking. + if node == nil || node.End() < start || node.Pos() > end { + return false + } for _, fn := range enabledHints { hints = append(hints, fn(node, tmap, info, &q)...) } From c10541a14b3e5db8a9ae9fad7640035745288ab3 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 29 Jun 2022 11:50:37 -0400 Subject: [PATCH 05/12] go/analysis/passes/fieldalignment: document "false sharing" ...and don't claim that the most compact field order is optimal. The exception is relatively obscure, but the fact that it exists is important because it means it is not safe to apply the code transformation unconditionally. Change-Id: I391fbc1872b578d5340dd7c8fded48be30b820e0 Reviewed-on: https://go-review.googlesource.com/c/tools/+/415057 Reviewed-by: Robert Findley --- go/analysis/passes/fieldalignment/fieldalignment.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/go/analysis/passes/fieldalignment/fieldalignment.go b/go/analysis/passes/fieldalignment/fieldalignment.go index 78afe94ab30..aff663046a3 100644 --- a/go/analysis/passes/fieldalignment/fieldalignment.go +++ b/go/analysis/passes/fieldalignment/fieldalignment.go @@ -23,7 +23,7 @@ import ( const Doc = `find structs that would use less memory if their fields were sorted This analyzer find structs that can be rearranged to use less memory, and provides -a suggested edit with the optimal order. +a suggested edit with the most compact order. Note that there are two different diagnostics reported. One checks struct size, and the other reports "pointer bytes" used. Pointer bytes is how many bytes of the @@ -41,6 +41,11 @@ has 24 pointer bytes because it has to scan further through the *uint32. struct { string; uint32 } has 8 because it can stop immediately after the string pointer. + +Be aware that the most compact order is not always the most efficient. +In rare cases it may cause two variables each updated by its own goroutine +to occupy the same CPU cache line, inducing a form of memory contention +known as "false sharing" that slows down both goroutines. ` var Analyzer = &analysis.Analyzer{ From b84d509d6ffee06a6a8d82fd218009fc7f548e76 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 29 Jun 2022 13:02:25 -0400 Subject: [PATCH 06/12] gopls/doc: regenerate documentation This change should have been included in https://go-review.googlesource.com/c/tools/+/415057 but I hastily submitted it without a CI run thinking "how can a doc only change break something?". Well now I know. Sorry. :( Change-Id: Ib0fd25fddd7f9580961b44dcad032d4851684f63 Reviewed-on: https://go-review.googlesource.com/c/tools/+/415058 Reviewed-by: Dmitri Shuralyov Run-TryBot: Alan Donovan gopls-CI: kokoro TryBot-Result: Gopher Robot Reviewed-by: Robert Findley Reviewed-by: Bryan Mills Auto-Submit: Bryan Mills --- gopls/doc/analyzers.md | 7 ++++++- internal/lsp/source/api_json.go | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md index f5c83d5771d..fd65c3a2a9d 100644 --- a/gopls/doc/analyzers.md +++ b/gopls/doc/analyzers.md @@ -131,7 +131,7 @@ of the second argument is not a pointer to a type implementing error. find structs that would use less memory if their fields were sorted This analyzer find structs that can be rearranged to use less memory, and provides -a suggested edit with the optimal order. +a suggested edit with the most compact order. Note that there are two different diagnostics reported. One checks struct size, and the other reports "pointer bytes" used. Pointer bytes is how many bytes of the @@ -150,6 +150,11 @@ has 24 pointer bytes because it has to scan further through the *uint32. has 8 because it can stop immediately after the string pointer. +Be aware that the most compact order is not always the most efficient. +In rare cases it may cause two variables each updated by its own goroutine +to occupy the same CPU cache line, inducing a form of memory contention +known as "false sharing" that slows down both goroutines. + **Disabled by default. Enable it by setting `"analyses": {"fieldalignment": true}`.** diff --git a/internal/lsp/source/api_json.go b/internal/lsp/source/api_json.go index ef683f31a01..4e2183cf4e6 100755 --- a/internal/lsp/source/api_json.go +++ b/internal/lsp/source/api_json.go @@ -280,7 +280,7 @@ var GeneratedAPIJSON = &APIJSON{ }, { Name: "\"fieldalignment\"", - Doc: "find structs that would use less memory if their fields were sorted\n\nThis analyzer find structs that can be rearranged to use less memory, and provides\na suggested edit with the optimal order.\n\nNote that there are two different diagnostics reported. One checks struct size,\nand the other reports \"pointer bytes\" used. Pointer bytes is how many bytes of the\nobject that the garbage collector has to potentially scan for pointers, for example:\n\n\tstruct { uint32; string }\n\nhave 16 pointer bytes because the garbage collector has to scan up through the string's\ninner pointer.\n\n\tstruct { string; *uint32 }\n\nhas 24 pointer bytes because it has to scan further through the *uint32.\n\n\tstruct { string; uint32 }\n\nhas 8 because it can stop immediately after the string pointer.\n", + Doc: "find structs that would use less memory if their fields were sorted\n\nThis analyzer find structs that can be rearranged to use less memory, and provides\na suggested edit with the most compact order.\n\nNote that there are two different diagnostics reported. One checks struct size,\nand the other reports \"pointer bytes\" used. Pointer bytes is how many bytes of the\nobject that the garbage collector has to potentially scan for pointers, for example:\n\n\tstruct { uint32; string }\n\nhave 16 pointer bytes because the garbage collector has to scan up through the string's\ninner pointer.\n\n\tstruct { string; *uint32 }\n\nhas 24 pointer bytes because it has to scan further through the *uint32.\n\n\tstruct { string; uint32 }\n\nhas 8 because it can stop immediately after the string pointer.\n\nBe aware that the most compact order is not always the most efficient.\nIn rare cases it may cause two variables each updated by its own goroutine\nto occupy the same CPU cache line, inducing a form of memory contention\nknown as \"false sharing\" that slows down both goroutines.\n", Default: "false", }, { @@ -866,7 +866,7 @@ var GeneratedAPIJSON = &APIJSON{ }, { Name: "fieldalignment", - Doc: "find structs that would use less memory if their fields were sorted\n\nThis analyzer find structs that can be rearranged to use less memory, and provides\na suggested edit with the optimal order.\n\nNote that there are two different diagnostics reported. One checks struct size,\nand the other reports \"pointer bytes\" used. Pointer bytes is how many bytes of the\nobject that the garbage collector has to potentially scan for pointers, for example:\n\n\tstruct { uint32; string }\n\nhave 16 pointer bytes because the garbage collector has to scan up through the string's\ninner pointer.\n\n\tstruct { string; *uint32 }\n\nhas 24 pointer bytes because it has to scan further through the *uint32.\n\n\tstruct { string; uint32 }\n\nhas 8 because it can stop immediately after the string pointer.\n", + Doc: "find structs that would use less memory if their fields were sorted\n\nThis analyzer find structs that can be rearranged to use less memory, and provides\na suggested edit with the most compact order.\n\nNote that there are two different diagnostics reported. One checks struct size,\nand the other reports \"pointer bytes\" used. Pointer bytes is how many bytes of the\nobject that the garbage collector has to potentially scan for pointers, for example:\n\n\tstruct { uint32; string }\n\nhave 16 pointer bytes because the garbage collector has to scan up through the string's\ninner pointer.\n\n\tstruct { string; *uint32 }\n\nhas 24 pointer bytes because it has to scan further through the *uint32.\n\n\tstruct { string; uint32 }\n\nhas 8 because it can stop immediately after the string pointer.\n\nBe aware that the most compact order is not always the most efficient.\nIn rare cases it may cause two variables each updated by its own goroutine\nto occupy the same CPU cache line, inducing a form of memory contention\nknown as \"false sharing\" that slows down both goroutines.\n", }, { Name: "httpresponse", From 1a196f04970c04bea7d88492e2618c03b60d4789 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 30 Jun 2022 10:50:45 -0400 Subject: [PATCH 07/12] internal/lsp/cache: don't build symbol info for non-Go files Our symbol query only searches Go files, so there is no point to building (broken) symbol information for non-Go files. Doing so introduces a lot more symbol handles that need to be tracked and walked. Change-Id: I96dd62766d079805fcb1d16eb361adfc0c31eea1 Reviewed-on: https://go-review.googlesource.com/c/tools/+/415199 Reviewed-by: Alan Donovan TryBot-Result: Gopher Robot Run-TryBot: Robert Findley gopls-CI: kokoro --- internal/lsp/cache/snapshot.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 8194750b331..e94ad7ba09c 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -1010,6 +1010,10 @@ func (s *snapshot) Symbols(ctx context.Context) map[span.URI][]source.Symbol { result = make(map[span.URI][]source.Symbol) ) s.files.Range(func(uri span.URI, f source.VersionedFileHandle) { + if s.View().FileKind(f) != source.Go { + return // workspace symbols currently supports only Go files. + } + // TODO(adonovan): upgrade errgroup and use group.SetLimit(nprocs). iolimit <- struct{}{} // acquire token group.Go(func() error { From 8865782bc0d76401a05a438cccb05486ca1e5c62 Mon Sep 17 00:00:00 2001 From: Suzy Mueller Date: Tue, 28 Jun 2022 15:44:45 -0400 Subject: [PATCH 08/12] internal/lsp: add text edits for unkeyed literals Add text edits that a user can accept to make the unkeyed composite literals keyed from the inlay hints. The text edits modify all of the unkeyed fields in a composite literal, since a mixture of keyed and unkeyed fields are not allowed. Change-Id: I0683fbaa5e22bc004b91c98fc09e495e797826ee Reviewed-on: https://go-review.googlesource.com/c/tools/+/414855 TryBot-Result: Gopher Robot gopls-CI: kokoro Reviewed-by: Jamal Carvalho Run-TryBot: Suzy Mueller --- internal/lsp/protocol/tsprotocol.go | 8 ++++++++ internal/lsp/source/inlay_hint.go | 11 ++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/internal/lsp/protocol/tsprotocol.go b/internal/lsp/protocol/tsprotocol.go index 5dd3d09e188..3a284bf45a6 100644 --- a/internal/lsp/protocol/tsprotocol.go +++ b/internal/lsp/protocol/tsprotocol.go @@ -2744,6 +2744,14 @@ type InlayHint = struct { * should fall back to a reasonable default. */ Kind InlayHintKind `json:"kind,omitempty"` + /** + * Optional text edits that are performed when accepting this inlay hint. + * + * *Note* that edits are expected to change the document so that the inlay + * hint (or its nearest variant) is now part of the document and the inlay + * hint itself is now obsolete. + */ + TextEdits []TextEdit `json:"textEdits,omitempty"` /** * The tooltip text when you hover over this item. */ diff --git a/internal/lsp/source/inlay_hint.go b/internal/lsp/source/inlay_hint.go index 009cc52fdd5..967752b5c51 100644 --- a/internal/lsp/source/inlay_hint.go +++ b/internal/lsp/source/inlay_hint.go @@ -344,7 +344,7 @@ func compositeLiteralFields(node ast.Node, tmap *lsppos.TokenMapper, info *types } var hints []protocol.InlayHint - + var allEdits []protocol.TextEdit for i, v := range compLit.Elts { if _, ok := v.(*ast.KeyValueExpr); !ok { start, ok := tmap.Position(v.Pos()) @@ -360,8 +360,17 @@ func compositeLiteralFields(node ast.Node, tmap *lsppos.TokenMapper, info *types Kind: protocol.Parameter, PaddingRight: true, }) + allEdits = append(allEdits, protocol.TextEdit{ + Range: protocol.Range{Start: start, End: start}, + NewText: strct.Field(i).Name() + ": ", + }) } } + // It is not allowed to have a mix of keyed and unkeyed fields, so + // have the text edits add keys to all fields. + for i := range hints { + hints[i].TextEdits = allEdits + } return hints } From 8314b7aa0d97fe43e050787d55308e25f265bc63 Mon Sep 17 00:00:00 2001 From: Suzy Mueller Date: Mon, 27 Jun 2022 18:15:05 -0400 Subject: [PATCH 09/12] go/analysis: add suggested fix for unkeyed composite literals Include a suggested fix with the diagnostic for unkeyed composite literals. This suggested fix will add the name of each of the fields. For golang/go#53062 Change-Id: I0c33191ff3cf66c95a9a055848274cc2b0c38224 Reviewed-on: https://go-review.googlesource.com/c/tools/+/414674 gopls-CI: kokoro TryBot-Result: Gopher Robot Reviewed-by: Alan Donovan Run-TryBot: Suzy Mueller --- go/analysis/passes/composite/composite.go | 41 ++++- .../passes/composite/composite_test.go | 2 +- .../passes/composite/testdata/src/a/a.go | 17 +++ .../composite/testdata/src/a/a.go.golden | 144 ++++++++++++++++++ .../testdata/src/a/a_fuzz_test.go.golden | 16 ++ .../testdata/src/typeparams/typeparams.go | 10 +- .../src/typeparams/typeparams.go.golden | 27 ++++ 7 files changed, 245 insertions(+), 12 deletions(-) create mode 100644 go/analysis/passes/composite/testdata/src/a/a.go.golden create mode 100644 go/analysis/passes/composite/testdata/src/a/a_fuzz_test.go.golden create mode 100644 go/analysis/passes/composite/testdata/src/typeparams/typeparams.go.golden diff --git a/go/analysis/passes/composite/composite.go b/go/analysis/passes/composite/composite.go index d3670aca97a..64e184d3439 100644 --- a/go/analysis/passes/composite/composite.go +++ b/go/analysis/passes/composite/composite.go @@ -7,6 +7,7 @@ package composite import ( + "fmt" "go/ast" "go/types" "strings" @@ -83,7 +84,8 @@ func run(pass *analysis.Pass) (interface{}, error) { } for _, typ := range structuralTypes { under := deref(typ.Underlying()) - if _, ok := under.(*types.Struct); !ok { + strct, ok := under.(*types.Struct) + if !ok { // skip non-struct composite literals continue } @@ -92,20 +94,47 @@ func run(pass *analysis.Pass) (interface{}, error) { continue } - // check if the CompositeLit contains an unkeyed field + // check if the struct contains an unkeyed field allKeyValue := true - for _, e := range cl.Elts { + var suggestedFixAvailable = len(cl.Elts) == strct.NumFields() + var missingKeys []analysis.TextEdit + for i, e := range cl.Elts { if _, ok := e.(*ast.KeyValueExpr); !ok { allKeyValue = false - break + if i >= strct.NumFields() { + break + } + field := strct.Field(i) + if !field.Exported() { + // Adding unexported field names for structs not defined + // locally will not work. + suggestedFixAvailable = false + break + } + missingKeys = append(missingKeys, analysis.TextEdit{ + Pos: e.Pos(), + End: e.Pos(), + NewText: []byte(fmt.Sprintf("%s: ", field.Name())), + }) } } if allKeyValue { - // all the composite literal fields are keyed + // all the struct fields are keyed continue } - pass.ReportRangef(cl, "%s composite literal uses unkeyed fields", typeName) + diag := analysis.Diagnostic{ + Pos: cl.Pos(), + End: cl.End(), + Message: fmt.Sprintf("%s struct literal uses unkeyed fields", typeName), + } + if suggestedFixAvailable { + diag.SuggestedFixes = []analysis.SuggestedFix{{ + Message: "Add field names to struct literal", + TextEdits: missingKeys, + }} + } + pass.Report(diag) return } }) diff --git a/go/analysis/passes/composite/composite_test.go b/go/analysis/passes/composite/composite_test.go index 952de8bfdad..7afaaa7ffd4 100644 --- a/go/analysis/passes/composite/composite_test.go +++ b/go/analysis/passes/composite/composite_test.go @@ -18,5 +18,5 @@ func Test(t *testing.T) { if typeparams.Enabled { pkgs = append(pkgs, "typeparams") } - analysistest.Run(t, testdata, composite.Analyzer, pkgs...) + analysistest.RunWithSuggestedFixes(t, testdata, composite.Analyzer, pkgs...) } diff --git a/go/analysis/passes/composite/testdata/src/a/a.go b/go/analysis/passes/composite/testdata/src/a/a.go index 3a5bc203b03..cd69d395173 100644 --- a/go/analysis/passes/composite/testdata/src/a/a.go +++ b/go/analysis/passes/composite/testdata/src/a/a.go @@ -11,6 +11,7 @@ import ( "go/scanner" "go/token" "image" + "sync" "unicode" ) @@ -79,6 +80,18 @@ var badStructLiteral = flag.Flag{ // want "unkeyed fields" nil, // Value "DefValue", } +var tooManyFieldsStructLiteral = flag.Flag{ // want "unkeyed fields" + "Name", + "Usage", + nil, // Value + "DefValue", + "Extra Field", +} +var tooFewFieldsStructLiteral = flag.Flag{ // want "unkeyed fields" + "Name", + "Usage", + nil, // Value +} var delta [3]rune @@ -100,6 +113,10 @@ var badScannerErrorList = scanner.ErrorList{ &scanner.Error{token.Position{}, "foobar"}, // want "unkeyed fields" } +// sync.Mutex has unexported fields. We expect a diagnostic but no +// suggested fix. +var mu = sync.Mutex{0, 0} // want "unkeyed fields" + // Check whitelisted structs: if vet is run with --compositewhitelist=false, // this line triggers an error. var whitelistedPoint = image.Point{1, 2} diff --git a/go/analysis/passes/composite/testdata/src/a/a.go.golden b/go/analysis/passes/composite/testdata/src/a/a.go.golden new file mode 100644 index 00000000000..fe73a2e0a1d --- /dev/null +++ b/go/analysis/passes/composite/testdata/src/a/a.go.golden @@ -0,0 +1,144 @@ +// Copyright 2012 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. + +// This file contains the test for untagged struct literals. + +package a + +import ( + "flag" + "go/scanner" + "go/token" + "image" + "sync" + "unicode" +) + +var Okay1 = []string{ + "Name", + "Usage", + "DefValue", +} + +var Okay2 = map[string]bool{ + "Name": true, + "Usage": true, + "DefValue": true, +} + +var Okay3 = struct { + X string + Y string + Z string +}{ + "Name", + "Usage", + "DefValue", +} + +var Okay4 = []struct { + A int + B int +}{ + {1, 2}, + {3, 4}, +} + +type MyStruct struct { + X string + Y string + Z string +} + +var Okay5 = &MyStruct{ + "Name", + "Usage", + "DefValue", +} + +var Okay6 = []MyStruct{ + {"foo", "bar", "baz"}, + {"aa", "bb", "cc"}, +} + +var Okay7 = []*MyStruct{ + {"foo", "bar", "baz"}, + {"aa", "bb", "cc"}, +} + +// Testing is awkward because we need to reference things from a separate package +// to trigger the warnings. + +var goodStructLiteral = flag.Flag{ + Name: "Name", + Usage: "Usage", +} +var badStructLiteral = flag.Flag{ // want "unkeyed fields" + Name: "Name", + Usage: "Usage", + Value: nil, // Value + DefValue: "DefValue", +} +var tooManyFieldsStructLiteral = flag.Flag{ // want "unkeyed fields" + "Name", + "Usage", + nil, // Value + "DefValue", + "Extra Field", +} +var tooFewFieldsStructLiteral = flag.Flag{ // want "unkeyed fields" + "Name", + "Usage", + nil, // Value +} + +var delta [3]rune + +// SpecialCase is a named slice of CaseRange to test issue 9171. +var goodNamedSliceLiteral = unicode.SpecialCase{ + {Lo: 1, Hi: 2, Delta: delta}, + unicode.CaseRange{Lo: 1, Hi: 2, Delta: delta}, +} +var badNamedSliceLiteral = unicode.SpecialCase{ + {Lo: 1, Hi: 2, Delta: delta}, // want "unkeyed fields" + unicode.CaseRange{Lo: 1, Hi: 2, Delta: delta}, // want "unkeyed fields" +} + +// ErrorList is a named slice, so no warnings should be emitted. +var goodScannerErrorList = scanner.ErrorList{ + &scanner.Error{Msg: "foobar"}, +} +var badScannerErrorList = scanner.ErrorList{ + &scanner.Error{Pos: token.Position{}, Msg: "foobar"}, // want "unkeyed fields" +} + +// sync.Mutex has unexported fields. We expect a diagnostic but no +// suggested fix. +var mu = sync.Mutex{0, 0} // want "unkeyed fields" + +// Check whitelisted structs: if vet is run with --compositewhitelist=false, +// this line triggers an error. +var whitelistedPoint = image.Point{1, 2} + +// Do not check type from unknown package. +// See issue 15408. +var unknownPkgVar = unicode.NoSuchType{"foo", "bar"} + +// A named pointer slice of CaseRange to test issue 23539. In +// particular, we're interested in how some slice elements omit their +// type. +var goodNamedPointerSliceLiteral = []*unicode.CaseRange{ + {Lo: 1, Hi: 2}, + &unicode.CaseRange{Lo: 1, Hi: 2}, +} +var badNamedPointerSliceLiteral = []*unicode.CaseRange{ + {Lo: 1, Hi: 2, Delta: delta}, // want "unkeyed fields" + &unicode.CaseRange{Lo: 1, Hi: 2, Delta: delta}, // want "unkeyed fields" +} + +// unicode.Range16 is whitelisted, so there'll be no vet error +var range16 = unicode.Range16{0xfdd0, 0xfdef, 1} + +// unicode.Range32 is whitelisted, so there'll be no vet error +var range32 = unicode.Range32{0x1fffe, 0x1ffff, 1} diff --git a/go/analysis/passes/composite/testdata/src/a/a_fuzz_test.go.golden b/go/analysis/passes/composite/testdata/src/a/a_fuzz_test.go.golden new file mode 100644 index 00000000000..20b652e88dd --- /dev/null +++ b/go/analysis/passes/composite/testdata/src/a/a_fuzz_test.go.golden @@ -0,0 +1,16 @@ +// Copyright 2022 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.18 +// +build go1.18 + +package a + +import "testing" + +var fuzzTargets = []testing.InternalFuzzTarget{ + {"Fuzz", Fuzz}, +} + +func Fuzz(f *testing.F) {} diff --git a/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go b/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go index dd5d57efed4..f9a5e1fb105 100644 --- a/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go +++ b/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go @@ -6,7 +6,7 @@ package typeparams import "typeparams/lib" -type localStruct struct { F int } +type localStruct struct{ F int } func F[ T1 ~struct{ f int }, @@ -20,8 +20,8 @@ func F[ _ = T1{2} _ = T2a{2} _ = T2b{2} // want "unkeyed fields" - _ = T3{1,2} - _ = T4{1,2} - _ = T5{1:2} - _ = T6{1:2} + _ = T3{1, 2} + _ = T4{1, 2} + _ = T5{1: 2} + _ = T6{1: 2} } diff --git a/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go.golden b/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go.golden new file mode 100644 index 00000000000..66cd9158cb6 --- /dev/null +++ b/go/analysis/passes/composite/testdata/src/typeparams/typeparams.go.golden @@ -0,0 +1,27 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package typeparams + +import "typeparams/lib" + +type localStruct struct{ F int } + +func F[ + T1 ~struct{ f int }, + T2a localStruct, + T2b lib.Struct, + T3 ~[]int, + T4 lib.Slice, + T5 ~map[int]int, + T6 lib.Map, +]() { + _ = T1{2} + _ = T2a{2} + _ = T2b{F: 2} // want "unkeyed fields" + _ = T3{1, 2} + _ = T4{1, 2} + _ = T5{1: 2} + _ = T6{1: 2} +} From e8e5b37084ab41357340419ff15cba5fb08af935 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 30 Jun 2022 14:01:50 -0400 Subject: [PATCH 10/12] internal/lsp/cache: don't construct a new metadata graph if no changes Change-Id: I3f074d1fd29cf7ad0323cec76154f9b2e31f7356 Reviewed-on: https://go-review.googlesource.com/c/tools/+/415494 Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Gopher Robot Reviewed-by: Alan Donovan --- internal/lsp/cache/graph.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/lsp/cache/graph.go b/internal/lsp/cache/graph.go index dc7d4faef78..ad39aa8d862 100644 --- a/internal/lsp/cache/graph.go +++ b/internal/lsp/cache/graph.go @@ -32,6 +32,10 @@ type metadataGraph struct { // Clone creates a new metadataGraph, applying the given updates to the // receiver. func (g *metadataGraph) Clone(updates map[PackageID]*KnownMetadata) *metadataGraph { + if len(updates) == 0 { + // Optimization: since the graph is immutable, we can return the receiver. + return g + } result := &metadataGraph{metadata: make(map[PackageID]*KnownMetadata, len(g.metadata))} // Copy metadata. for id, m := range g.metadata { From c77473fa95f1277e5bf9708de95d29e1ff350d34 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Thu, 30 Jun 2022 15:13:28 -0400 Subject: [PATCH 11/12] gopls: upgrade staticcheck to v0.3.2 Selectively upgrade only staticcheck, to pick up fixes for support to generic code. Change-Id: Ia625c4d46780139aa6e70447eebe1b6d476d4722 Reviewed-on: https://go-review.googlesource.com/c/tools/+/415495 TryBot-Result: Gopher Robot Reviewed-by: Alan Donovan gopls-CI: kokoro Run-TryBot: Robert Findley --- gopls/go.mod | 2 +- gopls/go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/gopls/go.mod b/gopls/go.mod index 5dc62d3df0a..bd118e226bb 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -11,7 +11,7 @@ require ( golang.org/x/sys v0.0.0-20220209214540-3681064d5158 golang.org/x/tools v0.1.11-0.20220523181440-ccb10502d1a5 golang.org/x/vuln v0.0.0-20220613164644-4eb5ba49563c - honnef.co/go/tools v0.3.0 + honnef.co/go/tools v0.3.2 mvdan.cc/gofumpt v0.3.0 mvdan.cc/xurls/v2 v2.4.0 ) diff --git a/gopls/go.sum b/gopls/go.sum index 91f552ef905..73a55fbad82 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -89,6 +89,8 @@ gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= honnef.co/go/tools v0.2.2/go.mod h1:lPVVZ2BS5TfnjLyizF7o7hv7j9/L+8cZY2hLyjP9cGY= honnef.co/go/tools v0.3.0 h1:2LdYUZ7CIxnYgskbUZfY7FPggmqnh6shBqfWa8Tn3XU= honnef.co/go/tools v0.3.0/go.mod h1:vlRD9XErLMGT+mDuofSr0mMMquscM/1nQqtRSsh6m70= +honnef.co/go/tools v0.3.2 h1:ytYb4rOqyp1TSa2EPvNVwtPQJctSELKaMyLfqNP4+34= +honnef.co/go/tools v0.3.2/go.mod h1:jzwdWgg7Jdq75wlfblQxO4neNaFFSvgc1tD5Wv8U0Yw= mvdan.cc/gofumpt v0.3.0 h1:kTojdZo9AcEYbQYhGuLf/zszYthRdhDNDUi2JKTxas4= mvdan.cc/gofumpt v0.3.0/go.mod h1:0+VyGZWleeIj5oostkOex+nDBA0eyavuDnDusAJ8ylo= mvdan.cc/unparam v0.0.0-20211214103731-d0ef000c54e5 h1:Jh3LAeMt1eGpxomyu3jVkmVZWW2MxZ1qIIV2TZ/nRio= From 9358addbaa72dd292045f8ba280aba316ca8eff1 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Tue, 28 Jun 2022 20:25:04 -0400 Subject: [PATCH 12/12] internal/lsp/cache: remove unused function This function was actually left behind, after making suggested changes to CL 413683. Change-Id: I6933e870ded9da5af06724c28839c37d58fb4cdc Reviewed-on: https://go-review.googlesource.com/c/tools/+/414856 TryBot-Result: Gopher Robot Reviewed-by: Alan Donovan gopls-CI: kokoro Run-TryBot: Robert Findley --- internal/lsp/cache/graph.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/internal/lsp/cache/graph.go b/internal/lsp/cache/graph.go index ad39aa8d862..88c9f147195 100644 --- a/internal/lsp/cache/graph.go +++ b/internal/lsp/cache/graph.go @@ -158,18 +158,3 @@ func (g *metadataGraph) reverseTransitiveClosure(includeInvalid bool, ids ...Pac visitAll(ids) return seen } - -func collectReverseTransitiveClosure(g *metadataGraph, includeInvalid bool, ids []PackageID, seen map[PackageID]struct{}) { - for _, id := range ids { - if _, ok := seen[id]; ok { - continue - } - m := g.metadata[id] - // Only use invalid metadata if we support it. - if m == nil || !(m.Valid || includeInvalid) { - continue - } - seen[id] = struct{}{} - collectReverseTransitiveClosure(g, includeInvalid, g.importedBy[id], seen) - } -}