From ca461f0c9ced621d2747c34046cdc5d9cab21d98 Mon Sep 17 00:00:00 2001 From: Fata Nugraha Date: Thu, 25 Jan 2024 21:02:51 +0800 Subject: [PATCH 01/10] Implement line group/split Address comments Add comment Add comment Simplify comment --- gopls/internal/golang/codeaction.go | 26 +++ gopls/internal/golang/fix.go | 4 + gopls/internal/golang/lines.go | 221 ++++++++++++++++++ .../marker/testdata/codeaction/grouplines.txt | 187 +++++++++++++++ .../testdata/codeaction/removeparam.txt | 8 +- .../marker/testdata/codeaction/splitlines.txt | 204 ++++++++++++++++ 6 files changed, 646 insertions(+), 4 deletions(-) create mode 100644 gopls/internal/golang/lines.go create mode 100644 gopls/internal/test/marker/testdata/codeaction/grouplines.txt create mode 100644 gopls/internal/test/marker/testdata/codeaction/splitlines.txt diff --git a/gopls/internal/golang/codeaction.go b/gopls/internal/golang/codeaction.go index a9aae821be8..fbcba0ebe28 100644 --- a/gopls/internal/golang/codeaction.go +++ b/gopls/internal/golang/codeaction.go @@ -304,6 +304,32 @@ func getRewriteCodeActions(pkg *cache.Package, pgf *ParsedGoFile, fh file.Handle commands = append(commands, cmd) } + if msg, ok, _ := CanSplitLines(pgf.File, pkg.FileSet(), start, end); ok { + cmd, err := command.NewApplyFixCommand(msg, command.ApplyFixArgs{ + Fix: fixSplitLines, + URI: pgf.URI, + Range: rng, + ResolveEdits: supportsResolveEdits(options), + }) + if err != nil { + return nil, err + } + commands = append(commands, cmd) + } + + if msg, ok, _ := CanGroupLines(pgf.File, pkg.FileSet(), start, end); ok { + cmd, err := command.NewApplyFixCommand(msg, command.ApplyFixArgs{ + Fix: fixGroupLines, + URI: pgf.URI, + Range: rng, + ResolveEdits: supportsResolveEdits(options), + }) + if err != nil { + return nil, err + } + 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. // diff --git a/gopls/internal/golang/fix.go b/gopls/internal/golang/fix.go index 6f07cb869c5..76baf57b08f 100644 --- a/gopls/internal/golang/fix.go +++ b/gopls/internal/golang/fix.go @@ -64,6 +64,8 @@ const ( fixExtractMethod = "extract_method" fixInlineCall = "inline_call" fixInvertIfCondition = "invert_if_condition" + fixSplitLines = "split_lines" + fixGroupLines = "group_lines" ) // ApplyFix applies the specified kind of suggested fix to the given @@ -115,6 +117,8 @@ func ApplyFix(ctx context.Context, fix string, snapshot *cache.Snapshot, fh file fixExtractVariable: singleFile(extractVariable), fixInlineCall: inlineCall, fixInvertIfCondition: singleFile(invertIfCondition), + fixSplitLines: singleFile(splitLines), + fixGroupLines: singleFile(groupLines), } fixer, ok := fixers[fix] if !ok { diff --git a/gopls/internal/golang/lines.go b/gopls/internal/golang/lines.go new file mode 100644 index 00000000000..a27ecd84c09 --- /dev/null +++ b/gopls/internal/golang/lines.go @@ -0,0 +1,221 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package golang + +import ( + "bytes" + "go/ast" + "go/token" + "go/types" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/ast/astutil" + "golang.org/x/tools/gopls/internal/util/safetoken" +) + +// CanSplitLines checks whether each item of the enclosing curly bracket/parens can be put into separate lines +// where each item occupies one line. +func CanSplitLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (string, bool, error) { + msg, lines, numElts, target := findSplitGroupTarget(file, fset, start, end) + if target == nil { + return "", false, nil + } + + // minus two to discount the parens/brackets. + if lines-2 == numElts { + return "", false, nil + } + + return "Split " + msg + " into separate lines", true, nil +} + +// CanGroupLines checks whether each item of the enclosing curly bracket/parens can be joined into a single line. +func CanGroupLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (string, bool, error) { + msg, lines, _, target := findSplitGroupTarget(file, fset, start, end) + if target == nil { + return "", false, nil + } + + if lines == 1 { + return "", false, nil + } + + return "Group " + msg + " into one line", true, nil +} + +func splitLines( + fset *token.FileSet, + start token.Pos, + end token.Pos, + src []byte, + file *ast.File, + _ *types.Package, + _ *types.Info, +) (*token.FileSet, *analysis.SuggestedFix, error) { + _, _, _, target := findSplitGroupTarget(file, fset, start, end) + if target == nil { + return fset, &analysis.SuggestedFix{}, nil + } + + // get the original line indent of target. + firstLineIndent := getBraceIndent(src, fset, target) + eltIndent := firstLineIndent + "\t" + + return fset, processLines(fset, target, src, file, ",\n", "\n", ",\n"+firstLineIndent, eltIndent), nil +} + +// getBraceIndent returns the line indent of the opening curly bracket/paren. +func getBraceIndent(src []byte, fset *token.FileSet, target ast.Node) string { + var pos token.Pos + switch node := target.(type) { + case *ast.FieldList: + pos = node.Opening + case *ast.CallExpr: + pos = node.Lparen + case *ast.CompositeLit: + pos = node.Lbrace + } + + split := bytes.Split(src, []byte("\n")) + targetLineNumber := safetoken.StartPosition(fset, pos).Line + firstLine := string(split[targetLineNumber-1]) + trimmed := strings.TrimSpace(string(firstLine)) + + return firstLine[:strings.Index(firstLine, trimmed)] +} + +func groupLines( + fset *token.FileSet, + start, end token.Pos, + src []byte, + file *ast.File, + _ *types.Package, + _ *types.Info, +) (*token.FileSet, *analysis.SuggestedFix, error) { + _, _, _, target := findSplitGroupTarget(file, fset, start, end) + if target == nil { + return fset, &analysis.SuggestedFix{}, nil + } + + return fset, processLines(fset, target, src, file, ", ", "", "", ""), nil +} + +// processLines is the common operation for both split and group lines because the only difference between them +// is the separating whitespace. +func processLines( + fset *token.FileSet, + target ast.Node, + src []byte, + _ *ast.File, + sep, prefix, suffix, indent string, +) *analysis.SuggestedFix { + var replPos, replEnd token.Pos + var lines []string + + switch node := target.(type) { + case *ast.FieldList: + replPos, replEnd = node.Opening+1, node.Closing + + for _, field := range node.List { + pos := safetoken.StartPosition(fset, field.Pos()) + end := safetoken.EndPosition(fset, field.End()) + lines = append(lines, indent+string(src[pos.Offset:end.Offset])) + } + case *ast.CallExpr: + replPos, replEnd = node.Lparen+1, node.Rparen + + for _, arg := range node.Args { + pos := safetoken.StartPosition(fset, arg.Pos()) + end := safetoken.EndPosition(fset, arg.End()) + lines = append(lines, indent+string(src[pos.Offset:end.Offset])) + } + case *ast.CompositeLit: + replPos, replEnd = node.Lbrace+1, node.Rbrace + + for _, arg := range node.Elts { + pos := safetoken.StartPosition(fset, arg.Pos()) + end := safetoken.EndPosition(fset, arg.End()) + lines = append(lines, indent+string(src[pos.Offset:end.Offset])) + } + } + + return &analysis.SuggestedFix{ + TextEdits: []analysis.TextEdit{{ + Pos: replPos, + End: replEnd, + NewText: []byte(prefix + strings.Join(lines, sep) + suffix), + }}, + } +} + +func findSplitGroupTarget( + file *ast.File, + fset *token.FileSet, + start, end token.Pos, +) (targetName string, numLines int, targetElts int, target ast.Node) { + // todo: retain /*-style comments and do nothing for //-style comments. + isValidTarget := func(opening token.Pos, closing token.Pos, numElts int) bool { + // current cursor is inside the parens/bracket + isInside := opening < start && end < closing + + // and it has more than 1 element + return isInside && numElts > 1 + } + + countLines := func(start, end token.Pos) int { + startPos := safetoken.StartPosition(fset, start) + endPos := safetoken.EndPosition(fset, end) + return endPos.Line - startPos.Line + 1 + } + + // find the closest enclosing parens/bracket from the cursor. + path, _ := astutil.PathEnclosingInterval(file, start, end) + for _, p := range path { + switch node := p.(type) { + // Case 1: target struct method declarations. + // function (...) someMethod(a int, b int, c int) (d int, e, int) {} + case *ast.FuncDecl: + fl := node.Type.Params + if isValidTarget(fl.Opening, fl.Closing, len(fl.List)) { + return "parameters", countLines(fl.Opening, fl.Closing), len(fl.List), fl + } + + fl = node.Type.Results + if fl != nil && isValidTarget(fl.Opening, fl.Closing, len(fl.List)) { + return "return values", countLines(fl.Opening, fl.Closing), len(fl.List), fl + } + + // Case 2: target function signature args and result. + // type someFunc func (a int, b int, c int) (d int, e int) + case *ast.FuncType: + fl := node.Params + if isValidTarget(fl.Opening, fl.Closing, len(fl.List)) { + return "parameters", countLines(fl.Opening, fl.Closing), len(fl.List), fl + } + + fl = node.Results + if fl != nil && isValidTarget(fl.Opening, fl.Closing, len(fl.List)) { + return "return values", countLines(fl.Opening, fl.Closing), len(fl.List), fl + } + + // Case 3: target function calls. + // someFunction(a, b, c) + case *ast.CallExpr: + if isValidTarget(node.Lparen, node.Rparen, len(node.Args)) { + return "parameters", countLines(node.Lparen, node.Rparen), len(node.Args), node + } + + // Case 4: target composite lit instantiation (structs, maps, arrays). + // A{b: 1, c: 2, d: 3} + case *ast.CompositeLit: + if isValidTarget(node.Lbrace, node.Rbrace, len(node.Elts)) { + return "elements", countLines(node.Lbrace, node.Rbrace), len(node.Elts), node + } + } + } + + return "", 0, 0, nil +} diff --git a/gopls/internal/test/marker/testdata/codeaction/grouplines.txt b/gopls/internal/test/marker/testdata/codeaction/grouplines.txt new file mode 100644 index 00000000000..9271a46c2ce --- /dev/null +++ b/gopls/internal/test/marker/testdata/codeaction/grouplines.txt @@ -0,0 +1,187 @@ +This test exercises the refactoring of putting arguments, return values, and composite literal elements into a +single line. + +-- go.mod -- +module unused.mod + +go 1.18 + +-- func_arg/func_arg.go -- +package func_arg + +func A( + a string, + b, c int64, + x int, /*@codeaction("x", "x", "refactor.rewrite", func_arg)*/ + y int, +) (r1 string, r2, r3 int64, r4 int, r5 int) { + return a, b, c, x, y +} + +-- @func_arg/func_arg/func_arg.go -- +package func_arg + +func A(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) { + return a, b, c, x, y +} + +-- func_ret/func_ret.go -- +package func_ret + +func A(a string, b, c int64, x int, y int) ( + r1 string, /*@codeaction("r1", "r1", "refactor.rewrite", func_ret)*/ + r2, r3 int64, + r4 int, + r5 int, +) { + return a, b, c, x, y +} + +-- @func_ret/func_ret/func_ret.go -- +package func_ret + +func A(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) { + return a, b, c, x, y +} + +-- functype_arg/functype_arg.go -- +package functype_arg + +type A func( + a string, + b, c int64, + x int, /*@codeaction("x", "x", "refactor.rewrite", functype_arg)*/ + y int, +) (r1 string, r2, r3 int64, r4 int, r5 int) + +-- @functype_arg/functype_arg/functype_arg.go -- +package functype_arg + +type A func(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) + +-- functype_ret/functype_ret.go -- +package functype_ret + +type A func(a string, b, c int64, x int, y int) ( + r1 string, /*@codeaction("r1", "r1", "refactor.rewrite", functype_ret)*/ + r2, r3 int64, + r4 int, + r5 int, +) + +-- @functype_ret/functype_ret/functype_ret.go -- +package functype_ret + +type A func(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) + +-- func_call/func_call.go -- +package func_call + +import "fmt" + +func a() { + fmt.Println( + 1, /*@codeaction("1", "1", "refactor.rewrite", func_call)*/ + 2, + 3, + fmt.Sprintf("hello %d", 4), + ) +} + +-- @func_call/func_call/func_call.go -- +package func_call + +import "fmt" + +func a() { + fmt.Println(1, 2, 3, fmt.Sprintf("hello %d", 4)) +} + +-- indent/indent.go -- +package indent + +import "fmt" + +func a() { + fmt.Println( + 1, + 2, + 3, + fmt.Sprintf( + "hello %d", /*@codeaction("hello", "hello", "refactor.rewrite", indent, "Group parameters into one line")*/ + 4, + )) +} + +-- @indent/indent/indent.go -- +package indent + +import "fmt" + +func a() { + fmt.Println( + 1, + 2, + 3, + fmt.Sprintf("hello %d", 4)) +} + +-- structelts/structelts.go -- +package structelts + +type A struct{ + a int + b int +} + +func a() { + _ = A{ + a: 1, + b: 2, /*@codeaction("b", "b", "refactor.rewrite", structelts)*/ + } +} + +-- @structelts/structelts/structelts.go -- +package structelts + +type A struct{ + a int + b int +} + +func a() { + _ = A{a: 1, b: 2} +} + +-- sliceelts/sliceelts.go -- +package sliceelts + +func a() { + _ = []int{ + 1, /*@codeaction("1", "1", "refactor.rewrite", sliceelts)*/ + 2, + } +} + +-- @sliceelts/sliceelts/sliceelts.go -- +package sliceelts + +func a() { + _ = []int{1, 2} +} + +-- mapelts/mapelts.go -- +package mapelts + +func a() { + _ = map[string]int{ + "a": 1, /*@codeaction("1", "1", "refactor.rewrite", mapelts)*/ + "b": 2, + } +} +-- @mapelts/mapelts/mapelts.go -- +package mapelts + +func a() { + _ = map[string]int{"a": 1, "b": 2} +} diff --git a/gopls/internal/test/marker/testdata/codeaction/removeparam.txt b/gopls/internal/test/marker/testdata/codeaction/removeparam.txt index 17a058f2b82..25ec6ae1d96 100644 --- a/gopls/internal/test/marker/testdata/codeaction/removeparam.txt +++ b/gopls/internal/test/marker/testdata/codeaction/removeparam.txt @@ -99,7 +99,7 @@ func _() { -- field/field.go -- package field -func Field(x int, field int) { //@codeaction("int", "int", "refactor.rewrite", field) +func Field(x int, field int) { //@codeaction("int", "int", "refactor.rewrite", field, "Refactor: remove unused parameter") } func _() { @@ -108,7 +108,7 @@ func _() { -- @field/field/field.go -- package field -func Field(field int) { //@codeaction("int", "int", "refactor.rewrite", field) +func Field(field int) { //@codeaction("int", "int", "refactor.rewrite", field, "Refactor: remove unused parameter") } func _() { @@ -162,7 +162,7 @@ func i() []any -- ellipsis2/ellipsis2.go -- package ellipsis2 -func Ellipsis2(_, _ int, rest ...int) { //@codeaction("_", "_", "refactor.rewrite", ellipsis2) +func Ellipsis2(_, _ int, rest ...int) { //@codeaction("_", "_", "refactor.rewrite", ellipsis2, "Refactor: remove unused parameter") } func _() { @@ -176,7 +176,7 @@ func h() (int, int) -- @ellipsis2/ellipsis2/ellipsis2.go -- package ellipsis2 -func Ellipsis2(_ int, rest ...int) { //@codeaction("_", "_", "refactor.rewrite", ellipsis2) +func Ellipsis2(_ int, rest ...int) { //@codeaction("_", "_", "refactor.rewrite", ellipsis2, "Refactor: remove unused parameter") } func _() { diff --git a/gopls/internal/test/marker/testdata/codeaction/splitlines.txt b/gopls/internal/test/marker/testdata/codeaction/splitlines.txt new file mode 100644 index 00000000000..a7238ca92c5 --- /dev/null +++ b/gopls/internal/test/marker/testdata/codeaction/splitlines.txt @@ -0,0 +1,204 @@ +This test exercises the refactoring of putting arguments, return values, and composite literal elements +into separate lines. + +-- go.mod -- +module unused.mod + +go 1.18 + +-- func_arg/func_arg.go -- +package func_arg + +func A(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) { //@codeaction("x", "x", "refactor.rewrite", func_arg) + return a, b, c, x, y +} + +-- @func_arg/func_arg/func_arg.go -- +package func_arg + +func A( + a string, + b, c int64, + x int, + y int, +) (r1 string, r2, r3 int64, r4 int, r5 int) { //@codeaction("x", "x", "refactor.rewrite", func_arg) + return a, b, c, x, y +} + +-- func_ret/func_ret.go -- +package func_ret + +func A(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) { //@codeaction("r1", "r1", "refactor.rewrite", func_ret) + return a, b, c, x, y +} + +-- @func_ret/func_ret/func_ret.go -- +package func_ret + +func A(a string, b, c int64, x int, y int) ( + r1 string, + r2, r3 int64, + r4 int, + r5 int, +) { //@codeaction("r1", "r1", "refactor.rewrite", func_ret) + return a, b, c, x, y +} + +-- functype_arg/functype_arg.go -- +package functype_arg + +type A func(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) //@codeaction("x", "x", "refactor.rewrite", functype_arg) + +-- @functype_arg/functype_arg/functype_arg.go -- +package functype_arg + +type A func( + a string, + b, c int64, + x int, + y int, +) (r1 string, r2, r3 int64, r4 int, r5 int) //@codeaction("x", "x", "refactor.rewrite", functype_arg) + +-- functype_ret/functype_ret.go -- +package functype_ret + +type A func(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) //@codeaction("r1", "r1", "refactor.rewrite", functype_ret) + +-- @functype_ret/functype_ret/functype_ret.go -- +package functype_ret + +type A func(a string, b, c int64, x int, y int) ( + r1 string, + r2, r3 int64, + r4 int, + r5 int, +) //@codeaction("r1", "r1", "refactor.rewrite", functype_ret) + +-- func_call/func_call.go -- +package func_call + +import "fmt" + +func a() { + fmt.Println(1, 2, 3, fmt.Sprintf("hello %d", 4)) //@codeaction("1", "1", "refactor.rewrite", func_call) +} + +-- @func_call/func_call/func_call.go -- +package func_call + +import "fmt" + +func a() { + fmt.Println( + 1, + 2, + 3, + fmt.Sprintf("hello %d", 4), + ) //@codeaction("1", "1", "refactor.rewrite", func_call) +} + +-- indent/indent.go -- +package indent + +import "fmt" + +func a() { + fmt.Println(1, 2, 3, fmt.Sprintf("hello %d", 4)) //@codeaction("hello", "hello", "refactor.rewrite", indent, "Split parameters into separate lines") +} + +-- @indent/indent/indent.go -- +package indent + +import "fmt" + +func a() { + fmt.Println(1, 2, 3, fmt.Sprintf( + "hello %d", + 4, + )) //@codeaction("hello", "hello", "refactor.rewrite", indent, "Split parameters into separate lines") +} + +-- indent2/indent2.go -- +package indent2 + +import "fmt" + +func a() { + fmt. + Println(1, 2, 3, fmt.Sprintf("hello %d", 4)) //@codeaction("1", "1", "refactor.rewrite", indent2, "Split parameters into separate lines") +} + +-- @indent2/indent2/indent2.go -- +package indent2 + +import "fmt" + +func a() { + fmt. + Println( + 1, + 2, + 3, + fmt.Sprintf("hello %d", 4), + ) //@codeaction("1", "1", "refactor.rewrite", indent2, "Split parameters into separate lines") +} + +-- structelts/structelts.go -- +package structelts + +type A struct{ + a int + b int +} + +func a() { + _ = A{a: 1, b: 2} //@codeaction("b", "b", "refactor.rewrite", structelts) +} + +-- @structelts/structelts/structelts.go -- +package structelts + +type A struct{ + a int + b int +} + +func a() { + _ = A{ + a: 1, + b: 2, + } //@codeaction("b", "b", "refactor.rewrite", structelts) +} + +-- sliceelts/sliceelts.go -- +package sliceelts + +func a() { + _ = []int{1, 2} //@codeaction("1", "1", "refactor.rewrite", sliceelts) +} + +-- @sliceelts/sliceelts/sliceelts.go -- +package sliceelts + +func a() { + _ = []int{ + 1, + 2, + } //@codeaction("1", "1", "refactor.rewrite", sliceelts) +} + +-- mapelts/mapelts.go -- +package mapelts + +func a() { + _ = map[string]int{"a": 1, "b": 2} //@codeaction("1", "1", "refactor.rewrite", mapelts) +} +-- @mapelts/mapelts/mapelts.go -- +package mapelts + +func a() { + _ = map[string]int{ + "a": 1, + "b": 2, + } //@codeaction("1", "1", "refactor.rewrite", mapelts) +} From a0db3579047b19232448334a7df885e02f845c6b Mon Sep 17 00:00:00 2001 From: Fata Nugraha Date: Thu, 25 Jan 2024 21:06:24 +0800 Subject: [PATCH 02/10] Fix test --- .../marker/testdata/codeaction/removeparam_resolve.txt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gopls/internal/test/marker/testdata/codeaction/removeparam_resolve.txt b/gopls/internal/test/marker/testdata/codeaction/removeparam_resolve.txt index 1c04aa047cf..c67e8a5d039 100644 --- a/gopls/internal/test/marker/testdata/codeaction/removeparam_resolve.txt +++ b/gopls/internal/test/marker/testdata/codeaction/removeparam_resolve.txt @@ -110,7 +110,7 @@ func _() { -- field/field.go -- package field -func Field(x int, field int) { //@codeaction("int", "int", "refactor.rewrite", field) +func Field(x int, field int) { //@codeaction("int", "int", "refactor.rewrite", field, "Refactor: remove unused parameter") } func _() { @@ -119,7 +119,7 @@ func _() { -- @field/field/field.go -- package field -func Field(field int) { //@codeaction("int", "int", "refactor.rewrite", field) +func Field(field int) { //@codeaction("int", "int", "refactor.rewrite", field, "Refactor: remove unused parameter") } func _() { @@ -173,7 +173,7 @@ func i() []any -- ellipsis2/ellipsis2.go -- package ellipsis2 -func Ellipsis2(_, _ int, rest ...int) { //@codeaction("_", "_", "refactor.rewrite", ellipsis2) +func Ellipsis2(_, _ int, rest ...int) { //@codeaction("_", "_", "refactor.rewrite", ellipsis2, "Refactor: remove unused parameter") } func _() { @@ -187,7 +187,7 @@ func h() (int, int) -- @ellipsis2/ellipsis2/ellipsis2.go -- package ellipsis2 -func Ellipsis2(_ int, rest ...int) { //@codeaction("_", "_", "refactor.rewrite", ellipsis2) +func Ellipsis2(_ int, rest ...int) { //@codeaction("_", "_", "refactor.rewrite", ellipsis2, "Refactor: remove unused parameter") } func _() { From 78116378353940e132d90c7b4f8aaa93ee4b857c Mon Sep 17 00:00:00 2001 From: Fata Nugraha Date: Sun, 4 Feb 2024 03:51:43 +0800 Subject: [PATCH 03/10] Handle /*-style comment properly --- gopls/internal/golang/lines.go | 60 ++++++++++++++----- .../marker/testdata/codeaction/grouplines.txt | 37 +++++++++--- .../marker/testdata/codeaction/splitlines.txt | 19 ++++++ 3 files changed, 93 insertions(+), 23 deletions(-) diff --git a/gopls/internal/golang/lines.go b/gopls/internal/golang/lines.go index a27ecd84c09..3245eef1148 100644 --- a/gopls/internal/golang/lines.go +++ b/gopls/internal/golang/lines.go @@ -9,6 +9,7 @@ import ( "go/ast" "go/token" "go/types" + "sort" "strings" "golang.org/x/tools/go/analysis" @@ -109,36 +110,67 @@ func processLines( fset *token.FileSet, target ast.Node, src []byte, - _ *ast.File, + file *ast.File, sep, prefix, suffix, indent string, ) *analysis.SuggestedFix { var replPos, replEnd token.Pos - var lines []string + var members []ast.Node switch node := target.(type) { case *ast.FieldList: replPos, replEnd = node.Opening+1, node.Closing - for _, field := range node.List { - pos := safetoken.StartPosition(fset, field.Pos()) - end := safetoken.EndPosition(fset, field.End()) - lines = append(lines, indent+string(src[pos.Offset:end.Offset])) + members = append(members, field) } case *ast.CallExpr: replPos, replEnd = node.Lparen+1, node.Rparen - for _, arg := range node.Args { - pos := safetoken.StartPosition(fset, arg.Pos()) - end := safetoken.EndPosition(fset, arg.End()) - lines = append(lines, indent+string(src[pos.Offset:end.Offset])) + members = append(members, arg) } case *ast.CompositeLit: replPos, replEnd = node.Lbrace+1, node.Rbrace - for _, arg := range node.Elts { - pos := safetoken.StartPosition(fset, arg.Pos()) - end := safetoken.EndPosition(fset, arg.End()) - lines = append(lines, indent+string(src[pos.Offset:end.Offset])) + members = append(members, arg) + } + } + + // save /*-style comments inside replPos and replEnd + for _, cg := range file.Comments { + if !strings.HasPrefix(cg.List[0].Text, "/*") { + continue + } + + if replPos <= cg.Pos() && cg.Pos() < replEnd { + members = append(members, cg) + } + } + + sort.Slice(members, func(i, j int) bool { + return members[i].Pos() < members[j].Pos() + }) + + getSrc := func(node ast.Node) string { + curPos := safetoken.StartPosition(fset, node.Pos()) + curEnd := safetoken.EndPosition(fset, node.End()) + return string(src[curPos.Offset:curEnd.Offset]) + } + + lines := []string{indent + getSrc(members[0])} + for i := 1; i < len(members); i++ { + pos := safetoken.EndPosition(fset, members[i-1].End()).Offset + end := safetoken.StartPosition(fset, members[i].Pos()).Offset + + // this will happen if we have a /*-style comment inside a Field, e.g. `a /*comment here */ int` + // we will ignore as it's included already when we write members[i-1] + if pos > end { + continue + } + + // at this point, the `,` token here must be the field delimiter. + if bytes.IndexByte(src[pos:end], ',') >= 0 { + lines = append(lines, indent+getSrc(members[i])) + } else { + lines[len(lines)-1] = lines[len(lines)-1] + " " + getSrc(members[i]) } } diff --git a/gopls/internal/test/marker/testdata/codeaction/grouplines.txt b/gopls/internal/test/marker/testdata/codeaction/grouplines.txt index 9271a46c2ce..fbfccbdda0e 100644 --- a/gopls/internal/test/marker/testdata/codeaction/grouplines.txt +++ b/gopls/internal/test/marker/testdata/codeaction/grouplines.txt @@ -21,7 +21,7 @@ func A( -- @func_arg/func_arg/func_arg.go -- package func_arg -func A(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) { +func A(a string, b, c int64, x int, /*@codeaction("x", "x", "refactor.rewrite", func_arg)*/ y int) (r1 string, r2, r3 int64, r4 int, r5 int) { return a, b, c, x, y } @@ -40,7 +40,7 @@ func A(a string, b, c int64, x int, y int) ( -- @func_ret/func_ret/func_ret.go -- package func_ret -func A(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) { +func A(a string, b, c int64, x int, y int) (r1 string, /*@codeaction("r1", "r1", "refactor.rewrite", func_ret)*/ r2, r3 int64, r4 int, r5 int) { return a, b, c, x, y } @@ -57,7 +57,7 @@ type A func( -- @functype_arg/functype_arg/functype_arg.go -- package functype_arg -type A func(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) +type A func(a string, b, c int64, x int, /*@codeaction("x", "x", "refactor.rewrite", functype_arg)*/ y int) (r1 string, r2, r3 int64, r4 int, r5 int) -- functype_ret/functype_ret.go -- package functype_ret @@ -72,7 +72,7 @@ type A func(a string, b, c int64, x int, y int) ( -- @functype_ret/functype_ret/functype_ret.go -- package functype_ret -type A func(a string, b, c int64, x int, y int) (r1 string, r2, r3 int64, r4 int, r5 int) +type A func(a string, b, c int64, x int, y int) (r1 string, /*@codeaction("r1", "r1", "refactor.rewrite", functype_ret)*/ r2, r3 int64, r4 int, r5 int) -- func_call/func_call.go -- package func_call @@ -94,7 +94,7 @@ package func_call import "fmt" func a() { - fmt.Println(1, 2, 3, fmt.Sprintf("hello %d", 4)) + fmt.Println(1, /*@codeaction("1", "1", "refactor.rewrite", func_call)*/ 2, 3, fmt.Sprintf("hello %d", 4)) } -- indent/indent.go -- @@ -123,7 +123,7 @@ func a() { 1, 2, 3, - fmt.Sprintf("hello %d", 4)) + fmt.Sprintf("hello %d", /*@codeaction("hello", "hello", "refactor.rewrite", indent, "Group parameters into one line")*/ 4)) } -- structelts/structelts.go -- @@ -150,7 +150,7 @@ type A struct{ } func a() { - _ = A{a: 1, b: 2} + _ = A{a: 1, b: 2, /*@codeaction("b", "b", "refactor.rewrite", structelts)*/} } -- sliceelts/sliceelts.go -- @@ -167,7 +167,7 @@ func a() { package sliceelts func a() { - _ = []int{1, 2} + _ = []int{1, /*@codeaction("1", "1", "refactor.rewrite", sliceelts)*/ 2} } -- mapelts/mapelts.go -- @@ -179,9 +179,28 @@ func a() { "b": 2, } } + -- @mapelts/mapelts/mapelts.go -- package mapelts func a() { - _ = map[string]int{"a": 1, "b": 2} + _ = map[string]int{"a": 1, /*@codeaction("1", "1", "refactor.rewrite", mapelts)*/ "b": 2} +} + +-- starcomment/starcomment.go -- +package starcomment + +func A( + /*1*/ x /*2*/ string /*3*/, /*@codeaction("x", "x", "refactor.rewrite", starcomment)*/ + /*4*/ y /*5*/ int /*6*/, +) (string, int) { + return x, y } + +-- @starcomment/starcomment/starcomment.go -- +package starcomment + +func A(/*1*/ x /*2*/ string /*3*/, /*@codeaction("x", "x", "refactor.rewrite", starcomment)*/ /*4*/ y /*5*/ int /*6*/) (string, int) { + return x, y +} + diff --git a/gopls/internal/test/marker/testdata/codeaction/splitlines.txt b/gopls/internal/test/marker/testdata/codeaction/splitlines.txt index a7238ca92c5..a02e39505e5 100644 --- a/gopls/internal/test/marker/testdata/codeaction/splitlines.txt +++ b/gopls/internal/test/marker/testdata/codeaction/splitlines.txt @@ -193,6 +193,7 @@ package mapelts func a() { _ = map[string]int{"a": 1, "b": 2} //@codeaction("1", "1", "refactor.rewrite", mapelts) } + -- @mapelts/mapelts/mapelts.go -- package mapelts @@ -202,3 +203,21 @@ func a() { "b": 2, } //@codeaction("1", "1", "refactor.rewrite", mapelts) } + +-- starcomment/starcomment.go -- +package starcomment + +func A(/*1*/ x /*2*/ string /*3*/, /*4*/ y /*5*/ int /*6*/) (string, int) { //@codeaction("x", "x", "refactor.rewrite", starcomment) + return x, y +} + +-- @starcomment/starcomment/starcomment.go -- +package starcomment + +func A( + /*1*/ x /*2*/ string /*3*/, + /*4*/ y /*5*/ int /*6*/, +) (string, int) { //@codeaction("x", "x", "refactor.rewrite", starcomment) + return x, y +} + From a52609e571734169784d56b355ba89dc932303a8 Mon Sep 17 00:00:00 2001 From: Fata Nugraha Date: Sun, 4 Feb 2024 04:45:02 +0800 Subject: [PATCH 04/10] Fix checker logic to only capture the first enclosing paren --- gopls/internal/golang/lines.go | 164 ++++++++++++++++++--------------- 1 file changed, 90 insertions(+), 74 deletions(-) diff --git a/gopls/internal/golang/lines.go b/gopls/internal/golang/lines.go index 3245eef1148..e17910936d6 100644 --- a/gopls/internal/golang/lines.go +++ b/gopls/internal/golang/lines.go @@ -20,13 +20,22 @@ import ( // CanSplitLines checks whether each item of the enclosing curly bracket/parens can be put into separate lines // where each item occupies one line. func CanSplitLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (string, bool, error) { - msg, lines, numElts, target := findSplitGroupTarget(file, fset, start, end) + msg, target := findSplitGroupTarget(file, start, end) if target == nil { return "", false, nil } - // minus two to discount the parens/brackets. - if lines-2 == numElts { + canSplit := false + members := getSplitGroupItems(target) + for i := 1; i < len(members); i++ { + prevLine := safetoken.EndPosition(fset, members[i-1].End()).Line + curLine := safetoken.StartPosition(fset, members[i].Pos()).Line + if prevLine == curLine { + canSplit = true + } + } + + if !canSplit { return "", false, nil } @@ -35,12 +44,22 @@ func CanSplitLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (s // CanGroupLines checks whether each item of the enclosing curly bracket/parens can be joined into a single line. func CanGroupLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (string, bool, error) { - msg, lines, _, target := findSplitGroupTarget(file, fset, start, end) + msg, target := findSplitGroupTarget(file, start, end) if target == nil { return "", false, nil } - if lines == 1 { + canGroup := false + members := getSplitGroupItems(target) + for i := 1; i < len(members); i++ { + prevLine := safetoken.EndPosition(fset, members[i-1].End()).Line + curLine := safetoken.StartPosition(fset, members[i].Pos()).Line + if prevLine != curLine { + canGroup = true + } + } + + if !canGroup { return "", false, nil } @@ -56,38 +75,16 @@ func splitLines( _ *types.Package, _ *types.Info, ) (*token.FileSet, *analysis.SuggestedFix, error) { - _, _, _, target := findSplitGroupTarget(file, fset, start, end) + _, target := findSplitGroupTarget(file, start, end) if target == nil { return fset, &analysis.SuggestedFix{}, nil } - // get the original line indent of target. firstLineIndent := getBraceIndent(src, fset, target) eltIndent := firstLineIndent + "\t" - return fset, processLines(fset, target, src, file, ",\n", "\n", ",\n"+firstLineIndent, eltIndent), nil } -// getBraceIndent returns the line indent of the opening curly bracket/paren. -func getBraceIndent(src []byte, fset *token.FileSet, target ast.Node) string { - var pos token.Pos - switch node := target.(type) { - case *ast.FieldList: - pos = node.Opening - case *ast.CallExpr: - pos = node.Lparen - case *ast.CompositeLit: - pos = node.Lbrace - } - - split := bytes.Split(src, []byte("\n")) - targetLineNumber := safetoken.StartPosition(fset, pos).Line - firstLine := string(split[targetLineNumber-1]) - trimmed := strings.TrimSpace(string(firstLine)) - - return firstLine[:strings.Index(firstLine, trimmed)] -} - func groupLines( fset *token.FileSet, start, end token.Pos, @@ -96,7 +93,7 @@ func groupLines( _ *types.Package, _ *types.Info, ) (*token.FileSet, *analysis.SuggestedFix, error) { - _, _, _, target := findSplitGroupTarget(file, fset, start, end) + _, target := findSplitGroupTarget(file, start, end) if target == nil { return fset, &analysis.SuggestedFix{}, nil } @@ -113,27 +110,19 @@ func processLines( file *ast.File, sep, prefix, suffix, indent string, ) *analysis.SuggestedFix { + // find the position of the opening/closing pair var replPos, replEnd token.Pos - var members []ast.Node - switch node := target.(type) { case *ast.FieldList: replPos, replEnd = node.Opening+1, node.Closing - for _, field := range node.List { - members = append(members, field) - } case *ast.CallExpr: replPos, replEnd = node.Lparen+1, node.Rparen - for _, arg := range node.Args { - members = append(members, arg) - } case *ast.CompositeLit: replPos, replEnd = node.Lbrace+1, node.Rbrace - for _, arg := range node.Elts { - members = append(members, arg) - } } + members := getSplitGroupItems(target) + // save /*-style comments inside replPos and replEnd for _, cg := range file.Comments { if !strings.HasPrefix(cg.List[0].Text, "/*") { @@ -160,8 +149,8 @@ func processLines( pos := safetoken.EndPosition(fset, members[i-1].End()).Offset end := safetoken.StartPosition(fset, members[i].Pos()).Offset - // this will happen if we have a /*-style comment inside a Field, e.g. `a /*comment here */ int` - // we will ignore as it's included already when we write members[i-1] + // this will happen if we have a /*-style comment inside of a Field, e.g. `a /*comment here */ int` + // we will ignore as it's included already when we write members[i-1]. if pos > end { continue } @@ -183,27 +172,12 @@ func processLines( } } -func findSplitGroupTarget( - file *ast.File, - fset *token.FileSet, - start, end token.Pos, -) (targetName string, numLines int, targetElts int, target ast.Node) { - // todo: retain /*-style comments and do nothing for //-style comments. - isValidTarget := func(opening token.Pos, closing token.Pos, numElts int) bool { - // current cursor is inside the parens/bracket - isInside := opening < start && end < closing - - // and it has more than 1 element - return isInside && numElts > 1 +// findSplitGroupTarget returns the first curly bracket/parens that encloses the current cursor. +func findSplitGroupTarget(file *ast.File, start, end token.Pos) (targetName string, target ast.Node) { + isCursorInside := func(opening token.Pos, closing token.Pos) bool { + return opening < start && end < closing } - countLines := func(start, end token.Pos) int { - startPos := safetoken.StartPosition(fset, start) - endPos := safetoken.EndPosition(fset, end) - return endPos.Line - startPos.Line + 1 - } - - // find the closest enclosing parens/bracket from the cursor. path, _ := astutil.PathEnclosingInterval(file, start, end) for _, p := range path { switch node := p.(type) { @@ -211,43 +185,85 @@ func findSplitGroupTarget( // function (...) someMethod(a int, b int, c int) (d int, e, int) {} case *ast.FuncDecl: fl := node.Type.Params - if isValidTarget(fl.Opening, fl.Closing, len(fl.List)) { - return "parameters", countLines(fl.Opening, fl.Closing), len(fl.List), fl + if isCursorInside(fl.Opening, fl.Closing) { + return "parameters", fl } fl = node.Type.Results - if fl != nil && isValidTarget(fl.Opening, fl.Closing, len(fl.List)) { - return "return values", countLines(fl.Opening, fl.Closing), len(fl.List), fl + if fl != nil && isCursorInside(fl.Opening, fl.Closing) { + return "return values", fl } // Case 2: target function signature args and result. // type someFunc func (a int, b int, c int) (d int, e int) case *ast.FuncType: fl := node.Params - if isValidTarget(fl.Opening, fl.Closing, len(fl.List)) { - return "parameters", countLines(fl.Opening, fl.Closing), len(fl.List), fl + if isCursorInside(fl.Opening, fl.Closing) { + return "parameters", fl } fl = node.Results - if fl != nil && isValidTarget(fl.Opening, fl.Closing, len(fl.List)) { - return "return values", countLines(fl.Opening, fl.Closing), len(fl.List), fl + if fl != nil && isCursorInside(fl.Opening, fl.Closing) { + return "return values", fl } // Case 3: target function calls. // someFunction(a, b, c) case *ast.CallExpr: - if isValidTarget(node.Lparen, node.Rparen, len(node.Args)) { - return "parameters", countLines(node.Lparen, node.Rparen), len(node.Args), node + if isCursorInside(node.Lparen, node.Rparen) { + return "parameters", node } // Case 4: target composite lit instantiation (structs, maps, arrays). // A{b: 1, c: 2, d: 3} case *ast.CompositeLit: - if isValidTarget(node.Lbrace, node.Rbrace, len(node.Elts)) { - return "elements", countLines(node.Lbrace, node.Rbrace), len(node.Elts), node + if isCursorInside(node.Lbrace, node.Rbrace) { + return "elements", node } } } - return "", 0, 0, nil + return "", nil +} + +// getSplitGroupItems returns the item that will be splitted/joined. +func getSplitGroupItems(target ast.Node) []ast.Node { + var items []ast.Node + + switch node := target.(type) { + case *ast.FieldList: + for _, field := range node.List { + items = append(items, field) + } + case *ast.CallExpr: + for _, arg := range node.Args { + items = append(items, arg) + } + case *ast.CompositeLit: + for _, arg := range node.Elts { + items = append(items, arg) + } + } + + return items +} + +// getBraceIndent returns the line indent of the opening curly bracket/paren. +func getBraceIndent(src []byte, fset *token.FileSet, target ast.Node) string { + var pos token.Pos + switch node := target.(type) { + case *ast.FieldList: + pos = node.Opening + case *ast.CallExpr: + pos = node.Lparen + case *ast.CompositeLit: + pos = node.Lbrace + } + + split := bytes.Split(src, []byte("\n")) + targetLineNumber := safetoken.StartPosition(fset, pos).Line + firstLine := string(split[targetLineNumber-1]) + trimmed := strings.TrimSpace(string(firstLine)) + + return firstLine[:strings.Index(firstLine, trimmed)] } From ecfda5207254fdf5583657db8cff1d178d1f809d Mon Sep 17 00:00:00 2001 From: Fata Nugraha Date: Sun, 4 Feb 2024 04:54:19 +0800 Subject: [PATCH 05/10] Do not suggest code action if it has comment in it and elts <= 1 --- gopls/internal/golang/lines.go | 82 +++++++++++++++++++++------------- 1 file changed, 51 insertions(+), 31 deletions(-) diff --git a/gopls/internal/golang/lines.go b/gopls/internal/golang/lines.go index e17910936d6..fe5a1db9457 100644 --- a/gopls/internal/golang/lines.go +++ b/gopls/internal/golang/lines.go @@ -25,21 +25,20 @@ func CanSplitLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (s return "", false, nil } - canSplit := false - members := getSplitGroupItems(target) - for i := 1; i < len(members); i++ { - prevLine := safetoken.EndPosition(fset, members[i-1].End()).Line - curLine := safetoken.StartPosition(fset, members[i].Pos()).Line - if prevLine == curLine { - canSplit = true - } + items := getSplitGroupItems(target) + if !canSplitGroupLines(file, target, len(items)) { + return "", false, nil } - if !canSplit { - return "", false, nil + for i := 1; i < len(items); i++ { + prevLine := safetoken.EndPosition(fset, items[i-1].End()).Line + curLine := safetoken.StartPosition(fset, items[i].Pos()).Line + if prevLine == curLine { + return "Split " + msg + " into separate lines", true, nil + } } - return "Split " + msg + " into separate lines", true, nil + return "", false, nil } // CanGroupLines checks whether each item of the enclosing curly bracket/parens can be joined into a single line. @@ -49,21 +48,38 @@ func CanGroupLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (s return "", false, nil } - canGroup := false - members := getSplitGroupItems(target) - for i := 1; i < len(members); i++ { - prevLine := safetoken.EndPosition(fset, members[i-1].End()).Line - curLine := safetoken.StartPosition(fset, members[i].Pos()).Line + items := getSplitGroupItems(target) + if !canSplitGroupLines(file, target, len(items)) { + return "", false, nil + } + + for i := 1; i < len(items); i++ { + prevLine := safetoken.EndPosition(fset, items[i-1].End()).Line + curLine := safetoken.StartPosition(fset, items[i].Pos()).Line if prevLine != curLine { - canGroup = true + return "Group " + msg + " into one line", true, nil } } - if !canGroup { - return "", false, nil + return "", false, nil +} + +// canSplitGroupLines determines whether we should split/group the lines or not. +func canSplitGroupLines(file *ast.File, target ast.Node, numItems int) bool { + haveDoubleSlashComments := false + pos, end := getBracePos(target) + for _, cg := range file.Comments { + if strings.HasPrefix(cg.List[0].Text, "/*") { + continue + } + + if pos <= cg.Pos() && cg.End() < end { + haveDoubleSlashComments = true + break + } } - return "Group " + msg + " into one line", true, nil + return numItems > 1 && !haveDoubleSlashComments } func splitLines( @@ -110,17 +126,7 @@ func processLines( file *ast.File, sep, prefix, suffix, indent string, ) *analysis.SuggestedFix { - // find the position of the opening/closing pair - var replPos, replEnd token.Pos - switch node := target.(type) { - case *ast.FieldList: - replPos, replEnd = node.Opening+1, node.Closing - case *ast.CallExpr: - replPos, replEnd = node.Lparen+1, node.Rparen - case *ast.CompositeLit: - replPos, replEnd = node.Lbrace+1, node.Rbrace - } - + replPos, replEnd := getBracePos(target) members := getSplitGroupItems(target) // save /*-style comments inside replPos and replEnd @@ -267,3 +273,17 @@ func getBraceIndent(src []byte, fset *token.FileSet, target ast.Node) string { return firstLine[:strings.Index(firstLine, trimmed)] } + +// getBracePos returns the position of the given target's opening and closing curly bracket/parens. +func getBracePos(target ast.Node) (opening token.Pos, closing token.Pos) { + var replPos, replEnd token.Pos + switch node := target.(type) { + case *ast.FieldList: + replPos, replEnd = node.Opening+1, node.Closing + case *ast.CallExpr: + replPos, replEnd = node.Lparen+1, node.Rparen + case *ast.CompositeLit: + replPos, replEnd = node.Lbrace+1, node.Rbrace + } + return replPos, replEnd +} From 306c61a5a5f0c338328aedec1bb0fa30159b6a43 Mon Sep 17 00:00:00 2001 From: Fata Nugraha Date: Sun, 4 Feb 2024 05:04:07 +0800 Subject: [PATCH 06/10] Fix comment --- gopls/internal/golang/lines.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gopls/internal/golang/lines.go b/gopls/internal/golang/lines.go index fe5a1db9457..228569615bd 100644 --- a/gopls/internal/golang/lines.go +++ b/gopls/internal/golang/lines.go @@ -117,8 +117,8 @@ func groupLines( return fset, processLines(fset, target, src, file, ", ", "", "", ""), nil } -// processLines is the common operation for both split and group lines because the only difference between them -// is the separating whitespace. +// processLines is the common operation for both split and group lines because this split/group operation is +// essentially a transformation of the separating whitespace. func processLines( fset *token.FileSet, target ast.Node, From d4697e613562f6ce672025762e8b071832691141 Mon Sep 17 00:00:00 2001 From: Fata Nugraha Date: Tue, 13 Feb 2024 17:56:09 +0800 Subject: [PATCH 07/10] Address review comments --- gopls/internal/golang/codeaction.go | 4 +- gopls/internal/golang/fix.go | 4 +- gopls/internal/golang/lines.go | 325 ++++++++---------- .../marker/testdata/codeaction/grouplines.txt | 40 +-- 4 files changed, 171 insertions(+), 202 deletions(-) diff --git a/gopls/internal/golang/codeaction.go b/gopls/internal/golang/codeaction.go index fbcba0ebe28..e49be43194f 100644 --- a/gopls/internal/golang/codeaction.go +++ b/gopls/internal/golang/codeaction.go @@ -317,9 +317,9 @@ func getRewriteCodeActions(pkg *cache.Package, pgf *ParsedGoFile, fh file.Handle commands = append(commands, cmd) } - if msg, ok, _ := CanGroupLines(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: fixGroupLines, + Fix: fixJoinLines, URI: pgf.URI, Range: rng, ResolveEdits: supportsResolveEdits(options), diff --git a/gopls/internal/golang/fix.go b/gopls/internal/golang/fix.go index 76baf57b08f..16908510b7c 100644 --- a/gopls/internal/golang/fix.go +++ b/gopls/internal/golang/fix.go @@ -65,7 +65,7 @@ const ( fixInlineCall = "inline_call" fixInvertIfCondition = "invert_if_condition" fixSplitLines = "split_lines" - fixGroupLines = "group_lines" + fixJoinLines = "group_lines" ) // ApplyFix applies the specified kind of suggested fix to the given @@ -118,7 +118,7 @@ func ApplyFix(ctx context.Context, fix string, snapshot *cache.Snapshot, fh file fixInlineCall: inlineCall, fixInvertIfCondition: singleFile(invertIfCondition), fixSplitLines: singleFile(splitLines), - fixGroupLines: singleFile(groupLines), + fixJoinLines: singleFile(joinLines), } fixer, ok := fixers[fix] if !ok { diff --git a/gopls/internal/golang/lines.go b/gopls/internal/golang/lines.go index 228569615bd..aefbef9c94d 100644 --- a/gopls/internal/golang/lines.go +++ b/gopls/internal/golang/lines.go @@ -1,9 +1,13 @@ -// Copyright 2023 The Go Authors. All rights reserved. +// Copyright 2024 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. package golang +// This file defines refactorings for splitting lists of elements +// (arguments, literals, etc) across multiple lines, and joining +// them into a single line. + import ( "bytes" "go/ast" @@ -20,13 +24,12 @@ import ( // CanSplitLines checks whether each item of the enclosing curly bracket/parens can be put into separate lines // where each item occupies one line. func CanSplitLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (string, bool, error) { - msg, target := findSplitGroupTarget(file, start, end) - if target == nil { + itemType, items, comments, _, _, _ := findSplitJoinTarget(fset, file, nil, start, end) + if itemType == "" { return "", false, nil } - items := getSplitGroupItems(target) - if !canSplitGroupLines(file, target, len(items)) { + if !canSplitJoinLines(items, comments) { return "", false, nil } @@ -34,22 +37,21 @@ func CanSplitLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (s prevLine := safetoken.EndPosition(fset, items[i-1].End()).Line curLine := safetoken.StartPosition(fset, items[i].Pos()).Line if prevLine == curLine { - return "Split " + msg + " into separate lines", true, nil + return "Split " + itemType + " into separate lines", true, nil } } return "", false, nil } -// CanGroupLines checks whether each item of the enclosing curly bracket/parens can be joined into a single line. -func CanGroupLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (string, bool, error) { - msg, target := findSplitGroupTarget(file, start, end) - if target == nil { +// CanJoinLines checks whether each item of the enclosing curly bracket/parens can be joined 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 } - items := getSplitGroupItems(target) - if !canSplitGroupLines(file, target, len(items)) { + if !canSplitJoinLines(items, comments) { return "", false, nil } @@ -57,186 +59,157 @@ func CanGroupLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (s prevLine := safetoken.EndPosition(fset, items[i-1].End()).Line curLine := safetoken.StartPosition(fset, items[i].Pos()).Line if prevLine != curLine { - return "Group " + msg + " into one line", true, nil + return "Join " + itemType + " into one line", true, nil } } return "", false, nil } -// canSplitGroupLines determines whether we should split/group the lines or not. -func canSplitGroupLines(file *ast.File, target ast.Node, numItems int) bool { - haveDoubleSlashComments := false - pos, end := getBracePos(target) - for _, cg := range file.Comments { - if strings.HasPrefix(cg.List[0].Text, "/*") { - continue - } +// canSplitJoinLines determines whether we should split/group the lines or not. +func canSplitJoinLines(items []ast.Node, comments []*ast.CommentGroup) bool { + if len(items) <= 1 { + return false + } - if pos <= cg.Pos() && cg.End() < end { - haveDoubleSlashComments = true - break + for _, cg := range comments { + if !strings.HasPrefix(cg.List[0].Text, "/*") { + return false // can't split/join lists containing "//" comments } } - return numItems > 1 && !haveDoubleSlashComments + return true } -func splitLines( - fset *token.FileSet, - start token.Pos, - end token.Pos, - src []byte, - file *ast.File, - _ *types.Package, - _ *types.Info, -) (*token.FileSet, *analysis.SuggestedFix, error) { - _, target := findSplitGroupTarget(file, start, end) - if target == nil { - return fset, &analysis.SuggestedFix{}, nil +// splitLines is a singleFile fixer. +func splitLines(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, _ *types.Package, _ *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) { + itemType, items, comments, indent, braceOpen, braceClose := findSplitJoinTarget(fset, file, src, start, end) + if itemType == "" { + return nil, nil, nil // no fix available } - firstLineIndent := getBraceIndent(src, fset, target) - eltIndent := firstLineIndent + "\t" - return fset, processLines(fset, target, src, file, ",\n", "\n", ",\n"+firstLineIndent, eltIndent), nil + return fset, processLines(fset, items, comments, src, braceOpen, braceClose, ",\n", "\n", ",\n"+indent, indent+"\t"), nil } -func groupLines( - fset *token.FileSet, - start, end token.Pos, - src []byte, - file *ast.File, - _ *types.Package, - _ *types.Info, -) (*token.FileSet, *analysis.SuggestedFix, error) { - _, target := findSplitGroupTarget(file, start, end) - if target == nil { - return fset, &analysis.SuggestedFix{}, nil +// joinLines is a singleFile fixer. +func joinLines(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, _ *types.Package, _ *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) { + itemType, items, comments, _, braceOpen, braceClose := findSplitJoinTarget(fset, file, src, start, end) + if itemType == "" { + return nil, nil, nil // no fix available } - return fset, processLines(fset, target, src, file, ", ", "", "", ""), nil + return fset, processLines(fset, items, comments, src, braceOpen, braceClose, ", ", "", "", ""), nil } // processLines is the common operation for both split and group lines because this split/group operation is // essentially a transformation of the separating whitespace. -func processLines( - fset *token.FileSet, - target ast.Node, - src []byte, - file *ast.File, - sep, prefix, suffix, indent string, -) *analysis.SuggestedFix { - replPos, replEnd := getBracePos(target) - members := getSplitGroupItems(target) - - // save /*-style comments inside replPos and replEnd - for _, cg := range file.Comments { - if !strings.HasPrefix(cg.List[0].Text, "/*") { - continue - } +func processLines(fset *token.FileSet, items []ast.Node, comments []*ast.CommentGroup, src []byte, braceOpen, braceClose token.Pos, sep, prefix, suffix, indent string) *analysis.SuggestedFix { + var nodes []ast.Node + nodes = append(nodes, items...) - if replPos <= cg.Pos() && cg.Pos() < replEnd { - members = append(members, cg) - } + // box *ast.CommentGroup to *ast.Node for easier processing later. + for _, cg := range comments { + nodes = append(nodes, cg) } - sort.Slice(members, func(i, j int) bool { - return members[i].Pos() < members[j].Pos() + sort.Slice(nodes, func(i, j int) bool { + return nodes[i].Pos() < nodes[j].Pos() }) - getSrc := func(node ast.Node) string { - curPos := safetoken.StartPosition(fset, node.Pos()) - curEnd := safetoken.EndPosition(fset, node.End()) - return string(src[curPos.Offset:curEnd.Offset]) + edits := []analysis.TextEdit{ + { + Pos: braceOpen + 1, + End: nodes[0].Pos(), + NewText: []byte(prefix + indent), + }, + { + Pos: nodes[len(nodes)-1].End(), + End: braceClose, + NewText: []byte(suffix), + }, } - lines := []string{indent + getSrc(members[0])} - for i := 1; i < len(members); i++ { - pos := safetoken.EndPosition(fset, members[i-1].End()).Offset - end := safetoken.StartPosition(fset, members[i].Pos()).Offset - - // this will happen if we have a /*-style comment inside of a Field, e.g. `a /*comment here */ int` - // we will ignore as it's included already when we write members[i-1]. + for i := 1; i < len(nodes); i++ { + pos, end := nodes[i-1].End(), nodes[i].Pos() if pos > end { + // this will happen if we have a /*-style comment inside of a Field + // e.g. `a /*comment here */ int` + // + // we will ignore as we only care about finding the field delimiter. continue } - // at this point, the `,` token here must be the field delimiter. - if bytes.IndexByte(src[pos:end], ',') >= 0 { - lines = append(lines, indent+getSrc(members[i])) - } else { - lines[len(lines)-1] = lines[len(lines)-1] + " " + getSrc(members[i]) + // at this point, the `,` token in between 2 nodes here must be the field delimiter. + posOffset := safetoken.EndPosition(fset, pos).Offset + endOffset := safetoken.StartPosition(fset, end).Offset + if bytes.IndexByte(src[posOffset:endOffset], ',') == -1 { + continue } - } - return &analysis.SuggestedFix{ - TextEdits: []analysis.TextEdit{{ - Pos: replPos, - End: replEnd, - NewText: []byte(prefix + strings.Join(lines, sep) + suffix), - }}, + edits = append(edits, analysis.TextEdit{Pos: pos, End: end, NewText: []byte(sep + indent)}) } + + return &analysis.SuggestedFix{TextEdits: edits} } -// findSplitGroupTarget returns the first curly bracket/parens that encloses the current cursor. -func findSplitGroupTarget(file *ast.File, start, end token.Pos) (targetName string, target ast.Node) { - isCursorInside := func(opening token.Pos, closing token.Pos) bool { - return opening < start && end < closing +// findSplitJoinTarget returns the first curly bracket/parens that encloses the current cursor. +func findSplitJoinTarget(fset *token.FileSet, file *ast.File, src []byte, start, end token.Pos) (itemType string, items []ast.Node, comments []*ast.CommentGroup, indent string, open, close token.Pos) { + isCursorInside := func(nodePos, nodeEnd token.Pos) bool { + return nodePos < start && end < nodeEnd } - path, _ := astutil.PathEnclosingInterval(file, start, end) - for _, p := range path { - switch node := p.(type) { - // Case 1: target struct method declarations. - // function (...) someMethod(a int, b int, c int) (d int, e, int) {} - case *ast.FuncDecl: - fl := node.Type.Params - if isCursorInside(fl.Opening, fl.Closing) { - return "parameters", fl - } - - fl = node.Type.Results - if fl != nil && isCursorInside(fl.Opening, fl.Closing) { - return "return values", fl - } - - // Case 2: target function signature args and result. - // type someFunc func (a int, b int, c int) (d int, e int) - case *ast.FuncType: - fl := node.Params - if isCursorInside(fl.Opening, fl.Closing) { - return "parameters", fl - } - - fl = node.Results - if fl != nil && isCursorInside(fl.Opening, fl.Closing) { - return "return values", fl - } - - // Case 3: target function calls. - // someFunction(a, b, c) - case *ast.CallExpr: - if isCursorInside(node.Lparen, node.Rparen) { - return "parameters", node - } - - // Case 4: target composite lit instantiation (structs, maps, arrays). - // A{b: 1, c: 2, d: 3} - case *ast.CompositeLit: - if isCursorInside(node.Lbrace, node.Rbrace) { - return "elements", node + findTarget := func() (targetType string, target ast.Node, open, close token.Pos) { + path, _ := astutil.PathEnclosingInterval(file, start, end) + for _, node := range path { + switch node := node.(type) { + case *ast.FuncDecl: + // target struct method declarations. + // function (...) someMethod(a int, b int, c int) (d int, e, int) {} + params := node.Type.Params + if isCursorInside(params.Opening, params.Closing) { + return "parameters", params, params.Opening, params.Closing + } + + results := node.Type.Results + if results != nil && isCursorInside(results.Opening, results.Closing) { + return "return values", results, results.Opening, results.Closing + } + case *ast.FuncType: + // target function signature args and result. + // type someFunc func (a int, b int, c int) (d int, e int) + params := node.Params + if isCursorInside(params.Opening, params.Closing) { + return "parameters", params, params.Opening, params.Closing + } + + results := node.Results + if results != nil && isCursorInside(results.Opening, results.Closing) { + return "return values", results, results.Opening, results.Closing + } + case *ast.CallExpr: + // target function calls. + // someFunction(a, b, c) + if isCursorInside(node.Lparen, node.Rparen) { + return "parameters", node, node.Lparen, node.Rparen + } + case *ast.CompositeLit: + // target composite lit instantiation (structs, maps, arrays). + // A{b: 1, c: 2, d: 3} + if isCursorInside(node.Lbrace, node.Rbrace) { + return "elements", node, node.Lbrace, node.Rbrace + } } } - } - return "", nil -} + return "", nil, 0, 0 + } -// getSplitGroupItems returns the item that will be splitted/joined. -func getSplitGroupItems(target ast.Node) []ast.Node { - var items []ast.Node + targetType, targetNode, open, close := findTarget() + if targetType == "" { + return "", nil, nil, "", 0, 0 + } - switch node := target.(type) { + switch node := targetNode.(type) { case *ast.FieldList: for _, field := range node.List { items = append(items, field) @@ -251,39 +224,35 @@ func getSplitGroupItems(target ast.Node) []ast.Node { } } - return items -} - -// getBraceIndent returns the line indent of the opening curly bracket/paren. -func getBraceIndent(src []byte, fset *token.FileSet, target ast.Node) string { - var pos token.Pos - switch node := target.(type) { - case *ast.FieldList: - pos = node.Opening - case *ast.CallExpr: - pos = node.Lparen - case *ast.CompositeLit: - pos = node.Lbrace + // preserve comments separately as it's not part of the targetNode AST. + for _, cg := range file.Comments { + if open <= cg.Pos() && cg.Pos() < close { + comments = append(comments, cg) + } } - split := bytes.Split(src, []byte("\n")) - targetLineNumber := safetoken.StartPosition(fset, pos).Line - firstLine := string(split[targetLineNumber-1]) - trimmed := strings.TrimSpace(string(firstLine)) - - return firstLine[:strings.Index(firstLine, trimmed)] -} + // indent is the leading whitespace before the opening curly bracket/paren. + // + // in case where we don't have access to src yet i.e. src == nil + // it's fine to return incorrect indent because we don't need it yet. + indent = "" + if len(src) > 0 { + var pos token.Pos + switch node := targetNode.(type) { + case *ast.FieldList: + pos = node.Opening + case *ast.CallExpr: + pos = node.Lparen + case *ast.CompositeLit: + pos = node.Lbrace + } -// getBracePos returns the position of the given target's opening and closing curly bracket/parens. -func getBracePos(target ast.Node) (opening token.Pos, closing token.Pos) { - var replPos, replEnd token.Pos - switch node := target.(type) { - case *ast.FieldList: - replPos, replEnd = node.Opening+1, node.Closing - case *ast.CallExpr: - replPos, replEnd = node.Lparen+1, node.Rparen - case *ast.CompositeLit: - replPos, replEnd = node.Lbrace+1, node.Rbrace + split := bytes.Split(src, []byte("\n")) + targetLineNumber := safetoken.StartPosition(fset, pos).Line + firstLine := string(split[targetLineNumber-1]) + trimmed := strings.TrimSpace(string(firstLine)) + indent = firstLine[:strings.Index(firstLine, trimmed)] } - return replPos, replEnd + + return targetType, items, comments, indent, open, close } diff --git a/gopls/internal/test/marker/testdata/codeaction/grouplines.txt b/gopls/internal/test/marker/testdata/codeaction/grouplines.txt index fbfccbdda0e..8d1134c5d6c 100644 --- a/gopls/internal/test/marker/testdata/codeaction/grouplines.txt +++ b/gopls/internal/test/marker/testdata/codeaction/grouplines.txt @@ -12,7 +12,7 @@ package func_arg func A( a string, b, c int64, - x int, /*@codeaction("x", "x", "refactor.rewrite", func_arg)*/ + x int /*@codeaction("x", "x", "refactor.rewrite", func_arg)*/, y int, ) (r1 string, r2, r3 int64, r4 int, r5 int) { return a, b, c, x, y @@ -21,7 +21,7 @@ func A( -- @func_arg/func_arg/func_arg.go -- package func_arg -func A(a string, b, c int64, x int, /*@codeaction("x", "x", "refactor.rewrite", func_arg)*/ y int) (r1 string, r2, r3 int64, r4 int, r5 int) { +func A(a string, b, c int64, x int /*@codeaction("x", "x", "refactor.rewrite", func_arg)*/, y int) (r1 string, r2, r3 int64, r4 int, r5 int) { return a, b, c, x, y } @@ -29,7 +29,7 @@ func A(a string, b, c int64, x int, /*@codeaction("x", "x", "refactor.rewrite", package func_ret func A(a string, b, c int64, x int, y int) ( - r1 string, /*@codeaction("r1", "r1", "refactor.rewrite", func_ret)*/ + r1 string /*@codeaction("r1", "r1", "refactor.rewrite", func_ret)*/, r2, r3 int64, r4 int, r5 int, @@ -40,7 +40,7 @@ func A(a string, b, c int64, x int, y int) ( -- @func_ret/func_ret/func_ret.go -- package func_ret -func A(a string, b, c int64, x int, y int) (r1 string, /*@codeaction("r1", "r1", "refactor.rewrite", func_ret)*/ r2, r3 int64, r4 int, r5 int) { +func A(a string, b, c int64, x int, y int) (r1 string /*@codeaction("r1", "r1", "refactor.rewrite", func_ret)*/, r2, r3 int64, r4 int, r5 int) { return a, b, c, x, y } @@ -50,20 +50,20 @@ package functype_arg type A func( a string, b, c int64, - x int, /*@codeaction("x", "x", "refactor.rewrite", functype_arg)*/ + x int /*@codeaction("x", "x", "refactor.rewrite", functype_arg)*/, y int, ) (r1 string, r2, r3 int64, r4 int, r5 int) -- @functype_arg/functype_arg/functype_arg.go -- package functype_arg -type A func(a string, b, c int64, x int, /*@codeaction("x", "x", "refactor.rewrite", functype_arg)*/ y int) (r1 string, r2, r3 int64, r4 int, r5 int) +type A func(a string, b, c int64, x int /*@codeaction("x", "x", "refactor.rewrite", functype_arg)*/, y int) (r1 string, r2, r3 int64, r4 int, r5 int) -- functype_ret/functype_ret.go -- package functype_ret type A func(a string, b, c int64, x int, y int) ( - r1 string, /*@codeaction("r1", "r1", "refactor.rewrite", functype_ret)*/ + r1 string /*@codeaction("r1", "r1", "refactor.rewrite", functype_ret)*/, r2, r3 int64, r4 int, r5 int, @@ -72,7 +72,7 @@ type A func(a string, b, c int64, x int, y int) ( -- @functype_ret/functype_ret/functype_ret.go -- package functype_ret -type A func(a string, b, c int64, x int, y int) (r1 string, /*@codeaction("r1", "r1", "refactor.rewrite", functype_ret)*/ r2, r3 int64, r4 int, r5 int) +type A func(a string, b, c int64, x int, y int) (r1 string /*@codeaction("r1", "r1", "refactor.rewrite", functype_ret)*/, r2, r3 int64, r4 int, r5 int) -- func_call/func_call.go -- package func_call @@ -81,7 +81,7 @@ import "fmt" func a() { fmt.Println( - 1, /*@codeaction("1", "1", "refactor.rewrite", func_call)*/ + 1 /*@codeaction("1", "1", "refactor.rewrite", func_call)*/, 2, 3, fmt.Sprintf("hello %d", 4), @@ -94,7 +94,7 @@ package func_call import "fmt" func a() { - fmt.Println(1, /*@codeaction("1", "1", "refactor.rewrite", func_call)*/ 2, 3, fmt.Sprintf("hello %d", 4)) + fmt.Println(1 /*@codeaction("1", "1", "refactor.rewrite", func_call)*/, 2, 3, fmt.Sprintf("hello %d", 4)) } -- indent/indent.go -- @@ -108,7 +108,7 @@ func a() { 2, 3, fmt.Sprintf( - "hello %d", /*@codeaction("hello", "hello", "refactor.rewrite", indent, "Group parameters into one line")*/ + "hello %d" /*@codeaction("hello", "hello", "refactor.rewrite", indent, "Join parameters into one line")*/, 4, )) } @@ -123,7 +123,7 @@ func a() { 1, 2, 3, - fmt.Sprintf("hello %d", /*@codeaction("hello", "hello", "refactor.rewrite", indent, "Group parameters into one line")*/ 4)) + fmt.Sprintf("hello %d" /*@codeaction("hello", "hello", "refactor.rewrite", indent, "Join parameters into one line")*/, 4)) } -- structelts/structelts.go -- @@ -137,7 +137,7 @@ type A struct{ func a() { _ = A{ a: 1, - b: 2, /*@codeaction("b", "b", "refactor.rewrite", structelts)*/ + b: 2 /*@codeaction("b", "b", "refactor.rewrite", structelts)*/, } } @@ -150,7 +150,7 @@ type A struct{ } func a() { - _ = A{a: 1, b: 2, /*@codeaction("b", "b", "refactor.rewrite", structelts)*/} + _ = A{a: 1, b: 2 /*@codeaction("b", "b", "refactor.rewrite", structelts)*/} } -- sliceelts/sliceelts.go -- @@ -158,7 +158,7 @@ package sliceelts func a() { _ = []int{ - 1, /*@codeaction("1", "1", "refactor.rewrite", sliceelts)*/ + 1 /*@codeaction("1", "1", "refactor.rewrite", sliceelts)*/, 2, } } @@ -167,7 +167,7 @@ func a() { package sliceelts func a() { - _ = []int{1, /*@codeaction("1", "1", "refactor.rewrite", sliceelts)*/ 2} + _ = []int{1 /*@codeaction("1", "1", "refactor.rewrite", sliceelts)*/, 2} } -- mapelts/mapelts.go -- @@ -175,7 +175,7 @@ package mapelts func a() { _ = map[string]int{ - "a": 1, /*@codeaction("1", "1", "refactor.rewrite", mapelts)*/ + "a": 1 /*@codeaction("1", "1", "refactor.rewrite", mapelts)*/, "b": 2, } } @@ -184,14 +184,14 @@ func a() { package mapelts func a() { - _ = map[string]int{"a": 1, /*@codeaction("1", "1", "refactor.rewrite", mapelts)*/ "b": 2} + _ = map[string]int{"a": 1 /*@codeaction("1", "1", "refactor.rewrite", mapelts)*/, "b": 2} } -- starcomment/starcomment.go -- package starcomment func A( - /*1*/ x /*2*/ string /*3*/, /*@codeaction("x", "x", "refactor.rewrite", starcomment)*/ + /*1*/ x /*2*/ string /*3*/ /*@codeaction("x", "x", "refactor.rewrite", starcomment)*/, /*4*/ y /*5*/ int /*6*/, ) (string, int) { return x, y @@ -200,7 +200,7 @@ func A( -- @starcomment/starcomment/starcomment.go -- package starcomment -func A(/*1*/ x /*2*/ string /*3*/, /*@codeaction("x", "x", "refactor.rewrite", starcomment)*/ /*4*/ y /*5*/ int /*6*/) (string, int) { +func A(/*1*/ x /*2*/ string /*3*/ /*@codeaction("x", "x", "refactor.rewrite", starcomment)*/, /*4*/ y /*5*/ int /*6*/) (string, int) { return x, y } From 34b319976c51d63053626c0028e37c16e65bb0c5 Mon Sep 17 00:00:00 2001 From: Fata Nugraha Date: Sun, 18 Feb 2024 01:05:45 +0800 Subject: [PATCH 08/10] Rename to join --- gopls/internal/golang/fix.go | 2 +- gopls/internal/golang/lines.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gopls/internal/golang/fix.go b/gopls/internal/golang/fix.go index 16908510b7c..4b196383175 100644 --- a/gopls/internal/golang/fix.go +++ b/gopls/internal/golang/fix.go @@ -65,7 +65,7 @@ const ( fixInlineCall = "inline_call" fixInvertIfCondition = "invert_if_condition" fixSplitLines = "split_lines" - fixJoinLines = "group_lines" + fixJoinLines = "join_lines" ) // ApplyFix applies the specified kind of suggested fix to the given diff --git a/gopls/internal/golang/lines.go b/gopls/internal/golang/lines.go index aefbef9c94d..fd492ec3a5b 100644 --- a/gopls/internal/golang/lines.go +++ b/gopls/internal/golang/lines.go @@ -21,8 +21,8 @@ import ( "golang.org/x/tools/gopls/internal/util/safetoken" ) -// CanSplitLines checks whether each item of the enclosing curly bracket/parens can be put into separate lines -// where each item occupies one line. +// 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 == "" { @@ -44,7 +44,7 @@ func CanSplitLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (s return "", false, nil } -// CanJoinLines checks whether each item of the enclosing curly bracket/parens can be joined into a single line. +// 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 == "" { @@ -66,7 +66,7 @@ func CanJoinLines(file *ast.File, fset *token.FileSet, start, end token.Pos) (st return "", false, nil } -// canSplitJoinLines determines whether we should split/group the lines or not. +// canSplitJoinLines determines whether we should split/join the lines or not. func canSplitJoinLines(items []ast.Node, comments []*ast.CommentGroup) bool { if len(items) <= 1 { return false @@ -101,7 +101,7 @@ func joinLines(fset *token.FileSet, start, end token.Pos, src []byte, file *ast. return fset, processLines(fset, items, comments, src, braceOpen, braceClose, ", ", "", "", ""), nil } -// processLines is the common operation for both split and group lines because this split/group operation is +// processLines is the common operation for both split and join lines because this split/join operation is // essentially a transformation of the separating whitespace. func processLines(fset *token.FileSet, items []ast.Node, comments []*ast.CommentGroup, src []byte, braceOpen, braceClose token.Pos, sep, prefix, suffix, indent string) *analysis.SuggestedFix { var nodes []ast.Node From bb36847d03390edc06a4b7c08e9b8fa095de2997 Mon Sep 17 00:00:00 2001 From: Fata Nugraha Date: Sat, 24 Feb 2024 06:18:33 +0800 Subject: [PATCH 09/10] Address comment --- gopls/internal/golang/lines.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/gopls/internal/golang/lines.go b/gopls/internal/golang/lines.go index fd492ec3a5b..17e9db454dc 100644 --- a/gopls/internal/golang/lines.go +++ b/gopls/internal/golang/lines.go @@ -13,6 +13,7 @@ import ( "go/ast" "go/token" "go/types" + "slices" "sort" "strings" @@ -104,21 +105,21 @@ func joinLines(fset *token.FileSet, start, end token.Pos, src []byte, file *ast. // processLines is the common operation for both split and join lines because this split/join operation is // essentially a transformation of the separating whitespace. func processLines(fset *token.FileSet, items []ast.Node, comments []*ast.CommentGroup, src []byte, braceOpen, braceClose token.Pos, sep, prefix, suffix, indent string) *analysis.SuggestedFix { - var nodes []ast.Node - nodes = append(nodes, items...) + nodes := slices.Clone(items) - // box *ast.CommentGroup to *ast.Node for easier processing later. + // box *ast.CommentGroup to ast.Node for easier processing later. for _, cg := range comments { nodes = append(nodes, cg) } + // Sort to interleave comments and nodes. sort.Slice(nodes, func(i, j int) bool { return nodes[i].Pos() < nodes[j].Pos() }) edits := []analysis.TextEdit{ { - Pos: braceOpen + 1, + Pos: token.Pos(int(braceOpen) + len("{")), End: nodes[0].Pos(), NewText: []byte(prefix + indent), }, @@ -143,6 +144,8 @@ func processLines(fset *token.FileSet, items []ast.Node, comments []*ast.Comment posOffset := safetoken.EndPosition(fset, pos).Offset endOffset := safetoken.StartPosition(fset, end).Offset if bytes.IndexByte(src[posOffset:endOffset], ',') == -1 { + // nodes[i] or nodes[i-1] is a comment hence no delimiter in between + // in such case, do nothing. continue } From d3c5b57d81f45864e79d7d86444b5b7892b06ffc Mon Sep 17 00:00:00 2001 From: Fata Nugraha Date: Tue, 27 Feb 2024 21:01:01 +0800 Subject: [PATCH 10/10] Use internal slice --- gopls/internal/golang/lines.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gopls/internal/golang/lines.go b/gopls/internal/golang/lines.go index 17e9db454dc..1c4b562280d 100644 --- a/gopls/internal/golang/lines.go +++ b/gopls/internal/golang/lines.go @@ -13,13 +13,13 @@ import ( "go/ast" "go/token" "go/types" - "slices" "sort" "strings" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/ast/astutil" "golang.org/x/tools/gopls/internal/util/safetoken" + "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