go/analysis/internal/analysisflags: -fix: remove unused imports
This change defines analysisflags.FormatSourceRemoveImports, a
replacement for format.Source that removes imports that are no
longer needed in a modified file.
It requires the types.Package for the unmodified file so that it
can tell what name is declared by an existing non-renaming import.
Any name declared by an import that is not free in the rest of
the file is considered redundant, and is removed.
Individual analyzers typically remove an import when they can
tell that this is necessary, but when several fixes are composed,
and each removes the second-last reference to an import, the
problem must be solved holistically; thus the right place is
in the anlayzer driver.
The new function is used in unitchecker, single/multichecker,
and analysistest.
It is tested indirectly via tests of individual analyzers such
as inline and modernize.
Fixes golang/go#72035
Change-Id: I490e160c11efd44ac63f5c33dac7c1b94c425258
Reviewed-on: https://go-review.googlesource.com/c/tools/+/717802
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go
index 520542b..3862a21 100644
--- a/go/analysis/analysistest/analysistest.go
+++ b/go/analysis/analysistest/analysistest.go
@@ -27,6 +27,7 @@
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/checker"
"golang.org/x/tools/go/analysis/internal"
+ "golang.org/x/tools/go/analysis/internal/analysisflags"
"golang.org/x/tools/go/packages"
"golang.org/x/tools/internal/diff"
"golang.org/x/tools/internal/testenv"
@@ -226,7 +227,7 @@
// check checks that the accumulated edits applied
// to the original content yield the wanted content.
check := func(prefix string, accumulated []diff.Edit, want []byte) {
- if err := applyDiffsAndCompare(filename, content, want, accumulated); err != nil {
+ if err := applyDiffsAndCompare(result.Pass.Pkg, filename, content, want, accumulated); err != nil {
t.Errorf("%s: %s", prefix, err)
}
}
@@ -281,7 +282,7 @@
// applyDiffsAndCompare applies edits to original and compares the results against
// want after formatting both. fileName is use solely for error reporting.
-func applyDiffsAndCompare(filename string, original, want []byte, edits []diff.Edit) error {
+func applyDiffsAndCompare(pkg *types.Package, filename string, original, want []byte, edits []diff.Edit) error {
// Relativize filename, for tidier errors.
if cwd, err := os.Getwd(); err == nil {
if rel, err := filepath.Rel(cwd, filename); err == nil {
@@ -296,7 +297,7 @@
if err != nil {
return fmt.Errorf("%s: error applying fixes: %v (see possible explanations at RunWithSuggestedFixes)", filename, err)
}
- fixed, err := format.Source(fixedBytes)
+ fixed, err := analysisflags.FormatSourceRemoveImports(pkg, fixedBytes)
if err != nil {
return fmt.Errorf("%s: error formatting resulting source: %v\n%s", filename, err, fixedBytes)
}
diff --git a/go/analysis/internal/analysisflags/fix.go b/go/analysis/internal/analysisflags/fix.go
index 72383fe..44a4356 100644
--- a/go/analysis/internal/analysisflags/fix.go
+++ b/go/analysis/internal/analysisflags/fix.go
@@ -7,24 +7,38 @@
// This file defines the -fix logic common to unitchecker and
// {single,multi}checker.
+// TODO(adonovan): move this file, and most of the contents of
+// internal/analysisinternal, into a new package, internal/driverlib,
+// since the library is for the driver, not analyzer, side of the
+// analysis API. Turn any dependencies on analysisflags state (i.e.
+// diffFlag) into explicit parameters (of ApplyFix).
+
import (
+ "bytes"
"fmt"
- "go/format"
+ "go/ast"
+ "go/parser"
+ "go/printer"
"go/token"
+ "go/types"
"log"
"maps"
"os"
"sort"
+ "strconv"
"golang.org/x/tools/go/analysis"
+ "golang.org/x/tools/go/ast/astutil"
"golang.org/x/tools/internal/analysisinternal"
+ "golang.org/x/tools/internal/astutil/free"
"golang.org/x/tools/internal/diff"
)
// FixAction abstracts a checker action (running one analyzer on one
// package) for the purposes of applying its diagnostics' fixes.
type FixAction struct {
- Name string // e.g. "analyzer@package"
+ Name string // e.g. "analyzer@package"
+ Pkg *types.Package // (for import removal)
FileSet *token.FileSet
ReadFileFunc analysisinternal.ReadFileFunc
Diagnostics []analysis.Diagnostic
@@ -58,15 +72,15 @@
// composition of the two fixes is semantically correct. Coalescing
// identical edits is appropriate for imports, but not for, say,
// increments to a counter variable; the correct resolution in that
-// case might be to increment it twice. Or consider two fixes that
-// each delete the penultimate reference to an import or local
-// variable: each fix is sound individually, and they may be textually
-// distant from each other, but when both are applied, the program is
-// no longer valid because it has an unreferenced import or local
-// variable.
-// TODO(adonovan): investigate replacing the final "gofmt" step with a
-// formatter that applies the unused-import deletion logic of
-// "goimports".
+// case might be to increment it twice.
+//
+// Or consider two fixes that each delete the penultimate reference to
+// a local variable: each fix is sound individually, and they may be
+// textually distant from each other, but when both are applied, the
+// program is no longer valid because it has an unreferenced local
+// variable. (ApplyFixes solves the analogous problem for imports by
+// eliminating imports whose name is unreferenced in the remainder of
+// the fixed file.)
//
// Merging depends on both the order of fixes and they order of edits
// within them. For example, if three fixes add import "a" twice and
@@ -134,8 +148,11 @@
// Apply each fix, updating the current state
// only if the entire fix can be cleanly merged.
- accumulatedEdits := make(map[string][]diff.Edit)
- goodFixes := 0
+ var (
+ accumulatedEdits = make(map[string][]diff.Edit)
+ filePkgs = make(map[string]*types.Package) // maps each file to an arbitrary package that includes it
+ goodFixes = 0
+ )
fixloop:
for _, fixact := range fixes {
// Convert analysis.TextEdits to diff.Edits, grouped by file.
@@ -144,6 +161,8 @@
for _, edit := range fixact.fix.TextEdits {
file := fixact.act.FileSet.File(edit.Pos)
+ filePkgs[file.Name()] = fixact.act.Pkg
+
baseline, err := getBaseline(fixact.act.ReadFileFunc, file.Name())
if err != nil {
log.Printf("skipping fix to file %s: %v", file.Name(), err)
@@ -214,7 +233,7 @@
}
// Attempt to format each file.
- if formatted, err := format.Source(final); err == nil {
+ if formatted, err := FormatSourceRemoveImports(filePkgs[file], final); err == nil {
final = formatted
}
@@ -282,3 +301,110 @@
return nil
}
+
+// FormatSourceRemoveImports is a variant of [format.Source] that
+// removes imports that became redundant when fixes were applied.
+//
+// Import removal is necessarily heuristic since we do not have type
+// information for the fixed file and thus cannot accurately tell
+// whether k is among the free names of T{k: 0}, which requires
+// knowledge of whether T is a struct type.
+func FormatSourceRemoveImports(pkg *types.Package, src []byte) ([]byte, error) {
+ // This function was reduced from the "strict entire file"
+ // path through [format.Source].
+
+ fset := token.NewFileSet()
+ file, err := parser.ParseFile(fset, "fixed.go", src, parser.ParseComments|parser.SkipObjectResolution)
+ if err != nil {
+ return nil, err
+ }
+
+ ast.SortImports(fset, file)
+
+ removeUnneededImports(fset, pkg, file)
+
+ // printerNormalizeNumbers means to canonicalize number literal prefixes
+ // and exponents while printing. See https://golang.org/doc/go1.13#gofmt.
+ //
+ // This value is defined in go/printer specifically for go/format and cmd/gofmt.
+ const printerNormalizeNumbers = 1 << 30
+ cfg := &printer.Config{
+ Mode: printer.UseSpaces | printer.TabIndent | printerNormalizeNumbers,
+ Tabwidth: 8,
+ }
+ var buf bytes.Buffer
+ if err := cfg.Fprint(&buf, fset, file); err != nil {
+ return nil, err
+ }
+ return buf.Bytes(), nil
+}
+
+// removeUnneededImports removes import specs that are not referenced
+// within the fixed file. It uses [free.Names] to heuristically
+// approximate the set of imported names needed by the body of the
+// file based only on syntax.
+//
+// pkg provides type information about the unmodified package, in
+// particular the name that would implicitly be declared by a
+// non-renaming import of a given existing dependency.
+func removeUnneededImports(fset *token.FileSet, pkg *types.Package, file *ast.File) {
+ // Map each existing dependency to its default import name.
+ // (We'll need this to interpret non-renaming imports.)
+ packageNames := make(map[string]string)
+ for _, imp := range pkg.Imports() {
+ packageNames[imp.Path()] = imp.Name()
+ }
+
+ // Compute the set of free names of the file,
+ // ignoring its import decls.
+ freenames := make(map[string]bool)
+ for _, decl := range file.Decls {
+ if decl, ok := decl.(*ast.GenDecl); ok && decl.Tok == token.IMPORT {
+ continue // skip import
+ }
+
+ // TODO(adonovan): we could do better than includeComplitIdents=false
+ // since we have type information about the unmodified package,
+ // which is a good source of heuristics.
+ const includeComplitIdents = false
+ maps.Copy(freenames, free.Names(decl, includeComplitIdents))
+ }
+
+ // Check whether each import's declared name is free (referenced) by the file.
+ var deletions []func()
+ for _, spec := range file.Imports {
+ path, err := strconv.Unquote(spec.Path.Value)
+ if err != nil {
+ continue // malformed import; ignore
+ }
+ explicit := "" // explicit PkgName, if any
+ if spec.Name != nil {
+ explicit = spec.Name.Name
+ }
+ name := explicit // effective PkgName
+ if name == "" {
+ // Non-renaming import: use package's default name.
+ name = packageNames[path]
+ }
+ switch name {
+ case "":
+ continue // assume it's a new import
+ case ".":
+ continue // dot imports are tricky
+ case "_":
+ continue // keep blank imports
+ }
+ if !freenames[name] {
+ // Import's effective name is not free in (not used by) the file.
+ // Enqueue it for deletion after the loop.
+ deletions = append(deletions, func() {
+ astutil.DeleteNamedImport(fset, file, explicit, path)
+ })
+ }
+ }
+
+ // Apply the deletions.
+ for _, del := range deletions {
+ del()
+ }
+}
diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go
index 02eca6d..4ba8788 100644
--- a/go/analysis/internal/checker/checker.go
+++ b/go/analysis/internal/checker/checker.go
@@ -196,6 +196,7 @@
if pass := internal.ActionPass(act); pass != nil {
fixActions[i] = analysisflags.FixAction{
Name: act.String(),
+ Pkg: act.Package.Types,
FileSet: act.Package.Fset,
ReadFileFunc: pass.ReadFile,
Diagnostics: act.Diagnostics,
diff --git a/go/analysis/passes/inline/gofix_test.go b/go/analysis/passes/inline/gofix_test.go
index 76114a0..adc1389 100644
--- a/go/analysis/passes/inline/gofix_test.go
+++ b/go/analysis/passes/inline/gofix_test.go
@@ -23,7 +23,7 @@
if testenv.Go1Point() < 24 {
testenv.NeedsGoExperiment(t, "aliastypeparams")
}
- analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), Analyzer, "a", "b")
+ analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), Analyzer, "a", "b", "rmimport")
}
func TestAllowBindingDeclFlag(t *testing.T) {
diff --git a/go/analysis/passes/inline/testdata/src/rmimport/rmimport.go b/go/analysis/passes/inline/testdata/src/rmimport/rmimport.go
new file mode 100644
index 0000000..257a9df
--- /dev/null
+++ b/go/analysis/passes/inline/testdata/src/rmimport/rmimport.go
@@ -0,0 +1,11 @@
+package rmimport
+
+import "a"
+
+// Test that application of two fixes that each remove the second-last
+// import of "a" results in removal of the import. This is implemented
+// by the general analysis fix logic, not by any one analyzer.
+func _() {
+ print(a.T{}.Two()) // want `should be inlined`
+ print(a.T{}.Two()) // want `should be inlined`
+}
diff --git a/go/analysis/passes/inline/testdata/src/rmimport/rmimport.go.golden b/go/analysis/passes/inline/testdata/src/rmimport/rmimport.go.golden
new file mode 100644
index 0000000..9a688b4
--- /dev/null
+++ b/go/analysis/passes/inline/testdata/src/rmimport/rmimport.go.golden
@@ -0,0 +1,9 @@
+package rmimport
+
+// Test that application of two fixes that each remove the second-last
+// import of "a" results in removal of the import. This is implemented
+// by the general analysis fix logic, not by any one analyzer.
+func _() {
+ print(2) // want `should be inlined`
+ print(2) // want `should be inlined`
+}
diff --git a/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go.golden b/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go.golden
index eb5c63b..70ecac4 100644
--- a/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go.golden
+++ b/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go.golden
@@ -1,7 +1,6 @@
package rangeint
import (
- "os"
os1 "os"
"rangeint/a"
)
diff --git a/go/analysis/unitchecker/unitchecker.go b/go/analysis/unitchecker/unitchecker.go
index b407bc7..4d9d7424 100644
--- a/go/analysis/unitchecker/unitchecker.go
+++ b/go/analysis/unitchecker/unitchecker.go
@@ -187,6 +187,7 @@
for i, res := range results {
fixActions[i] = analysisflags.FixAction{
Name: res.a.Name,
+ Pkg: res.pkg,
FileSet: fset,
ReadFileFunc: os.ReadFile,
Diagnostics: res.diagnostics,
@@ -482,9 +483,7 @@
results := make([]result, len(analyzers))
for i, a := range analyzers {
act := actions[a]
- results[i].a = a
- results[i].err = act.err
- results[i].diagnostics = act.diagnostics
+ results[i] = result{pkg, a, act.diagnostics, act.err}
}
data := facts.Encode()
@@ -499,6 +498,7 @@
}
type result struct {
+ pkg *types.Package
a *analysis.Analyzer
diagnostics []analysis.Diagnostic
err error
diff --git a/gopls/internal/analysis/maprange/testdata/basic.txtar b/gopls/internal/analysis/maprange/testdata/basic.txtar
index 1950e95..a1502c0 100644
--- a/gopls/internal/analysis/maprange/testdata/basic.txtar
+++ b/gopls/internal/analysis/maprange/testdata/basic.txtar
@@ -53,8 +53,6 @@
-- basic.go.golden --
package basic
-import "maps"
-
func _() {
m := make(map[int]int)
