Repository navigation
Fix string boundaries and bound layer() recovery - #58
Kristofer Baxter (kristofer-baxter) wants to merge 8 commits into
Conversation
An unclosed parenthesis in an at-rule prelude or a function call leaks its
scope to the end of the stylesheet. `@media (min-width: 40em{` scopes every
remaining line of the file as `meta.at-rule.media.header.css`, so the rest
of the stylesheet stops being highlighted as CSS. A single missing
character while typing takes the whole file with it.
The `end` pattern of each affected region gains one alternative: bail out
at a `{`. In these regions a brace cannot be part of the construct, so the
test is a single lookahead that reads one character. The closing
parenthesis moves to capture group 1 so that the bail-out, which consumes
nothing, does not claim the punctuation scope.
Applied at 16 sites: `@supports` conditions, media features, `@document`
argument functions, `layer()` in `@import`, `calc()`, the gradient, shape,
timing-function, transform and misc value functions, `url()`, the colour
functions, and the functional pseudo-classes.
The `@media` condition is the one place a brace can be legal, because
`<general-enclosed>` is `( <any-value>? )` and `<any-value>` admits a
balanced curly block. There the bail is narrowed to a `{` that is not
closed before the next brace, so `@media (a: {b}) {` still parses while the
unterminated form still recovers. The scan it needs is bounded by the
distance to that brace rather than by the length of the line.
Two regions are deliberately left alone. A `var()` fallback and a custom
function argument are declaration values, and `<declaration-value>` admits
a balanced curly block that legally spans lines, as in `--x: --foo({ ... })`.
The legal and the malformed forms are indistinguishable within one line, so
recovering there would cost legal CSS. Both are commented in place.
A string in an `@media` or `@supports` condition is now parsed by a
condition-local copy of the string rule rather than the shared one. Without
it, a legal brace inside a general-enclosed string, `@media (a: "x{")`,
opens the body early. The copy is local because the shared newline-escape
rule is used by every string in CSS; changing it altered tokenization in 12
of 15 measured contexts.
Verification: the 217 existing tests pass unchanged, and every character of
1.02M of Bootstrap, Bulma and normalize.css keeps the scope stack it has on
`main`. A broken-input matrix covers all ten regions that can hold an
unclosed parenthesis.
The guard costs nothing on well-formed CSS: `@media (min-width: 40em) {`
followed by 5000 rules takes 260ms here and 277ms on `main`, which is
within the noise between runs. On a file whose prelude is unterminated the
cost does change, because this grammar highlights the rest of the file
where `main` does not: 251ms against 26ms. That is the price of the fix. It
is linear in the size of the file, and it is the same work that
highlighting a well-formed file of that size already costs.
Six of the guards this branch adds could be removed without any test
failing, and two of the deliberate exemptions could be guarded without
any test failing. The existing recovery test only rejected scopes
containing `header`, `meta.function` or `scope.limit`, and a leaked
functional pseudo-class region is still `meta.selector.css`, so it
passed either way.
Pin the exact scopes instead, and cover:
- recovery for `:dir()`, `:lang()`, the `:is()` family and
`:nth-of-type()`, none of which had a distinguishing test
- recovery for a transform function in an at-rule condition
- both sides of the narrowed general-enclosed bail-out, so that
`@media (a: {b}) {` stays legal and `@media (a: {b {` recovers
- the declaration-value exemption, so that adding a guard to
`attr()`/`if()`/`style()`/`cycle()` fails
- a url token containing a brace, in `url()` and `url-prefix()`
Also replace the `of` clause assertion, which checked for the absence
of a scope name no rule can produce and so passed on main as well,
with one that pins the tokens.
|
Four follow-ups are staged on the fork, waiting on this PR:
They will be opened in this order, each rebased onto |
Can you describe what problem you are trying to solve? This change would make the syntax highlighter work considerable different from how CSS works in browsers, as you can see in this codepen: https://codepen.io/romainmenke/pen/KwWdMLQ It might give CSS authors the idea that there is an error recovery mechanism that doesn't exist. They might for example miss a single malformed block at the start of a stylesheet and be confused as to why it isn't working in a browser. |
|
If I am not mistaken, Both of these are malformed, and both are discarded by the browser. This is
The eight are what I set out to fix, six of them in this PR. One unclosed parenthesis takes the rest of the file with it. Selectors, values and strings below the error lose their scopes, and a word that happens to be a media feature name,
Your objection holds up. I put
Thirteen of thirteen discard everything that follows. That includes the unterminated string cases this PR also changes, Six of the thirteen leak on That is true of I picked recovery because of the editing case above. The other direction is available: drop the bail-outs here and make those five pseudo-classes leak as well, so that a malformed construct always signals that everything below it is dead. That is a change to scopes that exist today, which is why I did not reach for it first. Say which you prefer and I will send that instead. The four that follow add scopes for |
I personally prefer consistency with the spec and browsers. Deviating from the spec has two downsides:
I think that doing this kind of error recovery to aid in editing is self defeating as either:
Ensuring correct highlighting for any CSS that follows after a partial entry (author still typing, midway through a file) doesn't seem feasible to me. 
Interestingly I couldn't replicate the cases you describe with selectors. But I would also like to hear from other maintainers :) |
|
I am happy to follow whatever the maintainers settle on. If that is spec alignment, I will close this and send a smaller PR that makes the leaking uniform, so a malformed construct always ends highlighting for everything after it, including the pseudo-class cases that half return today. The four PRs behind this one add scopes for Waiting to hear from the other maintainers sounds right to me. I will hold off on changes until then. |
|
👋 VS Code maintainer here. I haven’t been directly involved in maintaining this grammar so far, but my team has, and I’d like to help unblock this. Thanks both for the thoughtful investigation here. I understand the concern about differing from browser error recovery. However, editor-facing parsers and tokenizers have somewhat different requirements: while someone is typing, the document will routinely be temporarily malformed, and we generally try to recover so that one incomplete construct does not degrade the editing experience for the remainder of the file. We follow this principle in several other parts of VS Code, including TypeScript parsing and bracket-pair colorization. Syntax highlighting is also not a guarantee that the source is valid or will be accepted by a browser. Correctness is better communicated through diagnostics and browser behavior than by allowing an incomplete construct to cause the rest of the file to lose meaningful highlighting. This PR appears to be quite careful about that trade-off: it preserves the existing behavior for valid CSS, exempts cases where recovery could conflict with legal syntax, adds tests for those boundaries, and makes the behavior more consistent with recovery that already happens on Given that this package exists to provide CSS support in VS Code, the editing experience should take precedence here. The change is careful, well tested, and I believe this recovery behavior is the right trade-off for an editor. I’m going to approve and merge this PR once the required checks are green. Thanks Kristofer Baxter (@kristofer-baxter) for the thorough implementation, and Romain Menke (@romainmenke) for pushing on the browser semantics -- the discussion helped clarify the trade-off and the boundaries of the change. |
|
Thanks everyone. I did some additional verification before merging and found valid CSS cases that change my conclusion about this particular implementation. The brace recovery in media conditions misidentifies braces inside valid a {
--x: calc({foo});
--y: rgb({foo});
--z: var(--fallback, calc({foo}));
}Exempting only I still believe the product principle discussed above holds: syntax highlighting is an editing aid rather than a browser-validity signal, and recovery from temporary malformed states is desirable where it can be done safely. However, preserving valid and forward-compatible CSS has to take precedence, and the current shared brace guard does not meet that requirement. My recommendation is therefore to narrow this PR:
This means accepting that the motivating Ordinary-property function recovery may still be possible by separating strict property-value contexts from unrestricted custom-property and declaration-value contexts, but that is a larger architectural change and should be considered separately rather than added to this PR. Apologies for initially moving too quickly toward merging. The additional regression cases materially change the implementation assessment. If narrowing the PR leaves too little of the original change, I’m also fine with closing it and extracting the safe fixes into a smaller follow-up. |
|
I'd say we first take this to an issue. It is much simpler to have syntax highlighting that actually matches the CSS spec. If we deviate from it with an error recovery mechanism that isn't backed by the spec and isn't documented anywhere we will make harder to maintain this package.
Signaling broken syntax is aiding the editor ;) |
Keep balanced blocks in media conditions and shared value functions. Retain the string fixes and recovery in selector and layer arguments. Replace the broad recovery matrix with tests for valid nested and multiline blocks. Remove unnecessary rule splits and document where recovery applies.
Kristofer Baxter (kristofer-baxter)
changed the title
62235ee to
fdc1c50
Compare
Add the exact custom-property example from the review. Cover balanced blocks in supports conditions, recognized functions in media conditions, and functions nested in var(), custom-function, attr() and if() arguments. Check continued-string scopes and preserve the decision not to recover shared value functions at a brace.
Keep brace recovery only in layer(). Restore selector endings to preserve comments, forgiving selector lists and general-enclosed supports conditions. Handle newlines after hexadecimal escapes and continued language strings. Add regressions for the adversarial review findings and remove the README policy section for separate discussion.
Kristofer Baxter (kristofer-baxter)
changed the title
Use shared string escape handling so hexadecimal-newline escapes and backslash continuations stay inside strings reached through nested functions. Keep unclosed-string diagnostics for unescaped newlines. Protect escaped quotes in media identifiers and unquoted function arguments. Recognize comments and escapes in language ranges without interpreting escaped slashes as comment openers. Keep comment openers out of arithmetic operators and miscellaneous argument matches so quotes inside comments cannot open strings. Add regressions for both quote styles, escape parity, line endings, nested functions, comments and malformed strings. Keep brace recovery limited to layer().
Kristofer Baxter (kristofer-baxter)
changed the title
Remove the continuation region from non-string escapes so a backslash at the end of an unquoted token cannot swallow subsequent declarations. Keep quoted-string continuations and layer-only brace recovery unchanged. Add regressions for nested functions, forgiving selectors, conditions, URL and document arguments, backslash parity, line endings, blank lines and EOF.
Keep escaped argument tokens together so identifier suffixes cannot start recognized functions. Leave unescaped block openers to the existing balanced-block rule, including after hexadecimal escape whitespace. End shared and language strings at raw form feeds while preserving escaped form feeds and hexadecimal continuation. Consume complete escapes before identifying unclosed strings. Add regressions for each review finding and controls for real function boundaries, escaped braces and valid string continuations. Keep brace recovery limited to layer().
4 participants
Fixes quoted-string continuations and termination, escaped identifier boundaries, and comment handling. Limits brace recovery to
layer()in@import; media conditions and shared value functions keep their existing endings.Adds regression tests for balanced blocks, escaped function names, multiline strings, form feeds, and following-rule scopes.
First of five changes split from #57. Four follow-ups are waiting on this PR: container queries;
@propertyand@starting-style; new value functions; and selector, property and media-feature additions.