gopls/internal/folding_range: allow more folding ranges

When the 'lineFoldingOnly' is set, the client ignores character
positions in folding ranges. (Vscode does this, eglot does not support
floating ranges at all.) For multiline ranges delimited by (), [], or {},
the existing code will not generate a folding range if there is anything
but whitespace after the opening (, [, or { on the same line.

This seems too restrictive, as it excludes ranges starting with
func foo() { // non-godoc comment
or
var x = []string{"a",
so this CL removes that test.

Fixes: golang/go#73735
Change-Id: Ie7aa5e735542b20a72e582e407b5d6ae2b8b2d29
Reviewed-on: https://go-review.googlesource.com/c/tools/+/694295
Reviewed-by: Madeline Kalil <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: pjweinbgo

gopls/internal/golang/folding_range.go

@@ -5,7 +5,6 @@
 package golang
 
 import (
-	"bytes"
 	"cmp"
 	"context"
 	"go/ast"
@@ -65,7 +64,7 @@ func FoldingRange(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle,
 		switch n := cur.Node().(type) {
 		case *ast.BlockStmt:
 			// Fold between positions of or lines between "{" and "}".
-			start, end = getLineFoldingRange(pgf, n.Lbrace, n.Rbrace, lineFoldingOnly)
+			start, end = bracketedFoldingRange(n.Lbrace, n.Rbrace)
 
 		case *ast.CaseClause:
 			// Fold from position of ":" to end.
@@ -77,19 +76,19 @@ func FoldingRange(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle,
 
 		case *ast.CallExpr:
 			// Fold between positions of or lines between "(" and ")".
-			start, end = getLineFoldingRange(pgf, n.Lparen, n.Rparen, lineFoldingOnly)
+			start, end = bracketedFoldingRange(n.Lparen, n.Rparen)
 
 		case *ast.FieldList:
 			// Fold between positions of or lines between opening parenthesis/brace and closing parenthesis/brace.
-			start, end = getLineFoldingRange(pgf, n.Opening, n.Closing, lineFoldingOnly)
+			start, end = bracketedFoldingRange(n.Opening, n.Closing)
 
 		case *ast.GenDecl:
 			// If this is an import declaration, set the kind to be protocol.Imports.
 			if n.Tok == token.IMPORT {
 				kind = protocol.Imports
 			}
 			// Fold between positions of or lines between "(" and ")".
-			start, end = getLineFoldingRange(pgf, n.Lparen, n.Rparen, lineFoldingOnly)
+			start, end = bracketedFoldingRange(n.Lparen, n.Rparen)
 
 		case *ast.BasicLit:
 			// Fold raw string literals from position of "`" to position of "`".
@@ -99,7 +98,7 @@ func FoldingRange(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle,
 
 		case *ast.CompositeLit:
 			// Fold between positions of or lines between "{" and "}".
-			start, end = getLineFoldingRange(pgf, n.Lbrace, n.Rbrace, lineFoldingOnly)
+			start, end = bracketedFoldingRange(n.Lbrace, n.Rbrace)
 
 		default:
 			panic(n)
@@ -136,9 +135,9 @@ func FoldingRange(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle,
 	return ranges, nil
 }
 
-// getLineFoldingRange returns the folding range for nodes with parentheses/braces/brackets
+// bracketedFoldingRange returns the folding range for nodes with parentheses/braces/brackets
 // that potentially can take up multiple lines.
-func getLineFoldingRange(pgf *parsego.File, open, close token.Pos, lineFoldingOnly bool) (token.Pos, token.Pos) {
+func bracketedFoldingRange(open, close token.Pos) (token.Pos, token.Pos) {
 	if !open.IsValid() || !close.IsValid() {
 		return token.NoPos, token.NoPos
 	}
@@ -147,56 +146,38 @@ func getLineFoldingRange(pgf *parsego.File, open, close token.Pos, lineFoldingOn
 		return token.NoPos, token.NoPos
 	}
 
-	if !lineFoldingOnly {
-		// Can fold between opening and closing parenthesis/brace
-		// even if they are on the same line.
-		return open + 1, close
-	}
-
 	// Clients with "LineFoldingOnly" set to true can fold only full lines.
-	// So, we return a folding range only when the closing parenthesis/brace
-	// and the end of the last argument/statement/element are on different lines.
+	// This is checked in the caller.
 	//
-	// We could skip the check for the opening parenthesis/brace and start of
-	// the first argument/statement/element. For example, the following code
+	// Clients that support folding ranges can display them in various ways
+	// (e.g., how are folding ranges marked? is the final line displayed?).
+	// The most common client
+	// is vscode, which displays the first line followed by ..., and then does not
+	// display any other lines in the range, but other clients might also display
+	// final line of the range. For example, the following code
 	//
 	//	var x = []string{"a",
 	//	"b",
 	//	"c" }
 	//
-	// can be folded to
+	// can be folded (in vscode) to
+	//
+	// var x = []string{"a", ...
+	//
+	// or in some other client
 	//
 	//	var x = []string{"a", ...
 	//	"c" }
 	//
-	// However, this might look confusing. So, check the lines of "open" and
-	// "start" positions as well.
-
-	// isOnlySpaceBetween returns true if there are only space characters between "from" and "to".
-	isOnlySpaceBetween := func(from token.Pos, to token.Pos) bool {
-		text, err := pgf.PosText(from, to)
-		if err != nil {
-			bug.Reportf("failed to get offsets: %s", err) // can't happen
-			return false
-		}
-		return len(bytes.TrimSpace(text)) == 0
-	}
-
-	nextLine := safetoken.Line(pgf.Tok, open) + 1
-	if nextLine > pgf.Tok.LineCount() {
-		return token.NoPos, token.NoPos
-	}
-	nextLineStart := pgf.Tok.LineStart(nextLine)
-	if !isOnlySpaceBetween(open+1, nextLineStart) {
-		return token.NoPos, token.NoPos
-	}
-
-	prevLineEnd := pgf.Tok.LineStart(safetoken.Line(pgf.Tok, close)) - 1 // there must be a previous line
-	if !isOnlySpaceBetween(prevLineEnd, close) {
-		return token.NoPos, token.NoPos
-	}
+	// This is a change in behavior. The old code would not fold this example,
+	// nor would it have folded
+	//
+	// func foo() { // a non-godoc comment
+	//  ...
+	// }
+	// which seems wrong.
 
-	return open + 1, prevLineEnd
+	return open + 1, close
 }
 
 // commentsFoldingRange returns the folding ranges for all comment blocks in file.

gopls/internal/settings/settings.go

@@ -1004,6 +1004,7 @@ func (o *Options) ForClientCapabilities(clientInfo *protocol.ClientInfo, caps pr
 	// Check if the client supports only line folding.
 
 	if fr := caps.TextDocument.FoldingRange; fr != nil {
+		// TODO(pjw): add telemetry
 		o.LineFoldingOnly = fr.LineFoldingOnly
 	}
 	// Check if the client supports hierarchical document symbols.

gopls/internal/test/marker/testdata/foldingrange/a_lineonly.txt

@@ -145,8 +145,8 @@ import (<0 kind="imports">
 	"fmt"
 	_ "log"
 	"sort"
-	"time"</0>
-)
+	"time"
+</0>)
 
 import _ "os"
 
@@ -157,15 +157,15 @@ func Bar() string {<2 kind="">
 	switch {<3 kind="">
 	case true:<4 kind="">
 		if true {<5 kind="">
-			fmt.Println("true")</5>
-		} else {<6 kind="">
-			fmt.Println("false")</6>
-		}</4>
+			fmt.Println("true")
+		</5>} else {<6 kind="">
+			fmt.Println("false")
+		</6>}</4>
 	case false:<7 kind="">
 		fmt.Println("false")</7>
 	default:<8 kind="">
-		fmt.Println("default")</3></8>
-	}
+		fmt.Println("default")</8>
+	</3>}
 	/* This is a multiline<9 kind="comment">
 	block
 	comment */</9>
@@ -177,93 +177,93 @@ func Bar() string {<2 kind="">
 	_ = []int{<11 kind="">
 		1,
 		2,
-		3,</11>
-	}
-	_ = [2]string{"d",
+		3,
+	</11>}
+	_ = [2]string{<12 kind="">"d",
 		"e",
-	}
-	_ = map[string]int{<12 kind="">
+	</12>}
+	_ = map[string]int{<13 kind="">
 		"a": 1,
 		"b": 2,
-		"c": 3,</12>
-	}
-	type T struct {<13 kind="">
+		"c": 3,
+	</13>}
+	type T struct {<14 kind="">
 		f string
 		g int
-		h string</13>
-	}
-	_ = T{<14 kind="">
+		h string
+	</14>}
+	_ = T{<15 kind="">
 		f: "j",
 		g: 4,
-		h: "i",</14>
-	}
+		h: "i",
+	</15>}
 	x, y := make(chan bool), make(chan bool)
-	select {<15 kind="">
-	case val := <-x:<16 kind="">
-		if val {<17 kind="">
-			fmt.Println("true from x")</17>
-		} else {<18 kind="">
-			fmt.Println("false from x")</18>
-		}</16>
-	case <-y:<19 kind="">
-		fmt.Println("y")</19>
-	default:<20 kind="">
-		fmt.Println("default")</15></20>
-	}
-	// This is a multiline comment<21 kind="comment">
-	// that is not a doc comment.</21>
-	return <22 kind="">`
+	select {<16 kind="">
+	case val := <-x:<17 kind="">
+		if val {<18 kind="">
+			fmt.Println("true from x")
+		</18>} else {<19 kind="">
+			fmt.Println("false from x")
+		</19>}</17>
+	case <-y:<20 kind="">
+		fmt.Println("y")</20>
+	default:<21 kind="">
+		fmt.Println("default")</21>
+	</16>}
+	// This is a multiline comment<22 kind="comment">
+	// that is not a doc comment.</22>
+	return <23 kind="">`
 this string
-is not indented`</2></22>
-}
+is not indented`</23>
+</2>}
 
-func _() {<23 kind="">
+func _() {<24 kind="">
 	slice := []int{1, 2, 3}
-	sort.Slice(slice, func(i, j int) bool {<24 kind="">
+	sort.Slice(<25 kind="">slice, func(i, j int) bool {<26 kind="">
 		a, b := slice[i], slice[j]
-		return a > b</24>
-	})
+		return a > b
+	</26>}</25>)
 
 	sort.Slice(slice, func(i, j int) bool { return slice[i] > slice[j] })
 
-	sort.Slice(<25 kind="">
+	sort.Slice(<27 kind="">
 		slice,
-		func(i, j int) bool {<26 kind="">
-			return slice[i] > slice[j]</26>
-		},</25>
-	)
+		func(i, j int) bool {<28 kind="">
+			return slice[i] > slice[j]
+		</28>},
+	</27>)
 
-	fmt.Println(<27 kind="">
+	fmt.Println(<29 kind="">
 		1, 2, 3,
-		4,</27>
-	)
+		4,
+	</29>)
 
-	fmt.Println(1, 2, 3,
+	fmt.Println(<30 kind="">1, 2, 3,
 		4, 5, 6,
 		7, 8, 9,
-		10)
+		10</30>)
 
 	// Call with ellipsis.
-	_ = fmt.Errorf(<28 kind="">
+	_ = fmt.Errorf(<31 kind="">
 		"test %d %d",
-		[]any{1, 2, 3}...,</28>
-	)
+		[]any{1, 2, 3}...,
+	</31>)
 
 	// Check multiline string.
-	fmt.Println(<29 kind="">
-		<30 kind="">`multi
+	fmt.Println(<32 kind="">
+		<33 kind="">`multi
 		line
 		string
-		`</30>,
-		1, 2, 3,</29>
-	)
+		`</33>,
+		1, 2, 3,
+	</32>)
 
 	// Call without arguments.
-	_ = time.Now()</23>
-}
+	_ = time.Now()
+</24>}
 
-func _(<31 kind="">
+func _(<34 kind="">
 	a int, b int,
-	c int,</31>
-) {
-}
+	c int,
+</34>) {<35 kind="">
+</35>}