fix(core): tracking vue bindings inside directives#9053
Conversation
|
f89c275 to
7baafa0
Compare
Merging this PR will degrade performance by 7.46%
Performance Changes
Comparing Footnotes
|
7baafa0 to
2278431
Compare
WalkthroughThis PR introduces support for parsing Vue and Svelte directive attributes as embedded JavaScript code. Changes include: adding a new Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.claude/skills/biome-developer/SKILL.md:
- Around line 244-247: The word "Markdown" in the guidance section is
lowercased; update the content in SKILL.md where the bullet items reference
"markdown" (e.g., the lines "- ✅ Ensure all markdown tables follow..." and "- ✅
Test documentation changes with markdown linters before committing") to
capitalize it as "Markdown" so the proper noun is used consistently across the
document.
In `@crates/biome_service/src/file_handlers/html.rs`:
- Around line 1116-1186: The rustdoc comments above the private helpers
parse_directive_string_value and parse_directive_text_expression are using ///
(doctest-style) but do not contain doctest code blocks; change those doc
comments to regular line comments (//) to comply with the repo guideline for
private helpers, i.e., replace the leading /// comment blocks describing
behavior for both functions with non-doctest // comments (preserving the
explanatory text), and ensure no public API is accidentally documented as
doctests; touch the comment blocks immediately preceding the
parse_directive_string_value and parse_directive_text_expression function
definitions.
- Around line 766-785: The loop over html_root.syntax().descendants() currently
only processes directives when directive.initializer() is Some(), skipping
Svelte shorthand directives (e.g., bind:value, class:active) that lack an
initializer; update the handling in the block that uses
AnySvelteDirective::cast_ref and directive.initializer() so that when
initializer() is None you still record the shorthand as a used identifier:
derive a suitable expression/content from the directive itself (for example
using the directive name/target) and push a corresponding (content.into(),
doc_source) into nodes (reusing embedded_file_source.with_embedding_kind and
parse_directive_text_expression for cases with an initializer), so shorthand
directives are treated as usages instead of being ignored.
- Around line 619-699: The directive parsing treats all Vue directives as JS
expressions, which breaks v-for and v-slot because they contain binding
patterns; update the handler to special-case VueVForDirective and
VueVSlotDirective (and VueVSlotShorthandDirective) instead of sending them to
parse_directive_string_value: detect these directives where initializer()
exists, and either (A) call a new helper (e.g., parse_directive_binding_pattern)
that parses binding patterns and returns identifier bindings to push into
nodes/embedded-bindings, or (B) skip adding them as expression embeddings so the
embedded-bindings layer won’t emit false unused-variable diagnostics; implement
the new branch alongside the existing
VueDirective/VueVOnShorthand/VueVBindShorthand handling and reuse
embedded_file_source.with_embedding_kind and nodes so only pure-expression
directives go to parse_directive_string_value.
| // Parse Vue directive attributes (v-on, v-bind, v-if, etc.) | ||
| for element in html_root.syntax().descendants() { | ||
| // Handle @click shorthand (VueVOnShorthandDirective) | ||
| if let Some(directive) = VueVOnShorthandDirective::cast_ref(&element) | ||
| && let Some(initializer) = directive.initializer() | ||
| { | ||
| let file_source = | ||
| embedded_file_source.with_embedding_kind(EmbeddingKind::Vue { | ||
| setup: false, | ||
| is_source: false, | ||
| }); | ||
| if let Some((content, doc_source)) = parse_directive_string_value( | ||
| &initializer, | ||
| cache, | ||
| biome_path, | ||
| settings, | ||
| file_source, | ||
| ) { | ||
| nodes.push((content.into(), doc_source)); | ||
| } | ||
| } | ||
|
|
||
| // Handle :prop shorthand (VueVBindShorthandDirective) | ||
| if let Some(directive) = VueVBindShorthandDirective::cast_ref(&element) | ||
| && let Some(initializer) = directive.initializer() | ||
| { | ||
| let file_source = | ||
| embedded_file_source.with_embedding_kind(EmbeddingKind::Vue { | ||
| setup: false, | ||
| is_source: false, | ||
| }); | ||
| if let Some((content, doc_source)) = parse_directive_string_value( | ||
| &initializer, | ||
| cache, | ||
| biome_path, | ||
| settings, | ||
| file_source, | ||
| ) { | ||
| nodes.push((content.into(), doc_source)); | ||
| } | ||
| } | ||
|
|
||
| // Handle #slot shorthand (VueVSlotShorthandDirective) | ||
| if let Some(directive) = VueVSlotShorthandDirective::cast_ref(&element) | ||
| && let Some(initializer) = directive.initializer() | ||
| { | ||
| let file_source = | ||
| embedded_file_source.with_embedding_kind(EmbeddingKind::Vue { | ||
| setup: false, | ||
| is_source: false, | ||
| }); | ||
| if let Some((content, doc_source)) = parse_directive_string_value( | ||
| &initializer, | ||
| cache, | ||
| biome_path, | ||
| settings, | ||
| file_source, | ||
| ) { | ||
| nodes.push((content.into(), doc_source)); | ||
| } | ||
| } | ||
|
|
||
| // Handle full directives (v-on:, v-bind:, v-if, v-show, etc.) | ||
| if let Some(directive) = VueDirective::cast_ref(&element) | ||
| && let Some(initializer) = directive.initializer() | ||
| { | ||
| let file_source = | ||
| embedded_file_source.with_embedding_kind(EmbeddingKind::Vue { | ||
| setup: false, | ||
| is_source: false, | ||
| }); | ||
| if let Some((content, doc_source)) = parse_directive_string_value( | ||
| &initializer, | ||
| cache, | ||
| biome_path, | ||
| settings, | ||
| file_source, | ||
| ) { | ||
| nodes.push((content.into(), doc_source)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the parse_directive_string_value function definition
rg "fn parse_directive_string_value" -A 30 crates/Repository: biomejs/biome
Length of output: 2905
🏁 Script executed:
# Also check for VueDirective type definitions and how they're used
rg "VueDirective|VueVSlotShorthandDirective|VueVOnShorthandDirective" crates/biome_service/src/file_handlers/html.rs -B 2 -A 2Repository: biomejs/biome
Length of output: 1294
🏁 Script executed:
# Check if there's special handling for v-for directives
rg "v-for|VueVFor" crates/ -iRepository: biomejs/biome
Length of output: 41634
🏁 Script executed:
# Look for embedded-bindings or binding pattern handling
rg "embedded.?binding|binding.?pattern" crates/ -iRepository: biomejs/biome
Length of output: 50370
🏁 Script executed:
# Check if v-for is being parsed or excluded
rg "v-for" crates/biome_service/src/file_handlers/html.rsRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look at the actual VueDirective type definition
rg "impl VueDirective|pub.*VueDirective" crates/biome_html_syntax --max-count=5 -B 2 -A 5Repository: biomejs/biome
Length of output: 6097
🏁 Script executed:
# Check the test case that's commented out more carefully
cat crates/biome_cli/tests/cases/handle_vue_files.rs | head -100Repository: biomejs/biome
Length of output: 2579
🏁 Script executed:
# Look for any directive name filtering logic
rg "directive.*name|name.*directive" crates/biome_service/src/file_handlers/html.rs -B 2 -A 2Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Check what the current test expects for v-for
cat crates/biome_cli/tests/snapshots/main_cases_handle_vue_files/no_unused_variables_in_vue_directives.snapRepository: biomejs/biome
Length of output: 1214
🏁 Script executed:
# Check the HTML parser definition - look for Vue directive patterns
rg "VueDirective" crates/biome_html_parser/src --max-count=10 -B 1 -A 3Repository: biomejs/biome
Length of output: 3351
🏁 Script executed:
# See if there's a test file specifically for v-for directive handling
find crates -name "*vueDirectives*" -o -name "*vue_directive*" | head -20Repository: biomejs/biome
Length of output: 426
🏁 Script executed:
# Look at the noUnusedVariables test for vue directives
cat crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/vueDirectives.vueRepository: biomejs/biome
Length of output: 878
🏁 Script executed:
# Check what the vueDirectives.vue.snap output actually shows
cat crates/biome_js_analyze/tests/specs/correctness/noUnusedVariables/vueDirectives.vue.snapRepository: biomejs/biome
Length of output: 3883
🏁 Script executed:
# Check if there's a rule or setting that filters directives by name
rg "v-for|v-on|v-bind|v-if|v-show" crates/biome_service/src/file_handlers/html.rs -B 3 -A 3Repository: biomejs/biome
Length of output: 666
🏁 Script executed:
# Look at the embedded_bindings to see if it has special logic for binding patterns
head -200 crates/biome_service/src/workspace/document/services/embedded_bindings.rsRepository: biomejs/biome
Length of output: 8364
Confirm: v-for and v-slot directives need special handling. This code parses all Vue directive values as JavaScript expressions, but v-for (item in items) and v-slot ({ item }) contain binding patterns rather than plain expressions. When parsed as JS, the binding declarations are lost, and the embedded-bindings layer (which extracts only from variable declarations) cannot recognise them. This causes false unused-variable diagnostics—as evidenced by the test snapshot where items is flagged as unused despite appearing in v-for.
Either add specific parsing logic for these directive types or filter to parse only pure-expression directives.
🤖 Prompt for AI Agents
In `@crates/biome_service/src/file_handlers/html.rs` around lines 619 - 699, The
directive parsing treats all Vue directives as JS expressions, which breaks
v-for and v-slot because they contain binding patterns; update the handler to
special-case VueVForDirective and VueVSlotDirective (and
VueVSlotShorthandDirective) instead of sending them to
parse_directive_string_value: detect these directives where initializer()
exists, and either (A) call a new helper (e.g., parse_directive_binding_pattern)
that parses binding patterns and returns identifier bindings to push into
nodes/embedded-bindings, or (B) skip adding them as expression embeddings so the
embedded-bindings layer won’t emit false unused-variable diagnostics; implement
the new branch alongside the existing
VueDirective/VueVOnShorthand/VueVBindShorthand handling and reuse
embedded_file_source.with_embedding_kind and nodes so only pure-expression
directives go to parse_directive_string_value.
There was a problem hiding this comment.
The bot is correct here, at least for v-for.
The syntax for v-for isn't an expression, its more like the <iter> part of a for loop for (<iter>) { ... }, but it also includes a range syntax. its not really js. related is #8062, it's more complicated than it seems.
And for v-slot, I think the bot might be right as well. https://vuejs.org/api/built-in-directives.html#v-slot I think the value for a v-slot directive is a variable or a object destructuring. (see the example on that page)
| // Parse Svelte directive attributes (bind:, class:, use:, etc.) | ||
| // Note: on: event handlers are legacy Svelte 3/4 syntax and not supported. | ||
| // Svelte 5 runes mode uses regular attributes for event handlers. | ||
| for element in html_root.syntax().descendants() { | ||
| // Handle special Svelte directives (bind:, class:, etc.) | ||
| if let Some(directive) = AnySvelteDirective::cast_ref(&element) | ||
| && let Some(initializer) = directive.initializer() | ||
| { | ||
| let file_source = embedded_file_source | ||
| .with_embedding_kind(EmbeddingKind::Svelte { is_source: false }); | ||
| if let Some((content, doc_source)) = parse_directive_text_expression( | ||
| &initializer, | ||
| cache, | ||
| biome_path, | ||
| settings, | ||
| file_source, | ||
| ) { | ||
| nodes.push((content.into(), doc_source)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me examine the file and surrounding context
cd /repo && cat -n crates/biome_service/src/file_handlers/html.rs | sed -n '750,800p'Repository: biomejs/biome
Length of output: 112
🏁 Script executed:
# Search for AnySvelteDirective definition
rg "enum AnySvelteDirective|struct AnySvelteDirective" -A 20Repository: biomejs/biome
Length of output: 1891
🏁 Script executed:
# Check what directive types are included
rg "AnySvelteDirective" -B 2 -A 5 | head -100Repository: biomejs/biome
Length of output: 7732
🏁 Script executed:
# Look for Svelte directive related enums and types
rg "SvelteDirective|bind:|class:|on:" --type rs -B 1 -A 1 | head -150Repository: biomejs/biome
Length of output: 83
🏁 Script executed:
# Check the parse_directive_text_expression function
rg "fn parse_directive_text_expression" -A 30Repository: biomejs/biome
Length of output: 2703
🏁 Script executed:
rg "SvelteOn|on:" xtask/codegen/html.ungram -A 2 -B 2Repository: biomejs/biome
Length of output: 1540
🏁 Script executed:
# Look for any event handler or on: handling in the codebase
rg "on:" crates/biome_service/src/file_handlers/html.rs -B 3 -A 3Repository: biomejs/biome
Length of output: 6620
🏁 Script executed:
# Check the directive_ext implementation for all directive types
rg "fn initializer" crates/biome_html_syntax/src/directive_ext.rs -A 30Repository: biomejs/biome
Length of output: 808
🏁 Script executed:
# Look for how shorthand directives are defined in the grammar
rg "SvelteBindDirective|SvelteClassDirective" xtask/codegen/html.ungram -B 2 -A 5Repository: biomejs/biome
Length of output: 647
🏁 Script executed:
# Look for SvelteDirectiveValue definition to understand optional values
rg "SvelteDirectiveValue =" xtask/codegen/html.ungram -A 5Repository: biomejs/biome
Length of output: 207
🏁 Script executed:
# Check if there's any handling for optional directive values
rg "SvelteDirective" xtask/codegen/html.ungram | grep -i "?"Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look at actual test files to see Svelte directive usage patterns
fd "svelte" --type f -path "*/tests/*" | head -10Repository: biomejs/biome
Length of output: 225
🏁 Script executed:
# Search for test files with Svelte directives to understand shorthand syntax
rg "class:" --type txt --type md -l 2>/dev/null | head -5Repository: biomejs/biome
Length of output: 72
🏁 Script executed:
# Check what initializer() returns for a directive without an explicit value
rg "pub fn initializer" crates/biome_html_syntax/src/directive_ext.rs -B 2 -A 15Repository: biomejs/biome
Length of output: 913
🏁 Script executed:
# Look for any tests that demonstrate Svelte shorthand directive usage
rg "class:active|bind:value" crates/ -B 2 -A 2 | head -40Repository: biomejs/biome
Length of output: 3616
🏁 Script executed:
# Check if there are any other code paths that handle directives without initializers
rg "AnySvelteDirective" crates/biome_service/src/file_handlers/html.rs -B 3 -A 8Repository: biomejs/biome
Length of output: 1527
Shorthand directives without explicit initializers are not parsed. The code requires directive.initializer() to be Some(), but Svelte shorthand syntax like bind:value or class:active (without ={...}) have optional initializers and thus won't be processed. This means variables in shorthand directives won't be flagged as used, potentially causing false noUnusedVariables warnings. Confirm whether shorthand support is intentional or if the initializer check needs adjusting.
(The note about on: handlers is accurate—they're indeed not in AnySvelteDirective and Svelte 5 runes mode uses regular attributes instead.)
🤖 Prompt for AI Agents
In `@crates/biome_service/src/file_handlers/html.rs` around lines 766 - 785, The
loop over html_root.syntax().descendants() currently only processes directives
when directive.initializer() is Some(), skipping Svelte shorthand directives
(e.g., bind:value, class:active) that lack an initializer; update the handling
in the block that uses AnySvelteDirective::cast_ref and directive.initializer()
so that when initializer() is None you still record the shorthand as a used
identifier: derive a suitable expression/content from the directive itself (for
example using the directive name/target) and push a corresponding
(content.into(), doc_source) into nodes (reusing
embedded_file_source.with_embedding_kind and parse_directive_text_expression for
cases with an initializer), so shorthand directives are treated as usages
instead of being ignored.
dyc3
left a comment
There was a problem hiding this comment.
This does fix all the false positives in my codebase that I was testing on! However, it might be a bit overzealous with regards to v-for and v-slot.
It requires special handling, yes. I preferred to keep it out of scope for this PR. I'll tackle it later |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
Closes #9013
Code generated with an agent. While steering the agent, I noticed some things that got wrong, so I took the opportunity to create a skill which should provide the correct behaviour inside the project. For example, the agent was trying to build the binary, while it could use the quick_test to read a CST. Hopefully it should help.
Test Plan
CI should stay green. Added new tests.
Docs