Add list, compound, and complex Selector nodes#20088
Conversation
Introduce complexity when dealing with `,`. If no `,` values occur, then we don't have to worry about the complexity of multiple selector lists.
A `compound` node is essentially a `group` selector that we had before A `complex` node is a combinator and the nodes This reflects the real CSS selector AST a bit more where you have a simple selector, compound selector, complex selector, selector list, and a relative selector
list and compound Selector nodeslist, compound, and complex Selector nodes
WalkthroughThis pull request refactors Tailwind's selector parser to use a hierarchical AST model with explicit composite node types. The parser now produces 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tailwindcss/src/selector-parser.ts`:
- Around line 165-167: The current() helper should not fabricate an empty
compound when nodes is empty; change its logic to return nodes[0] if
nodes.length === 1, return null (or undefined) when nodes.length === 0, and only
build compound(nodes) or complex(nodes) when nodes.length > 0 (use
containsCombinator to choose complex vs compound). Apply the same fix to the
analogous helpers at the other locations referenced (the same pattern around
lines 213-227, 352-356, and 492-495) and update callers to handle the
null/undefined result so empty list items are skipped instead of becoming
compound([]).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dcf38f22-73ba-48ac-af2f-4a1295021d94
📒 Files selected for processing (3)
packages/tailwindcss/src/canonicalize-candidates.tspackages/tailwindcss/src/selector-parser.test.tspackages/tailwindcss/src/selector-parser.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tailwindcss/src/selector-parser.ts (1)
300-324:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip quoted and escaped
)while scanning non-nesting function arguments.This fallback scan stops at the first
)even when it appears inside a quoted string or after an escape. Valid selectors like:lang("en)")get split incorrectly and leave the trailing quote/paren to the outer parser.Proposed fix
- for (let j = i + 1; j < input.length; j++) { - peekChar = input.charCodeAt(j) - if (peekChar === OPEN_PAREN) { - nesting++ - continue - } - if (peekChar === CLOSE_PAREN) { - if (nesting === 0) { - i = j - break - } - nesting-- - } - } + for (let j = i + 1; j < input.length; j++) { + peekChar = input.charCodeAt(j) + + if (peekChar === BACKSLASH) { + j += 1 + continue + } + + if (peekChar === SINGLE_QUOTE || peekChar === DOUBLE_QUOTE) { + let quote = peekChar + for (j = j + 1; j < input.length; j++) { + peekChar = input.charCodeAt(j) + if (peekChar === BACKSLASH) { + j += 1 + } else if (peekChar === quote) { + break + } + } + continue + } + + if (peekChar === OPEN_PAREN) { + nesting++ + continue + } + + if (peekChar === CLOSE_PAREN) { + if (nesting === 0) { + i = j + break + } + nesting-- + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tailwindcss/src/selector-parser.ts` around lines 300 - 324, The fallback scanner that finds the closing `)` for non-nesting functions (the loop starting at `for (let j = i + 1; j < input.length; j++)` in selector-parser.ts, guarded by the `node.value !== ':not'`/`:where`/`:has`/`:is` checks) incorrectly stops on `)` inside quoted strings or when escaped; modify that loop to skip over quoted segments and escaped characters by tracking quote state and backslashes: when encountering a quote (`'` or `"`) enter a quoted mode and advance until the matching unescaped quote, and treat a backslash as escaping the next character so a `\)` is ignored; only treat `(` and `)` as nesting tokens or a closing delimiter when not inside a quote and not escaped, and then set `i = j` when the real closing `)` is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/tailwindcss/src/selector-parser.ts`:
- Around line 300-324: The fallback scanner that finds the closing `)` for
non-nesting functions (the loop starting at `for (let j = i + 1; j <
input.length; j++)` in selector-parser.ts, guarded by the `node.value !==
':not'`/`:where`/`:has`/`:is` checks) incorrectly stops on `)` inside quoted
strings or when escaped; modify that loop to skip over quoted segments and
escaped characters by tracking quote state and backslashes: when encountering a
quote (`'` or `"`) enter a quoted mode and advance until the matching unescaped
quote, and treat a backslash as escaping the next character so a `\)` is
ignored; only treat `(` and `)` as nesting tokens or a closing delimiter when
not inside a quote and not escaped, and then set `i = j` when the real closing
`)` is found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fba5333a-235c-499d-b0db-eee2b4395d16
📒 Files selected for processing (3)
integrations/cli/plugins.test.tspackages/tailwindcss/src/selector-parser.test.tspackages/tailwindcss/src/selector-parser.ts
Confidence Score: 4/5Safe to merge for everyday Tailwind usage; the only changed behaviour is in an internal variant-signature normalisation path that tests do not exercise for multi-selector The parser rewrite is well-tested and correct. The one concern is in packages/tailwindcss/src/canonicalize-candidates.ts — specifically the Reviews (2): Last reviewed commit: "add test for invalid selectors" | Re-trigger Greptile |
| // Assumption: when we have a list of selectors: `.foo, .bar` we | ||
| // want to mark this as changed because this will be re-printed as | ||
| // `.foo,.bar`. | ||
| // | ||
| // It could be that this was already optimal, but then this would be | ||
| // a no-op situation. |
There was a problem hiding this comment.
The comment says the list "will be re-printed as
.foo,.bar" (no space), but toCss now joins list nodes with ', ' and always outputs .foo, .bar. The second sentence ("this would be a no-op") is also slightly misleading for selectors that were already written without a space.
| // Assumption: when we have a list of selectors: `.foo, .bar` we | |
| // want to mark this as changed because this will be re-printed as | |
| // `.foo,.bar`. | |
| // | |
| // It could be that this was already optimal, but then this would be | |
| // a no-op situation. | |
| // Assumption: when we have a list of selectors: `.foo,.bar` we | |
| // want to mark this as changed because this will be re-printed as | |
| // `.foo, .bar` (normalized with a space after the comma). | |
| // | |
| // It could be that this was already optimal, but the re-printing | |
| // ensures a consistent canonical form. |
There was a problem hiding this comment.
It is a tiny bit misleading, but I have a follow up PR ready where we introduce minification and use that by default in the canonicalization process. Let's adjust comments there.
Just to prevent regressions
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tailwindcss/src/selector-parser.test.ts (1)
52-91: ⚡ Quick winAssert invalid-selector behavior through parse→print→parse round-trips.
This test validates parse shape, but not the “re-print keeps it invalid” contract described above it. Adding round-trip assertions here would catch serializer regressions too.
🧪 Proposed coverage extension
it('should safely parse an invalid selector list', () => { + let roundTrip = (input: string) => parse(toCss(parse(input))) + expect(parse('.foo,')).toEqual([ { kind: 'list', nodes: [ { kind: 'selector', value: '.foo' }, { kind: 'compound', nodes: [] }, ], }, ]) @@ expect(parse('.foo,,.bar')).toEqual([ { kind: 'list', nodes: [ { kind: 'selector', value: '.foo' }, { kind: 'compound', nodes: [] }, { kind: 'selector', value: '.bar' }, ], }, ]) + + expect(roundTrip('.foo,')).toEqual(parse('.foo,')) + expect(roundTrip(',.foo')).toEqual(parse(',.foo')) + expect(roundTrip(',.foo,')).toEqual(parse(',.foo,')) + expect(roundTrip('.foo,,.bar')).toEqual(parse('.foo,,.bar')) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tailwindcss/src/selector-parser.test.ts` around lines 52 - 91, The tests currently assert the AST shape returned by parse(...) but don't verify that printing and re-parsing preserves the same invalid structure; for each case (using parse('.foo,') / parse(',.foo') / parse(',.foo,') / parse('.foo,,.bar')) call the serializer (print or the module's stringify function) on the parsed result, then parse the serialized string again and assert the re-parsed AST equals the original parse(...) output; update the test to perform these parse→print→parse round-trip assertions alongside the existing parse(...) equality checks to catch serializer regressions (refer to the parse function and the repository's print/stringify function).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/tailwindcss/src/selector-parser.test.ts`:
- Around line 52-91: The tests currently assert the AST shape returned by
parse(...) but don't verify that printing and re-parsing preserves the same
invalid structure; for each case (using parse('.foo,') / parse(',.foo') /
parse(',.foo,') / parse('.foo,,.bar')) call the serializer (print or the
module's stringify function) on the parsed result, then parse the serialized
string again and assert the re-parsed AST equals the original parse(...) output;
update the test to perform these parse→print→parse round-trip assertions
alongside the existing parse(...) equality checks to catch serializer
regressions (refer to the parse function and the repository's print/stringify
function).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef2c5dcf-1806-44df-b0b0-df24bb1f4759
📒 Files selected for processing (2)
packages/tailwindcss/src/canonicalize-candidates.tspackages/tailwindcss/src/selector-parser.test.ts
This PR is a follow-up of #20088 to further improve selectors, in particular the `combinator`. This PR explicitly types the `combinator` as: ```ts type Combinator = | ' ' // Descendant combinator | '>' // Child combinator | '+' // Next-sibling combinator | '~' // Subsequent-sibling combinator ``` This allows us to explicitly test for this pattern in various places, without us having to call `.trim()` first to know what the actual combinator was. In the selector parser itself, we already did a `.trim()` to know whether we were dealing with a descendant combinator or not. With this PR, we further ensure that there is no whitespace involved aroudn these combinators. This introduces a small problem because we need to be able to re-print a selector's AST. So if we don't track whitespace, we have to re-introduce it. But there are situations where we don't want it at all (during canonicalization). To solve this, we introduced a `minify = false` option in the `SelectorParser.toCss`. If it's false (the default), then we introduce whitespace, otherwise we remove all whitespace. ## Test plan 1. All existing tests pass 2. Manual cleanup/trimming of descendants is no longer necessary --------- Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
This PR introduces a few more nodes in the
SelectorParser:listnodecomplexnodecompoundnodeThese names are closer to the CSS Selector AST names (https://developer.mozilla.org/en-US/docs/Web/CSS/Guides/Selectors/Selector_structure), and are also used in other libraries.
The problem today is that there are situations where we parse a selector like:
#a.b > .c, .das:Which is a very simple structure, but this contains a flaw that is annoying to deal with in practice: In order to determine that we are dealing with multiple selectors, we have to loop through the nodes and see if a separator occurs somewhere.
The other fun thing is that we already know the difference between selectors, combinators and separators. So if we tweak this structure a little bit during parsing, then we can answer the question from above in a much simpler way:
With this PR, we will parse the selector as:
It definitely looks more complex, but now that we have a
listnode, we already know that we are dealing with multiple selectors.If you squint your eyes, in the inner part there is a
compoundselector. This is essentially a node where each sub-node can be squished together with no spaces whatsoever.The
complexselector is there just to group everything together. In other tools, a complex selector is often represented as:While I want to have the concept of a
complexnode, I didn't go with this syntax just because I want to keep the concept ofnodeswhich means that we don't need any special handling when usingwalk(which loops over.nodesinternally).The reason this complex node exists is because otherwise you would end up with this structure:
But if you look at the
listnode now, it's not clear that we are dealing with2selectors since there are 4 nodes. We could solve this by re-introducing the separator node (,). The fact that thelistexists tells us that we're dealing withnselectors. But to know which selectors we're dealing with, then we have to look for that,node again, which introduces the original problem.This is just an internal refactor to make future changes easier.
Test plan