Recover from unterminated at-rule preludes and function calls - #58
Recover from unterminated at-rule preludes and function calls#58Kristofer Baxter (kristofer-baxter) wants to merge 2 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.
|
For anyone reviewing this: the remaining four are staged on the fork and will In order, with the diff each will show once its base has merged:
Every branch runs its own suite green on the repository's Node version, and Happy to reorder these, or to fold any of them together, if that suits how you |
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. |
3 participants
First of the five pull requests #57 is being split into, per Romain Menke (@romainmenke)'s
suggestion there. This one fixes existing behaviour and depends on none of the
other four, so it comes first.
The problem
An unclosed parenthesis in an at-rule prelude or a function call leaks its
scope to the end of the stylesheet:
Everything after the first line becomes one token scoped
meta.at-rule.media.header.css. The rest of the file stops being highlightedas CSS. A single missing character while typing takes the whole file with it.
The change
The
endpattern of each affected region gains one alternative: bail out at a{. In these regions a brace cannot be part of the construct, so nothing elsehas to be checked and the test is a single lookahead. The closing parenthesis
moves to capture group 1 so the bail-out, which consumes nothing, does not
claim the punctuation scope.
Applied at 16 sites:
@supportsconditions, media features,@documentargument functions,
layer()in@import,calc(), the gradient, shape,timing-function, transform and misc value functions,
url(), the colorfunctions, and the functional pseudo-classes.
Why 16 and not 36
In #57 I described this as one change applied uniformly at 36 sites. That was
wrong, and splitting the branch is what exposed it.
Applied everywhere, the guard makes some malformed CSS worse than it is on
maintoday.mainhas no dedicated@containerrule, so an unterminated@container (width > {is already recovered by the generic at-rule header.Giving
@containera guarded rule of its own, with a bail that fires on any{, took that recovery away: the region swallowed the brace and leaked to theend of the file. Three regions regressed that way.
A brace is not illegal everywhere.
<general-enclosed>is( <any-value>? ),and
<any-value>admits a balanced curly block, so@media (a: {b}) {islegal CSS. Where that holds, the bail is narrowed to a
{that is not closedbefore the next brace. That recovers
@media (a: {b {without touching thelegal form, and reads only as far as that brace instead of to the end of the
line.
So the fix is 16 plain guards, one narrowed guard, and several regions left
deliberately unguarded, instead of 36 uniform ones.
What is deliberately left alone
var()fallbacks and custom function arguments are declaration values, and<declaration-value>admits a balanced curly block that legally spans lines:The legal and the malformed forms are indistinguishable within a single line,
so recovering there would cost legal CSS. Both are commented in place.
attr(),if(),style()andcycle()are declaration values too, and aresplit into their own rule so they can be exempted without exempting the rest of
the misc functions.
url()and@document url-prefix()keep no bail either. A url token maylegally contain a brace.
Strings in conditions
A string in an
@mediaor@supportscondition is now parsed by acondition-local copy of the string rule. Without it, a legal brace inside a
general-enclosed string,
@media (a: "x{"), opens the body early.The copy is local rather than a change to the shared rule because every string
in CSS uses the shared newline-escape rule. Changing its
endalteredtokenization in 13 of the 15 string contexts I measured, including plain
strings, attribute selector values and
@import. That is far more blast radiusthan this fix needs.
The single-quoted string inside
:lang()also gains the end-of-line fallbackthe shared string rule already has. Without it an unterminated range such as
:lang('enscoped every following line as string content to the end of thefile.
Two rules that had to be split
:nth-child()and:nth-of-type()shared one rule. They no longer do, becauseonly the child-indexed forms take an
of <selector>clause.:nth-of-type()keeps exactly the scopes it has on
main.Verification
The 217 existing tests pass unchanged; this branch runs 255, of which 249 pass
and 6 are skipped.
Every character of 1.02 MB of Bootstrap, Bulma and normalize.css keeps the
scope stack it has on
main. Zero characters are scoped worse. That is theproperty that mattered most while writing this: no legal CSS may be scoped
worse than before.
A matrix of malformed inputs covers thirteen regions that can hold an
unclosed parenthesis. On
main, eight of them leak to the end of the file.Six of those eight recover here, and none of the five that
mainalreadyhandles regresses.
The two that still leak both do so through a deliberate exemption:
@supports (display: grid{leaks through the declaration value inside thefeature query, and
@document url-prefix(x{through the url token. Guardingeither would cost the legal forms above.
On performance:
main@media (min-width: 40em) {+ 5000 rules@media (min-width: 40em{+ 5000 rulesThe guard costs nothing on well-formed CSS. On a file whose prelude is
unterminated the cost does change, because this grammar highlights the rest of
the file where
maindoes not. That work 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.