Code folding issue with trailing closures

[NSExceptional]

The following code only gets one folding expression, when it should have two, and it should ideally stop folding so that line 57 is not part of the folded range, since it has an opening brace on it as well, so you'd need to be able to show that line to be able to fold it too.

I would like to make a contribution to fix this myself, but I'm not sure where to start except that I know this is probably where the issue lies:

https://github.com/apple/sourcekit-lsp/blob/a9242f8a910487c92b1713eecb87149e69599d4a/Sources/SourceKitLSP/Swift/FoldingRange.swift#L128-L133

Imo, an easy fix—which would also address my personal opinion that we should mimic how Xcode code folds by not consuming the line with the closing } on it—would be to just adjust those lines to always stop on the line above the trailing brace. Then we wouldn't have to special-case this fix, either. Thoughts?

[ahoppen]

Tracked in Apple’s issue tracker as rdar://121580050

[ahoppen]

The following code only gets one folding expression, when it should have two

Just to be clear, you’re saying that it should have one for each trailing closure, right?

Imo, an easy fix—which would also address my personal opinion that we should mimic how Xcode code folds by not consuming the line with the closing } on it—would be to just adjust those lines to always stop on the line above the trailing brace. Then we wouldn't have to special-case this fix, either. Thoughts?

That sounds reasonable to me.

I would like to make a contribution to fix this myself, but I'm not sure where to start except that I know this is probably where the issue lies:

Thank you 🙏🏽 You’re already at the correct place for the implementation. I would start by adding a test case to https://github.com/apple/sourcekit-lsp/blob/main/Tests/SourceKitLSPTests/FoldingRangeTests.swift. Once you have that, you should be able to set breakpoints in FoldingRangeFinder to figure out the fix. My suspicion is that we need to change some positionAfterSkippingLeadingTrivia to position in there. But I might be wrong.

[NSExceptional]

Just to be clear, you’re saying that it should have one for each trailing closure, right?

Correct 👍🏻

an easy fix would be to just adjust those lines to always stop on the line above the trailing brace

Upon further thinking, this only fixes the issue of it covering up another brace... it wouldn't necessarily cause another folding range to be added for the closure. Right?

I'll get to debugging :)

[ahoppen]

I think (haven’t verified) that sourcekit-lsp does report a folding range for the second closure as well but VS Code just doesn’t display it because it overlaps with the folding range of the previous closure.

https://github.com/apple/sourcekit-lsp/blob/d5b06c88ed1848ef65d8457ea2be0e9bef129659/Sources/SourceKitLSP/Swift/FoldingRange.swift#L128-L133 should be generating the folding range for any closure.

[ahoppen]

@NSExceptional Are you still interested in pursuing this issue?

[NSExceptional]

Hey, yes, I'm sorry, I've just had a lot on my plate recently. I will try to get to it ASAP.

[ahoppen]

Not an issue at all. Just wanted to check.

[ahoppen]

I just checked and it looks like I fixed this in #1385.