From e3f11805648f901c0be286027fbc9d567d22dcf9 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 7 Feb 2024 12:51:00 -0500 Subject: [PATCH] gopls/internal/cache: fix crash in analysis The "missing Diagnostic.Code" assertion checks an invariant that isn't guaranteed. This change downgrades it to a bug.Reportf. Also, we add a similar assertion to analysistest.RunWithSuggestedFixes which has much better coverage of all the analyzer's different ways of constructing a SuggestedFix, unlike gopl's own superficial tests of each analyzer it carries. (This assertion may be enabled only for analyzers in x/tools, since the invariant is stricter than is required by the public API.) Also, fix a couple of places in unusedvariable where it could conceivably return a fix with no TextEdits; they are not intended to be lazy fixes. Fixes golang/go#65578 Change-Id: I5ed6301b028d184ea896988ca8f210fb8f3dd64f Reviewed-on: https://go-review.googlesource.com/c/tools/+/562397 LUCI-TryBot-Result: Go LUCI Auto-Submit: Alan Donovan Commit-Queue: Alan Donovan Reviewed-by: Robert Findley --- go/analysis/analysistest/analysistest.go | 26 ++++++++++++++++--- .../analysis/unusedvariable/unusedvariable.go | 12 +++++++-- gopls/internal/cache/errors.go | 8 +++--- 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go index 72b703f7b..95db20f4b 100644 --- a/go/analysis/analysistest/analysistest.go +++ b/go/analysis/analysistest/analysistest.go @@ -15,6 +15,7 @@ import ( "os" "path/filepath" "regexp" + "runtime" "sort" "strconv" "strings" @@ -128,6 +129,19 @@ type Testing interface { func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns ...string) []*Result { r := Run(t, dir, a, patterns...) + // If the immediate caller of RunWithSuggestedFixes is in + // x/tools, we apply stricter checks as required by gopls. + inTools := false + { + var pcs [1]uintptr + n := runtime.Callers(1, pcs[:]) + frames := runtime.CallersFrames(pcs[:n]) + fr, _ := frames.Next() + if fr.Func != nil && strings.HasPrefix(fr.Func.Name(), "golang.org/x/tools/") { + inTools = true + } + } + // Process each result (package) separately, matching up the suggested // fixes into a diff, which we will compare to the .golden file. We have // to do this per-result in case a file appears in two packages, such as in @@ -145,8 +159,14 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns // Validate edits, prepare the fileEdits map and read the file contents. for _, diag := range act.Diagnostics { - for _, sf := range diag.SuggestedFixes { - for _, edit := range sf.TextEdits { + for _, fix := range diag.SuggestedFixes { + + // Assert that lazy fixes have a Category (#65578, #65087). + if inTools && len(fix.TextEdits) == 0 && diag.Category == "" { + t.Errorf("missing Diagnostic.Category for SuggestedFix without TextEdits (gopls requires the category for the name of the fix command") + } + + for _, edit := range fix.TextEdits { start, end := edit.Pos, edit.End if !end.IsValid() { end = start @@ -175,7 +195,7 @@ func RunWithSuggestedFixes(t Testing, dir string, a *analysis.Analyzer, patterns if _, ok := fileEdits[file]; !ok { fileEdits[file] = make(map[string][]diff.Edit) } - fileEdits[file][sf.Message] = append(fileEdits[file][sf.Message], diff.Edit{ + fileEdits[file][fix.Message] = append(fileEdits[file][fix.Message], diff.Edit{ Start: file.Offset(start), End: file.Offset(end), New: string(edit.NewText), diff --git a/gopls/internal/analysis/unusedvariable/unusedvariable.go b/gopls/internal/analysis/unusedvariable/unusedvariable.go index 67866d0e1..f8a4db1d2 100644 --- a/gopls/internal/analysis/unusedvariable/unusedvariable.go +++ b/gopls/internal/analysis/unusedvariable/unusedvariable.go @@ -155,10 +155,14 @@ func removeVariableFromSpec(pass *analysis.Pass, path []ast.Node, stmt *ast.Valu // Find parent DeclStmt and delete it for _, node := range path { if declStmt, ok := node.(*ast.DeclStmt); ok { + edits := deleteStmtFromBlock(path, declStmt) + if len(edits) == 0 { + return nil // can this happen? + } return []analysis.SuggestedFix{ { Message: suggestedFixMessage(ident.Name), - TextEdits: deleteStmtFromBlock(path, declStmt), + TextEdits: edits, }, } } @@ -208,10 +212,14 @@ func removeVariableFromAssignment(path []ast.Node, stmt *ast.AssignStmt, ident * } // RHS does not have any side effects, delete the whole statement + edits := deleteStmtFromBlock(path, stmt) + if len(edits) == 0 { + return nil // can this happen? + } return []analysis.SuggestedFix{ { Message: suggestedFixMessage(ident.Name), - TextEdits: deleteStmtFromBlock(path, stmt), + TextEdits: edits, }, } } diff --git a/gopls/internal/cache/errors.go b/gopls/internal/cache/errors.go index 1ed193b5d..83382b0aa 100644 --- a/gopls/internal/cache/errors.go +++ b/gopls/internal/cache/errors.go @@ -21,10 +21,10 @@ import ( "strings" "golang.org/x/tools/go/packages" - "golang.org/x/tools/gopls/internal/file" "golang.org/x/tools/gopls/internal/cache/metadata" - "golang.org/x/tools/gopls/internal/protocol/command" + "golang.org/x/tools/gopls/internal/file" "golang.org/x/tools/gopls/internal/protocol" + "golang.org/x/tools/gopls/internal/protocol/command" "golang.org/x/tools/gopls/internal/settings" "golang.org/x/tools/gopls/internal/util/bug" "golang.org/x/tools/internal/typesinternal" @@ -345,8 +345,10 @@ func toSourceDiagnostic(srcAnalyzer *settings.Analyzer, gobDiag *gobDiagnostic) } // Ensure that the analyzer specifies a category for all its no-edit fixes. + // This is asserted by analysistest.RunWithSuggestedFixes, but there + // may be gaps in test coverage. if diag.Code == "" || diag.Code == "default" { - panic(fmt.Sprintf("missing Diagnostic.Code: %#v", *diag)) + bug.Reportf("missing Diagnostic.Code: %#v", *diag) } } }