support incremental styles from multiple matching scope selectors

[keith-hall]

Fixes #133 by applying all the StyleModifier matches in score order, lowest to highest, so that higher scored styles override the lower scoring ones.

This hopefully shouldn't have too much of a performance impact because color schemes don't often have multiple scope selector rules that match a single scope stack. However, if we need to, we potentially could improve the speed by iterating over the ThemeItem matches from highest score to lowest, and stop once all (relevant) StyleModifier fields have been populated. This would avoid applying styles which would later be overridden anyway.

[keith-hall]

actually, I just realized that this isn't going to work, I'll try to explain why in #133

[keith-hall]

Looking at it again, it should work - I just needed to update get_new_style as well. One thing I'm not sure about is that there could still be a bug in the following situation:

• a color scheme contains a "single" scope selector for, say comment, which applies a foreground color and a font style
• it also contains a "multiple" scope selector for, say, comment.line - keyword, which applies a background color and a font style
• the scope stack is comment.line.ext and keyword.other.ext is pushed onto the stack. get_new_style will find the single match for comment, and the "multiple" selector won't match. But it won't return a style modifier that will drop the background color off from where it was applied previously.

[trishume]

@keith-hall ping me when you have something that you think should work and I'll review then.

[keith-hall]

I had the idea of the HighlightState storing the ThemeItems that match at each level of the scope stack. This way, when a new scope is pushed onto the stack, the styles could be built up by applying all the styles from lower levels of the stack (if the scope selector from those ThemeItems still match), followed by the styles relating to the newly matching ThemeItems, and it would avoid the problem of having to "unapply" a style.

We could then also add an optimization that checks if any of the scopes in the color scheme's selectors are a prefix of the scope just pushed onto the stack, and only perform a full check on those, similar to how get_new_style was working with single scope selectors before this PR. (I am assuming that would be faster than just doing a full scoring check on all the color scheme's multi scope selectors outright.)

This kinda relies on the scoring/precedence for those earlier matches to never change, however, which we may not be able to take for granted. I'm worried that we could potentially still end up applying styles in the wrong order. So, realistically, it seems that the optimization for just getting the new style (modifier) when a new scope is pushed onto the stack isn't easy to get right ™️. Therefore I have removed it for now as part of this PR, so that at least syntect will behave correctly, even if not optimally.

What do you think @trishume ?

[trishume]

I haven't run the benchmarks but I'm guessing this will lead to a fairly significant performance regression. I'm reluctant to regress by as much as I think this will regress, but maybe it won't be so bad. I'd also like to spend some time thinking if there's a good way to do this, clearly Sublime is fast so there must be a fast way. Unfortunately this will require a good amount of attention so I probably won't get around to it until at least this weekend, but I'll put it on my TODO list.

[trishume]

So I just benched it and currently this causes a 40% regression in end-to-end highlighting performance, because this makes the highlighting step take 8 times longer.

Unless there's convincing examples of this ruining highlighting quality I'm not willing to accept this level of performance regression as is. I'm going to try and spend a bit figuring out if I can improve performance while maintaining correctness though.

This will be a breaking API change though so I'm going to do some of that today in hopes that I can fix this and then get a release out.

[trishume]

Closing in favour of #209