Folding Ranges implementation #1326

navya9singh · 2025-06-30T20:51:25Z

This PR implements folding ranges in the lsp. tsserver also had hint spans while lsp doesn't so I have combined those tests into folding tests.

into outlineViewImplementation

Copilot

Pull Request Overview

This pull request implements folding ranges support for the LSP by combining tests from tsserver and LSP, adding several utility functions and test cases to handle folding functionality in the compiler codebase. Key changes include:

Introducing new folding range support in the language service (internal/ls/folding.go) and integrating it via the LSP server (internal/lsp/server.go).
Adding a new helper function (GetLineEndOfPosition) in the scanner and refactoring certain utility functions in the printer.
Expanding the test coverage for outlining and folding with extensive tests in internal/ls/folding_test.go.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internal/scanner/scanner.go	Added GetLineEndOfPosition to compute the end position of a line.
internal/printer/utilities.go	Updated and renamed helper functions to use consistent naming conventions.
internal/lsp/server.go	Added folding range handling in LSP server.
internal/ls/utilities.go	Adjusted LSP range creation using astnav.GetStartOfNode.
internal/ls/folding_test.go	Introduced comprehensive tests for folding range functionality.
internal/ls/folding.go	Implemented folding range creation and management functions.
internal/ast/utilities.go	Added helper isDeclarationKind to classify declaration nodes.

Comments suppressed due to low confidence (1)

internal/scanner/scanner.go:2300

[nitpick] The naming of GetLineEndOfPosition is very similar to GetEndLinePosition, which could be confusing. Consider standardizing the naming to more clearly distinguish their roles (e.g., by using a consistent verb pattern).

