Skip to content

Store interpolations as opaque placeholders in Declaration#value - #39

Open
mindplay-dk wants to merge 1 commit into
hudochenkov:mainfrom
mindplay-dk:fix-raw-values-of-interpolations
Open

Store interpolations as opaque placeholders in Declaration#value#39
mindplay-dk wants to merge 1 commit into
hudochenkov:mainfrom
mindplay-dk:fix-raw-values-of-interpolations

Conversation

@mindplay-dk

Copy link
Copy Markdown

Full disclosure: I used Claude, but I did spend a few hours working on this, as best I could. 😊

I first assumed this was an issue with Stylelint:

stylelint/stylelint#9422

There are links there to demos, so please have a look at those to understand the problem.

After poking through your code, I eventually came across this guide, and I concluded the problem should most likely be fixed at the parser/syntax level in this project:

There is a way to represent raw values in the postcss model, and to the best of my understanding, your parser was putting JS values in properties where postcss (and thus Stylelint) expects to find CSS literal expressions - which leads to interpolations like ${() => '1em'} having unpredictable parse results, ultimately breaking declaration-property-max-values, which ends up counting the wrong number of values.

I have tested these modifications on a very large repository - it didn't cause any errors, or any new false positives, it cleared 100+ false positives from the results, and left me with 5 errors that were actual violations that declaration-property-max-values should catch.

So I do think we have pretty good evidence that this fix works. 😊

Below is Claude's summary of the work - it matches my somewhat limited understanding of the details, so I hope you can find the time to sanity check this.

(Note that the diff of the test is pretty bad here in Github - it breaks up in confusing ways because some tests have identical lines. You probably want to look at the updated file rather than the diff.)


Problem

Interpolations were written into Declaration#value as their raw JS source text (e.g. border-color: ${color + '40'} produced value ===
"${color + '40'}"). Any downstream consumer that runs a CSS value parser over Declaration#value — for example Stylelint's
declaration-property-max-values, which uses postcss-value-parser — then re-parses that JS as CSS and splits it on the spaces, quotes, +, :
etc. inside the expression. So ${color + '40'} is counted as 4 "values" and ${x ? '1em' : '2em'} as 5, producing false positives (and
misreporting value counts generally).

Solution

This follows the contract described in PostCSS's "Writing a Custom Syntax" guide, under Parser → Raw Values:

The default parser cleans CSS values from comments and spaces. It saves the original value with comments to node.raws.value.raw and uses it, if the node value was not changed.

Declaration#value is meant to be the semantic value, while the original source text lives in node.raws.value.raw and is emitted verbatim on
stringify (PostCSS's rawValue() returns raws.value.raw whenever raws.value.value === value).

The parser now represents each interpolation in Declaration#value as a single opaque placeholder token ($pssInterpolation, chosen to
contain no spaces, quotes or colons so a CSS value parser treats it as exactly one value), and stores the original text in
node.raws.value.raw. Interpolations mixed with static values (margin: 4px ${gap}) and multiple interpolations (${a}, ${b}) still count
correctly. Stringification is byte-for-byte identical because the untouched value falls back to raws.value.raw.

Tests

  • Updated the existing "simple interpolations → property value" cases in parse.test.js: value is now the opaque placeholder form rather
    than the raw source, and each also asserts raws.value ({ value, raw }) to lock in the semantic/original split.
  • Added regression cases in parse.test.js for the interpolated shapes that previously mis-split: string concatenation (${color + '40'}), a
    ternary (${x ? '16px' : '8px'}), and a static-value-plus-interpolation (4px ${gap}) which must remain a two-value declaration.
  • Added the same three inputs to the round-trip list in stringify.test.js to prove output is unchanged.

Interpolation source text was written directly into Declaration#value, so CSS value parsers (e.g. postcss-value-parser in Stylelint rules) re-parsed the JS as CSS and split it into multiple values, causing false positives such as declaration-property-max-values.

Represent each interpolation in Declaration#value as a single opaque placeholder and keep the original text in raws.value.raw, per PostCSS's custom-syntax raw-values contract, so value analysis sees one value and stringification stays byte-for-byte identical.
@hudochenkov

Copy link
Copy Markdown
Owner

Thank you for a good explanation of the problem!

The current behavior was specifically designed this way. That's the whole point of a custom syntax. There are many cases where we need to check the value exactly as it is present in the source file.

For example, at my work, I wrote a rule that uses declaration-property-value-disallowed-list under the hood to check that dimensional properties don't use design system spacing tokens. The rule forbids width: ${token.spacing6}, for example.

The other rule prevents developers with a Sass background from using negative values incorrectly with design system tokens. E.g., it forbids margin-left: -${token.spacing6.

To have such rules, we need the actual property value as written in the source file. The proposal in this PR will significantly reduce the usefulness of the custom syntax.

The approach the Stylelint team has taken over the years is to exclude non-standard syntax from the rule check when there is a chance it could cause false positives. The best solution, in my opinion, is to add isStandardSyntaxValue check to the declaration-property-max-values rule code.

@mindplay-dk

Copy link
Copy Markdown
Author

hmm, yes, I see the problem.

so just a thought, and this may be way too "out there", but could you ever imagine Stylelint providing a way for a custom syntax plugin to provide a (limited) parser function that plugins could call?

so like, instead of isStandardSyntaxValue perhaps the custom syntax plugin provides a way to actually parse custom syntax?

I guess maybe this is too tall an order for too little gain?

I mean, even trivial expressions like padding: ${large ? '10px 20px' : '10px'} aren't actually easy to parse, and requires the calling plugin to make two separate validations.

probably too crazy, right? 😊

I guess false positives are better than not knowing at all though? I'm not sure I'd enable an option to ignore non-CSS syntax - false positives at least encourage me to check and try to migrate to supported syntax. It's a lot of useless chore work and extra verbosity, but I guess that's the limitation we're stuck with, right? 😌

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.

3 participants