[WIP] feat(html/nursery): add noDupeElseIfBlocks, noDupeStyleProperties, noDupeUseDirectives#10504
[WIP] feat(html/nursery): add noDupeElseIfBlocks, noDupeStyleProperties, noDupeUseDirectives#10504Mokto wants to merge 24 commits into
Conversation
🦋 Changeset detectedLatest commit: fac0db7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Organic activityNo automation signals detected in the analyzed events. This is an automated analysis by AgentScan |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR extends Biome's HTML/Svelte parser to parse Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
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 `@crates/biome_html_formatter/src/shared.rs`:
- Line 21: FmtAnyAttributeInitializer currently formats SvelteInterpolatedString
with node.format().fmt(f) which ignores the parent compact flag; change the
FmtAnyAttributeInitializer branch for
AnyHtmlAttributeInitializer::SvelteInterpolatedString to call
node.format().with_options(self.compact).fmt(f) (or otherwise pass self.compact
into the formatter), and update the SvelteInterpolatedString formatter (and its
parts/chunks formatters) to accept and propagate the compact option (use
parts.format().with_options(compact) when writing parts) so the compact
formatting flag is threaded through end-to-end.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cac1a72-6711-4e86-af70-d1c121db4117
⛔ Files ignored due to path filters (10)
crates/biome_html_factory/src/generated/node_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_html_factory/src/generated/syntax_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_html_formatter/tests/specs/html/svelte/attribute_interpolation.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/error/svelte/attribute_interpolation_unterminated.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/attribute_interpolation.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/attribute_interpolation_edge_cases.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_syntax/src/generated/kind.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_html_syntax/src/generated/macros.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_html_syntax/src/generated/nodes.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_html_syntax/src/generated/nodes_mut.rsis excluded by!**/generated/**,!**/generated/**and included by**
📒 Files selected for processing (22)
.changeset/svelte-interpolated-attribute-strings.mdcrates/biome_html_analyze/src/lint/a11y/no_redundant_alt.rscrates/biome_html_formatter/src/generated.rscrates/biome_html_formatter/src/html/any/attribute_initializer.rscrates/biome_html_formatter/src/shared.rscrates/biome_html_formatter/src/svelte/any/interpolated_string_part.rscrates/biome_html_formatter/src/svelte/any/mod.rscrates/biome_html_formatter/src/svelte/auxiliary/interpolated_string.rscrates/biome_html_formatter/src/svelte/auxiliary/interpolated_string_chunk.rscrates/biome_html_formatter/src/svelte/auxiliary/mod.rscrates/biome_html_formatter/src/svelte/lists/interpolated_string_part_list.rscrates/biome_html_formatter/src/svelte/lists/mod.rscrates/biome_html_formatter/tests/specs/html/svelte/attribute_interpolation.sveltecrates/biome_html_parser/src/lexer/mod.rscrates/biome_html_parser/src/syntax/mod.rscrates/biome_html_parser/src/token_source.rscrates/biome_html_parser/tests/html_specs/error/svelte/attribute_interpolation_unterminated.sveltecrates/biome_html_parser/tests/html_specs/ok/svelte/attribute_interpolation.sveltecrates/biome_html_parser/tests/html_specs/ok/svelte/attribute_interpolation_edge_cases.sveltecrates/biome_html_syntax/src/attribute_ext.rsxtask/codegen/html.ungramxtask/codegen/src/html_kinds_src.rs
Merging this PR will degrade performance by 7.61%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
ematipico
left a comment
There was a problem hiding this comment.
Unfortunately the grammar is incorrect, as it might create issues. It should be reworked
| // A quoted attribute value that mixes literal text with one or more Svelte | ||
| // `{expression}` interpolations, e.g. `style="top: {top}px"` or `class="card {cls}"`. | ||
| // The literal segments (including the surrounding quotes) are kept as chunks so | ||
| // the original text round-trips, and each interpolation reuses | ||
| // `HtmlAttributeSingleTextExpression`. | ||
| SvelteInterpolatedString = | ||
| parts: SvelteInterpolatedStringPartList |
There was a problem hiding this comment.
Please keep comments inline with the document and the other documents
It's simply the grammar that handles, with a second line that heights the piece of code.
| SvelteInterpolatedStringChunk | ||
| | HtmlAttributeSingleTextExpression | ||
|
|
||
| SvelteInterpolatedStringChunk = 'html_string_literal' |
There was a problem hiding this comment.
This is incorrect. It's not a "string literal". Strings have a starting and closing quote. This here, doesn't have quotes, or it might have only one quote. For example
foo="{lorem} something {ipsum}"
The something part doesn't have a quote. I suggest checking the grammar used for JavaScript template literal, and take inspiration from them.
| 1: SVELTE_INTERPOLATED_STRING@11..22 | ||
| 0: SVELTE_INTERPOLATED_STRING_PART_LIST@11..22 | ||
| 0: SVELTE_INTERPOLATED_STRING_CHUNK@11..13 | ||
| 0: HTML_STRING_LITERAL@11..13 "\"a" [] [] |
There was a problem hiding this comment.
There's the issue I was talking about. A quote for a string literal, but it doesn't have the closing one.
Since string literals usually use the inner_string_text, it might result in a panic because it assumes it has both quotes.
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)
crates/biome_html_analyze/src/lint/a11y/use_button_type.rs (1)
90-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFalse positive: dynamic Svelte template values flagged as invalid.
When the attribute contains only interpolations (e.g.,
<button type="{buttonType}">),string_value()returnsNone, but we've already entered theis_static_stringblock. This causes us to return an error state instead of assuming valid (the intended behaviour per line 105-106).🐛 Proposed fix
- let is_static_string = value.as_html_string().is_some() - || value.as_svelte_template_attribute_value().is_some(); - if is_static_string { - // Static string value - validate it - if let Some(string_value) = value.string_value() - && ALLOWED_BUTTON_TYPES.contains(&&*string_value) - { - return None; - } - // Invalid static value - return Some(UseButtonTypeState { - missing_prop: false, - }); - } - - // Dynamic expression - assume valid - None + // Try to extract a static string value. If we get one, validate it. + // If the value is dynamic (expression or template with interpolations), assume valid. + if let Some(string_value) = value.string_value() { + if ALLOWED_BUTTON_TYPES.contains(&&*string_value) { + return None; + } + // Invalid static value + return Some(UseButtonTypeState { + missing_prop: false, + }); + } + + // Dynamic expression or template with interpolations - assume valid + None🤖 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 `@crates/biome_html_analyze/src/lint/a11y/use_button_type.rs` around lines 90 - 103, The code flags dynamic-only Svelte template attribute values as invalid because is_static_string is true when value.as_svelte_template_attribute_value() is Some even if value.string_value() is None; update the logic so that dynamic Svelte template values are treated as non-static (or explicitly return None for them). Concretely, modify the is_static_string check or add a guard in the block around value.string_value() so that if as_svelte_template_attribute_value() is Some but value.string_value() is None you return None (treat as valid) instead of returning UseButtonTypeState { missing_prop: false }; keep ALLOWED_BUTTON_TYPES usage for actual static string validation.
♻️ Duplicate comments (1)
crates/biome_html_formatter/src/shared.rs (1)
21-21:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThread
compactthrough this new Svelte branch.Line 21 drops
self.compact, unlike the neighbouring attribute initialiser branches. That means compact formatting stops atSvelteTemplateAttributeValue, and the downstream formatter incrates/biome_html_formatter/src/svelte/value/template_attribute_value.rs:12-17currently just writeselements.format()with no options to pick it back up. Compact mode has gone on holiday again.#!/bin/bash set -euo pipefail echo "== FmtAnyAttributeInitializer ==" fd -p 'shared.rs' crates/biome_html_formatter/src -x sed -n '12,24p' {} echo echo "== Svelte template attribute formatter ==" fd -p 'template_attribute_value.rs' crates/biome_html_formatter/src/svelte/value -x sed -n '1,80p' {} echo echo "== Existing compact-aware formatter calls nearby ==" rg -n -C2 'with_options\(self\.compact\)|with_options\(compact\)' crates/biome_html_formatter/srcExpected result: the new
SvelteTemplateAttributeValuearm is the odd one out inFmtAnyAttributeInitializer, and the template-attribute formatter shown above has no option plumbing to recover that flag.🤖 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 `@crates/biome_html_formatter/src/shared.rs` at line 21, The SvelteTemplateAttributeValue branch in FmtAnyAttributeInitializer drops self.compact so compact mode isn't propagated; update the AnyHtmlAttributeInitializer::SvelteTemplateAttributeValue arm to thread the compact flag into the child's formatter the same way other arms do (i.e., call the node's formatter with the parent's compact options rather than plain node.format().fmt(f)), and ensure the Svelte template attribute formatter (the template_attribute_value formatter that calls elements.format()) accepts and uses those options (propagate self.compact into elements.format() via with_options or the equivalent options plumbing so compact formatting is preserved).
🧹 Nitpick comments (1)
crates/biome_html_syntax/src/attribute_ext.rs (1)
26-26: ⚡ Quick winConsider using
text()instead oftext_trimmed()for template chunks.Template chunks must preserve their exact literal content, including any whitespace. Whilst
text_trimmed()likely works here (trivia-stripping rather than whitespace-stripping), usingtext()would be more explicit about preserving all content.Example:
style=" {x}"has a chunk" "(two spaces) that must not be trimmed.📝 Suggested change
- text.push_str(chunk.html_template_chunk_token().ok()?.text_trimmed()); + text.push_str(chunk.html_template_chunk_token().ok()?.text());🤖 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 `@crates/biome_html_syntax/src/attribute_ext.rs` at line 26, The template chunk handling currently uses chunk.html_template_chunk_token().ok()?.text_trimmed(), which can remove significant literal whitespace; update the code to call text() instead of text_trimmed() so that template chunks preserve exact content (replace the text_trimmed() call on the result of html_template_chunk_token() with text()). Ensure this change is applied where text.push_str(...) is invoked for template chunks in attribute_ext.rs.
🤖 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 `@crates/biome_html_analyze/src/lint/a11y/use_button_type.rs`:
- Around line 90-103: The code flags dynamic-only Svelte template attribute
values as invalid because is_static_string is true when
value.as_svelte_template_attribute_value() is Some even if value.string_value()
is None; update the logic so that dynamic Svelte template values are treated as
non-static (or explicitly return None for them). Concretely, modify the
is_static_string check or add a guard in the block around value.string_value()
so that if as_svelte_template_attribute_value() is Some but value.string_value()
is None you return None (treat as valid) instead of returning UseButtonTypeState
{ missing_prop: false }; keep ALLOWED_BUTTON_TYPES usage for actual static
string validation.
---
Duplicate comments:
In `@crates/biome_html_formatter/src/shared.rs`:
- Line 21: The SvelteTemplateAttributeValue branch in FmtAnyAttributeInitializer
drops self.compact so compact mode isn't propagated; update the
AnyHtmlAttributeInitializer::SvelteTemplateAttributeValue arm to thread the
compact flag into the child's formatter the same way other arms do (i.e., call
the node's formatter with the parent's compact options rather than plain
node.format().fmt(f)), and ensure the Svelte template attribute formatter (the
template_attribute_value formatter that calls elements.format()) accepts and
uses those options (propagate self.compact into elements.format() via
with_options or the equivalent options plumbing so compact formatting is
preserved).
---
Nitpick comments:
In `@crates/biome_html_syntax/src/attribute_ext.rs`:
- Line 26: The template chunk handling currently uses
chunk.html_template_chunk_token().ok()?.text_trimmed(), which can remove
significant literal whitespace; update the code to call text() instead of
text_trimmed() so that template chunks preserve exact content (replace the
text_trimmed() call on the result of html_template_chunk_token() with text()).
Ensure this change is applied where text.push_str(...) is invoked for template
chunks in attribute_ext.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e47bccf-bc8a-4098-8ba9-699e80d8e896
⛔ Files ignored due to path filters (25)
crates/biome_html_factory/src/generated/node_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_html_factory/src/generated/syntax_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_html_parser/tests/html_specs/error/svelte/attribute_interpolation_unterminated.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/attribute_interpolation.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/attribute_interpolation_edge_cases.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/await_multiline.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/component.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/component_with_attributes.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/directives/animate_in_each.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/directives/bind_checked.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/directives/bind_files.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/directives/bind_group.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/directives/style_basic.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/directives/style_important_modifier.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/directives/style_kebab_case.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/directives/style_multiple.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/directives/style_multiple_with_important.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/js_comments_in_component_tags.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/js_comments_in_tag.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/lowercase_component_member.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_parser/tests/html_specs/ok/svelte/svg_with_colon_attribute.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_syntax/src/generated/kind.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_html_syntax/src/generated/macros.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_html_syntax/src/generated/nodes.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_html_syntax/src/generated/nodes_mut.rsis excluded by!**/generated/**,!**/generated/**and included by**
📒 Files selected for processing (22)
crates/biome_html_analyze/src/lint/a11y/no_redundant_alt.rscrates/biome_html_analyze/src/lint/a11y/use_button_type.rscrates/biome_html_analyze/src/lint/nursery/no_script_url.rscrates/biome_html_formatter/src/generated.rscrates/biome_html_formatter/src/html/any/attribute_initializer.rscrates/biome_html_formatter/src/shared.rscrates/biome_html_formatter/src/svelte/any/directive_initializer_clause.rscrates/biome_html_formatter/src/svelte/any/mod.rscrates/biome_html_formatter/src/svelte/any/template_element.rscrates/biome_html_formatter/src/svelte/auxiliary/mod.rscrates/biome_html_formatter/src/svelte/auxiliary/template_chunk_element.rscrates/biome_html_formatter/src/svelte/lists/mod.rscrates/biome_html_formatter/src/svelte/lists/template_element_list.rscrates/biome_html_formatter/src/svelte/value/mod.rscrates/biome_html_formatter/src/svelte/value/template_attribute_value.rscrates/biome_html_parser/src/lexer/mod.rscrates/biome_html_parser/src/syntax/mod.rscrates/biome_html_parser/src/token_source.rscrates/biome_html_syntax/src/attribute_ext.rsxtask/codegen/html.ungramxtask/codegen/src/html_kinds_src.rsxtask/glue/src/lib.rs
✅ Files skipped from review due to trivial changes (6)
- crates/biome_html_formatter/src/svelte/any/directive_initializer_clause.rs
- crates/biome_html_formatter/src/svelte/lists/mod.rs
- crates/biome_html_formatter/src/svelte/value/mod.rs
- crates/biome_html_formatter/src/svelte/any/template_element.rs
- crates/biome_html_formatter/src/html/any/attribute_initializer.rs
- crates/biome_html_formatter/src/generated.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_html_formatter/src/svelte/any/mod.rs
- crates/biome_html_formatter/src/svelte/auxiliary/mod.rs
|
@ematipico i think it's ready again 👍 |
ematipico
left a comment
There was a problem hiding this comment.
The parser is now more powerful, but we should focus on the linter now, because that's where the users see the benefits.
This PR should add new test cases where now lint rules don't emit false positives, or catch more cases
| The HTML parser now parses Svelte `{expression}` interpolations inside quoted attribute values into structured nodes, instead of treating the whole value as an opaque string. | ||
|
|
||
| ```svelte | ||
| <div style="top: {top}px" class="card {cls}"></div> | ||
| ``` |
There was a problem hiding this comment.
You should reword this changeset. Biome is already able to parse it. Instead, since now we improved the parsing, and we can detect the bindings inside the interpolation, we should focus on talking about the lint rules that benefit from this change.
…lues
Quoted attribute values like `style="top: {top}px"` are now parsed into a
`SvelteInterpolatedString` node holding literal chunks and the existing
`HtmlAttributeSingleTextExpression` interpolations, instead of a single opaque
string token. Plain values without `{` are unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Value lex context Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ter spec Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…etection Replace the O(n) forward-scan in svelte_attribute_has_interpolation with a single O(n) lex pass (same work as before), then an O(1) last-byte check to detect interpolation. When interpolation is found, re_lex the opening quote as a standalone token so the template parser gets its l_quote. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace lookup_byte table dispatch with a direct two-comparison byte scan.
Since `{`, `"` and `'` are ASCII (< 0x80) they cannot appear as continuation
bytes in UTF-8 multi-byte sequences, so scanning byte-by-byte is safe and
allows the compiler to auto-vectorize the loop.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Guard the cur_text() call and interpolation detection behind Svelte.is_supported() to avoid overhead on every HTML_STRING_LITERAL in plain HTML/Vue/Astro files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…DupeUseDirectives
Port three eslint-plugin-svelte rules:
- noDupeElseIfBlocks: disallow duplicate conditions in {#if}/{:else if} chains
- noDupeStyleProperties: disallow duplicate style: directives on the same element
- noDupeUseDirectives: disallow duplicate use: directives on the same element
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cdf9621 to
fac0db7
Compare
|
Superseded by a clean PR from branch feat/svelte-dupe-rules — same changes without the unrelated Svelte parser work. |
|
Where's the PR? Can you link it? |
|
Actually I completely messed up that PR, it was supposed to be the html parser. Sorry about that @ematipico. the code is available there: #10555. Sorry for the double review :( |
Summary
Ports three rules from eslint-plugin-svelte to Biome, all targeting Svelte templates (
.sveltefiles):noDupeElseIfBlocks: disallow duplicate conditions in{#if}/{:else if}chainsnoDupeStyleProperties: disallow duplicatestyle:directives on the same elementnoDupeUseDirectives: disallow duplicateuse:directives on the same elementAll three rules are in the
nurserygroup, tagged withRuleDomain::Svelte, and markedrecommended: true.Test plan
cargo test -p biome_html_analyze)