Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Folding Ranges implementation#1326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
navya9singh wants to merge19 commits intomicrosoft:main
base:main
Choose a base branch
Loading
fromnavya9singh:outlineViewImplementation

Conversation

navya9singh
Copy link
Member

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.

@CopilotCopilotAI review requested due to automatic review settingsJune 30, 2025 20:51
Copy link
Contributor

@CopilotCopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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
FileDescription
internal/scanner/scanner.goAdded GetLineEndOfPosition to compute the end position of a line.
internal/printer/utilities.goUpdated and renamed helper functions to use consistent naming conventions.
internal/lsp/server.goAdded folding range handling in LSP server.
internal/ls/utilities.goAdjusted LSP range creation using astnav.GetStartOfNode.
internal/ls/folding_test.goIntroduced comprehensive tests for folding range functionality.
internal/ls/folding.goImplemented folding range creation and management functions.
internal/ast/utilities.goAdded 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 {

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

CopilotAIJun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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);

Copilot uses AI. Check for mistakes.

jakebailey and navya9singh reacted with thumbs up emoji
Comment on lines 1104 to 1109
[|/**
* Description
*
* @param {string} param
* @returns
*/|]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
MemberAuthor

@navya9singhnavya9singhJun 30, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

navya9singh reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

navya9singh reacted with thumbs up emoji
Copy link
MemberAuthor

@navya9singhnavya9singhJul 1, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

navya9singh reacted with thumbs up emoji
@@ -390,7 +390,7 @@ func isInRightSideOfInternalImportEqualsDeclaration(node *ast.Node) bool {
}

func (l*LanguageService)createLspRangeFromNode(node*ast.Node,file*ast.SourceFile)*lsproto.Range {
returnl.createLspRangeFromBounds(node.Pos(),node.End(),file)
returnl.createLspRangeFromBounds(astnav.GetStartOfNode(node,file,false/*includeJSDoc*/),node.End(),file)
Copy link
Member

@jakebaileyjakebaileyJul 1, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This seems weird; do we actually want to do that forall 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?)

gabritto reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think this is actually correct. In Strada this is equivalent tocreateTextSpanFromNode, which usesnode.getStart() as the start of the span.

navya9singh and iisaduan reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah, that's why I changed it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@@ -0,0 +1,1445 @@
package ls_test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 implementingverify.outliningSpansInCurrentFile, but if there's something missing, let me know.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Comment on lines 67 to 76
// Includes the EOF Token so that comments which aren't attached to statements are included
varcurr*ast.Node
currentTokenEnd:=0
ifstatements!=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)...)
returnfoldingRange

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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
varcurr*ast.Node
currentTokenEnd:=0
ifstatements!=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)...)
returnfoldingRange
// Visit the EOF Token so that comments which aren't attached to statements are included.
foldingRange=append(foldingRange,visitNode(sourceFile.EndOfFileToken,depthRemaining,sourceFile,l)...)
returnfoldingRange

navya9singh reacted with heart emoji
}elseifres[i].StartCharacter!=nil&&res[j].StartCharacter!=nil {
return*res[i].StartCharacter<*res[j].StartCharacter
}
return*res[i].EndCharacter<*res[j].EndCharacter
Copy link
Member

@DanielRosenwasserDanielRosenwasserJul 3, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Shouldn't you need to check thatEndCharacter not also possiblynil? And only check this ifStartCharacterEndLine differs?

navya9singh reacted with thumbs up emoji

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 theEnds at all).

But the main issue - we don't have span starts here. Instead we have onlyStartLine andStartCharacter. Those two are the primary things we need to sort by,before anything about theEnd.

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