func GetLineEndOfPosition(sourceFile ast.SourceFileLike, pos int) int {

Copilot · 2025-06-30T20:51:56Z

internal/ls/folding_test.go

+		},
+		{
+			title: "outliningSpansForArguments",
+			input: `console.log(123, 456)l;


There appears to be an extra 'l' at the end of the console.log statement; please verify if this was intended or if it is a typo.

Suggested change

input: `console.log(123, 456)l;

input: `console.log(123, 456);

internal/ast/utilities.go

internal/lsp/server.go

into outlineViewImplementation

jakebailey · 2025-06-30T23:22:09Z

internal/ls/folding_test.go

-		  [|/**
-		    * Description
-		    *
-		    * @param {string} param
-		    * @returns
-		    */|]


Does this crash, and that's why it's being removed? Can we leave the comment in and make sure we don't crash?

If we do crash, the editor will pop up an error message.

It doesn't crash, the range is just returned twice because of the IsDeclarationNode(node) change. So, in Strada this range would've returned once because we checked for isDeclarationKind(node.Kind) instead of IsDeclarationNode(node) returning true here. The behavior still remains the same in the editor even if it is returned twice, I tested it.

Gotcha; that seems fine, though it seems like visitNode could just skip any JSDoc nodes too?

Same reason for the other change in that same commit.
Btw, why is it that we're testing IsDeclarationNode(node) and not isDeclarationKind(node.Kind)?

With the new reparsed JS AST, JSDoc nodes are not actually declaration nodes anymore, as we should be inserting into the tree real nodes instead. But, that all is in flux.

I compared the debugging here with Strada, it's not exactly JSDoc, what's happening is that when the node is a BinaryExpression, then it returns true for IsDeclarationNode(node) which did not use to be the case in Strada for isDeclarationKind(node.Kind). This is causing the duplication of that range.

Oh, bleh, because BinaryExpressions are currently Declarations for JSDoc reasons. Can you easily special case that for now until that is resolved?

jakebailey · 2025-07-01T20:37:40Z

internal/ls/utilities.go

@@ -390,7 +390,7 @@ func isInRightSideOfInternalImportEqualsDeclaration(node *ast.Node) bool {
 }

 func (l *LanguageService) createLspRangeFromNode(node *ast.Node, file *ast.SourceFile) *lsproto.Range {
-	return l.createLspRangeFromBounds(node.Pos(), node.End(), file)
+	return l.createLspRangeFromBounds(astnav.GetStartOfNode(node, file, false /*includeJSDoc*/), node.End(), file)


This seems weird; do we actually want to do that for all nodes? What happens if we need to make a range out of a JSDoc range itself? (Should your calls be using the other helper below?)

I think this is actually correct. In Strada this is equivalent to createTextSpanFromNode, which uses node.getStart() as the start of the span.

Yeah, that's why I changed it here.

If that's how it was, then that seems fine, and maybe we can close the other PR?

internal/ls/folding.go

gabritto · 2025-07-02T17:41:53Z

internal/ls/folding_test.go

@@ -0,0 +1,1445 @@
+package ls_test


Any reason why those are unit tests and not fourslash tests? I think eventually we want to drop the services unit tests in favor of fourslash tests, so it would be nice to have them either in this PR or in a follow-up. I think all of the existing fourslash basics are in place for implementing verify.outliningSpansInCurrentFile, but if there's something missing, let me know.

Right, I'll send that as a follow-up PR

into outlineViewImplementation

DanielRosenwasser · 2025-07-03T18:06:17Z

internal/ls/folding.go

+	// Includes the EOF Token so that comments which aren't attached to statements are included
+	var curr *ast.Node
+	currentTokenEnd := 0
+	if statements != nil && statements.Nodes != nil {
+		curr = statements.Nodes[len(statements.Nodes)-1]
+		currentTokenEnd = curr.End()
+	}
+	scanner := scanner.GetScannerForSourceFile(sourceFile, currentTokenEnd)
+	foldingRange = append(foldingRange, visitNode(sourceFile.GetOrCreateToken(scanner.Token(), scanner.TokenFullStart(), scanner.TokenEnd(), curr), depthRemaining, sourceFile, l)...)
+	return foldingRange


Thanks to #1257 from @Andarist, you can now just write:

Suggested change

// Includes the EOF Token so that comments which aren't attached to statements are included

var curr *ast.Node

currentTokenEnd := 0

if statements != nil && statements.Nodes != nil {

curr = statements.Nodes[len(statements.Nodes)-1]

currentTokenEnd = curr.End()

}

scanner := scanner.GetScannerForSourceFile(sourceFile, currentTokenEnd)

foldingRange = append(foldingRange, visitNode(sourceFile.GetOrCreateToken(scanner.Token(), scanner.TokenFullStart(), scanner.TokenEnd(), curr), depthRemaining, sourceFile, l)...)

return foldingRange

// Visit the EOF Token so that comments which aren't attached to statements are included.

foldingRange = append(foldingRange, visitNode(sourceFile.EndOfFileToken, depthRemaining, sourceFile, l)...)

return foldingRange

DanielRosenwasser · 2025-07-03T18:34:06Z

internal/ls/folding.go

+		} else if res[i].StartCharacter != nil && res[j].StartCharacter != nil {
+			return *res[i].StartCharacter < *res[j].StartCharacter
+		}
+		return *res[i].EndCharacter < *res[j].EndCharacter


Shouldn't you need to check that EndCharacter not also possibly nil? And only check this if ~~StartCharacter~~ EndLine differs?

On top of that, the sorting here is off in a different way.

The original code sorts only by span starts. We don't sort by ends/length (and I guess we have no tests that are affected by that - so I don't have a strong opinion about sorting by the Ends at all).

But the main issue - we don't have span starts here. Instead we have only StartLine and StartCharacter. Those two are the primary things we need to sort by, before anything about the End.

We likely don't have any examples that will be affected - but we should get the sorting right here.

DanielRosenwasser · 2025-07-03T18:53:30Z

internal/ls/folding.go

+			collapsedTest := "#region"
+			if result.name != "" {
+				collapsedTest = result.name
+			}
+			regions = append(regions, &lsproto.FoldingRange{
+				StartLine:      span.Line,
+				StartCharacter: &span.Character,
+				Kind:           &foldingRangeKindRegion,
+				CollapsedText:  &collapsedTest,


Suggested change

collapsedTest := "#region"

if result.name != "" {

collapsedTest = result.name

}

regions = append(regions, &lsproto.FoldingRange{

StartLine: span.Line,

StartCharacter: &span.Character,

Kind: &foldingRangeKindRegion,

CollapsedText: &collapsedTest,

collapsedText := "#region"

if result.name != "" {

collapsedText = result.name

}

regions = append(regions, &lsproto.FoldingRange{

StartLine: span.Line,

StartCharacter: &span.Character,

Kind: &foldingRangeKindRegion,

CollapsedText: &collapsedText,

DanielRosenwasser · 2025-07-03T18:57:08Z

internal/ls/folding.go

+					if out == nil {
+						out = []*lsproto.FoldingRange{}
+					}


I don't think you need to do this, append will just handle this.

Suggested change

if out == nil {

out = []*lsproto.FoldingRange{}

}

internal/ls/folding.go

DanielRosenwasser · 2025-07-03T19:03:10Z

internal/ls/folding.go

+			if len(regions) > 0 {
+				region := regions[len(regions)-1]
+				regions = regions[:len(regions)-1]
+				if region != nil {


You don't need the nil check here, the code would have crashed if you indexed out-of-bounds but you had the len(regions) > 0 check above.

DanielRosenwasser · 2025-07-03T19:03:21Z

internal/ls/folding.go

+			continue
+		}
+		if result.isStart {
+			span := l.createLspPosition(strings.Index(sourceFile.Text()[currentLineStart:lineEnd], "//")+int(currentLineStart), sourceFile)


This isn't a span though, this is a position. I would call it commentStart or something like that

internal/ls/folding.go

into outlineViewImplementation

jakebailey · 2025-07-07T18:26:36Z

internal/ls/folding.go

+	_, sourceFile := l.getProgramAndFile(documentURI)
+	res := l.addNodeOutliningSpans(sourceFile)
+	res = append(res, l.addRegionOutliningSpans(sourceFile)...)
+	sort.Slice(res, func(i, j int) bool {


This should be slices.SortFunc; sort.Slice is old-school

jakebailey · 2025-07-07T18:31:29Z

internal/ls/folding.go

+func parseRegionDelimiter(lineText string) *regionDelimiterResult {
+	// We trim the leading whitespace and // without the regex since the
+	// multiple potential whitespace matches can make for some gnarly backtracking behavior
+	lineText = strings.TrimLeft(lineText, " \t")


The original code said lineText = lineText.trimStart(), so this code should say strings.TrimLeftFunc(lineText, unicode.IsSpace).

jakebailey · 2025-07-07T18:31:59Z

internal/ls/folding.go

+	if !strings.HasPrefix(lineText, "//") {
+		return nil
+	}
+	lineText = strings.TrimLeft(lineText[2:], " \t")


The original code trimmed both sides, so should be strings.TrimSpace(lineText[2:])

into outlineViewImplementation

DanielRosenwasser · 2025-07-08T23:31:52Z

internal/ls/folding.go

+		if a.StartLine != b.StartLine {
+			return cmp.Compare(a.StartLine, b.StartLine)
+		}
+		if a.StartCharacter != nil && b.StartCharacter != nil {
+			return cmp.Compare(*a.StartCharacter, *b.StartCharacter)
+		}
+		if a.EndLine != b.EndLine {
+			return cmp.Compare(a.EndLine, b.EndLine)
+		}
+		if a.EndCharacter != nil && b.EndCharacter != nil {
+			return cmp.Compare(*a.EndCharacter, *b.EndCharacter)
+		}
+		return 0


If we're sorting by the ends at all, you can't immediately return cmp.Compare(...) on StartCharacter - you have to check that they're actually different like you do in every other branch.

One way you could do this:

Suggested change

if a.StartLine != b.StartLine {

return cmp.Compare(a.StartLine, b.StartLine)

}

if a.StartCharacter != nil && b.StartCharacter != nil {

return cmp.Compare(*a.StartCharacter, *b.StartCharacter)

}

if a.EndLine != b.EndLine {

return cmp.Compare(a.EndLine, b.EndLine)

}

if a.EndCharacter != nil && b.EndCharacter != nil {

return cmp.Compare(*a.EndCharacter, *b.EndCharacter)

}

return 0

if a.StartLine != b.StartLine {

return cmp.Compare(a.StartLine, b.StartLine)

}

if a.StartCharacter != nil && b.StartCharacter != nil && *a.StartCharacter != *b.StartCharacter {

return cmp.Compare(*a.StartCharacter, *b.StartCharacter)

}

if a.EndLine != b.EndLine {

return cmp.Compare(a.EndLine, b.EndLine)

}

if a.EndCharacter != nil && b.EndCharacter != nil && *a.EndCharacter != *b.EndCharacter {

return cmp.Compare(*a.EndCharacter , *b.EndCharacter)

}

return 0

Another way you could do this:

Suggested change

if a.StartLine != b.StartLine {

return cmp.Compare(a.StartLine, b.StartLine)

}

if a.StartCharacter != nil && b.StartCharacter != nil {

return cmp.Compare(*a.StartCharacter, *b.StartCharacter)

}

if a.EndLine != b.EndLine {

return cmp.Compare(a.EndLine, b.EndLine)

}

if a.EndCharacter != nil && b.EndCharacter != nil {

return cmp.Compare(*a.EndCharacter, *b.EndCharacter)

}

return 0

if r := cmp.Compare(a.StartLine, b.StartLine); r != 0 {

return r

}

if a.StartCharacter != nil && b.StartCharacter != nil {

if r := cmp.Compare(*a.StartCharacter, *b.StartCharacter); r != 0 {

return r

}

}

if r := cmp.Compare(a.EndLine, b.EndLine); r != 0 {

return r

}

if a.EndCharacter != nil && b.EndCharacter != nil {

if r := cmp.Compare(*a.EndCharacter, *b.EndCharacter); r != 0 {

return r

}

}

return 0

DanielRosenwasser · 2025-07-08T23:35:46Z

internal/ls/folding.go

+
+	statements := sourceFile.Statements
+	n := len(statements.Nodes)
+	var foldingRange []*lsproto.FoldingRange


I actually think starting out with a nil slice is probably not ideal. Folding ranges are never cached on our side, so maybe it's best to just start out with some reasonable initial capacity (I dunno, I'm just going to throw out a nice round number like 40?).

This isn't a blocker for this PR.

DanielRosenwasser · 2025-07-08T23:39:19Z

internal/ls/folding.go

+	lineText = strings.TrimSpace(lineText[2:])
+	result := regionDelimiterRegExp.FindStringSubmatch(lineText)
+	if result != nil {
+		return &regionDelimiterResult{


@jakebailey it's minor, but any idea if the allocation can be eliminated here? I might just make parseRegionDelimiter return result, ok, but I get wanting to preserve the original code.

DanielRosenwasser · 2025-07-08T23:41:03Z

internal/ls/folding.go

+	case ast.KindTemplateExpression, ast.KindNoSubstitutionTemplateLiteral:
+		return spanForTemplateLiteral(n, sourceFile, l)
+	case ast.KindArrayBindingPattern:
+		return spanForNode(n, ast.KindOpenBracketToken, !ast.IsBindingElement(n.Parent) /*useFullStart */, sourceFile, l)


A lot of these useFullStart comments have a trailing space on the end. Can you clean that up?

DanielRosenwasser · 2025-07-08T23:54:38Z

internal/ls/folding.go

+	}
+
+	sourceText := sourceFile.Text()
+	comments(func(comment ast.CommentRange) bool {


Not that you can't, but why was this written this way instead of for comment := range GetLeadingCommentRanges?

DanielRosenwasser · 2025-07-08T23:55:10Z

internal/ls/folding.go

+	comments(func(comment ast.CommentRange) bool {
+		commentPos := comment.Pos()
+		commentEnd := comment.End()
+		// cancellationToken.throwIfCancellationRequested();


Should these be rewritten in terms of ctx.Err()? I see other places where this has been written, so I'm okay with doing that as a separate PR.

navya9singh added 5 commits June 23, 2025 14:04

porting folding

f1616c3

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

c32bbb7

into outlineViewImplementation

adding all tests

c691b7d

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

02723d4

into outlineViewImplementation

code clean up

1938da6

Copilot AI review requested due to automatic review settings June 30, 2025 20:51

Copilot AI reviewed Jun 30, 2025

View reviewed changes

jakebailey reviewed Jun 30, 2025

View reviewed changes

internal/ast/utilities.go Outdated Show resolved Hide resolved

jakebailey reviewed Jun 30, 2025

View reviewed changes

internal/lsp/server.go Outdated Show resolved Hide resolved

navya9singh added 2 commits June 30, 2025 16:18

cleanup

86df138

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

37babb6

into outlineViewImplementation

jakebailey reviewed Jun 30, 2025

View reviewed changes

adding check for binary expression

18a2737

jakebailey reviewed Jul 1, 2025

View reviewed changes

DanielRosenwasser reviewed Jul 2, 2025

View reviewed changes

internal/ls/folding.go Outdated Show resolved Hide resolved

gabritto reviewed Jul 2, 2025

View reviewed changes

navya9singh added 3 commits July 3, 2025 10:30

addressing pr comments

b7b020f

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

dd3f93d

into outlineViewImplementation

fixing test

7435726

navya9singh requested review from jakebailey, gabritto and DanielRosenwasser July 3, 2025 17:59

DanielRosenwasser requested changes Jul 3, 2025

View reviewed changes

using sourceFile.endOfFileToken

3527adb

navya9singh requested a review from DanielRosenwasser July 3, 2025 18:58

DanielRosenwasser requested changes Jul 3, 2025

View reviewed changes

navya9singh added 2 commits July 7, 2025 10:41

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

3a1f476

into outlineViewImplementation

addressing pr comments

08fbeda

jakebailey reviewed Jul 7, 2025

View reviewed changes

navya9singh added 2 commits July 7, 2025 17:08

addressing comments

30f9df7

Merge branch 'main' of https://github.com/navya9singh/typescript-go-repo

f68abaa

into outlineViewImplementation

DanielRosenwasser requested changes Jul 8, 2025

View reviewed changes

	input: `console.log(123, 456)l;
	input: `console.log(123, 456);

Folding Ranges implementation #1326

Are you sure you want to change the base?

Folding Ranges implementation #1326

Conversation

navya9singh commented Jun 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navya9singh Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navya9singh Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanielRosenwasser Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

navya9singh Jun 30, 2025 •

edited

Loading

navya9singh Jul 1, 2025 •

edited

Loading

jakebailey Jul 1, 2025 •

edited

Loading

DanielRosenwasser Jul 3, 2025 •

edited

Loading

DanielRosenwasser Jul 3, 2025 •

edited

Loading