From 128f61d438af442db4e5a489ccf8d06899c031cf Mon Sep 17 00:00:00 2001 From: "Hana (Hyang-Ah) Kim" Date: Fri, 11 Nov 2022 22:34:29 -0500 Subject: [PATCH] gopls/internal/lsp/mod: add related info Related info in module vulncheck diagnostics carries the location to the entry stack frame found from some of the call paths. Change-Id: I99eb60ab1c2cb7cc356eae8d2ba35a2167cb179e Reviewed-on: https://go-review.googlesource.com/c/tools/+/450278 TryBot-Result: Gopher Robot Run-TryBot: Hyang-Ah Hana Kim Reviewed-by: Robert Findley gopls-CI: kokoro Reviewed-by: Suzy Mueller --- gopls/internal/lsp/mod/diagnostics.go | 46 +++++++++++- gopls/internal/regtest/misc/vuln_test.go | 89 ++++++++++++++++++------ 2 files changed, 112 insertions(+), 23 deletions(-) diff --git a/gopls/internal/lsp/mod/diagnostics.go b/gopls/internal/lsp/mod/diagnostics.go index 2bebf6f229..3b74bd8835 100644 --- a/gopls/internal/lsp/mod/diagnostics.go +++ b/gopls/internal/lsp/mod/diagnostics.go @@ -18,6 +18,7 @@ import ( "golang.org/x/tools/gopls/internal/lsp/command" "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/lsp/source" + "golang.org/x/tools/gopls/internal/span" "golang.org/x/tools/internal/event" "golang.org/x/vuln/osv" ) @@ -213,6 +214,7 @@ func ModVulnerabilityDiagnostics(ctx context.Context, snapshot source.Snapshot, // Fixes will include only the upgrades for warning level diagnostics. var fixes []source.SuggestedFix var warning, info []string + var relatedInfo []source.RelatedInformation for _, mv := range vulns { mod, vuln := mv.mod, mv.vuln // It is possible that the source code was changed since the last @@ -238,6 +240,7 @@ func ModVulnerabilityDiagnostics(ctx context.Context, snapshot source.Snapshot, info = append(info, vuln.OSV.ID) } else { warning = append(warning, vuln.OSV.ID) + relatedInfo = append(relatedInfo, listRelatedInfo(ctx, snapshot, vuln)...) } // Upgrade to the exact version we offer the user, not the most recent. if fixedVersion := mod.FixedVersion; semver.IsValid(fixedVersion) && semver.Compare(req.Mod.Version, fixedVersion) < 0 { @@ -282,7 +285,6 @@ func ModVulnerabilityDiagnostics(ctx context.Context, snapshot source.Snapshot, default: fmt.Fprintf(&b, "%v has vulnerabilities used in the code: %v.", req.Mod.Path, strings.Join(warning, ", ")) } - vulnDiagnostics = append(vulnDiagnostics, &source.Diagnostic{ URI: fh.URI(), Range: rng, @@ -290,12 +292,54 @@ func ModVulnerabilityDiagnostics(ctx context.Context, snapshot source.Snapshot, Source: source.Vulncheck, Message: b.String(), SuggestedFixes: fixes, + Related: relatedInfo, }) } return vulnDiagnostics, nil } +func listRelatedInfo(ctx context.Context, snapshot source.Snapshot, vuln *govulncheck.Vuln) []source.RelatedInformation { + var ri []source.RelatedInformation + for _, m := range vuln.Modules { + for _, p := range m.Packages { + for _, c := range p.CallStacks { + if len(c.Frames) == 0 { + continue + } + entry := c.Frames[0] + pos := entry.Position + if pos.Filename == "" { + continue // token.Position Filename is an optional field. + } + uri := span.URIFromPath(pos.Filename) + startPos := protocol.Position{ + Line: uint32(pos.Line) - 1, + // We need to read the file contents to precisesly map + // token.Position (pos) to the UTF16-based column offset + // protocol.Position requires. That can be expensive. + // We need this related info to just help users to open + // the entry points of the callstack and once the file is + // open, we will compute the precise location based on the + // open file contents. So, use the beginning of the line + // as the position here instead of precise UTF16-based + // position computation. + Character: 0, + } + ri = append(ri, source.RelatedInformation{ + URI: uri, + Range: protocol.Range{ + Start: startPos, + End: startPos, + }, + Message: fmt.Sprintf("[%v] %v -> %v.%v", vuln.OSV.ID, entry.Name(), p.Path, c.Symbol), + }) + } + } + } + return ri +} + func formatMessage(v *govulncheck.Vuln) string { details := []byte(v.OSV.Details) // Remove any new lines that are not preceded or followed by a new line. diff --git a/gopls/internal/regtest/misc/vuln_test.go b/gopls/internal/regtest/misc/vuln_test.go index b38b184a61..b511eb7e43 100644 --- a/gopls/internal/regtest/misc/vuln_test.go +++ b/gopls/internal/regtest/misc/vuln_test.go @@ -9,9 +9,12 @@ package misc import ( "context" + "path/filepath" + "sort" "strings" "testing" + "github.com/google/go-cmp/cmp" "golang.org/x/tools/gopls/internal/lsp/command" "golang.org/x/tools/gopls/internal/lsp/protocol" . "golang.org/x/tools/gopls/internal/lsp/regtest" @@ -329,6 +332,10 @@ func TestRunVulncheckWarning(t *testing.T) { ReadDiagnostics("go.mod", gotDiagnostics), ), ) + env.OpenFile("x/x.go") + lineX := env.RegexpSearch("x/x.go", `c\.C1\(\)\.Vuln1\(\)`) + env.OpenFile("y/y.go") + lineY := env.RegexpSearch("y/y.go", `c\.C2\(\)\(\)`) wantDiagnostics := map[string]vulnDiagExpectation{ "golang.org/amod": { applyAction: "Upgrade to v1.0.4", @@ -340,6 +347,10 @@ func TestRunVulncheckWarning(t *testing.T) { "Upgrade to latest", "Upgrade to v1.0.4", }, + relatedInfo: []vulnRelatedInfo{ + {"x.go", uint32(lineX.Line), "[GO-2022-01]"}, // avuln.VulnData.Vuln1 + {"x.go", uint32(lineX.Line), "[GO-2022-01]"}, // avuln.VulnData.Vuln2 + }, }, }, codeActions: []string{ @@ -353,6 +364,9 @@ func TestRunVulncheckWarning(t *testing.T) { { msg: "golang.org/bmod has a vulnerability used in the code: GO-2022-02.", severity: protocol.SeverityWarning, + relatedInfo: []vulnRelatedInfo{ + {"y.go", uint32(lineY.Line), "[GO-2022-02]"}, // bvuln.Vuln + }, }, }, hover: []string{"GO-2022-02", "This is a long description of this vulnerability.", "No fix is available."}, @@ -364,8 +378,8 @@ func TestRunVulncheckWarning(t *testing.T) { // Check that the actions we get when including all diagnostics at a location return the same result gotActions := env.CodeAction("go.mod", modPathDiagnostics) - if !sameCodeActions(gotActions, want.codeActions) { - t.Errorf("code actions for %q do not match, expected %v, got %v\n", mod, want.codeActions, gotActions) + if diff := diffCodeActions(gotActions, want.codeActions); diff != "" { + t.Errorf("code actions for %q do not match, expected %v, got %v\n%v\n", mod, want.codeActions, gotActions, diff) continue } @@ -399,20 +413,12 @@ require ( }) } -func sameCodeActions(gotActions []protocol.CodeAction, want []string) bool { - gotTitles := make([]string, len(gotActions)) - for i, ca := range gotActions { - gotTitles[i] = ca.Title +func diffCodeActions(gotActions []protocol.CodeAction, want []string) string { + var gotTitles []string + for _, ca := range gotActions { + gotTitles = append(gotTitles, ca.Title) } - if len(gotTitles) != len(want) { - return false - } - for i := range want { - if gotTitles[i] != want[i] { - return false - } - } - return true + return cmp.Diff(want, gotTitles) } const workspace2 = ` @@ -483,8 +489,8 @@ func TestRunVulncheckInfo(t *testing.T) { modPathDiagnostics := testVulnDiagnostics(t, env, mod, want, gotDiagnostics) // Check that the actions we get when including all diagnostics at a location return the same result gotActions := env.CodeAction("go.mod", modPathDiagnostics) - if !sameCodeActions(gotActions, want.codeActions) { - t.Errorf("code actions for %q do not match, expected %v, got %v\n", mod, want.codeActions, gotActions) + if diff := diffCodeActions(gotActions, want.codeActions); diff != "" { + t.Errorf("code actions for %q do not match, expected %v, got %v\n%v\n", mod, want.codeActions, gotActions, diff) continue } } @@ -513,12 +519,16 @@ func testVulnDiagnostics(t *testing.T, env *Env, requireMod string, want vulnDia continue } if diag.Severity != w.severity { - t.Errorf("incorrect severity for %q, expected %s got %s\n", w.msg, w.severity, diag.Severity) + t.Errorf("incorrect severity for %q, want %s got %s\n", w.msg, w.severity, diag.Severity) } - + sort.Slice(w.relatedInfo, func(i, j int) bool { return w.relatedInfo[i].less(w.relatedInfo[j]) }) + if got, want := summarizeRelatedInfo(diag.RelatedInformation), w.relatedInfo; !cmp.Equal(got, want) { + t.Errorf("related info for %q do not match, want %v, got %v\n", w.msg, want, got) + } + // Check expected code actions appear. gotActions := env.CodeAction("go.mod", []protocol.Diagnostic{*diag}) - if !sameCodeActions(gotActions, w.codeActions) { - t.Errorf("code actions for %q do not match, expected %v, got %v\n", w.msg, w.codeActions, gotActions) + if diff := diffCodeActions(gotActions, w.codeActions); diff != "" { + t.Errorf("code actions for %q do not match, want %v, got %v\n%v\n", w.msg, w.codeActions, gotActions, diff) continue } @@ -527,7 +537,7 @@ func testVulnDiagnostics(t *testing.T, env *Env, requireMod string, want vulnDia hover, _ := env.Hover("go.mod", pos) for _, part := range want.hover { if !strings.Contains(hover.Value, part) { - t.Errorf("hover contents for %q do not match, expected %v, got %v\n", w.msg, strings.Join(want.hover, ","), hover.Value) + t.Errorf("hover contents for %q do not match, want %v, got %v\n", w.msg, strings.Join(want.hover, ","), hover.Value) break } } @@ -536,12 +546,47 @@ func testVulnDiagnostics(t *testing.T, env *Env, requireMod string, want vulnDia return modPathDiagnostics } +// summarizeRelatedInfo converts protocol.DiagnosticRelatedInformation to vulnRelatedInfo +// that captures only the part that we want to test. +func summarizeRelatedInfo(rinfo []protocol.DiagnosticRelatedInformation) []vulnRelatedInfo { + var res []vulnRelatedInfo + for _, r := range rinfo { + filename := filepath.Base(r.Location.URI.SpanURI().Filename()) + message, _, _ := strings.Cut(r.Message, " ") + line := r.Location.Range.Start.Line + res = append(res, vulnRelatedInfo{filename, line, message}) + } + sort.Slice(res, func(i, j int) bool { + return res[i].less(res[j]) + }) + return res +} + +type vulnRelatedInfo struct { + Filename string + Line uint32 + Message string +} + type vulnDiag struct { msg string severity protocol.DiagnosticSeverity // codeActions is a list titles of code actions that we get with this // diagnostics as the context. codeActions []string + // relatedInfo is related info message prefixed by the file base. + // See summarizeRelatedInfo. + relatedInfo []vulnRelatedInfo +} + +func (i vulnRelatedInfo) less(j vulnRelatedInfo) bool { + if i.Filename != j.Filename { + return i.Filename < j.Filename + } + if i.Line != j.Line { + return i.Line < j.Line + } + return i.Message < j.Message } // wantVulncheckModDiagnostics maps a module path in the require