Allow bracket pairs to share open tokens or close tokens in the colorizer #132504

DragWx · 2021-09-07T02:40:40Z

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 the Token class (from tokenizer.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.ts was updated to account for the new pairsWith property, and a very basic test for the correct category and pairsWith properties (plus all the other properties too, but especially these) of tokens loaded from languages was added to a new file brackets.test.ts.

This pull request doesn't address any other issues, only bracket sharing.

ghost · 2021-09-07T02:40:53Z

All CLA requirements met.

hediet · 2021-09-10T10:22:07Z

src/vs/editor/common/model/bracketPairColorizer/ast.ts

@@ -458,9 +458,11 @@ export class InvalidBracketAstNode extends BaseAstNode {

 	public readonly unopenedBrackets: SmallImmutableSet<number>;

-	constructor(category: number, length: Length, denseKeyProvider: DenseKeyProvider<number>) {
+	constructor(pairsWith: number[], length: Length, denseKeyProvider: DenseKeyProvider<number>) {


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

At least, pairsWith should also be an SmallImmutableSet I think.

I agree, re: SmallImmutableSet. I can make that change if you'd like.

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.

I think there should be no performance impact for languages that just have { ... }, ( ... ) and [...].

Probably SmallImmutableSet can do that.

hediet · 2021-09-10T10:53:24Z

src/vs/editor/common/model/bracketPairColorizer/brackets.ts

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


This code duplication is not so nice.

Agreed, I can make it less redundant.

DragWx · 2021-09-11T00:02:22Z

The Token class is changed such that category and pairsWith are now SmallImmutableSet<number> | null type.
They are only null when token is neither an opening nor closing bracket.
They are now directly compared with each other using intersects, which eliminated the need for DenseKeyProvider in parser.ts.
DenseKeyProvider was instead added to brackets.ts for use when scanning in brackets from language definition.
Tests updated to match updated Token class definition.

hediet · 2021-09-17T18:08:00Z

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.

…icrosoft#132209)

DragWx · 2021-09-18T20:30:16Z

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!

hediet · 2021-09-20T07:23:33Z

Thank you!

lramos15 assigned hediet Sep 7, 2021

hediet reviewed Sep 10, 2021

View reviewed changes

maxnordlund mentioned this pull request Sep 15, 2021

Bracket Highlight does not detect word boundary for language using word as bracket #132162

Closed

hediet force-pushed the bpc-shared-brackets branch from c0b772e to 388735b Compare September 17, 2021 18:05

Allow colorized bracket pairs to share opening or closing brackets (m…

65cb8e5

…icrosoft#132209)

DragWx force-pushed the bpc-shared-brackets branch from 848b2ba to 65cb8e5 Compare September 18, 2021 20:26

hediet merged commit 616310c into microsoft:main Sep 20, 2021

hediet added this to the September 2021 milestone Sep 20, 2021

hediet linked an issue Sep 20, 2021 that may be closed by this pull request

[Feature Request] Bracket Pair Colorization: Allow bracket pairs to share open tokens or close tokens #132209

Closed

hediet mentioned this pull request Sep 20, 2021

[Feature Request] Bracket Pair Colorization: Allow bracket pairs to share open tokens or close tokens #132209

Closed

hellbill approved these changes Oct 21, 2021

View reviewed changes

github-actions bot locked and limited conversation to collaborators Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow bracket pairs to share open tokens or close tokens in the colorizer #132504

Allow bracket pairs to share open tokens or close tokens in the colorizer #132504

DragWx commented Sep 7, 2021

ghost commented Sep 7, 2021 •

edited by ghost

Loading

hediet Sep 10, 2021 •

edited

Loading

hediet Sep 10, 2021

DragWx Sep 10, 2021

DragWx Sep 10, 2021

hediet Sep 10, 2021

hediet Sep 10, 2021

DragWx Sep 10, 2021

DragWx commented Sep 11, 2021

hediet commented Sep 17, 2021

DragWx commented Sep 18, 2021

hediet commented Sep 20, 2021

Allow bracket pairs to share open tokens or close tokens in the colorizer #132504

Allow bracket pairs to share open tokens or close tokens in the colorizer #132504

Conversation

DragWx commented Sep 7, 2021

ghost commented Sep 7, 2021 • edited by ghost Loading

hediet Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

hediet Sep 10, 2021

Choose a reason for hiding this comment

DragWx Sep 10, 2021

Choose a reason for hiding this comment

DragWx Sep 10, 2021

Choose a reason for hiding this comment

hediet Sep 10, 2021

Choose a reason for hiding this comment

hediet Sep 10, 2021

Choose a reason for hiding this comment

DragWx Sep 10, 2021

Choose a reason for hiding this comment

DragWx commented Sep 11, 2021

hediet commented Sep 17, 2021

DragWx commented Sep 18, 2021

hediet commented Sep 20, 2021

ghost commented Sep 7, 2021 •

edited by ghost

Loading

hediet Sep 10, 2021 •

edited

Loading