navya9singh reacted with heart emoji
Comment on lines 86 to 94
collapsedTest:="#region"
ifresult.name!="" {
collapsedTest=result.name
}
regions=append(regions,&lsproto.FoldingRange{
StartLine:span.Line,
StartCharacter:&span.Character,
Kind:&foldingRangeKindRegion,
CollapsedText:&collapsedTest,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

navya9singh reacted with thumbs up emoji
Comment on lines 101 to 103
ifout==nil {
out= []*lsproto.FoldingRange{}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Suggested change
ifout==nil {
out= []*lsproto.FoldingRange{}
}

navya9singh reacted with thumbs up emoji
iflen(regions)>0 {
region:=regions[len(regions)-1]
regions=regions[:len(regions)-1]
ifregion!=nil {
Copy link
Member

@DanielRosenwasserDanielRosenwasserJul 3, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

navya9singh reacted with thumbs up emoji
continue
}
ifresult.isStart {
span:=l.createLspPosition(strings.Index(sourceFile.Text()[currentLineStart:lineEnd],"//")+int(currentLineStart),sourceFile)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This isn't aspan though, this is a position. I would call itcommentStart or something like that

navya9singh reacted with thumbs up emoji
_, sourceFile := l.getProgramAndFile(documentURI)
res := l.addNodeOutliningSpans(sourceFile)
res = append(res, l.addRegionOutliningSpans(sourceFile)...)
sort.Slice(res, func(i, j int) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This should beslices.SortFunc;sort.Slice is old-school

DanielRosenwasser and navya9singh reacted with laugh emoji
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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The original code saidlineText = lineText.trimStart(), so this code should saystrings.TrimLeftFunc(lineText, unicode.IsSpace).

navya9singh reacted with thumbs up emoji
if !strings.HasPrefix(lineText, "//") {
return nil
}
lineText = strings.TrimLeft(lineText[2:], " \t")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

navya9singh reacted with thumbs up emoji
Comment on lines 23 to 35
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

One way you could do this:

Suggested change
ifa.StartLine!= b.StartLine {
returncmp.Compare(a.StartLine,b.StartLine)
}
ifa.StartCharacter!=nil&&b.StartCharacter!=nil {
returncmp.Compare(*a.StartCharacter,*b.StartCharacter)
}
ifa.EndLine!= b.EndLine {
returncmp.Compare(a.EndLine,b.EndLine)
}
ifa.EndCharacter!=nil&&b.EndCharacter!=nil {
returncmp.Compare(*a.EndCharacter,*b.EndCharacter)
}
return0
ifa.StartLine!= b.StartLine {
returncmp.Compare(a.StartLine,b.StartLine)
}
ifa.StartCharacter!=nil&&b.StartCharacter!=nil&&*a.StartCharacter!=*b.StartCharacter{
returncmp.Compare(*a.StartCharacter,*b.StartCharacter)
}
ifa.EndLine!= b.EndLine {
returncmp.Compare(a.EndLine,b.EndLine)
}
ifa.EndCharacter!=nil&&b.EndCharacter!=nil&&*a.EndCharacter!=*b.EndCharacter{
returncmp.Compare(*a.EndCharacter,*b.EndCharacter)
}
return0

Another way you could do this:

Suggested change
ifa.StartLine!= b.StartLine {
returncmp.Compare(a.StartLine,b.StartLine)
}
ifa.StartCharacter!=nil&&b.StartCharacter!=nil {
returncmp.Compare(*a.StartCharacter,*b.StartCharacter)
}
ifa.EndLine!= b.EndLine {
returncmp.Compare(a.EndLine,b.EndLine)
}
ifa.EndCharacter!=nil&&b.EndCharacter!=nil {
returncmp.Compare(*a.EndCharacter,*b.EndCharacter)
}
return0
ifr:=cmp.Compare(a.StartLine,b.StartLine);r!=0 {
returnr
}
ifa.StartCharacter!=nil&&b.StartCharacter!=nil {
ifr:=cmp.Compare(*a.StartCharacter,*b.StartCharacter);r!=0 {
returnr
}
}
ifr:=cmp.Compare(a.EndLine,b.EndLine);r!=0 {
returnr
}
ifa.EndCharacter!=nil&&b.EndCharacter!=nil {
ifr:=cmp.Compare(*a.EndCharacter,*b.EndCharacter);r!=0 {
returnr
}
}
return0

navya9singh reacted with thumbs up emoji

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

navya9singh reacted with thumbs up emoji
lineText = strings.TrimSpace(lineText[2:])
result := regionDelimiterRegExp.FindStringSubmatch(lineText)
if result != nil {
return &regionDelimiterResult{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

navya9singh reacted with thumbs up emoji
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

navya9singh reacted with thumbs up emoji
comments(func(comment ast.CommentRange) bool {
commentPos := comment.Pos()
commentEnd := comment.End()
// cancellationToken.throwIfCancellationRequested();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, it's used in a lot of places so I fix that in a separate PR.
Is that supposed to replacecancellationToken?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ithink so.

Comment on lines +140 to +142
iftokenAtPosition==nil {
tokenAtPosition=astnav.GetTokenAtPosition(file,position)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Pretty sure@gabritto omitted this intentionally in a recent PR (might be mixing details up).

The comment above says

// Unlike the TS implementation, this function *will not* compute default values for// `precedingToken` and `tokenAtPosition`.

so I think you should explicitly pass in atokenAtPosition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ah yes, thanks for catching that, I didn't realize this PR was using this function.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jakebaileyjakebaileyjakebailey left review comments

Copilot code reviewCopilotCopilot left review comments

@DanielRosenwasserDanielRosenwasserDanielRosenwasser requested changes

@gabrittogabrittoAwaiting requested review from gabritto

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@navya9singh@DanielRosenwasser@jakebailey@gabritto
[8]ページ先頭
©2009-2025 Movatter.jp