Skip to content

Commit 4991e7d

Browse files
adonovangopherbot
authored andcommitted
gopls/internal/golang: use pgf.Cursor in CodeAction fix
This CL pushes down the pgf.Cursor into internal interfaces so that it is avaiable where needed. The existing implementations have not been updated to use it. As a rule of thumb, any place that calls PathEnclosingInterval would be better off using Cursor; the exception is when there's a public API that accepts a 'path []Node'. Change-Id: I18313809808fa83cc1f2a1d51850a9fdcf43ecdb Reviewed-on: https://go-review.googlesource.com/c/tools/+/650398 LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Alan Donovan <[email protected]> Reviewed-by: Robert Findley <[email protected]> Commit-Queue: Alan Donovan <[email protected]>
1 parent 877c1d1 commit 4991e7d

File tree

11 files changed

+84
-52
lines changed

11 files changed

+84
-52
lines changed

gopls/internal/analysis/fillstruct/fillstruct.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
"golang.org/x/tools/gopls/internal/fuzzy"
2929
"golang.org/x/tools/gopls/internal/util/safetoken"
3030
"golang.org/x/tools/internal/analysisinternal"
31+
"golang.org/x/tools/internal/astutil/cursor"
3132
"golang.org/x/tools/internal/typeparams"
3233
"golang.org/x/tools/internal/typesinternal"
3334
)
@@ -129,15 +130,15 @@ const FixCategory = "fillstruct" // recognized by gopls ApplyFix
129130

