Skip to content

Commit 5f1186b

Browse files
madelinekalilgopherbot
madelinekalil
authored and
gopherbot
committed
gopls/internal/analysis/driverutil: remove unnecessary new imports
Previously, the logic that removes unused imports after applying a fix skipped all newly added imports. Some fixes, such as the inliner, conservatively add imports that may actually be unnecessary, so we should remove those too. This modifies the search for default import names to include transitive dependencies. A new import will not be found in the direct dependencies, but there is a chance that a DFS of the import graph will find the default import name, which is needed to definitely say whether the import is not used in the current file and can be deleted. Also, fixes an existing test file issue76190 that wasn't running with suggested fixes. Fixes golang/go#77610 Change-Id: I1518fca35d76a805e798c8e5d02d4d22ec01caa2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/752500 Auto-Submit: Madeline Kalil <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
1 parent ff45494 commit 5f1186b

File tree

6 files changed

+132
-28
lines changed

6 files changed

+132
-28
lines changed

‎go/analysis/passes/inline/inline_test.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,16 @@ func TestAnalyzer(t *testing.T) {
2626
}
2727
analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), Analyzer, "a", "b", "rmimport")
2828

29-
dir := testfiles.ExtractTxtarFileToTmp(t, "testdata/src/issue76190.txtar")
30-
analysistest.Run(t, dir, Analyzer, "example.com/a", "example.com/b")
29+
dir1 := testfiles.ExtractTxtarFileToTmp(t, "testdata/src/issue76190.txtar")
30+
analysistest.RunWithSuggestedFixes(t, dir1, Analyzer, "example.com/a", "example.com/b")
31+
32+
dir2 := testfiles.ExtractTxtarFileToTmp(t, "testdata/src/issue77610.txtar")
33+
analysistest.RunWithSuggestedFixes(t, dir2, Analyzer,
34+
"example.com/a",
35+
"example.com/b",
36+
"example.com/c",
37+
"example.com/d",
38+
)
3139
}
3240

