From f91c4a586ecd9c7ff3ce7fa3ee98e22853df7b52 Mon Sep 17 00:00:00 2001 From: Suzy Mueller Date: Fri, 6 Aug 2021 13:27:00 -0600 Subject: [PATCH] internal/lsp: use LookupParent for finding scope The lexicalLookup function is used to determine if a use of an object would be shadowed by a different definition after a rename. Switch to using LookupParent which is more careful about the positions of the identifiers. Fixes golang/go#47583 Change-Id: I3dbdf79e537ce637d1276ddbecb094db21f1c26d Reviewed-on: https://go-review.googlesource.com/c/tools/+/340551 Trust: Suzy Mueller Run-TryBot: Suzy Mueller TryBot-Result: Go Bot Reviewed-by: Robert Findley --- internal/lsp/source/rename_check.go | 27 ++--------- internal/lsp/testdata/rename/shadow/shadow.go | 20 ++++++++ .../testdata/rename/shadow/shadow.go.golden | 48 +++++++++++++++++++ internal/lsp/testdata/summary.txt.golden | 2 +- 4 files changed, 72 insertions(+), 25 deletions(-) create mode 100644 internal/lsp/testdata/rename/shadow/shadow.go create mode 100644 internal/lsp/testdata/rename/shadow/shadow.go.golden diff --git a/internal/lsp/source/rename_check.go b/internal/lsp/source/rename_check.go index a46254c3cd..3aafc391e6 100644 --- a/internal/lsp/source/rename_check.go +++ b/internal/lsp/source/rename_check.go @@ -195,7 +195,7 @@ func (r *renamer) checkInLexicalScope(from types.Object, pkg Package) { // The name r.to is defined in a superblock. // Is that name referenced from within this block? forEachLexicalRef(pkg, to, func(id *ast.Ident, block *types.Scope) bool { - _, obj := lexicalLookup(block, from.Name(), id.Pos()) + _, obj := block.LookupParent(from.Name(), id.Pos()) if obj == from { // super-block conflict r.errorf(from.Pos(), "renaming this %s %q to %q", @@ -215,10 +215,9 @@ func (r *renamer) checkInLexicalScope(from types.Object, pkg Package) { forEachLexicalRef(pkg, from, func(id *ast.Ident, block *types.Scope) bool { // Find the block that defines the found reference. // It may be an ancestor. - fromBlock, _ := lexicalLookup(block, from.Name(), id.Pos()) - + fromBlock, _ := block.LookupParent(from.Name(), id.Pos()) // See what r.to would resolve to in the same scope. - toBlock, to := lexicalLookup(block, r.to, id.Pos()) + toBlock, to := block.LookupParent(r.to, id.Pos()) if to != nil { // sub-block conflict if deeper(toBlock, fromBlock) { @@ -249,26 +248,6 @@ func (r *renamer) checkInLexicalScope(from types.Object, pkg Package) { } } -// lexicalLookup is like (*types.Scope).LookupParent but respects the -// environment visible at pos. It assumes the relative position -// information is correct with each file. -func lexicalLookup(block *types.Scope, name string, pos token.Pos) (*types.Scope, types.Object) { - for b := block; b != nil; b = b.Parent() { - obj := b.Lookup(name) - // The scope of a package-level object is the entire package, - // so ignore pos in that case. - // No analogous clause is needed for file-level objects - // since no reference can appear before an import decl. - if obj == nil || obj.Pkg() == nil { - continue - } - if b == obj.Pkg().Scope() || obj.Pos() < pos { - return b, obj - } - } - return nil, nil -} - // deeper reports whether block x is lexically deeper than y. func deeper(x, y *types.Scope) bool { if x == y || x == nil { diff --git a/internal/lsp/testdata/rename/shadow/shadow.go b/internal/lsp/testdata/rename/shadow/shadow.go new file mode 100644 index 0000000000..38329b4fea --- /dev/null +++ b/internal/lsp/testdata/rename/shadow/shadow.go @@ -0,0 +1,20 @@ +package shadow + +func _() { + a := true + b, c, _ := A(), B(), D() //@rename("A", "a"),rename("B", "b"),rename("b", "c"),rename("D", "d") + d := false + _, _, _, _ = a, b, c, d +} + +func A() int { + return 0 +} + +func B() int { + return 0 +} + +func D() int { + return 0 +} diff --git a/internal/lsp/testdata/rename/shadow/shadow.go.golden b/internal/lsp/testdata/rename/shadow/shadow.go.golden new file mode 100644 index 0000000000..6281bcdd91 --- /dev/null +++ b/internal/lsp/testdata/rename/shadow/shadow.go.golden @@ -0,0 +1,48 @@ +-- a-rename -- +renaming this func "A" to "a" would cause this reference to become shadowed by this intervening var definition +-- b-rename -- +package shadow + +func _() { + a := true + b, c, _ := A(), b(), D() //@rename("A", "a"),rename("B", "b"),rename("b", "c"),rename("D", "d") + d := false + _, _, _, _ = a, b, c, d +} + +func A() int { + return 0 +} + +func b() int { + return 0 +} + +func D() int { + return 0 +} + +-- c-rename -- +renaming this var "b" to "c" conflicts with var in same block +-- d-rename -- +package shadow + +func _() { + a := true + b, c, _ := A(), B(), d() //@rename("A", "a"),rename("B", "b"),rename("b", "c"),rename("D", "d") + d := false + _, _, _, _ = a, b, c, d +} + +func A() int { + return 0 +} + +func B() int { + return 0 +} + +func d() int { + return 0 +} + diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden index 521dad27bf..7143365912 100644 --- a/internal/lsp/testdata/summary.txt.golden +++ b/internal/lsp/testdata/summary.txt.golden @@ -20,7 +20,7 @@ DefinitionsCount = 95 TypeDefinitionsCount = 18 HighlightsCount = 69 ReferencesCount = 27 -RenamesCount = 37 +RenamesCount = 41 PrepareRenamesCount = 7 SymbolsCount = 5 WorkspaceSymbolsCount = 20