gopls/internal/lsp/source: multiline errors from rename
This changes restores the full glory of the multi-line
error messages produced by the gorename tool, which
cite up to three source locations in the explanation
of a renaming conflict. LSP provides no way to preserve
the structure of the error, so we do our best to format
the filename concisely but informatively.
Change-Id: I4e56a818e48f94a19f732f3142917e67869212f1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/466495
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/internal/lsp/source/rename.go b/gopls/internal/lsp/source/rename.go
index f56ed40..7b00f88 100644
--- a/gopls/internal/lsp/source/rename.go
+++ b/gopls/internal/lsp/source/rename.go
@@ -35,8 +35,7 @@
snapshot Snapshot
refs []*ReferenceInfo
objsToUpdate map[types.Object]bool
- hadConflicts bool
- errors string
+ conflicts []string
from, to string
satisfyConstraints map[satisfy.Constraint]bool
packages map[*types.Package]Package // may include additional packages that are a dep of pkg.
@@ -647,13 +646,11 @@
// Check that the renaming of the identifier is ok.
for _, ref := range refs {
r.check(ref.obj)
- if r.hadConflicts { // one error is enough.
- break
+ if len(r.conflicts) > 0 {
+ // Stop at first error.
+ return nil, fmt.Errorf("%s", strings.Join(r.conflicts, "\n"))
}
}
- if r.hadConflicts {
- return nil, fmt.Errorf("%s", r.errors)
- }
changes, err := r.update()
if err != nil {
diff --git a/gopls/internal/lsp/source/rename_check.go b/gopls/internal/lsp/source/rename_check.go
index d01d228..69b0c4a 100644
--- a/gopls/internal/lsp/source/rename_check.go
+++ b/gopls/internal/lsp/source/rename_check.go
@@ -12,18 +12,59 @@
"go/ast"
"go/token"
"go/types"
+ "path/filepath"
"reflect"
"strings"
"unicode"
"golang.org/x/tools/go/ast/astutil"
+ "golang.org/x/tools/gopls/internal/lsp/safetoken"
"golang.org/x/tools/refactor/satisfy"
)
// errorf reports an error (e.g. conflict) and prevents file modification.
func (r *renamer) errorf(pos token.Pos, format string, args ...interface{}) {
- r.hadConflicts = true
- r.errors += fmt.Sprintf(format, args...)
+ // Conflict error messages in the old gorename tool (whence this
+ // logic originated) contain rich information associated with
+ // multiple source lines, such as:
+ //
+ // p/a.go:1:2: renaming "x" to "y" here
+ // p/b.go:3:4: \t would cause this reference to "y"
+ // p/c.go:5:5: \t to become shadowed by this intervening declaration.
+ //
+ // Unfortunately LSP provides no means to transmit the
+ // structure of this error, so we format the positions briefly
+ // using dir/file.go where dir is the base name of the parent
+ // directory.
+
+ var conflict strings.Builder
+
+ // Add prefix of (truncated) position.
+ if pos != token.NoPos {
+ var fset *token.FileSet
+ for _, pkg := range r.packages {
+ fset = pkg.FileSet()
+ break
+ }
+ if fset != nil {
+ // TODO(adonovan): skip position of first error if it is
+ // on the same line as the renaming itself.
+ posn := safetoken.StartPosition(fset, pos).String()
+ segments := strings.Split(filepath.ToSlash(posn), "/")
+ if n := len(segments); n > 2 {
+ segments = segments[n-2:]
+ }
+ posn = strings.Join(segments, "/")
+ fmt.Fprintf(&conflict, "%s:", posn)
+
+ if !strings.HasPrefix(format, "\t") {
+ conflict.WriteByte(' ')
+ }
+ }
+ }
+
+ fmt.Fprintf(&conflict, format, args...)
+ r.conflicts = append(r.conflicts, conflict.String())
}
// check performs safety checks of the renaming of the 'from' object to r.to.
diff --git a/gopls/internal/lsp/testdata/rename/shadow/shadow.go.golden b/gopls/internal/lsp/testdata/rename/shadow/shadow.go.golden
index 6281bcd..a34b5c0 100644
--- a/gopls/internal/lsp/testdata/rename/shadow/shadow.go.golden
+++ b/gopls/internal/lsp/testdata/rename/shadow/shadow.go.golden
@@ -1,5 +1,7 @@
-- a-rename --
-renaming this func "A" to "a" would cause this reference to become shadowed by this intervening var definition
+shadow/shadow.go:10:6: renaming this func "A" to "a"
+shadow/shadow.go:5:13: would cause this reference to become shadowed
+shadow/shadow.go:4:2: by this intervening var definition
-- b-rename --
package shadow
@@ -23,7 +25,8 @@
}
-- c-rename --
-renaming this var "b" to "c" conflicts with var in same block
+shadow/shadow.go:5:2: renaming this var "b" to "c"
+shadow/shadow.go:5:5: conflicts with var in same block
-- d-rename --
package shadow
