gopls/internal/golang: simplify "rewrite" code actions
Two of these actions used the Inspector for AST traversal,
even though in typical use they inspect only a small selection,
and at most twice, which is not enough to break even.
(This should be a slight performance improvement too.)
Also, unexport various functions.
(More poking around trying to enumerate gopls' feature set...)
Change-Id: Ia08d79329c84c750e525074dad4c0e2e029b9557
Reviewed-on: https://go-review.googlesource.com/c/tools/+/582938
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/analysis/fillstruct/fillstruct.go b/gopls/internal/analysis/fillstruct/fillstruct.go
index fd8d04e..55f2cec 100644
--- a/gopls/internal/analysis/fillstruct/fillstruct.go
+++ b/gopls/internal/analysis/fillstruct/fillstruct.go
@@ -24,7 +24,6 @@
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/ast/astutil"
- "golang.org/x/tools/go/ast/inspector"
"golang.org/x/tools/gopls/internal/util/safetoken"
"golang.org/x/tools/internal/aliases"
"golang.org/x/tools/internal/analysisinternal"
@@ -33,32 +32,35 @@
)
// Diagnose computes diagnostics for fillable struct literals overlapping with
-// the provided start and end position.
+// the provided start and end position of file f.
//
// The diagnostic contains a lazy fix; the actual patch is computed
// (via the ApplyFix command) by a call to [SuggestedFix].
//
-// If either start or end is invalid, the entire package is inspected.
-func Diagnose(inspect *inspector.Inspector, start, end token.Pos, pkg *types.Package, info *types.Info) []analysis.Diagnostic {
+// If either start or end is invalid, the entire file is inspected.
+func Diagnose(f *ast.File, start, end token.Pos, pkg *types.Package, info *types.Info) []analysis.Diagnostic {
var diags []analysis.Diagnostic
- nodeFilter := []ast.Node{(*ast.CompositeLit)(nil)}
- inspect.Preorder(nodeFilter, func(n ast.Node) {
- expr := n.(*ast.CompositeLit)
-
- if (start.IsValid() && expr.End() < start) || (end.IsValid() && expr.Pos() > end) {
- return // non-overlapping
+ ast.Inspect(f, func(n ast.Node) bool {
+ if n == nil {
+ return true // pop
}
-
+ if start.IsValid() && n.End() < start || end.IsValid() && n.Pos() > end {
+ return false // skip non-overlapping subtree
+ }
+ expr, ok := n.(*ast.CompositeLit)
+ if !ok {
+ return true
+ }
typ := info.TypeOf(expr)
if typ == nil {
- return
+ return true
}
// Find reference to the type declaration of the struct being initialized.
typ = typeparams.Deref(typ)
tStruct, ok := typeparams.CoreType(typ).(*types.Struct)
if !ok {
- return
+ return true
}
// Inv: typ is the possibly-named struct type.
@@ -66,7 +68,7 @@
// Skip any struct that is already populated or that has no fields.
if fieldCount == 0 || fieldCount == len(expr.Elts) {
- return
+ return true
}
// Are any fields in need of filling?
@@ -80,7 +82,7 @@
fillableFields = append(fillableFields, fmt.Sprintf("%s: %s", field.Name(), field.Type().String()))
}
if len(fillableFields) == 0 {
- return
+ return true
}
// Derive a name for the struct type.
@@ -116,6 +118,7 @@
// No TextEdits => computed later by gopls.
}},
})
+ return true
})
return diags
diff --git a/gopls/internal/analysis/fillstruct/fillstruct_test.go b/gopls/internal/analysis/fillstruct/fillstruct_test.go
index f90998f..e0ad83d 100644
--- a/gopls/internal/analysis/fillstruct/fillstruct_test.go
+++ b/gopls/internal/analysis/fillstruct/fillstruct_test.go
@@ -10,21 +10,19 @@
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/analysistest"
- "golang.org/x/tools/go/analysis/passes/inspect"
- "golang.org/x/tools/go/ast/inspector"
"golang.org/x/tools/gopls/internal/analysis/fillstruct"
)
// analyzer allows us to test the fillstruct code action using the analysistest
// harness. (fillstruct used to be a gopls analyzer.)
var analyzer = &analysis.Analyzer{
- Name: "fillstruct",
- Doc: "test only",
- Requires: []*analysis.Analyzer{inspect.Analyzer},
+ Name: "fillstruct",
+ Doc: "test only",
Run: func(pass *analysis.Pass) (any, error) {
- inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
- for _, d := range fillstruct.Diagnose(inspect, token.NoPos, token.NoPos, pass.Pkg, pass.TypesInfo) {
- pass.Report(d)
+ for _, f := range pass.Files {
+ for _, diag := range fillstruct.Diagnose(f, token.NoPos, token.NoPos, pass.Pkg, pass.TypesInfo) {
+ pass.Report(diag)
+ }
}
return nil, nil
},
diff --git a/gopls/internal/analysis/fillswitch/fillswitch.go b/gopls/internal/analysis/fillswitch/fillswitch.go
index b93ade0..12f116e 100644
--- a/gopls/internal/analysis/fillswitch/fillswitch.go
+++ b/gopls/internal/analysis/fillswitch/fillswitch.go
@@ -12,22 +12,22 @@
"go/types"
"golang.org/x/tools/go/analysis"
- "golang.org/x/tools/go/ast/inspector"
)
// Diagnose computes diagnostics for switch statements with missing cases
-// overlapping with the provided start and end position.
+// overlapping with the provided start and end position of file f.
//
-// If either start or end is invalid, the entire package is inspected.
-func Diagnose(inspect *inspector.Inspector, start, end token.Pos, pkg *types.Package, info *types.Info) []analysis.Diagnostic {
+// If either start or end is invalid, the entire file is inspected.
+func Diagnose(f *ast.File, start, end token.Pos, pkg *types.Package, info *types.Info) []analysis.Diagnostic {
var diags []analysis.Diagnostic
- nodeFilter := []ast.Node{(*ast.SwitchStmt)(nil), (*ast.TypeSwitchStmt)(nil)}
- inspect.Preorder(nodeFilter, func(n ast.Node) {
+ ast.Inspect(f, func(n ast.Node) bool {
+ if n == nil {
+ return true // pop
+ }
if start.IsValid() && n.End() < start ||
end.IsValid() && n.Pos() > end {
- return // non-overlapping
+ return false // skip non-overlapping subtree
}
-
var fix *analysis.SuggestedFix
switch n := n.(type) {
case *ast.SwitchStmt:
@@ -35,17 +35,15 @@
case *ast.TypeSwitchStmt:
fix = suggestedFixTypeSwitch(n, pkg, info)
}
-
- if fix == nil {
- return
+ if fix != nil {
+ diags = append(diags, analysis.Diagnostic{
+ Message: fix.Message,
+ Pos: n.Pos(),
+ End: n.Pos() + token.Pos(len("switch")),
+ SuggestedFixes: []analysis.SuggestedFix{*fix},
+ })
}
-
- diags = append(diags, analysis.Diagnostic{
- Message: fix.Message,
- Pos: n.Pos(),
- End: n.Pos() + token.Pos(len("switch")),
- SuggestedFixes: []analysis.SuggestedFix{*fix},
- })
+ return true
})
return diags
diff --git a/gopls/internal/analysis/fillswitch/fillswitch_test.go b/gopls/internal/analysis/fillswitch/fillswitch_test.go
index 15d3ef1..bf70aa3 100644
--- a/gopls/internal/analysis/fillswitch/fillswitch_test.go
+++ b/gopls/internal/analysis/fillswitch/fillswitch_test.go
@@ -10,21 +10,19 @@
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/analysistest"
- "golang.org/x/tools/go/analysis/passes/inspect"
- "golang.org/x/tools/go/ast/inspector"
"golang.org/x/tools/gopls/internal/analysis/fillswitch"
)
// analyzer allows us to test the fillswitch code action using the analysistest
// harness.
var analyzer = &analysis.Analyzer{
- Name: "fillswitch",
- Doc: "test only",
- Requires: []*analysis.Analyzer{inspect.Analyzer},
+ Name: "fillswitch",
+ Doc: "test only",
Run: func(pass *analysis.Pass) (any, error) {
- inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
- for _, d := range fillswitch.Diagnose(inspect, token.NoPos, token.NoPos, pass.Pkg, pass.TypesInfo) {
- pass.Report(d)
+ for _, f := range pass.Files {
+ for _, diag := range fillswitch.Diagnose(f, token.NoPos, token.NoPos, pass.Pkg, pass.TypesInfo) {
+ pass.Report(diag)
+ }
}
return nil, nil
},
diff --git a/gopls/internal/golang/change_quote.go b/gopls/internal/golang/change_quote.go
index 919b935..761a836 100644
--- a/gopls/internal/golang/change_quote.go
+++ b/gopls/internal/golang/change_quote.go
@@ -19,17 +19,13 @@
"golang.org/x/tools/internal/diff"
)
-// ConvertStringLiteral reports whether we can convert between raw and interpreted
-// string literals in the [start, end), along with a CodeAction containing the edits.
+// convertStringLiteral reports whether we can convert between raw and interpreted
+// string literals in the [start, end) range, along with a CodeAction containing the edits.
//
// Only the following conditions are true, the action in result is valid
// - [start, end) is enclosed by a string literal
// - if the string is interpreted string, need check whether the convert is allowed
-func ConvertStringLiteral(pgf *parsego.File, fh file.Handle, rng protocol.Range) (protocol.CodeAction, bool) {
- startPos, endPos, err := pgf.RangePos(rng)
- if err != nil {
- return protocol.CodeAction{}, false // e.g. invalid range
- }
+func convertStringLiteral(pgf *parsego.File, fh file.Handle, startPos, endPos token.Pos) (protocol.CodeAction, bool) {
path, _ := astutil.PathEnclosingInterval(pgf.File, startPos, endPos)
lit, ok := path[0].(*ast.BasicLit)
if !ok || lit.Kind != token.STRING {
diff --git a/gopls/internal/golang/codeaction.go b/gopls/internal/golang/codeaction.go
index eb4e28b..29ae363 100644
--- a/gopls/internal/golang/codeaction.go
+++ b/gopls/internal/golang/codeaction.go
@@ -11,7 +11,6 @@
"go/ast"
"strings"
- "golang.org/x/tools/go/ast/inspector"
"golang.org/x/tools/gopls/internal/analysis/fillstruct"
"golang.org/x/tools/gopls/internal/analysis/fillswitch"
"golang.org/x/tools/gopls/internal/cache"
@@ -299,17 +298,17 @@
actions = append(actions, newCodeAction("Refactor: remove unused parameter", protocol.RefactorRewrite, &cmd, nil, options))
}
- if action, ok := ConvertStringLiteral(pgf, fh, rng); ok {
- actions = append(actions, action)
- }
-
start, end, err := pgf.RangePos(rng)
if err != nil {
return nil, err
}
+ if action, ok := convertStringLiteral(pgf, fh, start, end); ok {
+ actions = append(actions, action)
+ }
+
var commands []protocol.Command
- if _, ok, _ := CanInvertIfCondition(pgf.File, start, end); ok {
+ if _, ok, _ := canInvertIfCondition(pgf.File, start, end); ok {
cmd, err := command.NewApplyFixCommand("Invert 'if' condition", command.ApplyFixArgs{
Fix: fixInvertIfCondition,
URI: pgf.URI,
@@ -322,7 +321,7 @@
commands = append(commands, cmd)
}
- if msg, ok, _ := CanSplitLines(pgf.File, pkg.FileSet(), start, end); ok {
+ if msg, ok, _ := canSplitLines(pgf.File, pkg.FileSet(), start, end); ok {
cmd, err := command.NewApplyFixCommand(msg, command.ApplyFixArgs{
Fix: fixSplitLines,
URI: pgf.URI,
@@ -335,7 +334,7 @@
commands = append(commands, cmd)
}
- if msg, ok, _ := CanJoinLines(pgf.File, pkg.FileSet(), start, end); ok {
+ if msg, ok, _ := canJoinLines(pgf.File, pkg.FileSet(), start, end); ok {
cmd, err := command.NewApplyFixCommand(msg, command.ApplyFixArgs{
Fix: fixJoinLines,
URI: pgf.URI,
@@ -348,12 +347,10 @@
commands = append(commands, cmd)
}
- // N.B.: an inspector only pays for itself after ~5 passes, which means we're
- // currently not getting a good deal on this inspection.
- //
- // TODO: Consider removing the inspection after convenienceAnalyzers are removed.
- inspect := inspector.New([]*ast.File{pgf.File})
- for _, diag := range fillstruct.Diagnose(inspect, start, end, pkg.Types(), pkg.TypesInfo()) {
+ // fillstruct.Diagnose is a lazy analyzer: all it gives us is
+ // the (start, end, message) of each SuggestedFix; the actual
+ // edit is computed only later by ApplyFix, which calls fillstruct.SuggestedFix.
+ for _, diag := range fillstruct.Diagnose(pgf.File, start, end, pkg.Types(), pkg.TypesInfo()) {
rng, err := pgf.Mapper.PosRange(pgf.Tok, diag.Pos, diag.End)
if err != nil {
return nil, err
@@ -372,7 +369,7 @@
}
}
- for _, diag := range fillswitch.Diagnose(inspect, start, end, pkg.Types(), pkg.TypesInfo()) {
+ for _, diag := range fillswitch.Diagnose(pgf.File, start, end, pkg.Types(), pkg.TypesInfo()) {
edits, err := suggestedFixToEdits(ctx, snapshot, pkg.FileSet(), &diag.SuggestedFixes[0])
if err != nil {
return nil, err
diff --git a/gopls/internal/golang/invertifcondition.go b/gopls/internal/golang/invertifcondition.go
index 377e1ce..16eaaa3 100644
--- a/gopls/internal/golang/invertifcondition.go
+++ b/gopls/internal/golang/invertifcondition.go
@@ -18,7 +18,7 @@
// invertIfCondition is a singleFileFixFunc that inverts an if/else statement
func invertIfCondition(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, _ *types.Package, _ *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
- ifStatement, _, err := CanInvertIfCondition(file, start, end)
+ ifStatement, _, err := canInvertIfCondition(file, start, end)
if err != nil {
return nil, nil, err
}
@@ -239,9 +239,9 @@
return []byte(string(invertedBefore) + string(whitespaceAfterBefore) + newOpWithTrailingWhitespace + string(invertedAfter)), nil
}
-// CanInvertIfCondition reports whether we can do invert-if-condition on the
+// canInvertIfCondition reports whether we can do invert-if-condition on the
// code in the given range
-func CanInvertIfCondition(file *ast.File, start, end token.Pos) (*ast.IfStmt, bool, error) {
+func canInvertIfCondition(file *ast.File, start, end token.Pos) (*ast.IfStmt, bool, error) {
path, _ := astutil.PathEnclosingInterval(file, start, end)
for _, node := range path {
stmt, isIfStatement := node.(*ast.IfStmt)
diff --git a/gopls/internal/golang/lines.go b/gopls/internal/golang/lines.go
index 1c4b562..761fee9 100644
--- a/gopls/internal/golang/lines.go
+++ b/gopls/internal/golang/lines.go
@@ -22,9 +22,9 @@
"golang.org/x/tools/gopls/internal/util/slices"
)
-// CanSplitLines checks whether we can split lists of elements inside an enclosing curly bracket/parens into separate
-// lines.
-func CanSplitLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (string, bool, error) {
+// canSplitLines checks whether we can split lists of elements inside
+// an enclosing curly bracket/parens into separate lines.
+func canSplitLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (string, bool, error) {
itemType, items, comments, _, _, _ := findSplitJoinTarget(fset, file, nil, start, end)
if itemType == "" {
return "", false, nil
@@ -45,8 +45,9 @@
return "", false, nil
}
-// CanJoinLines checks whether we can join lists of elements inside an enclosing curly bracket/parens into a single line.
-func CanJoinLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (string, bool, error) {
+// canJoinLines checks whether we can join lists of elements inside an
+// enclosing curly bracket/parens into a single line.
+func canJoinLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (string, bool, error) {
itemType, items, comments, _, _, _ := findSplitJoinTarget(fset, file, nil, start, end)
if itemType == "" {
return "", false, nil
