gopls/internal/lsp/source: fix Fix titles

Previously, many diagnostics were produced with
messages that were in fact the titles of suggested fixes
(e.g. "Fill struct S"); conversely, many suggested fixes
appeared in the client UI with a message that was in fact
the text of the diagnostic it was supposed to fix
(e.g. "undeclared name: x").

This change reorganizes the way suggested fixes are handled
so that, for fixes produced by analyzers, the analyzer output
(Diagnostic and SuggestedFix) determines the LSP output.
An SuggestedFix with no edits is assumed
to have fixer support in ApplyFix; the name of the fixer
is determined by the Diagnostic's Category.
(Ideally we would use SuggestedFix.Category in case
a diagnostic has more than one lazy fix, but there's
no such field.)

Fixers whose commands are constructed directly by the
Code Action method are essentially unchanged.

There is still a need for some kind of framework for
registering (analyzers, category, fixer) triples, but
this is a first step.

Fixes golang/go#65087

Change-Id: Id72f3fd187d42df9c3711f4d3be140f9ea7af1d7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556755
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

gopls/doc/commands.md

@@ -66,7 +66,14 @@ Args:
 
 ```
 {
-	// The fix to apply.
+	// The name of the fix to apply.
+	//
+	// For fixes suggested by analyzers, this is a string constant
+	// advertised by the analyzer that matches the Category of
+	// the analysis.Diagnostic with a SuggestedFix containing no edits.
+	//
+	// For fixes suggested by code actions, this is a string agreed
+	// upon by the code action and source.ApplyFix.
 	"Fix": string,
 	// The file URI for the document to fix.
 	"URI": string,

gopls/internal/analysis/embeddirective/embeddirective.go

@@ -26,9 +26,7 @@ var Analyzer = &analysis.Analyzer{
 	URL:              "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/embeddirective",
 }
 
-// source.fixedByImportingEmbed relies on this message to filter
-// out fixable diagnostics from this Analyzer.
-const MissingImportMessage = `must import "embed" when using go:embed directives`
+const FixCategory = "addembedimport" // recognized by gopls ApplyFix
 
 func run(pass *analysis.Pass) (interface{}, error) {
 	for _, f := range pass.Files {
@@ -46,28 +44,39 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		}
 
 		for _, c := range comments {
-			report := func(msg string) {
-				pass.Report(analysis.Diagnostic{
-					Pos:     c.Pos(),
-					End:     c.Pos() + token.Pos(len("//go:embed")),
-					Message: msg,
-				})
-			}
+			pos, end := c.Pos(), c.Pos()+token.Pos(len("//go:embed"))
 
 			if !hasEmbedImport {
-				report(MissingImportMessage)
+				pass.Report(analysis.Diagnostic{
+					Pos:      pos,
+					End:      end,
+					Message:  `must import "embed" when using go:embed directives`,
+					Category: FixCategory,
+					SuggestedFixes: []analysis.SuggestedFix{{
+						Message: `Add missing "embed" import`,
+						// No TextEdits => computed by a gopls command.
+					}},
+				})
 			}
 
+			var msg string
 			spec := nextVarSpec(c, f)
 			switch {
 			case spec == nil:
-				report(`go:embed directives must precede a "var" declaration`)
+				msg = `go:embed directives must precede a "var" declaration`
 			case len(spec.Names) != 1:
-				report("declarations following go:embed directives must define a single variable")
+				msg = "declarations following go:embed directives must define a single variable"
 			case len(spec.Values) > 0:
-				report("declarations following go:embed directives must not specify a value")
+				msg = "declarations following go:embed directives must not specify a value"
 			case !embeddableType(pass.TypesInfo.Defs[spec.Names[0]]):
-				report("declarations following go:embed directives must be of type string, []byte or embed.FS")
+				msg = "declarations following go:embed directives must be of type string, []byte or embed.FS"
+			}
+			if msg != "" {
+				pass.Report(analysis.Diagnostic{
+					Pos:     pos,
+					End:     end,
+					Message: msg,
+				})
 			}
 		}
 	}

gopls/internal/analysis/fillstruct/fillstruct.go

@@ -130,18 +130,25 @@ func DiagnoseFillableStructs(inspect *inspector.Inspector, start, end token.Pos,
 			if i < totalFields {
 				fillableFields = append(fillableFields, "...")
 			}
-			name = fmt.Sprintf("anonymous struct { %s }", strings.Join(fillableFields, ", "))
+			name = fmt.Sprintf("anonymous struct{ %s }", strings.Join(fillableFields, ", "))
 		}
 		diags = append(diags, analysis.Diagnostic{
-			Message: fmt.Sprintf("Fill %s", name),
-			Pos:     expr.Pos(),
-			End:     expr.End(),
+			Message:  fmt.Sprintf("%s literal has missing fields", name),
+			Pos:      expr.Pos(),
+			End:      expr.End(),
+			Category: FixCategory,
+			SuggestedFixes: []analysis.SuggestedFix{{
+				Message: fmt.Sprintf("Fill %s", name),
+				// No TextEdits => computed later by gopls.
+			}},
 		})
 	})
 
 	return diags
 }
 
+const FixCategory = "fillstruct" // recognized by gopls ApplyFix
+
 // SuggestedFix computes the suggested fix for the kinds of
 // diagnostics produced by the Analyzer above.
 func SuggestedFix(fset *token.FileSet, start, end token.Pos, content []byte, file *ast.File, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {

gopls/internal/analysis/fillstruct/testdata/src/a/a.go

@@ -19,16 +19,16 @@ type basicStruct struct {
 	foo int
 }
 
-var _ = basicStruct{} // want `Fill basicStruct`
+var _ = basicStruct{} // want `basicStruct literal has missing fields`
 
 type twoArgStruct struct {
 	foo int
 	bar string
 }
 
-var _ = twoArgStruct{} // want `Fill twoArgStruct`
+var _ = twoArgStruct{} // want `twoArgStruct literal has missing fields`
 
-var _ = twoArgStruct{ // want `Fill twoArgStruct`
+var _ = twoArgStruct{ // want `twoArgStruct literal has missing fields`
 	bar: "bar",
 }
 
@@ -37,9 +37,9 @@ type nestedStruct struct {
 	basic basicStruct
 }
 
-var _ = nestedStruct{} // want `Fill nestedStruct`
+var _ = nestedStruct{} // want `nestedStruct literal has missing fields`
 
-var _ = data.B{} // want `Fill b.B`
+var _ = data.B{} // want `b.B literal has missing fields`
 
 type typedStruct struct {
 	m  map[string]int
@@ -49,25 +49,25 @@ type typedStruct struct {
 	a  [2]string
 }
 
-var _ = typedStruct{} // want `Fill typedStruct`
+var _ = typedStruct{} // want `typedStruct literal has missing fields`
 
 type funStruct struct {
 	fn func(i int) int
 }
 
-var _ = funStruct{} // want `Fill funStruct`
+var _ = funStruct{} // want `funStruct literal has missing fields`
 
 type funStructComplex struct {
 	fn func(i int, s string) (string, int)
 }
 
-var _ = funStructComplex{} // want `Fill funStructComplex`
+var _ = funStructComplex{} // want `funStructComplex literal has missing fields`
 
 type funStructEmpty struct {
 	fn func()
 }
 
-var _ = funStructEmpty{} // want `Fill funStructEmpty`
+var _ = funStructEmpty{} // want `funStructEmpty literal has missing fields`
 
 type Foo struct {
 	A int
@@ -78,7 +78,7 @@ type Bar struct {
 	Y *Foo
 }
 
-var _ = Bar{} // want `Fill Bar`
+var _ = Bar{} // want `Bar literal has missing fields`
 
 type importedStruct struct {
 	m  map[*ast.CompositeLit]ast.Field
@@ -89,25 +89,24 @@ type importedStruct struct {
 	st ast.CompositeLit
 }
 
-var _ = importedStruct{} // want `Fill importedStruct`
+var _ = importedStruct{} // want `importedStruct literal has missing fields`
 
 type pointerBuiltinStruct struct {
 	b *bool
 	s *string
 	i *int
 }
 
-var _ = pointerBuiltinStruct{} // want `Fill pointerBuiltinStruct`
+var _ = pointerBuiltinStruct{} // want `pointerBuiltinStruct literal has missing fields`
 
 var _ = []ast.BasicLit{
-	{}, // want `Fill go/ast.BasicLit`
+	{}, // want `go/ast.BasicLit literal has missing fields`
 }
 
-var _ = []ast.BasicLit{{}, // want "go/ast.BasicLit"
-}
+var _ = []ast.BasicLit{{}} // want "go/ast.BasicLit literal has missing fields"
 
 type unsafeStruct struct {
 	foo unsafe.Pointer
 }
 
-var _ = unsafeStruct{} // want `Fill unsafeStruct`
+var _ = unsafeStruct{} // want `unsafeStruct literal has missing fields`

gopls/internal/analysis/fillstruct/testdata/src/typeparams/typeparams.go

@@ -12,16 +12,16 @@ type basicStruct[T any] struct {
 	foo T
 }
 
-var _ = basicStruct[int]{} // want `Fill basicStruct\[int\]`
+var _ = basicStruct[int]{} // want `basicStruct\[int\] literal has missing fields`
 
 type twoArgStruct[F, B any] struct {
 	foo F
 	bar B
 }
 
-var _ = twoArgStruct[string, int]{} // want `Fill twoArgStruct\[string, int\]`
+var _ = twoArgStruct[string, int]{} // want `twoArgStruct\[string, int\] literal has missing fields`
 
-var _ = twoArgStruct[int, string]{ // want `Fill twoArgStruct\[int, string\]`
+var _ = twoArgStruct[int, string]{ // want `twoArgStruct\[int, string\] literal has missing fields`
 	bar: "bar",
 }
 
@@ -30,19 +30,19 @@ type nestedStruct struct {
 	basic basicStruct[int]
 }
 
-var _ = nestedStruct{} // want "Fill nestedStruct"
+var _ = nestedStruct{} // want "nestedStruct literal has missing fields"
 
 func _[T any]() {
 	type S struct{ t T }
-	x := S{} // want "Fill S"
+	x := S{} // want "S"
 	_ = x
 }
 
 func Test() {
 	var tests = []struct {
 		a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p string
 	}{
-		{}, // want "Fill anonymous struct { a: string, b: string, c: string, ... }"
+		{}, // want "anonymous struct{ a: string, b: string, c: string, ... } literal has missing fields"
 	}
 	for _, test := range tests {
 		_ = test

gopls/internal/analysis/stubmethods/stubmethods.go

@@ -87,13 +87,22 @@ func DiagnosticForError(fset *token.FileSet, file *ast.File, start, end token.Po
 		return analysis.Diagnostic{}, false
 	}
 	qf := typesutil.FileQualifier(file, si.Concrete.Obj().Pkg(), info)
+	iface := types.TypeString(si.Interface.Type(), qf)
 	return analysis.Diagnostic{
-		Pos:     start,
-		End:     end,
-		Message: fmt.Sprintf("Implement %s", types.TypeString(si.Interface.Type(), qf)),
+		Pos: start,
+		End: end,
+		Message: fmt.Sprintf("Type %v lacks methods of interface %s",
+			types.TypeString(si.Concrete, qf), iface),
+		Category: FixCategory,
+		SuggestedFixes: []analysis.SuggestedFix{{
+			Message: fmt.Sprintf("Declare missing methods of %s", iface),
+			// No TextEdits => computed later by gopls.
+		}},
 	}, true
 }
 
+const FixCategory = "stubmethods" // recognized by gopls ApplyFix
+
 // StubInfo represents a concrete type
 // that wants to stub out an interface type
 type StubInfo struct {

gopls/internal/analysis/undeclaredname/undeclared.go

@@ -103,12 +103,20 @@ func runForError(pass *analysis.Pass, err types.Error) {
 	offset := safetoken.StartPosition(pass.Fset, err.Pos).Offset
 	end := tok.Pos(offset + len(name)) // TODO(adonovan): dubious! err.Pos + len(name)??
 	pass.Report(analysis.Diagnostic{
-		Pos:     err.Pos,
-		End:     end,
-		Message: err.Msg,
+		Pos:      err.Pos,
+		End:      end,
+		Message:  err.Msg,
+		Category: FixCategory,
+		SuggestedFixes: []analysis.SuggestedFix{{
+			Message: fmt.Sprintf("Create variable %q", name),
+			// No TextEdits => computed by a gopls command
+		}},
 	})
 }
 
+const FixCategory = "undeclaredname" // recognized by gopls ApplyFix
+
+// SuggestedFix computes the edits for the lazy (no-edits) fix suggested by the analyzer.
 func SuggestedFix(fset *token.FileSet, start, end token.Pos, content []byte, file *ast.File, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
 	pos := start // don't use the end
 	path, _ := astutil.PathEnclosingInterval(file, pos, pos)
@@ -146,7 +154,7 @@ func SuggestedFix(fset *token.FileSet, start, end token.Pos, content []byte, fil
 	// Create the new local variable statement.
 	newStmt := fmt.Sprintf("%s := %s", ident.Name, indent)
 	return fset, &analysis.SuggestedFix{
-		Message: fmt.Sprintf("Create variable \"%s\"", ident.Name),
+		Message: fmt.Sprintf("Create variable %q", ident.Name),
 		TextEdits: []analysis.TextEdit{{
 			Pos:     insertBeforeStmt.Pos(),
 			End:     insertBeforeStmt.Pos(),

gopls/internal/analysis/unusedparams/unusedparams.go

@@ -28,6 +28,8 @@ var Analyzer = &analysis.Analyzer{
 	URL:      "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/unusedparams",
 }
 
+const FixCategory = "unusedparam" // recognized by gopls ApplyFix
+
 func run(pass *analysis.Pass) (any, error) {
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 
@@ -264,28 +266,28 @@ func run(pass *analysis.Pass) (any, error) {
 					if len(field.Names) > 1 {
 						start, end = id.Pos(), id.End()
 					}
-					// This diagnostic carries an edit-based fix to
-					// rename the parameter. Gopls's settings
-					// associate this analyzer with a command-based fix
-					// (RemoveUnusedParam, implemented by
-					// source.RemoveUnusedParameter), so the user
-					// will see a second Code Action offering to
-					// "Remove unused parameter x" (although currently
-					// the fix title is actually that of the diagnostic,
-					// i.e. "unused parameter: x", which is a poor UX.
-					// We should fix that bug in cache.toSourceDiagnostic).
+					// This diagnostic carries both an edit-based fix to
+					// rename the unused parameter, and a command-based fix
+					// to remove it (see source.RemoveUnusedParameter).
 					pass.Report(analysis.Diagnostic{
-						Pos:     start,
-						End:     end,
-						Message: fmt.Sprintf("unused parameter: %s", id.Name),
-						SuggestedFixes: []analysis.SuggestedFix{{
-							Message: `Rename parameter to "_"`,
-							TextEdits: []analysis.TextEdit{{
-								Pos:     id.Pos(),
-								End:     id.End(),
-								NewText: []byte("_"),
-							}},
-						}},
+						Pos:      start,
+						End:      end,
+						Message:  fmt.Sprintf("unused parameter: %s", id.Name),
+						Category: FixCategory,
+						SuggestedFixes: []analysis.SuggestedFix{
+							{
+								Message: `Rename parameter to "_"`,
+								TextEdits: []analysis.TextEdit{{
+									Pos:     id.Pos(),
+									End:     id.End(),
+									NewText: []byte("_"),
+								}},
+							},
+							{
+								Message: fmt.Sprintf("Remove unused parameter %q", id.Name),
+								// No TextEdits => computed by gopls command
+							},
+						},
 					})
 				}
 			}

gopls/internal/lsp/cache/diagnostics.go

@@ -37,7 +37,7 @@ type Diagnostic struct {
 	URI      protocol.DocumentURI // of diagnosed file (not diagnostic documentation)
 	Range    protocol.Range
 	Severity protocol.DiagnosticSeverity
-	Code     string
+	Code     string // analysis.Diagnostic.Category (or "default" if empty) or hidden go/types error code
 	CodeHref string
 
 	// Source is a human-readable description of the source of the error.