From eca45d481ddc3db098346c997108ae663320f623 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 26 Mar 2020 23:25:15 -0400 Subject: [PATCH] internal/lsp: refactor references/rename/implementations As part of investigating golang/go#38100, I noticed a few things that I wanted to clean up. Mostly, for renames, we were calling qualifiedObjAtProtocolPos twice, so I factored out a shared helper function. I also added an error return for builtins so that callers don't have to check. Change-Id: I28c75c801cbec1611736af931cfa72befd219201 Reviewed-on: https://go-review.googlesource.com/c/tools/+/225777 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Rohan Challa --- internal/lsp/references.go | 3 -- internal/lsp/source/implementation.go | 34 ++++++++++++------- internal/lsp/source/references.go | 34 +++++++++---------- internal/lsp/source/rename.go | 24 +++---------- .../lsp/primarymod/rename/b/b.go.golden | 2 +- 5 files changed, 43 insertions(+), 54 deletions(-) diff --git a/internal/lsp/references.go b/internal/lsp/references.go index 57e92c09da..8a5700dd11 100644 --- a/internal/lsp/references.go +++ b/internal/lsp/references.go @@ -20,19 +20,16 @@ func (s *Server) references(ctx context.Context, params *protocol.ReferenceParam if err != nil { return nil, err } - var locations []protocol.Location for _, ref := range references { refRange, err := ref.Range() if err != nil { return nil, err } - locations = append(locations, protocol.Location{ URI: protocol.URIFromSpanURI(ref.URI()), Range: refRange, }) } - return locations, nil } diff --git a/internal/lsp/source/implementation.go b/internal/lsp/source/implementation.go index 3c8c2497c3..58ae3e1f92 100644 --- a/internal/lsp/source/implementation.go +++ b/internal/lsp/source/implementation.go @@ -6,6 +6,7 @@ package source import ( "context" + "errors" "fmt" "go/ast" "go/token" @@ -13,7 +14,6 @@ import ( "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/telemetry/event" - errors "golang.org/x/xerrors" ) func Implementation(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position) ([]protocol.Location, error) { @@ -24,7 +24,6 @@ func Implementation(ctx context.Context, s Snapshot, f FileHandle, pp protocol.P if err != nil { return nil, err } - var locations []protocol.Location for _, impl := range impls { if impl.pkg == nil || len(impl.pkg.CompiledGoFiles()) == 0 { @@ -61,7 +60,6 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol. if err != nil { return nil, err } - for _, qo := range qos { var ( queryType types.Type @@ -201,6 +199,8 @@ type qualifiedObject struct { sourcePkg Package } +var errBuiltin = errors.New("builtin object") + // qualifiedObjsAtProtocolPos returns info for all the type.Objects // referenced at the given position. An object will be returned for // every package that the file belongs to. @@ -212,11 +212,11 @@ func qualifiedObjsAtProtocolPos(ctx context.Context, s Snapshot, fh FileHandle, // Check all the packages that the file belongs to. var qualifiedObjs []qualifiedObject for _, ph := range phs { - pkg, err := ph.Check(ctx) + searchpkg, err := ph.Check(ctx) if err != nil { return nil, err } - astFile, pos, err := getASTFile(pkg, fh, pp) + astFile, pos, err := getASTFile(searchpkg, fh, pp) if err != nil { return nil, err } @@ -230,10 +230,10 @@ func qualifiedObjsAtProtocolPos(ctx context.Context, s Snapshot, fh FileHandle, // If leaf represents an implicit type switch object or the type // switch "assign" variable, expand to all of the type switch's // implicit objects. - if implicits := typeSwitchImplicits(pkg, path); len(implicits) > 0 { + if implicits := typeSwitchImplicits(searchpkg, path); len(implicits) > 0 { objs = append(objs, implicits...) } else { - obj := pkg.GetTypesInfo().ObjectOf(leaf) + obj := searchpkg.GetTypesInfo().ObjectOf(leaf) if obj == nil { return nil, fmt.Errorf("no object for %q", leaf.Name) } @@ -241,22 +241,30 @@ func qualifiedObjsAtProtocolPos(ctx context.Context, s Snapshot, fh FileHandle, } case *ast.ImportSpec: // Look up the implicit *types.PkgName. - obj := pkg.GetTypesInfo().Implicits[leaf] + obj := searchpkg.GetTypesInfo().Implicits[leaf] if obj == nil { return nil, fmt.Errorf("no object for import %q", importPath(leaf)) } objs = append(objs, obj) } pkgs := make(map[*types.Package]Package) - pkgs[pkg.GetTypes()] = pkg - for _, imp := range pkg.Imports() { + pkgs[searchpkg.GetTypes()] = searchpkg + for _, imp := range searchpkg.Imports() { pkgs[imp.GetTypes()] = imp } for _, obj := range objs { + if obj.Parent() == types.Universe { + return nil, fmt.Errorf("%w %q", errBuiltin, obj.Name()) + } + pkg, ok := pkgs[obj.Pkg()] + if !ok { + event.Error(ctx, fmt.Sprintf("no package for obj %s: %v", obj, obj.Pkg()), err) + continue + } qualifiedObjs = append(qualifiedObjs, qualifiedObject{ obj: obj, - pkg: pkgs[obj.Pkg()], - sourcePkg: pkg, + pkg: pkg, + sourcePkg: searchpkg, node: path[0], }) } @@ -264,7 +272,7 @@ func qualifiedObjsAtProtocolPos(ctx context.Context, s Snapshot, fh FileHandle, // Return an error if no objects were found since callers will assume that // the slice has at least 1 element. if len(qualifiedObjs) == 0 { - return nil, errors.Errorf("no object found") + return nil, fmt.Errorf("no object found") } return qualifiedObjs, nil } diff --git a/internal/lsp/source/references.go b/internal/lsp/source/references.go index c5cea84b87..d1cc60261a 100644 --- a/internal/lsp/source/references.go +++ b/internal/lsp/source/references.go @@ -6,6 +6,7 @@ package source import ( "context" + "errors" "go/ast" "go/token" "go/types" @@ -31,14 +32,19 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit defer done() qualifiedObjs, err := qualifiedObjsAtProtocolPos(ctx, s, f, pp) + // Don't return references for builtin types. + if errors.Is(err, errBuiltin) { + return nil, nil + } if err != nil { return nil, err } - // Don't return references for builtin types. - if qualifiedObjs[0].obj.Parent() == types.Universe { - return nil, nil - } + return references(ctx, s, qualifiedObjs, includeDeclaration) +} +// references is a helper function used by both References and Rename, +// to avoid recomputing qualifiedObjsAtProtocolPos. +func references(ctx context.Context, s Snapshot, qos []qualifiedObject, includeDeclaration bool) ([]*ReferenceInfo, error) { var ( references []*ReferenceInfo seen = make(map[token.Position]bool) @@ -47,23 +53,21 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit // Make sure declaration is the first item in the response. if includeDeclaration { - rng, err := objToMappedRange(s.View(), qualifiedObjs[0].pkg, qualifiedObjs[0].obj) + rng, err := objToMappedRange(s.View(), qos[0].pkg, qos[0].obj) if err != nil { return nil, err } - - ident, _ := qualifiedObjs[0].node.(*ast.Ident) + ident, _ := qos[0].node.(*ast.Ident) references = append(references, &ReferenceInfo{ mappedRange: rng, - Name: qualifiedObjs[0].obj.Name(), + Name: qos[0].obj.Name(), ident: ident, - obj: qualifiedObjs[0].obj, - pkg: qualifiedObjs[0].pkg, + obj: qos[0].obj, + pkg: qos[0].pkg, isDeclaration: true, }) } - - for _, qo := range qualifiedObjs { + for _, qo := range qos { var searchPkgs []Package // Only search dependents if the object is exported. @@ -72,7 +76,6 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit if err != nil { return nil, err } - for _, ph := range reverseDeps { pkg, err := ph.Check(ctx) if err != nil { @@ -81,22 +84,18 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit searchPkgs = append(searchPkgs, pkg) } } - // Add the package in which the identifier is declared. searchPkgs = append(searchPkgs, qo.pkg) - for _, pkg := range searchPkgs { for ident, obj := range pkg.GetTypesInfo().Uses { if obj != qo.obj { continue } - pos := fset.Position(ident.Pos()) if seen[pos] { continue } seen[pos] = true - rng, err := posToMappedRange(s.View(), pkg, ident.Pos(), ident.End()) if err != nil { return nil, err @@ -111,6 +110,5 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit } } } - return references, nil } diff --git a/internal/lsp/source/rename.go b/internal/lsp/source/rename.go index f2d2c27cd8..d47a604d48 100644 --- a/internal/lsp/source/rename.go +++ b/internal/lsp/source/rename.go @@ -49,30 +49,22 @@ func PrepareRename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Po if err != nil { return nil, err } - - // Do not rename builtin identifiers. - if qos[0].obj.Parent() == types.Universe { - return nil, errors.Errorf("cannot rename builtin %q", qos[0].obj.Name()) - } - - mr, err := posToMappedRange(s.View(), qos[0].sourcePkg, qos[0].node.Pos(), qos[0].node.End()) + node, obj, pkg := qos[0].node, qos[0].obj, qos[0].sourcePkg + mr, err := posToMappedRange(s.View(), pkg, node.Pos(), node.End()) if err != nil { return nil, err } - rng, err := mr.Range() if err != nil { return nil, err } - - if _, isImport := qos[0].node.(*ast.ImportSpec); isImport { + if _, isImport := node.(*ast.ImportSpec); isImport { // We're not really renaming the import path. rng.End = rng.Start } - return &PrepareItem{ Range: rng, - Text: qos[0].obj.Name(), + Text: obj.Name(), }, nil } @@ -95,19 +87,13 @@ func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position, if !isValidIdentifier(newName) { return nil, errors.Errorf("invalid identifier to rename: %q", newName) } - // Do not rename builtin identifiers. - if obj.Parent() == types.Universe { - return nil, errors.Errorf("cannot rename builtin %q", obj.Name()) - } if pkg == nil || pkg.IsIllTyped() { return nil, errors.Errorf("package for %s is ill typed", f.Identity().URI) } - - refs, err := References(ctx, s, f, pp, true) + refs, err := references(ctx, s, qos, true) if err != nil { return nil, err } - r := renamer{ ctx: ctx, fset: s.View().Session().Cache().FileSet(), diff --git a/internal/lsp/testdata/lsp/primarymod/rename/b/b.go.golden b/internal/lsp/testdata/lsp/primarymod/rename/b/b.go.golden index a402a58cf4..c39509e6be 100644 --- a/internal/lsp/testdata/lsp/primarymod/rename/b/b.go.golden +++ b/internal/lsp/testdata/lsp/primarymod/rename/b/b.go.golden @@ -10,4 +10,4 @@ func _() { } -- uint-rename -- -cannot rename builtin "int" +builtin object "int"