Skip to content

Add list, compound, and complex Selector nodes#20088

Merged
RobinMalfait merged 10 commits into
mainfrom
feat/selector-lists
May 20, 2026
Merged

Add list, compound, and complex Selector nodes#20088
RobinMalfait merged 10 commits into
mainfrom
feat/selector-lists

Conversation

@RobinMalfait

@RobinMalfait RobinMalfait commented May 20, 2026

Copy link
Copy Markdown
Member

This PR introduces a few more nodes in the SelectorParser:

  • A list node
  • A complex node
  • A compound node

These 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, .d as:

[
  { kind: 'selector', value: '#a' },
  { kind: 'selector', value: '.b' },
  { kind: 'combinator', value: ' > ' },
  { kind: 'selector', value: '.c' },
  { kind: 'separator', value: ', ' },
  { kind: 'selector', value: '.d' }
]

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:

[
  {
    kind: 'list',
    nodes: [
      {
        kind: 'complex',
        nodes: [
          {
            kind: 'compound',
            nodes: [
              { kind: 'selector', value: '#a' },
              { kind: 'selector', value: '.b' }
            ]
          },
          { kind: 'combinator', value: '>' },
          { kind: 'selector', value: '.c' }
        ]
      },
      { kind: 'selector', value: '.d' }
    ]
  }
]

It definitely looks more complex, but now that we have a list node, we already know that we are dealing with multiple selectors.

If you squint your eyes, in the inner part there is a compound selector. This is essentially a node where each sub-node can be squished together with no spaces whatsoever.

The complex selector is there just to group everything together. In other tools, a complex selector is often represented as:

{
  kind: 'complex',
  combinator: '>',
  lhs: {},
  rhs: {},
}

While I want to have the concept of a complex node, I didn't go with this syntax just because I want to keep the concept of nodes which means that we don't need any special handling when using walk (which loops over .nodes internally).

The reason this complex node exists is because otherwise you would end up with this structure:

[
  {
    kind: 'list',
    nodes: [
      {
        kind: 'compound',
        nodes: [
          { kind: 'selector', value: '#a' },
          { kind: 'selector', value: '.b' }
        ]
      },
      { kind: 'combinator', value: '>' },
      { kind: 'selector', value: '.c' }
      { kind: 'selector', value: '.d' }
    ]
  }
]

But if you look at the list node now, it's not clear that we are dealing with 2 selectors since there are 4 nodes. We could solve this by re-introducing the separator node (,). The fact that the list exists tells us that we're dealing with n selectors. 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

  1. Everything still works as expected (all tests pass)
  2. No public API breaking changes, this parser was never exposed

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
@RobinMalfait RobinMalfait requested a review from a team as a code owner May 20, 2026 12:33
@RobinMalfait RobinMalfait changed the title Add list and compound Selector nodes Add list, compound, and complex Selector nodes May 20, 2026
@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This pull request refactors Tailwind's selector parser to use a hierarchical AST model with explicit composite node types. The parser now produces SelectorListNode, SelectorComplexNode, and SelectorCompoundNode structures instead of a flat representation. The parse function was rewritten with a state machine using target, containsCombinator, currentList, and contextStack to handle nested parsing contexts. The toCss serialization function was updated to render these new node kinds. Test expectations were adjusted to verify the new AST shape for selectors, functions, combinators, and nesting. The candidate canonicalization logic was adapted to work with the restructured AST, including new helpers for detecting single selectors and updated conditions for attribute selectors, combinators, and function handling.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: introducing three new selector node types (list, compound, complex) into the parser.
Description check ✅ Passed The description is directly related to the changeset. It explains the motivation, design decisions, and structure of the three new node types being introduced.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot 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.

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

📥 Commits

Reviewing files that changed from the base of the PR and between cea8c97 and 47efcdd.

📒 Files selected for processing (3)
  • packages/tailwindcss/src/canonicalize-candidates.ts
  • packages/tailwindcss/src/selector-parser.test.ts
  • packages/tailwindcss/src/selector-parser.ts

Comment thread packages/tailwindcss/src/selector-parser.ts

@coderabbitai coderabbitai Bot 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.

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 win

Skip 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

📥 Commits

Reviewing files that changed from the base of the PR and between 47efcdd and 4b97639.

📒 Files selected for processing (3)
  • integrations/cli/plugins.test.ts
  • packages/tailwindcss/src/selector-parser.test.ts
  • packages/tailwindcss/src/selector-parser.ts

@greptile-apps

greptile-apps Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5

Safe 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 :is() arguments.

The parser rewrite is well-tested and correct. The one concern is in createVariantSignatureCache, where two :is() normalisation checks were not updated to account for the new list and compound node wrapping: multi-selector :is(.foo, .bar) now loses its wrapper during signature computation (changing the hash), and the *:pseudo simplification branch is unreachable for compound selectors. No existing tests cover these paths, and real-world Tailwind variants are unlikely to hit them, but the behaviour is different from before the PR.

packages/tailwindcss/src/canonicalize-candidates.ts — specifically the createVariantSignatureCache walk at lines 2774–2789.

Reviews (2): Last reviewed commit: "add test for invalid selectors" | Re-trigger Greptile

Comment on lines +2758 to +2763
// 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.

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.

P2 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.

Suggested change
// 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (1)
packages/tailwindcss/src/selector-parser.test.ts (1)

52-91: ⚡ Quick win

Assert 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4b97639 and f458458.

📒 Files selected for processing (2)
  • packages/tailwindcss/src/canonicalize-candidates.ts
  • packages/tailwindcss/src/selector-parser.test.ts

@RobinMalfait RobinMalfait merged commit fc17df0 into main May 20, 2026
7 checks passed
@RobinMalfait RobinMalfait deleted the feat/selector-lists branch May 20, 2026 13:04
RobinMalfait added a commit that referenced this pull request May 20, 2026
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>
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.

1 participant