gopls/internal/lsp/source: Add SuggestedFix for embeddirective Analyzer

Give the embeddirective source.Analyzer a Fix that can
add missing "embed" imports. Since not all reports
from the analyzer is this diagnostic, give source.Analyzer
an IsFixable method to filter diagnostics.

Updates golang/go#50262

Change-Id: I24abdd2d1a88fc5cabd3197ef438c4da041a6a9c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/509056
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>

gopls/internal/lsp/analysis/embeddirective/embeddirective.go

@@ -2,7 +2,8 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-// Package embeddirective defines an Analyzer that validates import for //go:embed directive.
+// Package embeddirective defines an Analyzer that validates //go:embed directives.
+// The analyzer defers fixes to it's parent source.Analyzer.
 package embeddirective
 
 import (
@@ -26,6 +27,10 @@ var Analyzer = &analysis.Analyzer{
 	RunDespiteErrors: true,
 }
 
+// source.fixedByImportingEmbed relies on this message to filter
+// out fixable diagnostics from this Analyzer.
+const MissingImportMessage = `must import "embed" when using go:embed directives`
+
 func run(pass *analysis.Pass) (interface{}, error) {
 	for _, f := range pass.Files {
 		comments := embedDirectiveComments(f)
@@ -51,7 +56,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			}
 
 			if !hasEmbedImport {
-				report(`must import "embed" when using go:embed directives`)
+				report(MissingImportMessage)
 			}
 
 			spec := nextVarSpec(c, f)

gopls/internal/lsp/cache/errors.go

@@ -339,17 +339,20 @@ func toSourceDiagnostic(srcAnalyzer *source.Analyzer, gobDiag *gobDiagnostic) *s
 	}
 
 	diag := &source.Diagnostic{
-		URI:            gobDiag.Location.URI.SpanURI(),
-		Range:          gobDiag.Location.Range,
-		Severity:       severity,
-		Code:           gobDiag.Code,
-		CodeHref:       gobDiag.CodeHref,
-		Source:         source.AnalyzerErrorKind(gobDiag.Source),
-		Message:        gobDiag.Message,
-		Related:        related,
-		SuggestedFixes: fixes,
-		Tags:           srcAnalyzer.Tag,
+		URI:      gobDiag.Location.URI.SpanURI(),
+		Range:    gobDiag.Location.Range,
+		Severity: severity,
+		Code:     gobDiag.Code,
+		CodeHref: gobDiag.CodeHref,
+		Source:   source.AnalyzerErrorKind(gobDiag.Source),
+		Message:  gobDiag.Message,
+		Related:  related,
+		Tags:     srcAnalyzer.Tag,
+	}
+	if srcAnalyzer.FixesDiagnostic(diag) {
+		diag.SuggestedFixes = fixes
 	}
+
 	// If the fixes only delete code, assume that the diagnostic is reporting dead code.
 	if onlyDeletions(fixes) {
 		diag.Tags = append(diag.Tags, protocol.Unnecessary)

gopls/internal/lsp/source/fix.go

@@ -13,10 +13,12 @@ import (
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/gopls/internal/bug"
+	"golang.org/x/tools/gopls/internal/lsp/analysis/embeddirective"
 	"golang.org/x/tools/gopls/internal/lsp/analysis/fillstruct"
 	"golang.org/x/tools/gopls/internal/lsp/analysis/undeclaredname"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	"golang.org/x/tools/gopls/internal/span"
+	"golang.org/x/tools/internal/imports"
 )
 
 type (
@@ -41,6 +43,7 @@ const (
 	ExtractFunction   = "extract_function"
 	ExtractMethod     = "extract_method"
 	InvertIfCondition = "invert_if_condition"
+	AddEmbedImport    = "add_embed_import"
 )
 
 // suggestedFixes maps a suggested fix command id to its handler.
@@ -52,6 +55,7 @@ var suggestedFixes = map[string]SuggestedFixFunc{
 	ExtractMethod:     singleFile(extractMethod),
 	InvertIfCondition: singleFile(invertIfCondition),
 	StubMethods:       stubSuggestedFixFunc,
+	AddEmbedImport:    addEmbedImport,
 }
 
 // singleFile calls analyzers that expect inputs for a single file
@@ -138,3 +142,50 @@ func ApplyFix(ctx context.Context, fix string, snapshot Snapshot, fh FileHandle,
 	}
 	return edits, nil
 }
+
+// fixedByImportingEmbed returns true if diag can be fixed by addEmbedImport.
+func fixedByImportingEmbed(diag *Diagnostic) bool {
+	if diag == nil {
+		return false
+	}
+	return diag.Message == embeddirective.MissingImportMessage
+}
+
+// addEmbedImport adds a missing embed "embed" import with blank name.
+func addEmbedImport(ctx context.Context, snapshot Snapshot, fh FileHandle, rng protocol.Range) (*token.FileSet, *analysis.SuggestedFix, error) {
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
+	if err != nil {
+		return nil, nil, fmt.Errorf("narrow pkg: %w", err)
+	}
+
+	// Like source.AddImport, but with _ as Name and using our pgf.
+	protoEdits, err := ComputeOneImportFixEdits(snapshot, pgf, &imports.ImportFix{
+		StmtInfo: imports.ImportInfo{
+			ImportPath: "embed",
+			Name:       "_",
+		},
+		FixType: imports.AddImport,
+	})
+	if err != nil {
+		return nil, nil, fmt.Errorf("compute edits: %w", err)
+	}
+
+	var edits []analysis.TextEdit
+	for _, e := range protoEdits {
+		start, end, err := pgf.RangePos(e.Range)
+		if err != nil {
+			return nil, nil, fmt.Errorf("map range: %w", err)
+		}
+		edits = append(edits, analysis.TextEdit{
+			Pos:     start,
+			End:     end,
+			NewText: []byte(e.NewText),
+		})
+	}
+
+	fix := &analysis.SuggestedFix{
+		Message:   "Add embed import",
+		TextEdits: edits,
+	}
+	return pkg.FileSet(), fix, nil
+}

gopls/internal/lsp/source/options.go

@@ -1574,8 +1574,13 @@ func defaultAnalyzers() map[string]*Analyzer {
 		unusedparams.Analyzer.Name:     {Analyzer: unusedparams.Analyzer, Enabled: false},
 		unusedwrite.Analyzer.Name:      {Analyzer: unusedwrite.Analyzer, Enabled: false},
 		useany.Analyzer.Name:           {Analyzer: useany.Analyzer, Enabled: false},
-		embeddirective.Analyzer.Name:   {Analyzer: embeddirective.Analyzer, Enabled: true},
 		timeformat.Analyzer.Name:       {Analyzer: timeformat.Analyzer, Enabled: true},
+		embeddirective.Analyzer.Name: {
+			Analyzer:        embeddirective.Analyzer,
+			Enabled:         true,
+			Fix:             AddEmbedImport,
+			fixesDiagnostic: fixedByImportingEmbed,
+		},
 
 		// gofmt -s suite:
 		simplifycompositelit.Analyzer.Name: {

gopls/internal/lsp/source/view.go

@@ -879,6 +879,10 @@ type Analyzer struct {
 	// the analyzer's suggested fixes through a Command, not a TextEdit.
 	Fix string
 
+	// fixesDiagnostic reports if a diagnostic from the analyzer can be fixed by Fix.
+	// If nil then all diagnostics from the analyzer are assumed to be fixable.
+	fixesDiagnostic func(*Diagnostic) bool
+
 	// ActionKind is the kind of code action this analyzer produces. If
 	// unspecified the type defaults to quickfix.
 	ActionKind []protocol.CodeActionKind
@@ -908,6 +912,14 @@ func (a Analyzer) IsEnabled(options *Options) bool {
 	return a.Enabled
 }
 
+// FixesDiagnostic returns true if Analyzer.Fix can fix the Diagnostic.
+func (a Analyzer) FixesDiagnostic(d *Diagnostic) bool {
+	if a.fixesDiagnostic == nil {
+		return true
+	}
+	return a.fixesDiagnostic(d)
+}
+
 // Declare explicit types for package paths, names, and IDs to ensure that we
 // never use an ID where a path belongs, and vice versa. If we confused these,
 // it would result in confusing errors because package IDs often look like

gopls/internal/lsp/testdata/embeddirective/embed.txt

@@ -0,0 +1 @@
+text

gopls/internal/lsp/testdata/embeddirective/fix_import.go

@@ -0,0 +1,18 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package embeddirective
+
+import (
+	"io"
+	"os"
+)
+
+//go:embed embed.txt //@suggestedfix("//go:embed", "quickfix", "")
+var t string
+
+func unused() {
+	_ = os.Stdin
+	_ = io.EOF
+}

gopls/internal/lsp/testdata/embeddirective/fix_import.go.golden

@@ -0,0 +1,21 @@
+-- suggestedfix_fix_import_12_1 --
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package embeddirective
+
+import (
+	_ "embed"
+	"io"
+	"os"
+)
+
+//go:embed embed.txt //@suggestedfix("//go:embed", "quickfix", "")
+var t string
+
+func unused() {
+	_ = os.Stdin
+	_ = io.EOF
+}
+

gopls/internal/lsp/testdata/summary.txt.golden

@@ -11,7 +11,7 @@ CaseSensitiveCompletionsCount = 4
 DiagnosticsCount = 23
 FoldingRangesCount = 2
 SemanticTokenCount = 3
-SuggestedFixCount = 73
+SuggestedFixCount = 74
 MethodExtractionCount = 8
 DefinitionsCount = 46
 TypeDefinitionsCount = 18

gopls/internal/lsp/testdata/summary_go1.18.txt.golden

@@ -11,7 +11,7 @@ CaseSensitiveCompletionsCount = 4
 DiagnosticsCount = 23
 FoldingRangesCount = 2
 SemanticTokenCount = 3
-SuggestedFixCount = 79
+SuggestedFixCount = 80
 MethodExtractionCount = 8
 DefinitionsCount = 46
 TypeDefinitionsCount = 18

gopls/internal/lsp/testdata/summary_go1.21.txt.golden

@@ -11,7 +11,7 @@ CaseSensitiveCompletionsCount = 4
 DiagnosticsCount = 24
 FoldingRangesCount = 2
 SemanticTokenCount = 3
-SuggestedFixCount = 79
+SuggestedFixCount = 80
 MethodExtractionCount = 8
 DefinitionsCount = 46
 TypeDefinitionsCount = 18

gopls/internal/regtest/diagnostics/analysis_test.go

@@ -85,3 +85,43 @@ func main() {
 		})
 	}
 }
+
+// Test the embed directive analyzer.
+//
+// There is a fix for missing imports, but it should not trigger for other
+// kinds of issues reported by the analayzer, here the variable
+// declaration following the embed directive is wrong.
+func TestNoSuggestedFixesForEmbedDirectiveDeclaration(t *testing.T) {
+	const generated = `
+-- go.mod --
+module mod.com
+
+go 1.20
+
+-- foo.txt --
+FOO
+
+-- main.go --
+package main
+
+import _ "embed"
+
+//go:embed foo.txt
+var foo, bar string
+
+func main() {
+	_ = foo
+}
+`
+	Run(t, generated, func(t *testing.T, env *Env) {
+		env.OpenFile("main.go")
+		var d protocol.PublishDiagnosticsParams
+		env.AfterChange(
+			Diagnostics(env.AtRegexp("main.go", "//go:embed")),
+			ReadDiagnostics("main.go", &d),
+		)
+		if fixes := env.GetQuickFixes("main.go", d.Diagnostics); len(fixes) != 0 {
+			t.Errorf("got quick fixes %v, wanted none", fixes)
+		}
+	})
+}