fix(oxide): extract classes adjacent to [% ... %] template tags (#20233)#20268
fix(oxide): extract classes adjacent to [% ... %] template tags (#20233)#20268ther12k wants to merge 1 commit into
Conversation
…s#20233) Template Toolkit, Text::Xslate, ExpressionEngine, and similar template engines use [% ... %] as their tag delimiters. The scanner was dropping classes adjacent to these tags: neither '[' nor ']' was a valid boundary character, so a candidate like 'bg-white[%' failed the after-boundary check even after the tag was identified. Fix: three coordinated changes — a 3-layer [% ... %] skip and the minimum boundary additions required for adjacent candidates to extract. 3-layer [% ... %] skip in extract() ------------------------------------ The skip is implemented in the outer extractor loop, not inside the candidate machine, because the inner utility machine can terminate a candidate whose last char immediately precedes '[', advancing the cursor PAST '[' before any in-machine [% check could see it. 1. Top-of-loop [% check in extract() — handles input-start and post-whitespace arrivals (when cursor enters an iteration parked on '['). 2. Post-match [% check after candidate_machine.next() returns — handles the common case where next() returns Done with cursor.pos on the '[' that opens a template tag. Uses 'continue' to skip the bottom cursor.advance() since the tag-skip already moved past '['. 3. [% check in extract_sub_candidates() — prevents spawning an inner CandidateMachine at '%' (after '[') that would otherwise re-extract template keywords like 'if', 'cond', 'end'. The skip is non-greedy (first %] ends the tag). Nested [% ... %] inside the body is not a thing in any of the target template languages. If no %] is found (e.g. a typo), the normal candidate extraction path takes over so the rest of the input still parses. Boundary additions (why they belong with this fix) -------------------------------------------------- Adding ']' as a valid before-boundary and '[' as a valid after-boundary is required for candidates adjacent to template tags to extract: - 'bg-white[%': before-boundary 'e' (Common), after-boundary '[' — '[' is now After-class. - 'bg-white[% end %]': same as above. - 'bg-white]%': before-boundary ']' — ']' is now also valid as before-boundary (it already lived in After for [class.foo] syntax, so we special-case the check rather than overwrite the enum). Carve-out in has_valid_boundaries: '[' as after-boundary is rejected when the candidate span itself already contains '['. This preserves 'one arbitrary value per utility' semantics: bg-[red][blue] still extracts nothing. Tests (in test_template_toolkit_syntax) --------------------------------------- - Issue reproduction: [% IF ... %]bg-white/40[% ELSE %]bg-white/10 - Lowercase keywords - Static class adjacent to a [% %] block - [color:red] still extracts (arbitrary property regression guard) - bg-[red][blue] still extracts nothing (carve-out regression guard) - bg-red-500[% if x %] — candidate before tag, no trailing - bg-red-500[% x %]bg-blue-500[% y %]text-white — multi-tag loop - [% if x %] bg-red-500 — top-of-loop check at input start - bg-red-500 [% if x %] — post-whitespace arrival path All 115 oxide lib tests pass. cargo fmt --check clean. cargo clippy errors are pre-existing on main (verified via diff against origin/main) and unrelated to this change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe oxide extractor gains support for Template Toolkit–style 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Confidence Score: 3/5Safe to merge for the tested cases (named utilities adjacent to template tags), but arbitrary-value utilities immediately before a template tag will silently fail to extract. The carve-out in boundary.rs drops arbitrary-value utilities like bg-[red] when immediately followed by a [% template tag — an untested combination that would affect real Template Toolkit users using arbitrary values. crates/oxide/src/extractor/boundary.rs — the carve-out in has_valid_boundaries (lines 58-60) needs to distinguish a bare [ from a template-tag [% before rejecting the span. Reviews (1): Last reviewed commit: "fix: extract classes adjacent to [% ... ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if after == b'[' && span.slice(input).contains(&b'[') { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The check after == b'[' && span.slice(input).contains(&b'[') rejects any span that already contains [ when the very next character is [. That correctly blocks bg-[red][blue], but it also silently drops a perfectly valid case: an arbitrary-value utility that immediately precedes a [%…%] template tag, e.g. bg-[red][% if x %]. In that input, span is bg-[red] (contains [), after is [ (from [%), so the carve-out fires and bg-[red] is never emitted — even though the template tag would be skipped by the outer loop moments later.
The fix is to peek one character further and exempt [% from the rejection. In has_valid_boundaries, replace the current block with:
if after == b'[' && span.slice(input).contains(&b'[') {
let peek = input.get(span.end + 2).copied().unwrap_or(b'\0');
if peek != b'%' {
return false;
}
}| fn test_template_toolkit_syntax() { | ||
| // The exact reproduction from the issue. | ||
| assert_extract_candidates_contains( | ||
| r#"<div class="[% IF $is_open %]bg-white/40[% ELSE %]bg-white/10[% END %]"></div>"#, | ||
| vec!["bg-white/40", "bg-white/10"], | ||
| ); | ||
|
|
||
| // Lowercase keywords. | ||
| assert_extract_candidates_contains( | ||
| r#"<div class="[% if cond %]flex[% end %]"></div>"#, | ||
| vec!["flex"], | ||
| ); | ||
|
|
||
| // Static class adjacent to a `[% %]` block. | ||
| assert_extract_candidates_contains( | ||
| r#"<div class="bg-blue-500 [% if cond %]bg-red-500[% end %]"></div>"#, | ||
| vec!["bg-blue-500", "bg-red-500"], | ||
| ); | ||
|
|
||
| // `[color:red]` (arbitrary property) must STILL extract as a whole | ||
| // arbitrary property, not be skipped as a template tag. We use the | ||
| // non-strict contains-checker because `class` is also legitimately | ||
| // extracted from the `<div class="...">` attribute name. | ||
| assert_extract_candidates_contains( | ||
| r#"<div class="[color:red]"></div>"#, | ||
| vec!["[color:red]"], | ||
| ); | ||
|
|
||
| // `bg-[red][blue]` (arbitrary value followed by arbitrary value) must | ||
| // STILL be invalid — `[` alone is not a template-tag opener. | ||
| assert_extract_sorted_candidates("bg-[red][blue]", vec![]); | ||
|
|
||
| // Candidate immediately before a tag with no trailing class. Locks in | ||
| // the before-boundary `]` path independently of the after-boundary | ||
| // `[` path (recommended by review). | ||
| assert_extract_sorted_candidates("bg-red-500[% if x %]", vec!["bg-red-500"]); | ||
|
|
||
| // Multiple adjacent tags in the same string. Guards the | ||
| // `cursor.advance_by(rel + 2)` + `continue` loop in the outer | ||
| // extractor — each tag must be skipped independently without | ||
| // consuming or merging surrounding candidates (recommended by | ||
| // review). Uses multi-char utilities (1-letter utilities followed | ||
| // by `[` are not extractable — pre-existing limitation of the | ||
| // named utility machine). | ||
| assert_extract_sorted_candidates( | ||
| "bg-red-500[% x %]bg-blue-500[% y %]text-white", | ||
| vec!["bg-red-500", "bg-blue-500", "text-white"], | ||
| ); | ||
|
|
||
| // `[%` at input start, class follows the tag. Exercises the | ||
| // top-of-loop `[%` skip in `extract()` (covers the case where | ||
| // the cursor enters the iteration parked on `[`). | ||
| assert_extract_sorted_candidates("[% if x %] bg-red-500", vec!["bg-red-500"]); | ||
|
|
||
| // `[%` after whitespace (no candidate before). Exercises the | ||
| // post-whitespace-arrival path in the top-of-loop `[%` skip. | ||
| assert_extract_sorted_candidates("bg-red-500 [% if x %]", vec!["bg-red-500"]); | ||
| } | ||
|
|
||
| // https://github.com/tailwindlabs/tailwindcss/issues/17050 | ||
| #[test] | ||
| fn test_haml_syntax() { |
There was a problem hiding this comment.
The test suite covers named utilities like bg-red-500 and bg-white/40 before/after [%…%] tags, but none of the nine cases involve an arbitrary-value utility (one whose span contains [) immediately followed by a template tag, e.g. bg-[red][% if x %] or [% if x %]bg-[#ff0000]. This is exactly the combination that the carve-out in has_valid_boundaries incorrectly blocks. Adding a test like assert_extract_sorted_candidates("bg-[red][% if x %]", vec!["bg-[red]"]) would catch the regression.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Hey! Thanks for the PR, but I believe this is the wrong approach. This makes things slower for everyone even if you don't use the special You also mention Please don't open PRs that are purely vibe coded. Thanks. |
2 participants
Fixes #20233
Problem
Template Toolkit, Text::Xslate, ExpressionEngine, and similar template engines use
[% ... %]as their tag delimiters. The scanner was dropping classes adjacent to these tags: neither[nor]was a valid boundary character, so a candidate likebg-white[%failed the after-boundary check even after the tag was identified.Fix — three coordinated changes
1. 3-layer
[% ... %]skip inextract()Implemented in the outer extractor loop, not inside the candidate machine, because the inner utility machine can terminate a candidate whose last char immediately precedes
[, advancing the cursor PAST[before any in-machine[%check could see it.[%check inextract()— handles input-start and post-whitespace arrivals (cursor enters an iteration parked on[).[%check aftercandidate_machine.next()returns — handles the common case wherenext()returnsDonewith cursor.pos on the[that opens a template tag. Usescontinueto skip the bottomcursor.advance()since the tag-skip already moved past[.[%check inextract_sub_candidates()— prevents spawning an innerCandidateMachineat%(after[) that would otherwise re-extract template keywords likeif,cond,end.The skip is non-greedy (first
%]ends the tag). Nested[% ... %]inside the body isn't a thing in any of the target template languages. If no%]is found (e.g. a typo), the normal candidate extraction path takes over so the rest of the input still parses.2. Boundary additions (minimum required)
Adding
]as a valid before-boundary and[as a valid after-boundary is required for candidates adjacent to template tags to extract:bg-white[%— before-boundarye(Common), after-boundary[.[is now After-class.bg-white[% end %]— same as above.bg-white]%— before-boundary].]is now also valid as before-boundary (it already lived in After for[class.foo]syntax, so we special-case the check rather than overwrite the enum).3. Carve-out in
has_valid_boundaries[as after-boundary is rejected when the candidate span itself already contains[. This preserves the "one arbitrary value per utility" semantics:bg-[red][blue]still extracts nothing.Tests
Added
test_template_toolkit_syntax(9 cases) incrates/oxide/src/extractor/mod.rs:[% IF ... %]bg-white/40[% ELSE %]bg-white/10[% %]block[color:red]still extracts (arbitrary property regression guard)bg-[red][blue]still extracts nothing (carve-out regression guard)bg-red-500[% if x %]— candidate before tag, no trailingbg-red-500[% x %]bg-blue-500[% y %]text-white— multi-tag loop[% if x %] bg-red-500— top-of-loop check at input startbg-red-500 [% if x %]— post-whitespace arrival pathVerification
cargo test -p oxide→ 115 passed, 0 failed, 12 ignored (1 new test added, was 114 baseline)cargo fmt --check→ cleancargo clippy --workspace --all-targets→ 9 errors, 0 new from this diff (verified viagit diff origin/main..HEAD— my changes don't introduce new patterns). Pre-existing clippy onorigin/main(verified with the same diff on the parent commit).cargo testdoes not exercise this path under the default test config; the new test inmod.rsis a unit test that runs undercargo test -p oxide.Scope
fix(oxide):— all changes are isolated tocrates/oxide/src/extractor/. No public API changes, no breaking changes.Files changed