Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v4: Bug with how the parser handles nested quote_stack and bracket_stack characters when in a string context #13607

Closed
brandonmcconnell opened this issue Apr 29, 2024 · 2 comments · Fixed by #13608

Comments

@brandonmcconnell
Copy link
Contributor

brandonmcconnell commented Apr 29, 2024

Technical Configuration
What version of Tailwind CSS are you using?
Next (v4, latest)
What build tool (or framework if it abstracts the build tool) are you using?
`pnpm build && pnpm test`
What version of Node.js are you using?
v21.6.1
What browser are you using?
N/A
What operating system are you using?
macOS

Reproduction URL

The accepted test results from #13596

Describe your issue

attn @adamwathan @RobinMalfait

This issue delves into some of the issues surrounding why the parser doesn't properly handle supports-[content:"("]:grid, first explored in #13596. This specific case turned out to be quite the rabbit hole.

From what I can tell, Tailwind's Rust parser runs into issues related to its quote_stack and bracket_stack when encountering a construct like '('.

None of these process correctly, apparently for this same reason:

  • supports-[content:'(']
  • supports-[content:'[']
  • supports-[content:'"']
  • supports-[content:"'"]

This Tailwind Play example demonstrates the issue I'm describing: https://play.tailwindcss.com/EAhSfQFfTa

I can think of a few potential solutions, all of which come with trade-offs:

  1. We could interpret everything after an open quote as a string until we hit the closing quote. This would break some things, but we'd be able to handle brackets or quotes within strings.

  2. We could always ignore bracket_stack characters within quote_stack characters, but not ignore quote_stack characters within bracket_stack characters.

  3. Specifically enforce escaping characters related to quote_stack and bracket_stack when they're in a string context. This would mean that supports-[content:"("] would need to be written as supports-[content:"\("] to work.

    • supports-[content:'('] -> supports-[content:'\(']
    • supports-[content:'['] -> supports-[content:'\[']
    • supports-[content:'"'] -> supports-[content:'\"']
    • supports-[content:\"'\"] -> supports-[content:\"\'\"]
      ☝🏼 this last one might be problematic for more reasons than one, related to escaping the same quotes used when opening the class attribute as well as escaping the inner quote_stack character
  4. We could strictly enforce escaping special characters like these in string contexts.

    This might be most consistent and with common escape patterns, but it would also be one of the most disruptive solutions.

    In the case of my Multi plugin, for example, I was surprised—when chatting with @RobinMalfait (see discussion)—to discover that nesting quotes in arbitrary values is not only supported but that they appear to be infinitely nestable.

    In other words, this works:

    multi-['multi-['multi-['font-bold']'];text-green-700;before:content-['$']']
    

    If escapes were required to resolve this issue, each level of nesting might require a greater level of escape characters, which could get out of hand quickly. For example, the above would need to be written like this:

    multi-['multi-[\'multi-[\\\'font-bold\\\']\'];text-green-700;before:content-[\'$\']']
    

    Of course, it would be convenient for me not to need to update my plugin(s) with new syntax and safety measures again, and such escaping would probably render my plugin too cumbersome to justify using. However, greater parity with CSS is probably a higher and more evergreen standard here.

    Simply wrapping the arbitrary value in quotes was an easy fix, but the magic around it allowing nested quotes may be the culprit behind "(" not working, to some degree.

    conversation between Brandon McConnell and Robin Malfait about the nature of quoted strings in Tailwind CSS


I'm new to Rust, so all of this is "to the best of my current knowledge" from what I could grok reading through the parser. I also consulted a colleague who is more experienced with Rust to corroborate my findings and theories, and they came to roughly the same conclusions.

@adamwathan
Copy link
Member

Got a fix for that lined up here:

#13608

Actually had nothing to do with the Rust stuff — if you add this test to the Rust parser you can see it passes:

  #[test]
  fn it_should_keep_candidates_with_brackets_in_arbitrary_values_inside_quotes() {
      let candidates = run("content-['hello_[_]_world']", false);
      assert_eq!(candidates, vec!["content-['hello_[_]_world']"]);
  
+     let candidates = run("supports-[content:\"(\")]:grid", false);
+     assert_eq!(candidates, vec!["supports-[content:\"(\")]:grid"]);
  }

The Rust parser's job is just to extract strings from template files and pass those potential class name strings to the JS engine, so if it successfully pulls out the string then the problem is not in Rust but in JS somewhere 👍

Tailwind Play doesn't run any of the Rust code at all either, so when something can be reproduced in Play it's very likely a JS engine bug rather than a bug in the Rust parser.

@brandonmcconnell
Copy link
Contributor Author

@adamwathan Good to know, and thanks!

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 a pull request may close this issue.

2 participants