Allow bracket pairs to share open tokens or close tokens in the colorizer #132504
Conversation
|
|
| public readonly unopenedBrackets: SmallImmutableSet<number>; | ||
|
|
||
| constructor(category: number, length: Length, denseKeyProvider: DenseKeyProvider<number>) { | ||
| constructor(pairsWith: number[], length: Length, denseKeyProvider: DenseKeyProvider<number>) { |
There was a problem hiding this comment.
Have you tried using category: number instead?
All this code is highly performance critical, and it would be nice if we don't need an array here.
Basically, if a symbol begin1 is closed by end and begin2 is also closed by end, then begin1 and begin2 currently have distinct categories. Even more, end is registered twice with both the category of begin1 and of begin2. However, only the first registration of end is actually being matched, so begin2 and end never match.
I think a more performant solution would be to assign begin1 and begin2 the same category (which makes sense, as they are interchangeable from the perspective of bracket pair colorization). Then everything else would work out of the box.
Hmm, maybe this does not work.
Let's say begin11 and begin12 are both closed by end1.
Also, begin2 is closed by both end1 and end2.
Then, unfortunately, end1 and end2 aren't interchangeable.
begin2 ---------
begin12 | These should match
end2 -----------
There was a problem hiding this comment.
At least, pairsWith should also be an SmallImmutableSet I think.
There was a problem hiding this comment.
I agree, re: SmallImmutableSet. I can make that change if you'd like.
There was a problem hiding this comment.
And yes, for the record, my first attempt was as you suggested, and it works for shared brackets which only have one-to-many relationships, but there are issues when brackets have many-to-many relationships. The plugin I'm using to test this scenario is LaTeX Workshop, which has brackets defined as follows:
["{", "}"],
["[", "]"],
["(", ")"],
["\\left(", "\\right)"],
["\\left(", "\\right."],
["\\left.", "\\right)"],
["\\left[", "\\right]"],
["\\left[", "\\right."],
["\\left.", "\\right]"],
["\\left\\{", "\\right\\}"],
["\\left\\{", "\\right."],
["\\left.", "\\right\\}"],
["\\left<", "\\right>"]Trying, for example, to get \left( \right., \left. \right), but not \left. \right. to match is what made me jump to using a pairsWith list.
There was a problem hiding this comment.
I think there should be no performance impact for languages that just have { ... }, ( ... ) and [...].
Probably SmallImmutableSet can do that.
| let currIdx = idxOffset; | ||
|
|
||
| let existingOpeningBracket = categoryCache.get(pair[0]); | ||
| if (existingOpeningBracket === undefined) { | ||
| // Opening bracket is new, so create a new category for it and add it. | ||
| tokens.addBracket(languageId, pair[0], TokenKind.OpeningBracket, idxOffset, []); | ||
| categoryCache.set(pair[0], idxOffset); | ||
| idxOffset++; | ||
| } else { | ||
| // Opening bracket exists already, remember its category. | ||
| currIdx = existingOpeningBracket; | ||
| } | ||
|
|
||
| let existingClosingBracket = pairsWithCache.get(pair[1]); | ||
| if (existingClosingBracket === undefined) { | ||
| // Closing bracket is new, so tie it to the opening bracket's category and add it. | ||
| tokens.addBracket(languageId, pair[1], TokenKind.ClosingBracket, -1, [currIdx]); | ||
| pairsWithCache.set(pair[1], [currIdx]); | ||
| } else { | ||
| // Closing bracket exists already, so update its pairsWith property with the opening bracket's category. | ||
| existingClosingBracket.push(currIdx); | ||
| tokens.addBracket(languageId, pair[1], TokenKind.ClosingBracket, -1, existingClosingBracket); | ||
| } |
There was a problem hiding this comment.
This code duplication is not so nice.
There was a problem hiding this comment.
Agreed, I can make it less redundant.
|
The |
c0b772e to
388735b
Compare
|
Thanks for the PR! I did some refactorings and will merge this PR soon. I'd be very happy if you could fix the tests and do a git squash/rebase on the latest main! I got rid of the "category" which tbh. was a design mistake I did. I made it very clear now that all bracket ids refer to the opening bracket. Also, there should be no performance impact for languages that don't share brackets. |
848b2ba to
65cb8e5
Compare
|
BPC tests updated, commits squashed and rebased with latest main. This is my first time doing a squash/rebase so I hope I did it correctly! |
|
Thank you! |
This addresses issue #132209.
This change modifies the bracket pair colorizer to allow opening brackets to share the same closing bracket, and closing brackets to share the same opening bracket, which is a feature in certain languages, such as Verilog and LaTeX.
To do this, a
pairsWith: number[]property was added to theTokenclass (fromtokenizer.ts), used by closing brackets only, to allow them to specify multiple category numbers (used by opening brackets) to which they can match.When loading bracket pairs from languages, new category numbers are only given to unique opening brackets, and duplicate closing brackets get the associated opening bracket's category number added to their pairsWith property.
Finally, every instance of comparing opening bracket category numbers with closing bracket category numbers was replaced with appropriate logic to check if the opening bracket category number exists within the closing bracket's pairsWith property.
The existing test within
tokenizer.test.tswas updated to account for the newpairsWithproperty, and a very basic test for the correctcategoryandpairsWithproperties (plus all the other properties too, but especially these) of tokens loaded from languages was added to a new filebrackets.test.ts.This pull request doesn't address any other issues, only bracket sharing.