Skip to content

VS Code: fix sub-grammars honoring embed ends - #435

Open
dmarcotte wants to merge 3 commits into
kson-org:mainfrom
dmarcotte:fix-unending-embed-highlights-bug
Open

dmarcotte wants to merge 3 commits into
kson-org:mainfrom
dmarcotte:fix-unending-embed-highlights-bug

Conversation

@dmarcotte

Copy link
Copy Markdown
Contributor

In many cases (including any time an embed block's end delimiter was not on its own line), the highlighting from an embedded grammar was breaking out of its embed block because it was not configured to properly detect the embed block's end.

The root cause is that TextMate only tests the end of the rule when it is not currently inside another highlighting rule, so injected grammars with open multi-line constructs (a JS comment, for instance, or Markdown's indented-code or paragraph rule), were never tested against our end pattern.

Fix this by wrapping each injected grammar in a while rule expressing "this line holds no end-delimiter". The engine re-tests while conditions at the start of each line, so we never miss being consulted.

Note that we modified the generator to write a rule per embed delimiter since the while condition has no way to know which embed delimiter opened its block.

The one very minor tradeoff here is that due to the limitations of TextMate's line-by-line testing, the last line of an embed block which does not have a newline before its embed-end goes unhighlighted by the injected grammar. I call this tradeoff "very minor" because the formatter always puts the embed end delimiter on its own line (lined up with the block's minimum indent), so this will be rare in practice.

In many cases (including any time an embed block's end delimiter was
not on its own line), the highlighting from an embedded grammar was
breaking out of its embed block because it was not configured to
properly detect the embed block's end.

The root cause is that TextMate only tests the `end` of the rule when it
is not currently inside another highlighting rule, so injected grammars
with open multi-line constructs (a JS comment, for instance, or
Markdown's indented-code or paragraph rule), were never tested against
our `end` pattern.

Fix this by wrapping each injected grammar in a `while` rule expressing
"this line holds no end-delimiter". The engine re-tests `while`
conditions at the start of each line, so we never miss being consulted.

Note that we modified the generator to write a rule per embed delimiter
since the `while` condition has no way to know which embed delimiter
opened its block.

The one very minor tradeoff here is that due to the limitations
of TextMate's line-by-line testing, the last line of an embed block
which does not have a newline before its embed-end goes unhighlighted by
the injected grammar. I call this tradeoff "very minor" because the
formatter always puts the embed end delimiter on its own line (lined up
with the block's minimum indent), so this will be rare in practice.
`generate-tm-embed-block.ts` now writes a correct and typo-free
reference back to itself in its generated content.

@joeslice joeslice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved:
Nice simplifications in this one.

: lang.contentName;

// Build the patterns array
const noEndDelimiterOnLine = `(^|\\G)(?!.*${endDelimiterPattern})`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, reading about \G sent me down a fun TextMate rabbit hole.

// that ends the tag. The lookahead is non-capturing so `end` can backreference \1.
begin: `(%|\\$)[ \\t\\r]*${langPattern}${isGeneric ? '' : '(?=[ \\t\\r]|$)'}(:|.|\\s)*?$`,
// that ends the tag.
begin: `(${delimiterPattern})[ \\t\\r]*${langPattern}${isGeneric ? '' : '(?=[ \\t\\r]|$)'}.*?$`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess these capture groups aren't really necessary, but doesn't seem to hurt. Especially if we later want to use them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants