refactor/rename: fix spurious conflict report when renaming x/foo to y/foo
There is no need to check for import conflicts when the package name
doesn't change. Add test.
Also, when reporting a (non-spurious) import conflict, make clear that
the error is just a warning. Add a test for same.
Change-Id: Idde0483b502cd041fd893230fec06b8533f54f3c
Reviewed-on: https://go-review.googlesource.com/24917
Reviewed-by: Michael Matloob <matloob@golang.org>
diff --git a/refactor/rename/mvpkg_test.go b/refactor/rename/mvpkg_test.go
index e07b6b4..9476c17 100644
--- a/refactor/rename/mvpkg_test.go
+++ b/refactor/rename/mvpkg_test.go
@@ -7,8 +7,10 @@
import (
"fmt"
"go/build"
+ "go/token"
"io/ioutil"
"path/filepath"
+ "reflect"
"regexp"
"strings"
"testing"
@@ -112,9 +114,10 @@
func TestMoves(t *testing.T) {
tests := []struct {
- ctxt *build.Context
- from, to string
- want map[string]string
+ ctxt *build.Context
+ from, to string
+ want map[string]string
+ wantWarnings []string
}{
// Simple example.
{
@@ -267,6 +270,77 @@
/* import " this is not an import comment */
`},
},
+ // Import name conflict generates a warning, not an error.
+ {
+ ctxt: fakeContext(map[string][]string{
+ "x": {},
+ "a": {`package a; type A int`},
+ "b": {`package b; type B int`},
+ "conflict": {`package conflict
+
+import "a"
+import "b"
+var _ a.A
+var _ b.B
+`},
+ "ok": {`package ok
+import "b"
+var _ b.B
+`},
+ }),
+ from: "b", to: "x/a",
+ want: map[string]string{
+ "/go/src/a/0.go": `package a; type A int`,
+ "/go/src/ok/0.go": `package ok
+
+import "x/a"
+
+var _ a.B
+`,
+ "/go/src/conflict/0.go": `package conflict
+
+import "a"
+import "x/a"
+
+var _ a.A
+var _ b.B
+`,
+ "/go/src/x/a/0.go": `package a
+
+type B int
+`,
+ },
+ wantWarnings: []string{
+ `/go/src/conflict/0.go:4:8: renaming this imported package name "b" to "a"`,
+ `/go/src/conflict/0.go:3:8: conflicts with imported package name in same block`,
+ `/go/src/conflict/0.go:3:8: skipping update of this file`,
+ },
+ },
+ // Rename with same base name.
+ {
+ ctxt: fakeContext(map[string][]string{
+ "x": {},
+ "y": {},
+ "x/foo": {`package foo
+
+type T int
+`},
+ "main": {`package main; import "x/foo"; var _ foo.T`},
+ }),
+ from: "x/foo", to: "y/foo",
+ want: map[string]string{
+ "/go/src/y/foo/0.go": `package foo
+
+type T int
+`,
+ "/go/src/main/0.go": `package main
+
+import "y/foo"
+
+var _ foo.T
+`,
+ },
+ },
}
for _, test := range tests {
@@ -296,6 +370,10 @@
}
got[path] = string(bytes)
})
+ var warnings []string
+ reportError = func(posn token.Position, message string) {
+ warnings = append(warnings, posn.String()+": "+message)
+ }
writeFile = func(filename string, content []byte) error {
got[filename] = string(content)
return nil
@@ -318,6 +396,13 @@
continue
}
+ if !reflect.DeepEqual(warnings, test.wantWarnings) {
+ t.Errorf("%s: unexpected warnings:\n%s\nwant:\n%s",
+ prefix,
+ strings.Join(warnings, "\n"),
+ strings.Join(test.wantWarnings, "\n"))
+ }
+
for file, wantContent := range test.want {
k := filepath.FromSlash(file)
gotContent, ok := got[k]
diff --git a/refactor/rename/rename.go b/refactor/rename/rename.go
index be47701..766bbc2 100644
--- a/refactor/rename/rename.go
+++ b/refactor/rename/rename.go
@@ -174,11 +174,13 @@
fmt.Fprintf(os.Stderr, "%s: %s\n", posn, message)
}
-// importName renames imports of the package with the given path in
-// the given package. If fromName is not empty, only imports as
-// fromName will be renamed. If the renaming would lead to a conflict,
-// the file is left unchanged.
+// importName renames imports of fromPath within the package specified by info.
+// If fromName is not empty, importName renames only imports as fromName.
+// If the renaming would lead to a conflict, the file is left unchanged.
func importName(iprog *loader.Program, info *loader.PackageInfo, fromPath, fromName, to string) error {
+ if fromName == to {
+ return nil // no-op (e.g. rename x/foo to y/foo)
+ }
for _, f := range info.Files {
var from types.Object
for _, imp := range f.Imports {
@@ -203,6 +205,8 @@
}
r.check(from)
if r.hadConflicts {
+ reportError(iprog.Fset.Position(f.Imports[0].Pos()),
+ "skipping update of this file")
continue // ignore errors; leave the existing name
}
if err := r.update(); err != nil {
