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.

DanielRosenwasser · 2025-07-02T05:28:13Z

internal/ls/folding.go

+		currentTokenEnd = curr.End()
+	}
+	scanner := scanner.GetScannerForSourceFile(sourceFile, currentTokenEnd)
+	statements.Nodes = append(statements.Nodes, sourceFile.GetOrCreateToken(scanner.Token(), scanner.TokenFullStart(), scanner.TokenEnd(), curr))


Doesn't this mutate the SourceFile's contents? Did this not cause issues when running the language server for you?

In any case, I don't think allocating an entirely new array was ever necessary. Just call visitNode at the end on the EOF. I just sent a change in Strada to do this. microsoft/TypeScript#61987

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{}

}

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

internal/ls/folding.go

+		if result == nil || isInComment(sourceFile, int(currentLineStart), nil) != nil {
+			continue
+		}
+		if result.isStart {


Suggested change

if result.isStart {

if result.isStart {

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

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

internal/ls/folding.go

+			if result.name != "" {
+				collapsedTest = result.name
+			}
+			regions = append(regions, &lsproto.FoldingRange{


Okay, there is a subtlety both here and in the original code about how the end position isn't filled in yet.

Suggested change

regions = append(regions, &lsproto.FoldingRange{

// Our spans start out with some initial data.

// On every `#endregion`, we'll come back to these `FoldingRange`s

// and fill in their EndLine/EndCharacter.

regions = append(regions, &lsproto.FoldingRange{

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

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

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

-			regions = append(regions, &lsproto.FoldingRange{
+			// Our spans start out with some initial data.
+			// On every `#endregion`, we'll come back to these `FoldingRange`s
+			// and fill in their EndLine/EndCharacter.
+			regions = append(regions, &lsproto.FoldingRange{

Folding Ranges implementation #1326

Are you sure you want to change the base?

Folding Ranges implementation #1326

Uh oh!

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!

DanielRosenwasser Jul 2, 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!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

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!

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!

Uh oh!

navya9singh Jun 30, 2025 •

edited

Loading

navya9singh Jul 1, 2025 •

edited

Loading

jakebailey Jul 1, 2025 •

edited

Loading

DanielRosenwasser Jul 2, 2025 •

edited

Loading

DanielRosenwasser Jul 3, 2025 •

edited

Loading

DanielRosenwasser Jul 3, 2025 •

edited

Loading