(GH-793) Add syntax folding

[glennsarti]

This commit adds the the code to return folding ranges when the server receives
a FoldingRangeRequest. This commit;

• Uses a similar method to that implemented in the VS Code extension [1]
• Uses the PowerShell tokeniser instead of the AST. This is due to the AST
  ignoring comment sections. Without the comment parsing folding for #region
  etc. will not work
• Translates the tests in VS Code [1] into equivalent C# tests

[1] PowerShell/vscode-powershell#1355

---

Note that I cannot use the AST for detecting folding regions as the AST drops the comments in the tree. So I have to use the tokens from the lexer instead (sad-panda)

[glennsarti]

@TylerLeonhardt @rjmholt This is ready for review.

[glennsarti]

@rkeithhill Fixed your comments. CI is green

[TylerLeonhardt]

Looking good! 😄

[glennsarti]

@TylerLeonhardt I've addressed your points except for the Async name change.

[rjmholt]

Haven't quite finished looking at the technical logic, but it looks really great so far!

Once we have this in, we could even look at using the AST instead for expression and block folding (since we already have the AST).

[rjmholt]

Is this based on the VSCode type? Would it be better as an enum? We could extend that as easily as a string, but it's faster to check, less error prone and can't be null.

[rjmholt]

Given that the VSCode API can have this as null, a common convention in C# is to have a None value in the enum:

enum MatchKind
{
    None = 0,
    Comment,
    Region
}

(Or similar)

[glennsarti]

See discussion above about strings and enums. This type is String (https://github.com/Microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.foldingRange.ts#L94-L99)

With 3 (at this time) well known strings (https://github.com/Microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.foldingRange.ts#L51-L67)

 * Enum of known range kinds
 */
export enum FoldingRangeKind {
	/**
	 * Folding range for a comment
	 */
	Comment = 'comment',
	/**
	 * Folding range for a imports or includes
	 */
	Imports = 'imports',
	/**
	 * Folding range for a region (e.g. `#region`)
	 */
	Region = 'region'
}

I couldn't use an enum as C# enums are ints (well, ordinal) not strings. I later just used string constants as I only needed it in one place

[rjmholt]

Is it because it's a JSON deserialisation thing? If so we should take a quick look at how we deserialise messages and see if we can use [JsonConverter(typeof(StringEnumConverter))] or something like that on the type. Being able to use enums would just be really nice here.

[glennsarti]

In this case it's serialisation only. We don't receive this from a client.

[rjmholt]

Ok, I'm good with how we do this. I looked into restructuring it to allow enums, but given that it took several changes in various places, I might save it to go with some other work I've been wanting to do.

[glennsarti]

I think I've addressed all questions again. Ready for review.

[rjmholt]

LGTM!

[TylerLeonhardt]

Had a few more comments 😄

[TylerLeonhardt]

what about using a HashSet?

HashSet<int> startLines = new HashSet();
foldableRegions.RemoveAll((item) => {
    if ( startLines.Contains(item.StartLine))
    {
        return true;
    }
    else
    {
        startLines.Add(item.StartLine);
        return false;
    }
});

• disclaimer: compiled with my mind *

[glennsarti]

I mean you can, but;

1. All of the matching functions return an array of folding references anyway
2. Not sure if a hash would be any faster
3. I need to sort the hashes anyway so may as well keep it as an array
4. The data/objects I'm manipulating here don't feel very "hash-y", they feel better represented as an array of stuff.

[TylerLeonhardt]

I could be misunderstanding the problem...

So doing a Binary search in Big-O is O(logn) on each element (O(n) makes this an O(nlogn) operation.

By using a HashSet which has an O(1) insert and lookup, that means you only have to go through each element making it an O(n) operation overall.

The HashSet suggested was only used as a helper to prevent using a search. It wouldn't be used anywhere else.

[glennsarti]

I can try and add it in...it'll make the creation logic look a bit funky. I'll slap on another commit and see what it looks like ...

I mean, we're not talking big numbers here normally, even for 200 elements do you really think there's a perceptual difference?

[glennsarti]

Ok so I did some performance testing.

Given a PowerShell script with 3840 foldable regions (Which is a HUGE amount) the FoldableRegions method took 121.4589ms. If I remove the foldableRegions.Sort(); and the duplicate checking that comes done to 120.0091ms

Net gain of 1.5ms if I do no sorting or dupe checking at all. That's a speed increase of 1%. If you factor in actually tokenizing the document first, that speed improvement becomes < 0.5%

IMO, the choice of sorting algorithm used here is statistically insignificant compared to overall operation for a large level of folding regions. The speed increase becomes practically zero if you're only talking < 100 regions.

Ref: https://gist.github.com/glennsarti/b4af7c9b8aa6bcfc0b7cd70ae027530b

[rjmholt]

I feel like rather than stripping unwanted things out of the list after the fact, the ideal would be to never add them in the first place (same deal with the nulls). But to make sure that new regions displace others, we'd need to use a new data structure to accumulate the values.

So I'm happy enough with how it's done currently, and we can look at it again later if we need to.

[glennsarti]

There is a lot of room for speed improvements in how I'm currently getting the tokens. I've raised #798 to track to the work post-merge so it doesn't get forgotten.

[rjmholt]

LGTM! Thanks @glennsarti

[rjmholt]

I feel like rather than stripping unwanted things out of the list after the fact, the ideal would be to never add them in the first place (same deal with the nulls). But to make sure that new regions displace others, we'd need to use a new data structure to accumulate the values.

So I'm happy enough with how it's done currently, and we can look at it again later if we need to.

[rjmholt]

Ok, I'm good with how we do this. I looked into restructuring it to allow enums, but given that it took several changes in various places, I might save it to go with some other work I've been wanting to do.

[rkeithhill]

LGTM. Thanks for making the code style changes!

[glennsarti]

@TylerLeonhardt I can't find any comments you raised that I haven't addressed. Is there anything else stopping approval?

[TylerLeonhardt]

LGTM! Glad to see this 😊 more editors can take advantage of this awesome feature. I left a small nit

[TylerLeonhardt]

bool defaults to false so the = false shouldn't be needed I think. Since bool is a non-nullable type

[glennsarti]

IMO, in this case, being explicit makes it more obvious what's going on. I don't feel there's any maintenance cost with leaving this in.

[TylerLeonhardt]

Works for me 👍