internal/lsp: add a test for gc annotation details code lens This change adds a test to check the functionality of the GC details code lens, including the behavior of toggling it on and off. This exposed a Windows bug that was mentioned on Slack, which can be fixed by adjusting the URI. I also refactored a bit of the code to use a JSON decoder, which simplifies things a little bit. Change-Id: I7737107cf020fa35bca245485a3b1a1416920fd2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/256940 Trust: Rebecca Stambler <rstambler@golang.org> Run-TryBot: Rebecca Stambler <rstambler@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Peter Weinberger <pjw@google.com> (cherry picked from commit 0eae6ac92e00cc207496dba0031d11256e1156c6) Reviewed-on: https://go-review.googlesource.com/c/tools/+/257598
diff --git a/gopls/internal/regtest/codelens_test.go b/gopls/internal/regtest/codelens_test.go index 3fcfdce..5dd3afb 100644 --- a/gopls/internal/regtest/codelens_test.go +++ b/gopls/internal/regtest/codelens_test.go
@@ -5,6 +5,8 @@ package regtest import ( + "runtime" + "strings" "testing" "golang.org/x/tools/internal/lsp/protocol" @@ -177,24 +179,7 @@ ` runner.Run(t, shouldRemoveDep, func(t *testing.T, env *Env) { env.OpenFile("go.mod") - lenses := env.CodeLens("go.mod") - want := "Tidy module" - var found protocol.CodeLens - for _, lens := range lenses { - if lens.Command.Title == want { - found = lens - break - } - } - if found.Command.Command == "" { - t.Fatalf("did not find lens %q, got %v", want, found.Command) - } - if _, err := env.Editor.Server.ExecuteCommand(env.Ctx, &protocol.ExecuteCommandParams{ - Command: found.Command.Command, - Arguments: found.Command.Arguments, - }); err != nil { - t.Fatal(err) - } + env.ExecuteCodeLensCommand("go.mod", source.CommandTidy) env.Await(NoOutstandingWork()) got := env.ReadWorkspaceFile("go.mod") const wantGoMod = `module mod.com @@ -239,19 +224,62 @@ env.Await(env.DiagnosticAtRegexp("cgo.go", `C\.(fortytwo)`)) // Regenerate cgo, fixing the diagnostic. - lenses := env.CodeLens("cgo.go") - var lens protocol.CodeLens - for _, l := range lenses { - if l.Command.Command == source.CommandRegenerateCgo.Name { - lens = l + env.ExecuteCodeLensCommand("cgo.go", source.CommandRegenerateCgo) + env.Await(EmptyDiagnostics("cgo.go")) + }) +} + +func TestGCDetails(t *testing.T) { + testenv.NeedsGo1Point(t, 15) + if runtime.GOOS == "android" { + t.Skipf("the gc details code lens doesn't work on Android") + } + + const mod = ` +-- go.mod -- +module mod.com + +go 1.15 +-- main.go -- +package main + +import "fmt" + +func main() { + var x string + fmt.Println(x) +} +` + withOptions( + EditorConfig{CodeLens: map[string]bool{"gc_details": true}}, + ).run(t, mod, func(t *testing.T, env *Env) { + env.Await(InitialWorkspaceLoad) + env.OpenFile("main.go") + env.ExecuteCodeLensCommand("main.go", source.CommandToggleDetails) + d := &protocol.PublishDiagnosticsParams{} + env.Await( + OnceMet( + DiagnosticAt("main.go", 6, 12), + ReadDiagnostics("main.go", d), + ), + ) + // Confirm that the diagnostics come from the gc details code lens. + var found bool + for _, d := range d.Diagnostics { + if d.Severity != protocol.SeverityInformation { + t.Fatalf("unexpected diagnostic severity %v, wanted Information", d.Severity) + } + if strings.Contains(d.Message, "x escapes") { + found = true } } - if _, err := env.Editor.Server.ExecuteCommand(env.Ctx, &protocol.ExecuteCommandParams{ - Command: lens.Command.Command, - Arguments: lens.Command.Arguments, - }); err != nil { - t.Fatal(err) + if !found { + t.Fatalf(`expected to find diagnostic with message "escape(x escapes to heap)", found none`) } - env.Await(EmptyDiagnostics("cgo.go")) + // Toggle the GC details code lens again so now it should be off. + env.ExecuteCodeLensCommand("main.go", source.CommandToggleDetails) + env.Await( + EmptyDiagnostics("main.go"), + ) }) }
diff --git a/gopls/internal/regtest/wrappers.go b/gopls/internal/regtest/wrappers.go index 8d8136f..294d303 100644 --- a/gopls/internal/regtest/wrappers.go +++ b/gopls/internal/regtest/wrappers.go
@@ -10,6 +10,7 @@ "golang.org/x/tools/internal/lsp/fake" "golang.org/x/tools/internal/lsp/protocol" + "golang.org/x/tools/internal/lsp/source" errors "golang.org/x/xerrors" ) @@ -259,6 +260,29 @@ return lens } +// ExecuteCodeLensCommand executes the command for the code lens matching the +// given command name. +func (e *Env) ExecuteCodeLensCommand(path string, cmd *source.Command) { + lenses := e.CodeLens(path) + var lens protocol.CodeLens + var found bool + for _, l := range lenses { + if l.Command.Command == cmd.Name { + lens = l + found = true + } + } + if !found { + e.T.Fatalf("found no command with the title %s", cmd.Name) + } + if _, err := e.Editor.Server.ExecuteCommand(e.Ctx, &protocol.ExecuteCommandParams{ + Command: lens.Command.Command, + Arguments: lens.Command.Arguments, + }); err != nil { + e.T.Fatal(err) + } +} + // References calls textDocument/references for the given path at the given // position. func (e *Env) References(path string, pos fake.Pos) []protocol.Location {
diff --git a/internal/lsp/command.go b/internal/lsp/command.go index 2f0f4d5..e6937ef 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go
@@ -11,8 +11,6 @@ "fmt" "io" "io/ioutil" - "log" - "path" "path/filepath" "golang.org/x/tools/internal/event" @@ -203,7 +201,7 @@ if err := source.UnmarshalArgs(args, &fileURI); err != nil { return err } - pkgDir := span.URIFromPath(path.Dir(fileURI.Filename())) + pkgDir := span.URIFromPath(filepath.Dir(fileURI.Filename())) s.gcOptimizationDetailsMu.Lock() if _, ok := s.gcOptimizatonDetails[pkgDir]; ok { delete(s.gcOptimizatonDetails, pkgDir) @@ -211,8 +209,6 @@ s.gcOptimizatonDetails[pkgDir] = struct{}{} } s.gcOptimizationDetailsMu.Unlock() - event.Log(ctx, fmt.Sprintf("gc_details %s now %v %v", pkgDir, s.gcOptimizatonDetails[pkgDir], - s.gcOptimizatonDetails)) // need to recompute diagnostics. // so find the snapshot sv, err := s.session.ViewOf(fileURI) @@ -290,7 +286,6 @@ var failedTests int for _, funcName := range tests { args := []string{pkgPath, "-v", "-count=1", "-run", fmt.Sprintf("^%s$", funcName)} - log.Printf("running with these args: %v", args) if err := snapshot.RunGoCommandPiped(ctx, "test", args, out, out); err != nil { if errors.Is(err, context.Canceled) { return err
diff --git a/internal/lsp/source/code_lens.go b/internal/lsp/source/code_lens.go index 427b548..2aee889 100644 --- a/internal/lsp/source/code_lens.go +++ b/internal/lsp/source/code_lens.go
@@ -242,14 +242,12 @@ if err != nil { return nil, err } - return []protocol.CodeLens{ - { - Range: rng, - Command: protocol.Command{ - Title: "Toggle gc annotation details", - Command: CommandToggleDetails.Name, - Arguments: jsonArgs, - }, + return []protocol.CodeLens{{ + Range: rng, + Command: protocol.Command{ + Title: "Toggle gc annotation details", + Command: CommandToggleDetails.Name, + Arguments: jsonArgs, }, - }, nil + }}, nil }
diff --git a/internal/lsp/source/gc_annotations.go b/internal/lsp/source/gc_annotations.go index 2d85026..8aa13cb 100644 --- a/internal/lsp/source/gc_annotations.go +++ b/internal/lsp/source/gc_annotations.go
@@ -20,6 +20,7 @@ func GCOptimizationDetails(ctx context.Context, snapshot Snapshot, pkgDir span.URI) (map[VersionedFileIdentity][]*Diagnostic, error) { outDir := filepath.Join(os.TempDir(), fmt.Sprintf("gopls-%d.details", os.Getpid())) + if err := os.MkdirAll(outDir, 0700); err != nil { return nil, err } @@ -28,7 +29,15 @@ return nil, err } defer os.Remove(tmpFile.Name()) - args := []string{fmt.Sprintf("-gcflags=-json=0,%s", outDir), + + outDirURI := span.URIFromPath(outDir) + // GC details doesn't handle Windows URIs in the form of "file:///C:/...", + // so rewrite them to "file://C:/...". See golang/go#41614. + if !strings.HasPrefix(outDir, "/") { + outDirURI = span.URI(strings.Replace(string(outDirURI), "file:///", "file://", 1)) + } + args := []string{ + fmt.Sprintf("-gcflags=-json=0,%s", outDirURI), fmt.Sprintf("-o=%s", tmpFile.Name()), pkgDir.Filename(), } @@ -44,100 +53,114 @@ opts := snapshot.View().Options() var parseError error for _, fn := range files { - fname, v, err := parseDetailsFile(fn) + uri, diagnostics, err := parseDetailsFile(fn, opts) if err != nil { // expect errors for all the files, save 1 parseError = err } - if !strings.HasSuffix(fname, ".go") { - continue // <autogenerated> - } - uri := span.URIFromPath(fname) - x := snapshot.FindFile(uri) - if x == nil { + fh := snapshot.FindFile(uri) + if fh == nil { continue } - v = filterDiagnostics(v, opts) - reports[x.VersionedFileIdentity()] = v + reports[fh.VersionedFileIdentity()] = diagnostics } return reports, parseError } -func filterDiagnostics(v []*Diagnostic, o *Options) []*Diagnostic { - var ans []*Diagnostic - for _, x := range v { - if x.Source != "go compiler" { - continue - } - if o.Annotations["noInline"] && - (strings.HasPrefix(x.Message, "canInline") || - strings.HasPrefix(x.Message, "cannotInline") || - strings.HasPrefix(x.Message, "inlineCall")) { - continue - } else if o.Annotations["noEscape"] && - (strings.HasPrefix(x.Message, "escape") || x.Message == "leak") { - continue - } else if o.Annotations["noNilcheck"] && strings.HasPrefix(x.Message, "nilcheck") { - continue - } else if o.Annotations["noBounds"] && - (strings.HasPrefix(x.Message, "isInBounds") || - strings.HasPrefix(x.Message, "isSliceInBounds")) { - continue - } - ans = append(ans, x) - } - return ans -} - -func parseDetailsFile(fn string) (string, []*Diagnostic, error) { - buf, err := ioutil.ReadFile(fn) +func parseDetailsFile(filename string, options *Options) (span.URI, []*Diagnostic, error) { + buf, err := ioutil.ReadFile(filename) if err != nil { - return "", nil, err // This is an internal error. Likely ever file will fail. + return "", nil, err } - var fname string - var ans []*Diagnostic - lines := bytes.Split(buf, []byte{'\n'}) - for i, l := range lines { - if len(l) == 0 { - continue - } + var ( + uri span.URI + i int + diagnostics []*Diagnostic + ) + type metadata struct { + File string `json:"file,omitempty"` + } + for dec := json.NewDecoder(bytes.NewReader(buf)); dec.More(); { + // The first element always contains metadata. if i == 0 { - x := make(map[string]interface{}) - if err := json.Unmarshal(l, &x); err != nil { - return "", nil, fmt.Errorf("internal error (%v) parsing first line of json file %s", - err, fn) + i++ + m := new(metadata) + if err := dec.Decode(m); err != nil { + return "", nil, err } - fname = x["file"].(string) + if !strings.HasSuffix(m.File, ".go") { + continue // <autogenerated> + } + uri = span.URIFromPath(m.File) continue } - y := protocol.Diagnostic{} - if err := json.Unmarshal(l, &y); err != nil { - return "", nil, fmt.Errorf("internal error (%#v) parsing json file for %s", err, fname) + d := new(protocol.Diagnostic) + if err := dec.Decode(d); err != nil { + return "", nil, err } - y.Range.Start.Line-- // change from 1-based to 0-based - y.Range.Start.Character-- - y.Range.End.Line-- - y.Range.End.Character-- - msg := y.Code.(string) - if y.Message != "" { - msg = fmt.Sprintf("%s(%s)", msg, y.Message) + msg := d.Code.(string) + if msg != "" { + msg = fmt.Sprintf("%s(%s)", msg, d.Message) } - x := Diagnostic{ - Range: y.Range, - Message: msg, - Source: y.Source, - Severity: y.Severity, + if skipDiagnostic(msg, d.Source, options) { + continue } - for _, ri := range y.RelatedInformation { - x.Related = append(x.Related, RelatedInformation{ + var related []RelatedInformation + for _, ri := range d.RelatedInformation { + related = append(related, RelatedInformation{ URI: ri.Location.URI.SpanURI(), - Range: ri.Location.Range, + Range: zeroIndexedRange(ri.Location.Range), Message: ri.Message, }) } - ans = append(ans, &x) + diagnostic := &Diagnostic{ + Range: zeroIndexedRange(d.Range), + Message: msg, + Severity: d.Severity, + Source: d.Source, + Tags: d.Tags, + Related: related, + } + diagnostics = append(diagnostics, diagnostic) + i++ } - return fname, ans, nil + return uri, diagnostics, nil +} + +// skipDiagnostic reports whether a given diagnostic should be shown to the end +// user, given the current options. +func skipDiagnostic(msg, source string, o *Options) bool { + if source != "go compiler" { + return false + } + switch { + case o.Annotations["noInline"]: + return strings.HasPrefix(msg, "canInline") || + strings.HasPrefix(msg, "cannotInline") || + strings.HasPrefix(msg, "inlineCall") + case o.Annotations["noEscape"]: + return strings.HasPrefix(msg, "escape") || msg == "leak" + case o.Annotations["noNilcheck"]: + return strings.HasPrefix(msg, "nilcheck") + case o.Annotations["noBounds"]: + return strings.HasPrefix(msg, "isInBounds") || + strings.HasPrefix(msg, "isSliceInBounds") + } + return false +} + +// The range produced by the compiler is 1-indexed, so subtract range by 1. +func zeroIndexedRange(rng protocol.Range) protocol.Range { + return protocol.Range{ + Start: protocol.Position{ + Line: rng.Start.Line - 1, + Character: rng.Start.Character - 1, + }, + End: protocol.Position{ + Line: rng.End.Line - 1, + Character: rng.End.Character - 1, + }, + } } func findJSONFiles(dir string) ([]string, error) {