gopls/internal/analysis/embeddirective: call AddImport directly

This CL causes the embeddirective analyzer to compute the necessary
edits itself, using AddImport, rather than using gopls' hacky
lazy-edits mechanism, which requires gopls's ApplyFix command
to compute the edits later.

Also, support blank imports in AddImport.

Change-Id: Ie51a316503933fbea05ed418ed251f7af9cc624f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/705475
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

Author: adonovan

gopls/internal/analysis/embeddirective/embeddirective.go

@@ -26,8 +26,6 @@ var Analyzer = &analysis.Analyzer{
 	URL:              "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/embeddirective",
 }
 
-const FixCategory = "addembedimport" // recognized by gopls ApplyFix
-
 func run(pass *analysis.Pass) (any, error) {
 	for _, f := range pass.Files {
 		comments := embedDirectiveComments(f)
@@ -47,16 +45,19 @@ func run(pass *analysis.Pass) (any, error) {
 			pos, end := c.Pos(), c.Pos()+token.Pos(len("//go:embed"))
 
 			if !hasEmbedImport {
-				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.
-					}},
-				})
+				// Add blank import of "embed".
+				_, _, edits := analysisinternal.AddImport(pass.TypesInfo, f, "_", "embed", "", c.Pos())
+				if len(edits) > 0 {
+					pass.Report(analysis.Diagnostic{
+						Pos:     pos,
+						End:     end,
+						Message: `must import "embed" when using go:embed directives`,
+						SuggestedFixes: []analysis.SuggestedFix{{
+							Message:   `Add missing "embed" import`,
+							TextEdits: edits,
+						}},
+					})
+				}
 			}
 
 			var msg string

gopls/internal/analysis/embeddirective/testdata/src/a/import_missing.go.golden

@@ -0,0 +1,18 @@
+// Copyright 2022 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 a
+
+import (
+	_ "embed"
+	"fmt"
+)
+
+//go:embed embedtext // want "must import \"embed\" when using go:embed directives"
+var s string
+
+// This is main function
+func main() {
+	fmt.Println(s)
+}

gopls/internal/golang/fix.go

@@ -10,15 +10,13 @@ import (
 	"go/token"
 
 	"golang.org/x/tools/go/analysis"
-	"golang.org/x/tools/gopls/internal/analysis/embeddirective"
 	"golang.org/x/tools/gopls/internal/analysis/fillstruct"
 	"golang.org/x/tools/gopls/internal/analysis/unusedparams"
 	"golang.org/x/tools/gopls/internal/cache"
 	"golang.org/x/tools/gopls/internal/cache/parsego"
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/util/bug"
-	"golang.org/x/tools/internal/imports"
 )
 
 // A fixer is a function that suggests a fix for a diagnostic produced
@@ -94,8 +92,7 @@ func ApplyFix(ctx context.Context, fix string, snapshot *cache.Snapshot, fh file
 	fixers := map[string]fixer{
 		// Fixes for analyzer-provided diagnostics.
 		// These match the Diagnostic.Category.
-		embeddirective.FixCategory: addEmbedImport,
-		fillstruct.FixCategory:     singleFile(fillstruct.SuggestedFix),
+		fillstruct.FixCategory: singleFile(fillstruct.SuggestedFix),
 
 		// Ad-hoc fixers: these are used when the command is
 		// constructed directly by logic in server/code_action.
@@ -183,36 +180,3 @@ func suggestedFixToDocumentChange(ctx context.Context, snapshot *cache.Snapshot,
 	}
 	return changes, nil
 }
-
-// addEmbedImport adds a missing embed "embed" import with blank name.
-func addEmbedImport(ctx context.Context, snapshot *cache.Snapshot, pkg *cache.Package, pgf *parsego.File, _, _ token.Pos) (*token.FileSet, *analysis.SuggestedFix, error) {
-	// Like golang.AddImport, but with _ as Name and using our pgf.
-	protoEdits, err := ComputeImportFixEdits(snapshot.Options().Local, pgf.Src, &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, err // e.g. invalid range
-		}
-		edits = append(edits, analysis.TextEdit{
-			Pos:     start,
-			End:     end,
-			NewText: []byte(e.NewText),
-		})
-	}
-
-	return pkg.FileSet(), &analysis.SuggestedFix{
-		Message:   "Add embed import",
-		TextEdits: edits,
-	}, nil
-}

internal/analysisinternal/analysis.go

@@ -193,18 +193,23 @@ func CheckReadable(pass *analysis.Pass, filename string) error {
 	return fmt.Errorf("Pass.ReadFile: %s is not among OtherFiles, IgnoredFiles, or names of Files", filename)
 }
 
-// AddImport checks whether this file already imports pkgpath and
-// that import is in scope at pos. If so, it returns the name under
-// which it was imported and a zero edit. Otherwise, it adds a new
-// import of pkgpath, using a name derived from the preferred name,
-// and returns the chosen name, a prefix to be concatenated with member
-// to form a qualified name, and the edit for the new import.
+// AddImport checks whether this file already imports pkgpath and that
+// the import is in scope at pos. If so, it returns the name under
+// which it was imported and no edits. Otherwise, it adds a new import
+// of pkgpath, using a name derived from the preferred name, and
+// returns the chosen name, a prefix to be concatenated with member to
+// form a qualified name, and the edit for the new import.
 //
-// In the special case that pkgpath is dot-imported then member, the
-// identifier for which the import is being added, is consulted. If
-// member is not shadowed at pos, AddImport returns (".", "", nil).
-// (AddImport accepts the caller's implicit claim that the imported
-// package declares member.)
+// The member argument indicates the name of the desired symbol within
+// the imported package. This is needed in the case when the existing
+// import is a dot import, because then it is possible that the
+// desired symbol is shadowed by other declarations in the current
+// package. If member is not shadowed at pos, AddImport returns (".",
+// "", nil). (AddImport accepts the caller's implicit claim that the
+// imported package declares member.)
+//
+// Use a preferredName of "_" to request a blank import;
+// member is ignored in this case.
 //
 // It does not mutate its arguments.
 func AddImport(info *types.Info, file *ast.File, preferredName, pkgpath, member string, pos token.Pos) (name, prefix string, newImport []analysis.TextEdit) {
@@ -220,6 +225,10 @@ func AddImport(info *types.Info, file *ast.File, preferredName, pkgpath, member
 		pkgname := info.PkgNameOf(spec)
 		if pkgname != nil && pkgname.Imported().Path() == pkgpath {
 			name = pkgname.Name()
+			if preferredName == "_" {
+				// Request for blank import; any existing import will do.
+				return name, "", nil
+			}
 			if name == "." {
 				// The scope of ident must be the file scope.
 				if s, _ := scope.LookupParent(member, pos); s == info.Scopes[file] {
@@ -232,8 +241,12 @@ func AddImport(info *types.Info, file *ast.File, preferredName, pkgpath, member
 	}
 
 	// We must add a new import.
+
 	// Ensure we have a fresh name.
-	newName := FreshName(scope, pos, preferredName)
+	newName := preferredName
+	if preferredName != "_" {
+		newName = FreshName(scope, pos, preferredName)
+	}
 
 	// Create a new import declaration either before the first existing
 	// declaration (which must exist), including its comments; or
@@ -246,6 +259,7 @@ func AddImport(info *types.Info, file *ast.File, preferredName, pkgpath, member
 	if newName != preferredName || newName != pathpkg.Base(pkgpath) {
 		newText = fmt.Sprintf("%s %q", newName, pkgpath)
 	}
+
 	decl0 := file.Decls[0]
 	var before ast.Node = decl0
 	switch decl0 := decl0.(type) {