3341
func TestAllowBindingDeclFlag(t *testing.T) {

‎go/analysis/passes/inline/testdata/src/issue76190.txtar‎

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -55,23 +55,23 @@ package a
5555
import "testing"
5656

5757
func TestF(t *testing.T) {
58-
F() // not inlined
59-
G() // want "Call of a.G should be inlined"
58+
F() // not inlined
59+
print("G") // want "Call of a.G should be inlined"
6060
}
6161

6262
func TestG_comment(t *testing.T) {
63-
F() // want "Call of a.G should be inlined"
64-
G() // not inlined
63+
print("F") // want "Call of a.F should be inlined"
64+
G() // not inlined
6565
}
6666

6767
func TestT_F(t *testing.T) {
68-
T(0).F() // not inlined
69-
T(0).G() // want "Call of a.G should be inlined"
68+
T(0).F() // not inlined
69+
print("T.G") // want `Call of \(a.T\).G should be inlined`
7070
}
7171

7272
func TestT_G(t *testing.T) {
73-
T(0).F() // want "Call of a.G should be inlined"
74-
T(0).G() // not inlined
73+
print("T.F") // want `Call of \(a.T\).F should be inlined`
74+
T(0).G() // not inlined
7575
}
7676

7777
-- a/a_x_test.go --
@@ -111,23 +111,23 @@ import (
111111
)
112112

113113
func TestF(t *testing.T) {
114-
a.F() // not inlined
115-
a.G() // want "Call of a.G should be inlined"
114+
a.F() // not inlined
115+
print("G") // want "Call of a.G should be inlined"
116116
}
117117

118118
func TestG_comment(t *testing.T) {
119-
a.F() // want "Call of a.G should be inlined"
120-
a.G() // not inlined
119+
print("F") // want "Call of a.F should be inlined"
120+
a.G() // not inlined
121121
}
122122

123123
func TestT_F(t *testing.T) {
124-
a.T(0).F() // not inlined
125-
a.T(0).G() // want "Call of a.G should be inlined"
124+
a.T(0).F() // not inlined
125+
print("T.G") // want `Call of \(a.T\).G should be inlined`
126126
}
127127

128128
func TestT_G(t *testing.T) {
129-
a.T(0).F() // want "Call of a.G should be inlined"
130-
a.T(0).G() // not inlined
129+
print("T.F") // want `Call of \(a.T\).F should be inlined`
130+
a.T(0).G() // not inlined
131131
}
132132

133133
-- b/b_test.go --
@@ -146,10 +146,9 @@ func TestF(t *testing.T) {
146146
package b_test
147147

148148
import (
149-
"example.com/a"
150149
"testing"
151150
)
152151

153152
func TestF(t *testing.T) {
154-
print() // want "Call of a.F should be inlined"
153+
print("F") // want "Call of a.F should be inlined"
155154
}

‎go/analysis/passes/inline/testdata/src/issue77610.txtar‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
This test verifies that unused imports conservatively added by the inliner are
2+
removed in FormatSourceRemoveImports.
3+
4+
-- go.mod --
5+
module example.com
6+
7+
-- a/a.go --
8+
package a
9+
10+
import "io"
11+
12+
//go:fix inline
13+
func OldHello(w io.Writer) error { // want OldHello:"goFixInline a.OldHello"
14+
return NewHello(w)
15+
}
16+
17+
func NewHello(w io.Writer) error {
18+
_, err := w.Write([]byte("Hello, World!"))
19+
return err
20+
}
21+
22+
-- b/b.go --
23+
package b
24+
25+
import (
26+
"strings"
27+
"example.com/a"
28+
)
29+
30+
func _() {
31+
var buf strings.Builder
32+
// The inliner wants to add an import for "io"; make sure post-processing of the fix removes it.
33+
a.OldHello(&buf) // want "Call of a.OldHello should be inlined"
34+
}
35+
36+
-- b/b.go.golden --
37+
package b
38+
39+
import (
40+
"strings"
41+
"example.com/a"
42+
)
43+
44+
func _() {
45+
var buf strings.Builder
46+
// The inliner wants to add an import for "io"; make sure post-processing of the fix removes it.
47+
a.NewHello(&buf) // want "Call of a.OldHello should be inlined"
48+
}
49+
50+
-- c/c.go --
51+
package c
52+
53+
import "encoding/json"
54+
55+
//go:fix inline
56+
func OldProcess(m json.Marshaler) error { // want OldProcess:"goFixInline c.OldProcess"
57+
return NewProcess(m)
58+
}
59+
60+
func NewProcess(m json.Marshaler) error {
61+
_, err := m.MarshalJSON()
62+
return err
63+
}
64+
65+
-- d/d.go --
66+
package d
67+
68+
import (
69+
"time"
70+
71+
"example.com/c"
72+
)
73+
74+
func _() {
75+
t := time.Now()
76+
// The inliner wants to add an import for "encoding/json"; make sure post-processing of the fix removes it.
77+
c.OldProcess(t) // want "Call of c.OldProcess should be inlined"
78+
}
79+
80+
-- d/d.go.golden --
81+
package d
82+
83+
import (
84+
"time"
85+
86+
"example.com/c"
87+
)
88+
89+
func _() {
90+
t := time.Now()
91+
// The inliner wants to add an import for "encoding/json"; make sure post-processing of the fix removes it.
92+
c.NewProcess(t) // want "Call of c.OldProcess should be inlined"
93+
}

‎gopls/internal/test/marker/testdata/codeaction/removeparam_imports.txt‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,6 @@ func _() {
8989
-- @b/a/a1.go --
9090
package a
9191

92-
import "mod.test/c"
93-
9492
import "mod.test/b"
9593

9694
func _() {
@@ -104,7 +102,6 @@ package a
104102
import (
105103
"mod.test/b"
106104
. "mod.test/b"
107-
"mod.test/c"
108105
)
109106

110107
func _() {

‎internal/analysis/driverutil/fix.go‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -386,11 +386,20 @@ func FormatSourceRemoveImports(pkg *types.Package, src []byte) ([]byte, error) {
386386
// particular the name that would implicitly be declared by a
387387
// non-renaming import of a given existing dependency.
388388
func removeUnneededImports(fset *token.FileSet, pkg *types.Package, file *ast.File) {
389-
// Map each existing dependency to its default import name.
389+
// Map each existing dependency and its transitive dependencies to its default import name.
390390
// (We'll need this to interpret non-renaming imports.)
391391
packageNames := make(map[string]string)
392+
var visit func(pkg *types.Package)
393+
visit = func(pkg *types.Package) {
394+
if packageNames[pkg.Path()] == "" {
395+
packageNames[pkg.Path()] = pkg.Name()
396+
for _, imp := range pkg.Imports() {
397+
visit(imp)
398+
}
399+
}
400+
}
392401
for _, imp := range pkg.Imports() {
393-
packageNames[imp.Path()] = imp.Name()
402+
visit(imp)
394403
}
395404

396405
// Compute the set of free names of the file,
@@ -426,7 +435,7 @@ func removeUnneededImports(fset *token.FileSet, pkg *types.Package, file *ast.Fi
426435
}
427436
switch name {
428437
case "":
429-
continue // assume it's a new import
438+
continue // assume it's a new import, and we didn't find its default name while searching the import graph
430439
case ".":
431440
continue // dot imports are tricky
432441
case "_":

‎internal/refactor/inline/testdata/assignment.txtar‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,6 @@ func _() {
101101
-- b2 --
102102
package a
103103

104-
import "testdata/c"
105-
106104
import "testdata/b"
107105

108106
func _() {

0 commit comments

Comments
 (0)