Skip to content

Commit 1aa7e72

Browse files
adonovangopherbot
adonovan
authored and
gopherbot
committed
gopls/internal/lsp/source: avoid qualifiedObjectsAtProtocolPos
This change to PrepareRename eliminates one of the last two calls to qualifiedObjectsAtProtocolPos. All it needs is the current file and the object name, so it was overkill. The last use (from Rename) will be removed in a follow-up; it is much more involved. Change-Id: I64a06d95f7d2a88ace0f3c168bad7f0a8c0a7a04 Reviewed-on: https://go-review.googlesource.com/c/tools/+/463896 Run-TryBot: Alan Donovan <adonovan@google.com> Reviewed-by: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Alan Donovan <adonovan@google.com>
1 parent 5ed33df commit 1aa7e72

File tree

2 files changed

+85
-90
lines changed

2 files changed

+85
-90
lines changed

‎gopls/internal/lsp/source/references2.go

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ func ordinaryReferences(ctx context.Context, snapshot Snapshot, uri span.URI, pp
223223
if err != nil {
224224
return nil, err
225225
}
226-
candidates, err := objectsAt(pkg.GetTypesInfo(), pgf.File, pos)
226+
candidates, _, err := objectsAt(pkg.GetTypesInfo(), pgf.File, pos)
227227
if err != nil {
228228
return nil, err
229229
}
@@ -446,7 +446,7 @@ func localReferences(ctx context.Context, snapshot Snapshot, declURI span.URI, d
446446
if err != nil {
447447
return err
448448
}
449-
targets, err := objectsAt(pkg.GetTypesInfo(), pgf.File, pos)
449+
targets, _, err := objectsAt(pkg.GetTypesInfo(), pgf.File, pos)
450450
if err != nil {
451451
return err // unreachable? (probably caught earlier)
452452
}
@@ -521,11 +521,12 @@ func effectiveReceiver(obj types.Object) types.Type {
521521
// position.
522522
//
523523
// Each object is mapped to the syntax node that was treated as an
524-
// identifier, which is not always an ast.Ident.
525-
func objectsAt(info *types.Info, file *ast.File, pos token.Pos) (map[types.Object]ast.Node, error) {
524+
// identifier, which is not always an ast.Ident. The second component
525+
// of the result is the innermost node enclosing pos.
526+
func objectsAt(info *types.Info, file *ast.File, pos token.Pos) (map[types.Object]ast.Node, ast.Node, error) {
526527
path := pathEnclosingObjNode(file, pos)
527528
if path == nil {
528-
return nil, ErrNoIdentFound
529+
return nil, nil, ErrNoIdentFound
529530
}
530531

531532
targets := make(map[types.Object]ast.Node)
@@ -542,23 +543,23 @@ func objectsAt(info *types.Info, file *ast.File, pos token.Pos) (map[types.Objec
542543
} else {
543544
obj := info.ObjectOf(leaf)
544545
if obj == nil {
545-
return nil, fmt.Errorf("%w for %q", errNoObjectFound, leaf.Name)
546+
return nil, nil, fmt.Errorf("%w for %q", errNoObjectFound, leaf.Name)
546547
}
547548
targets[obj] = leaf
548549
}
549550
case *ast.ImportSpec:
550551
// Look up the implicit *types.PkgName.
551552
obj := info.Implicits[leaf]
552553
if obj == nil {
553-
return nil, fmt.Errorf("%w for import %s", errNoObjectFound, UnquoteImportPath(leaf))
554+
return nil, nil, fmt.Errorf("%w for import %s", errNoObjectFound, UnquoteImportPath(leaf))
554555
}
555556
targets[obj] = leaf
556557
}
557558

558559
if len(targets) == 0 {
559-
return nil, fmt.Errorf("objectAt: internal error: no targets") // can't happen
560+
return nil, nil, fmt.Errorf("objectAt: internal error: no targets") // can't happen
560561
}
561-
return targets, nil
562+
return targets, path[0], nil
562563
}
563564

564565
// globalReferences reports each cross-package reference to one of the

‎gopls/internal/lsp/source/rename.go

Lines changed: 75 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -59,94 +59,98 @@ func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp prot
5959
defer done()
6060

6161
// Is the cursor within the package name declaration?
62-
pgf, inPackageName, err := parsePackageNameDecl(ctx, snapshot, f, pp)
63-
if err != nil {
62+
if pgf, inPackageName, err := parsePackageNameDecl(ctx, snapshot, f, pp); err != nil {
6463
return nil, err, err
64+
} else if inPackageName {
65+
item, err := prepareRenamePackageName(ctx, snapshot, pgf)
66+
return item, err, err
6567
}
66-
if inPackageName {
67-
// Does the client support file renaming?
68-
fileRenameSupported := false
69-
for _, op := range snapshot.View().Options().SupportedResourceOperations {
70-
if op == protocol.Rename {
71-
fileRenameSupported = true
72-
break
73-
}
74-
}
75-
if !fileRenameSupported {
76-
err := errors.New("can't rename package: LSP client does not support file renaming")
77-
return nil, err, err
78-
}
79-
80-
// Check validity of the metadata for the file's containing package.
81-
fileMeta, err := snapshot.MetadataForFile(ctx, f.URI())
82-
if err != nil {
83-
return nil, err, err
84-
}
85-
86-
if len(fileMeta) == 0 {
87-
err := fmt.Errorf("no packages found for file %q", f.URI())
88-
return nil, err, err
89-
}
90-
meta := fileMeta[0]
91-
if meta.Name == "main" {
92-
err := errors.New("can't rename package \"main\"")
93-
return nil, err, err
94-
}
95-
if strings.HasSuffix(string(meta.Name), "_test") {
96-
err := errors.New("can't rename x_test packages")
97-
return nil, err, err
98-
}
99-
if meta.Module == nil {
100-
err := fmt.Errorf("can't rename package: missing module information for package %q", meta.PkgPath)
101-
return nil, err, err
102-
}
103-
if meta.Module.Path == string(meta.PkgPath) {
104-
err := fmt.Errorf("can't rename package: package path %q is the same as module path %q", meta.PkgPath, meta.Module.Path)
105-
return nil, err, err
106-
}
10768

108-
// Return the location of the package declaration.
109-
rng, err := pgf.NodeRange(pgf.File.Name)
110-
if err != nil {
111-
return nil, err, err
112-
}
113-
return &PrepareItem{
114-
Range: rng,
115-
Text: string(meta.Name),
116-
}, nil, nil
69+
// Ordinary (non-package) renaming.
70+
//
71+
// Type-check the current package, locate the reference at the position,
72+
// validate the object, and report its name and range.
73+
//
74+
// TODO(adonovan): in all cases below, we return usererr=nil,
75+
// which means we return (nil, nil) at the protocol
76+
// layer. This seems like a bug, or at best an exploitation of
77+
// knowledge of VSCode-specific behavior. Can we avoid that?
78+
pkg, pgf, err := PackageForFile(ctx, snapshot, f.URI(), TypecheckFull, NarrowestPackage)
79+
if err != nil {
80+
return nil, nil, err
11781
}
118-
119-
// Prepare for ordinary (non-package) renaming.
120-
121-
qos, err := qualifiedObjsAtProtocolPos(ctx, snapshot, f.URI(), pp)
82+
pos, err := pgf.PositionPos(pp)
83+
if err != nil {
84+
return nil, nil, err
85+
}
86+
targets, node, err := objectsAt(pkg.GetTypesInfo(), pgf.File, pos)
12287
if err != nil {
123-
return nil, nil, err // => suppress error to user
88+
return nil, nil, err
89+
}
90+
var obj types.Object
91+
for obj = range targets {
92+
break // pick one arbitrarily
12493
}
125-
node, obj, pkg := qos[0].node, qos[0].obj, qos[0].sourcePkg
12694
if err := checkRenamable(obj); err != nil {
127-
return nil, nil, err // => suppress error to user
95+
return nil, nil, err
12896
}
129-
130-
result, err := computePrepareRenameResp(ctx, snapshot, pkg, node, obj.Name())
97+
rng, err := pgf.NodeRange(node)
13198
if err != nil {
132-
return nil, nil, err // -> suppress error to user
99+
return nil, nil, err
133100
}
134-
return result, nil, nil
101+
if _, isImport := node.(*ast.ImportSpec); isImport {
102+
// We're not really renaming the import path.
103+
rng.End = rng.Start
104+
}
105+
return &PrepareItem{
106+
Range: rng,
107+
Text: obj.Name(),
108+
}, nil, nil
135109
}
136110

137-
func computePrepareRenameResp(ctx context.Context, snapshot Snapshot, pkg Package, node ast.Node, text string) (*PrepareItem, error) {
138-
mr, err := posToMappedRange(ctx, snapshot, pkg, node.Pos(), node.End())
111+
func prepareRenamePackageName(ctx context.Context, snapshot Snapshot, pgf *ParsedGoFile) (*PrepareItem, error) {
112+
// Does the client support file renaming?
113+
fileRenameSupported := false
114+
for _, op := range snapshot.View().Options().SupportedResourceOperations {
115+
if op == protocol.Rename {
116+
fileRenameSupported = true
117+
break
118+
}
119+
}
120+
if !fileRenameSupported {
121+
return nil, errors.New("can't rename package: LSP client does not support file renaming")
122+
}
123+
124+
// Check validity of the metadata for the file's containing package.
125+
fileMeta, err := snapshot.MetadataForFile(ctx, pgf.URI)
139126
if err != nil {
140127
return nil, err
141128
}
142-
rng := mr.Range()
143-
if _, isImport := node.(*ast.ImportSpec); isImport {
144-
// We're not really renaming the import path.
145-
rng.End = rng.Start
129+
if len(fileMeta) == 0 {
130+
return nil, fmt.Errorf("no packages found for file %q", pgf.URI)
131+
}
132+
meta := fileMeta[0]
133+
if meta.Name == "main" {
134+
return nil, fmt.Errorf("can't rename package \"main\"")
135+
}
136+
if strings.HasSuffix(string(meta.Name), "_test") {
137+
return nil, fmt.Errorf("can't rename x_test packages")
138+
}
139+
if meta.Module == nil {
140+
return nil, fmt.Errorf("can't rename package: missing module information for package %q", meta.PkgPath)
141+
}
142+
if meta.Module.Path == string(meta.PkgPath) {
143+
return nil, fmt.Errorf("can't rename package: package path %q is the same as module path %q", meta.PkgPath, meta.Module.Path)
144+
}
145+
146+
// Return the location of the package declaration.
147+
rng, err := pgf.NodeRange(pgf.File.Name)
148+
if err != nil {
149+
return nil, err
146150
}
147151
return &PrepareItem{
148152
Range: rng,
149-
Text: text,
153+
Text: string(meta.Name),
150154
}, nil
151155
}
152156

@@ -871,13 +875,8 @@ func qualifiedObjsAtProtocolPos(ctx context.Context, s Snapshot, uri span.URI, p
871875
// A qualifiedObject is the result of resolving a reference from an
872876
// identifier to an object.
873877
type qualifiedObject struct {
874-
// definition
875878
obj types.Object // the referenced object
876879
pkg Package // the Package that defines the object (nil => universe)
877-
878-
// reference (optional)
879-
node ast.Node // the reference (*ast.Ident or *ast.ImportSpec) to the object
880-
sourcePkg Package // the Package containing node
881880
}
882881

883882
// A positionKey identifies a byte offset within a file (URI).
@@ -1009,12 +1008,7 @@ func qualifiedObjsAtLocation(ctx context.Context, s Snapshot, key positionKey, s
10091008
event.Error(ctx, fmt.Sprintf("no package for obj %s: %v", obj, obj.Pkg()), err)
10101009
continue
10111010
}
1012-
qualifiedObjs = append(qualifiedObjs, qualifiedObject{
1013-
obj: obj,
1014-
pkg: pkg,
1015-
sourcePkg: searchpkg,
1016-
node: path[0],
1017-
})
1011+
qualifiedObjs = append(qualifiedObjs, qualifiedObject{obj: obj, pkg: pkg})
10181012
seenObjs[obj] = true
10191013

10201014
// If the qualified object is in another file (or more likely, another

0 commit comments

Comments
 (0)