130131
// SuggestedFix computes the suggested fix for the kinds of
131132
// diagnostics produced by the Analyzer above.
132-
func SuggestedFix(fset *token.FileSet, start, end token.Pos, content []byte, file *ast.File, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
133+
func SuggestedFix(fset *token.FileSet, start, end token.Pos, content []byte, curFile cursor.Cursor, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
133134
if info == nil {
134135
return nil, nil, fmt.Errorf("nil types.Info")
135136
}
136137

137138
pos := start // don't use the end
138139

139-
// TODO(rstambler): Using ast.Inspect would probably be more efficient than
140-
// calling PathEnclosingInterval. Switch this approach.
140+
// TODO(adonovan): simplify, using Cursor.
141+
file := curFile.Node().(*ast.File)
141142
path, _ := astutil.PathEnclosingInterval(file, pos, pos)
142143
if len(path) == 0 {
143144
return nil, nil, fmt.Errorf("no enclosing ast.Node")

gopls/internal/golang/codeaction.go

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -325,8 +325,7 @@ func quickFix(ctx context.Context, req *codeActionsRequest) error {
325325
case strings.Contains(msg, "missing method"),
326326
strings.HasPrefix(msg, "cannot convert"),
327327
strings.Contains(msg, "not implement"):
328-
path, _ := astutil.PathEnclosingInterval(req.pgf.File, start, end)
329-
si := stubmethods.GetIfaceStubInfo(req.pkg.FileSet(), info, path, start)
328+
si := stubmethods.GetIfaceStubInfo(req.pkg.FileSet(), info, req.pgf, start, end)
330329
if si != nil {
331330
qual := typesinternal.FileQualifier(req.pgf.File, si.Concrete.Obj().Pkg())
332331
iface := types.TypeString(si.Interface.Type(), qual)
@@ -338,8 +337,7 @@ func quickFix(ctx context.Context, req *codeActionsRequest) error {
338337
// Offer a "Declare missing method T.f" code action.
339338
// See [stubMissingCalledFunctionFixer] for command implementation.
340339
case strings.Contains(msg, "has no field or method"):
341-
path, _ := astutil.PathEnclosingInterval(req.pgf.File, start, end)
342-
si := stubmethods.GetCallStubInfo(req.pkg.FileSet(), info, path, start)
340+
si := stubmethods.GetCallStubInfo(req.pkg.FileSet(), info, req.pgf, start, end)
343341
if si != nil {
344342
msg := fmt.Sprintf("Declare missing method %s.%s", si.Receiver.Obj().Name(), si.MethodName)
345343
req.addApplyFixAction(msg, fixMissingCalledFunction, req.loc)
@@ -462,7 +460,7 @@ func goDoc(ctx context.Context, req *codeActionsRequest) error {
462460
// refactorExtractFunction produces "Extract function" code actions.
463461
// See [extractFunction] for command implementation.
464462
func refactorExtractFunction(ctx context.Context, req *codeActionsRequest) error {
465-
if _, ok, _, _ := canExtractFunction(req.pgf.Tok, req.start, req.end, req.pgf.Src, req.pgf.File); ok {
463+
if _, ok, _, _ := canExtractFunction(req.pgf.Tok, req.start, req.end, req.pgf.Src, req.pgf.Cursor); ok {
466464
req.addApplyFixAction("Extract function", fixExtractFunction, req.loc)
467465
}
468466
return nil
@@ -471,7 +469,7 @@ func refactorExtractFunction(ctx context.Context, req *codeActionsRequest) error
471469
// refactorExtractMethod produces "Extract method" code actions.
472470
// See [extractMethod] for command implementation.
473471
func refactorExtractMethod(ctx context.Context, req *codeActionsRequest) error {
474-
if _, ok, methodOK, _ := canExtractFunction(req.pgf.Tok, req.start, req.end, req.pgf.Src, req.pgf.File); ok && methodOK {
472+
if _, ok, methodOK, _ := canExtractFunction(req.pgf.Tok, req.start, req.end, req.pgf.Src, req.pgf.Cursor); ok && methodOK {
475473
req.addApplyFixAction("Extract method", fixExtractMethod, req.loc)
476474
}
477475
return nil
@@ -481,7 +479,7 @@ func refactorExtractMethod(ctx context.Context, req *codeActionsRequest) error {
481479
// See [extractVariable] for command implementation.
482480
func refactorExtractVariable(ctx context.Context, req *codeActionsRequest) error {
483481
info := req.pkg.TypesInfo()
484-
if exprs, err := canExtractVariable(info, req.pgf.File, req.start, req.end, false); err == nil {
482+
if exprs, err := canExtractVariable(info, req.pgf.Cursor, req.start, req.end, false); err == nil {
485483
// Offer one of refactor.extract.{constant,variable}
486484
// based on the constness of the expression; this is a
487485
// limitation of the codeActionProducers mechanism.
@@ -507,7 +505,7 @@ func refactorExtractVariableAll(ctx context.Context, req *codeActionsRequest) er
507505
info := req.pkg.TypesInfo()
508506
// Don't suggest if only one expr is found,
509507
// otherwise it will duplicate with [refactorExtractVariable]
510-
if exprs, err := canExtractVariable(info, req.pgf.File, req.start, req.end, true); err == nil && len(exprs) > 1 {
508+
if exprs, err := canExtractVariable(info, req.pgf.Cursor, req.start, req.end, true); err == nil && len(exprs) > 1 {
511509
start, end, err := req.pgf.NodeOffsets(exprs[0])
512510
if err != nil {
513511
return err
@@ -664,7 +662,7 @@ func refactorRewriteChangeQuote(ctx context.Context, req *codeActionsRequest) er
664662
// refactorRewriteInvertIf produces "Invert 'if' condition" code actions.
665663
// See [invertIfCondition] for command implementation.
666664
func refactorRewriteInvertIf(ctx context.Context, req *codeActionsRequest) error {
667-
if _, ok, _ := canInvertIfCondition(req.pgf.File, req.start, req.end); ok {
665+
if _, ok, _ := canInvertIfCondition(req.pgf.Cursor, req.start, req.end); ok {
668666
req.addApplyFixAction("Invert 'if' condition", fixInvertIfCondition, req.loc)
669667
}
670668
return nil
@@ -674,7 +672,7 @@ func refactorRewriteInvertIf(ctx context.Context, req *codeActionsRequest) error
674672
// See [splitLines] for command implementation.
675673
func refactorRewriteSplitLines(ctx context.Context, req *codeActionsRequest) error {
676674
// TODO(adonovan): opt: don't set needPkg just for FileSet.
677-
if msg, ok, _ := canSplitLines(req.pgf.File, req.pkg.FileSet(), req.start, req.end); ok {
675+
if msg, ok, _ := canSplitLines(req.pgf.Cursor, req.pkg.FileSet(), req.start, req.end); ok {
678676
req.addApplyFixAction(msg, fixSplitLines, req.loc)
679677
}
680678
return nil
@@ -684,7 +682,7 @@ func refactorRewriteSplitLines(ctx context.Context, req *codeActionsRequest) err
684682
// See [joinLines] for command implementation.
685683
func refactorRewriteJoinLines(ctx context.Context, req *codeActionsRequest) error {
686684
// TODO(adonovan): opt: don't set needPkg just for FileSet.
687-
if msg, ok, _ := canJoinLines(req.pgf.File, req.pkg.FileSet(), req.start, req.end); ok {
685+
if msg, ok, _ := canJoinLines(req.pgf.Cursor, req.pkg.FileSet(), req.start, req.end); ok {
688686
req.addApplyFixAction(msg, fixJoinLines, req.loc)
689687
}
690688
return nil

gopls/internal/golang/extract.go

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -24,17 +24,18 @@ import (
2424
"golang.org/x/tools/gopls/internal/util/bug"
2525
"golang.org/x/tools/gopls/internal/util/safetoken"
2626
"golang.org/x/tools/internal/analysisinternal"
27+
"golang.org/x/tools/internal/astutil/cursor"
2728
"golang.org/x/tools/internal/typesinternal"
2829
)
2930

3031
// extractVariable implements the refactor.extract.{variable,constant} CodeAction command.
31-
func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, _ *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
32-
return extractExprs(fset, start, end, src, file, info, false)
32+
func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, curFile cursor.Cursor, _ *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
33+
return extractExprs(fset, start, end, src, curFile, info, false)
3334
}
3435

3536
// extractVariableAll implements the refactor.extract.{variable,constant}-all CodeAction command.
36-
func extractVariableAll(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, _ *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
37-
return extractExprs(fset, start, end, src, file, info, true)
37+
func extractVariableAll(fset *token.FileSet, start, end token.Pos, src []byte, curFile cursor.Cursor, _ *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
38+
return extractExprs(fset, start, end, src, curFile, info, true)
3839
}
3940

4041
// extractExprs replaces occurrence(s) of a specified expression within the same function
@@ -43,9 +44,11 @@ func extractVariableAll(fset *token.FileSet, start, end token.Pos, src []byte, f
4344
//
4445
// The new variable/constant is declared as close as possible to the first found expression
4546
// within the deepest common scope accessible to all candidate occurrences.
46-
func extractExprs(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, info *types.Info, all bool) (*token.FileSet, *analysis.SuggestedFix, error) {
47+
func extractExprs(fset *token.FileSet, start, end token.Pos, src []byte, curFile cursor.Cursor, info *types.Info, all bool) (*token.FileSet, *analysis.SuggestedFix, error) {
48+
file := curFile.Node().(*ast.File)
49+
// TODO(adonovan): simplify, using Cursor.
4750
tokFile := fset.File(file.FileStart)
48-
exprs, err := canExtractVariable(info, file, start, end, all)
51+
exprs, err := canExtractVariable(info, curFile, start, end, all)
4952
if err != nil {
5053
return nil, nil, fmt.Errorf("cannot extract: %v", err)
5154
}
@@ -381,10 +384,12 @@ func stmtToInsertVarBefore(path []ast.Node, variables []*variable) (ast.Stmt, er
381384
// canExtractVariable reports whether the code in the given range can be
382385
// extracted to a variable (or constant). It returns the selected expression or, if 'all',
383386
// all structurally equivalent expressions within the same function body, in lexical order.
384-
func canExtractVariable(info *types.Info, file *ast.File, start, end token.Pos, all bool) ([]ast.Expr, error) {
387+
func canExtractVariable(info *types.Info, curFile cursor.Cursor, start, end token.Pos, all bool) ([]ast.Expr, error) {
385388
if start == end {
386389
return nil, fmt.Errorf("empty selection")
387390
}
391+
file := curFile.Node().(*ast.File)
392+
// TODO(adonovan): simplify, using Cursor.
388393
path, exact := astutil.PathEnclosingInterval(file, start, end)
389394
if !exact {
390395
return nil, fmt.Errorf("selection is not an expression")
@@ -571,13 +576,13 @@ type returnVariable struct {
571576
}
572577

573578
// extractMethod refactors the selected block of code into a new method.
574-
func extractMethod(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
575-
return extractFunctionMethod(fset, start, end, src, file, pkg, info, true)
579+
func extractMethod(fset *token.FileSet, start, end token.Pos, src []byte, curFile cursor.Cursor, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
580+
return extractFunctionMethod(fset, start, end, src, curFile, pkg, info, true)
576581
}
577582

578583
// extractFunction refactors the selected block of code into a new function.
579-
func extractFunction(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
580-
return extractFunctionMethod(fset, start, end, src, file, pkg, info, false)
584+
func extractFunction(fset *token.FileSet, start, end token.Pos, src []byte, curFile cursor.Cursor, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
585+
return extractFunctionMethod(fset, start, end, src, curFile, pkg, info, false)
581586
}
582587

583588
// extractFunctionMethod refactors the selected block of code into a new function/method.
@@ -588,17 +593,19 @@ func extractFunction(fset *token.FileSet, start, end token.Pos, src []byte, file
588593
// and return values of the extracted function/method. Lastly, we construct the call
589594
// of the function/method and insert this call as well as the extracted function/method into
590595
// their proper locations.
591-
func extractFunctionMethod(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, pkg *types.Package, info *types.Info, isMethod bool) (*token.FileSet, *analysis.SuggestedFix, error) {
596+
func extractFunctionMethod(fset *token.FileSet, start, end token.Pos, src []byte, curFile cursor.Cursor, pkg *types.Package, info *types.Info, isMethod bool) (*token.FileSet, *analysis.SuggestedFix, error) {
592597
errorPrefix := "extractFunction"
593598
if isMethod {
594599
errorPrefix = "extractMethod"
595600
}
596601

602+
file := curFile.Node().(*ast.File)
603+
// TODO(adonovan): simplify, using Cursor.
597604
tok := fset.File(file.FileStart)
598605
if tok == nil {
599606
return nil, nil, bug.Errorf("no file for position")
600607
}
601-
p, ok, methodOk, err := canExtractFunction(tok, start, end, src, file)
608+
p, ok, methodOk, err := canExtractFunction(tok, start, end, src, curFile)
602609
if (!ok && !isMethod) || (!methodOk && isMethod) {
603610
return nil, nil, fmt.Errorf("%s: cannot extract %s: %v", errorPrefix,
604611
safetoken.StartPosition(fset, start), err)
@@ -1086,7 +1093,10 @@ func moveParamToFrontIfFound(params []ast.Expr, paramTypes []*ast.Field, x, sel
10861093
// their cursors for whitespace. To support this use case, we must manually adjust the
10871094
// ranges to match the correct AST node. In this particular example, we would adjust
10881095
// rng.Start forward to the start of 'if' and rng.End backward to after '}'.
1089-
func adjustRangeForCommentsAndWhiteSpace(tok *token.File, start, end token.Pos, content []byte, file *ast.File) (token.Pos, token.Pos, error) {
1096+
func adjustRangeForCommentsAndWhiteSpace(tok *token.File, start, end token.Pos, content []byte, curFile cursor.Cursor) (token.Pos, token.Pos, error) {
1097+
file := curFile.Node().(*ast.File)
1098+
// TODO(adonovan): simplify, using Cursor.
1099+
10901100
// Adjust the end of the range to after leading whitespace and comments.
10911101
prevStart := token.NoPos
10921102
startComment := sort.Search(len(file.Comments), func(i int) bool {
@@ -1410,12 +1420,14 @@ type fnExtractParams struct {
14101420

14111421
// canExtractFunction reports whether the code in the given range can be
14121422
// extracted to a function.
1413-
func canExtractFunction(tok *token.File, start, end token.Pos, src []byte, file *ast.File) (*fnExtractParams, bool, bool, error) {
1423+
func canExtractFunction(tok *token.File, start, end token.Pos, src []byte, curFile cursor.Cursor) (*fnExtractParams, bool, bool, error) {
14141424
if start == end {
14151425
return nil, false, false, fmt.Errorf("start and end are equal")
14161426
}
14171427
var err error
1418-
start, end, err = adjustRangeForCommentsAndWhiteSpace(tok, start, end, src, file)
1428+
file := curFile.Node().(*ast.File)
1429+
// TODO(adonovan): simplify, using Cursor.
1430+
start, end, err = adjustRangeForCommentsAndWhiteSpace(tok, start, end, src, curFile)
14191431
if err != nil {
14201432
return nil, false, false, err
14211433
}

gopls/internal/golang/fix.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ package golang
77
import (
88
"context"
99
"fmt"
10-
"go/ast"
1110
"go/token"
1211
"go/types"
1312

@@ -20,6 +19,7 @@ import (
2019
"golang.org/x/tools/gopls/internal/file"
2120
"golang.org/x/tools/gopls/internal/protocol"
2221
"golang.org/x/tools/gopls/internal/util/bug"
22+
"golang.org/x/tools/internal/astutil/cursor"
2323
"golang.org/x/tools/internal/imports"
2424
)
2525

@@ -47,12 +47,12 @@ type fixer func(ctx context.Context, s *cache.Snapshot, pkg *cache.Package, pgf
4747
// TODO(adonovan): move fillstruct and undeclaredname into this
4848
// package, so we can remove the import restriction and push
4949
// the singleFile wrapper down into each singleFileFixer?
50-
type singleFileFixer func(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error)
50+
type singleFileFixer func(fset *token.FileSet, start, end token.Pos, src []byte, curFile cursor.Cursor, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error)
5151

5252
// singleFile adapts a single-file fixer to a Fixer.
5353
func singleFile(fixer1 singleFileFixer) fixer {
5454
return func(ctx context.Context, snapshot *cache.Snapshot, pkg *cache.Package, pgf *parsego.File, start, end token.Pos) (*token.FileSet, *analysis.SuggestedFix, error) {
55-
return fixer1(pkg.FileSet(), start, end, pgf.Src, pgf.File, pkg.Types(), pkg.TypesInfo())
55+
return fixer1(pkg.FileSet(), start, end, pgf.Src, pgf.Cursor, pkg.Types(), pkg.TypesInfo())
5656
}
5757
}
5858

gopls/internal/golang/invertifcondition.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,12 @@ import (
1414
"golang.org/x/tools/go/analysis"
1515
"golang.org/x/tools/go/ast/astutil"
1616
"golang.org/x/tools/gopls/internal/util/safetoken"
17+
"golang.org/x/tools/internal/astutil/cursor"
1718
)
1819

1920
// invertIfCondition is a singleFileFixFunc that inverts an if/else statement
20-
func invertIfCondition(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, _ *types.Package, _ *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
21-
ifStatement, _, err := canInvertIfCondition(file, start, end)
21+
func invertIfCondition(fset *token.FileSet, start, end token.Pos, src []byte, curFile cursor.Cursor, _ *types.Package, _ *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
22+
ifStatement, _, err := canInvertIfCondition(curFile, start, end)
2223
if err != nil {
2324
return nil, nil, err
2425
}
@@ -241,7 +242,9 @@ func invertAndOr(fset *token.FileSet, expr *ast.BinaryExpr, src []byte) ([]byte,
241242

242243
// canInvertIfCondition reports whether we can do invert-if-condition on the
243244
// code in the given range.
244-
func canInvertIfCondition(file *ast.File, start, end token.Pos) (*ast.IfStmt, bool, error) {
245+
func canInvertIfCondition(curFile cursor.Cursor, start, end token.Pos) (*ast.IfStmt, bool, error) {
246+
file := curFile.Node().(*ast.File)
247+
// TODO(adonovan): simplify, using Cursor.
245248
path, _ := astutil.PathEnclosingInterval(file, start, end)
246249
for _, node := range path {
247250
stmt, isIfStatement := node.(*ast.IfStmt)

0 commit comments

Comments
 (0)