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 <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Commit-Queue: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

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),

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,
 			},
 		}
 	}

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)
 			}
 		}
 	}