fix(semantic): resolve Svelte $store references in semantic model#9286
fix(semantic): resolve Svelte $store references in semantic model#9286Hugo-Polloli wants to merge 9 commits intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 93a52b4 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 |
github-actions
bot
added
A-CLI
Hugo-Polloli
force-pushed
the
fix/6316-svelte-store-undeclared
branch
from
1ae1ed3 to
0d2ff9b
Compare
|
(this is just some targetted tests for now, not ready for review, I'd prefer a real "semantic" resolution over the current monkey patch of this PR, trying to see how big a proper fix would be) |
Hugo-Polloli
force-pushed
the
fix/6316-svelte-store-undeclared
branch
from
0d2ff9b to
030813d
Compare
Hugo-Polloli
changed the title
|
@dyc3 Also, as a side-note, running |
|
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:
WalkthroughIntroduces a SemanticFlavor (Vanilla | Svelte) and threads it through semantic model construction, the semantic event extractor and builder, and document/snippet services. Adds Svelte-aware logic to resolve Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/biome_service/src/file_handlers/html.rs (1)
924-928: Recommended DRY pass: centralise flavour-aware JS service creation.Both blocks now do the same flavour derivation +
with_js_semantic_modeldance; a shared helper would reduce drift risk.Also applies to: 1010-1017
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/file_handlers/html.rs` around lines 924 - 928, Duplicate logic computing a SemanticFlavor and creating a JsDocumentServices with that flavor should be extracted into a single helper (e.g., a function like js_services_for or create_flavored_js_services) that takes the inputs used above (the file_source, path, and snippet/tree) and returns the JsDocumentServices instance; replace both usages that call SemanticFlavor::from_source_type_and_path(...) followed by JsDocumentServices::default().with_js_semantic_model(&snippet.tree(), flavor).into() with calls to the new helper to centralize flavor derivation and with_js_semantic_model invocation.crates/biome_service/src/file_handlers/javascript.rs (1)
864-872: Optional: extract a shared helper for flavour-awaresemantic_modelcreation.Both call sites build the same
SemanticModelOptionspattern; centralising it would keep future tweaks in one place.Also applies to: 1315-1327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/file_handlers/javascript.rs` around lines 864 - 872, Extract a small helper (e.g., fn semantic_options_for_path(path: &BiomePath) -> SemanticModelOptions) that builds SemanticModelOptions with flavor set via SemanticFlavor::from_file_path(path.as_path().as_str()) and default for other fields, then replace the duplicated option construction in debug_semantic_model and the other call site (around the 1315-1327 block) to call this helper when invoking semantic_model; keep function name and visibility local/private to the module.crates/biome_js_semantic/src/semantic_model/tests.rs (1)
293-390: Optional tidy-up: extract a tiny Svelte-model test helper.The repeated
SemanticModelOptions { flavor: SemanticFlavor::Svelte, .. }setup could be centralised to keep these tests leaner as the matrix grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_semantic/src/semantic_model/tests.rs` around lines 293 - 390, Summarize repeated setup and add a small test helper: many tests repeatedly construct SemanticModelOptions { flavor: SemanticFlavor::Svelte, ..SemanticModelOptions::default() } before calling semantic_model; extract that into a helper like fn svelte_options() -> SemanticModelOptions (or svelte_options_with_globals(globals: impl IntoIterator<Item=String>) if needed) that returns the same configured SemanticModelOptions, then replace the inline constructions in tests such as ok_semantic_model_svelte_rune_is_not_normalized_as_store, ok_semantic_model_svelte_dollar_identifier_binding_uses_exact_name, ok_semantic_model_svelte_store_dereference_can_resolve_outer_value_binding, ok_semantic_model_svelte_store_dereference_resolves_configured_global, ok_semantic_model_svelte_double_dollar_reference_is_not_normalized with calls to svelte_options() (and add globals to the returned options where the test inserts into options.globals).crates/biome_cli/tests/cases/handle_svelte_files.rs (1)
560-637: Consider a small table-driven helper for these three tests.Same steps, different fixture payloads — a helper would trim duplication and keep future additions painless.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_cli/tests/cases/handle_svelte_files.rs` around lines 560 - 637, These three tests (no_undeclared_variables_handles_svelte_store_dereference, no_undeclared_variables_handles_svelte_module_store_dereference, no_undeclared_variables_handles_svelte_module_type_only_store_dereference) duplicate the same setup and assertions; refactor into a small table-driven helper that takes the fixture path/name and the fixture contents constant (SVELTE_STORE_DEREFERENCE_FILE, SVELTE_MODULE_STORE_DEREFERENCE_FILE, SVELTE_MODULE_TYPE_ONLY_STORE_DEREFERENCE_FILE), calls the common setup (MemoryFileSystem::default, BufferConsole::default, inserts the file, calls run_cli with Args::from([... "lint", "--only=noUndeclaredVariables", file.as_str()])), asserts result.is_err(), and calls assert_cli_snapshot with the expected snapshot name; then replace each test with a one-liner that invokes the helper (or iterate a vec of (snapshot_name, file_name, fixture_const) and run the same assertions) while keeping run_cli, Args, and assert_cli_snapshot usage identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_semantic/src/events.rs`:
- Around line 1114-1126: The branch detecting namespace imports used as types
(JsSyntaxKind::JS_NAMESPACE_IMPORT_SPECIFIER && matches!(&name,
BindingName::Type(_))) correctly pushes a SemanticEvent::UnresolvedReference but
then falls through and later also emits Read/HoistedRead for the same usage;
change the control flow in the loop handling references (the surrounding
iterator/loop where reference is processed) so that after calling
self.stash.push_back(SemanticEvent::UnresolvedReference { is_read:
!reference.is_write(), range: reference.range() }) you immediately skip further
processing for that reference (e.g., add a continue or otherwise return from
that branch) to avoid emitting duplicate/conflicting Read/HoistedRead events.
Ensure the short-circuit is placed within the same scope that contains the
existing push_back so only that reference is affected.
---
Nitpick comments:
In `@crates/biome_cli/tests/cases/handle_svelte_files.rs`:
- Around line 560-637: These three tests
(no_undeclared_variables_handles_svelte_store_dereference,
no_undeclared_variables_handles_svelte_module_store_dereference,
no_undeclared_variables_handles_svelte_module_type_only_store_dereference)
duplicate the same setup and assertions; refactor into a small table-driven
helper that takes the fixture path/name and the fixture contents constant
(SVELTE_STORE_DEREFERENCE_FILE, SVELTE_MODULE_STORE_DEREFERENCE_FILE,
SVELTE_MODULE_TYPE_ONLY_STORE_DEREFERENCE_FILE), calls the common setup
(MemoryFileSystem::default, BufferConsole::default, inserts the file, calls
run_cli with Args::from([... "lint", "--only=noUndeclaredVariables",
file.as_str()])), asserts result.is_err(), and calls assert_cli_snapshot with
the expected snapshot name; then replace each test with a one-liner that invokes
the helper (or iterate a vec of (snapshot_name, file_name, fixture_const) and
run the same assertions) while keeping run_cli, Args, and assert_cli_snapshot
usage identical.
In `@crates/biome_js_semantic/src/semantic_model/tests.rs`:
- Around line 293-390: Summarize repeated setup and add a small test helper:
many tests repeatedly construct SemanticModelOptions { flavor:
SemanticFlavor::Svelte, ..SemanticModelOptions::default() } before calling
semantic_model; extract that into a helper like fn svelte_options() ->
SemanticModelOptions (or svelte_options_with_globals(globals: impl
IntoIterator<Item=String>) if needed) that returns the same configured
SemanticModelOptions, then replace the inline constructions in tests such as
ok_semantic_model_svelte_rune_is_not_normalized_as_store,
ok_semantic_model_svelte_dollar_identifier_binding_uses_exact_name,
ok_semantic_model_svelte_store_dereference_can_resolve_outer_value_binding,
ok_semantic_model_svelte_store_dereference_resolves_configured_global,
ok_semantic_model_svelte_double_dollar_reference_is_not_normalized with calls to
svelte_options() (and add globals to the returned options where the test inserts
into options.globals).
In `@crates/biome_service/src/file_handlers/html.rs`:
- Around line 924-928: Duplicate logic computing a SemanticFlavor and creating a
JsDocumentServices with that flavor should be extracted into a single helper
(e.g., a function like js_services_for or create_flavored_js_services) that
takes the inputs used above (the file_source, path, and snippet/tree) and
returns the JsDocumentServices instance; replace both usages that call
SemanticFlavor::from_source_type_and_path(...) followed by
JsDocumentServices::default().with_js_semantic_model(&snippet.tree(),
flavor).into() with calls to the new helper to centralize flavor derivation and
with_js_semantic_model invocation.
In `@crates/biome_service/src/file_handlers/javascript.rs`:
- Around line 864-872: Extract a small helper (e.g., fn
semantic_options_for_path(path: &BiomePath) -> SemanticModelOptions) that builds
SemanticModelOptions with flavor set via
SemanticFlavor::from_file_path(path.as_path().as_str()) and default for other
fields, then replace the duplicated option construction in debug_semantic_model
and the other call site (around the 1315-1327 block) to call this helper when
invoking semantic_model; keep function name and visibility local/private to the
module.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
crates/biome_cli/tests/snapshots/main_cases_handle_svelte_files/no_undeclared_variables_handles_svelte_module_store_dereference.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_handle_svelte_files/no_undeclared_variables_handles_svelte_module_type_only_store_dereference.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_handle_svelte_files/no_undeclared_variables_handles_svelte_store_dereference.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidDoubleDollarStoreDereference.svelte.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidStoreDereference.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidTypeOnlyStoreDereference.svelte.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validDollarPrefixedIdentifier.svelte.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validStoreDereference.svelte.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validSvelteGlobalStoreDereference.svelte.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (22)
.changeset/tasty-wings-bake.mdcrates/biome_cli/tests/cases/handle_svelte_files.rscrates/biome_js_analyze/src/lint/correctness/no_undeclared_variables.rscrates/biome_js_analyze/src/services/semantic.rscrates/biome_js_analyze/src/utils/rename.rscrates/biome_js_analyze/tests/spec_tests.rscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidDoubleDollarStoreDereference.svelte.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidStoreDereference.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidTypeOnlyStoreDereference.svelte.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validDollarPrefixedIdentifier.svelte.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validStoreDereference.svelte.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validSvelteGlobalStoreDereference.svelte.options.jsoncrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validSvelteGlobalStoreDereference.svelte.tscrates/biome_js_semantic/src/events.rscrates/biome_js_semantic/src/semantic_model.rscrates/biome_js_semantic/src/semantic_model/builder.rscrates/biome_js_semantic/src/semantic_model/tests.rscrates/biome_js_syntax/src/file_source.rscrates/biome_service/src/file_handlers/html.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_service/src/workspace/document/mod.rscrates/biome_service/src/workspace/server.rs
Merging this PR will not alter performance
Comparing 
Footnotes
|
Hugo-Polloli
force-pushed
the
fix/6316-svelte-store-undeclared
branch
from
239c0d5 to
cf1c3ca
Compare
| } | ||
| } | ||
|
|
||
| fn js_document_services_for_snippet( |
There was a problem hiding this comment.
I understand now, but this function shouldn't be here, it doesn' have anything to do with html
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_semantic/src/events.rs (1)
943-960: Nice flavour gate; consider removing the hard-coded$slice offset.The Svelte-only guard is solid. One small refactor: this still hard-codes
TextSize::from(1)after callingJsFileSource::svelte_store_reference_name, so extraction logic is split across two places again.Optional tidy-up
- JsFileSource::svelte_store_reference_name(name.text())?; - - let store_name_start = TextSize::from(1); + let store_name = JsFileSource::svelte_store_reference_name(name.text())?; + let store_name_start = + name.len() - TextSize::from(u32::try_from(store_name.len()).ok()?); Some(BindingName::Value( name.clone() .slice(TextRange::new(store_name_start, name.len())), ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_semantic/src/events.rs` around lines 943 - 960, In maybe_svelte_store_dereference_binding_name, avoid the hard-coded TextSize::from(1) by using the result from JsFileSource::svelte_store_reference_name instead of discarding it; update JsFileSource::svelte_store_reference_name to return the slice start (or a TextRange/offset) for the svelte `$` prefix and then use that returned value to build the new BindingName in maybe_svelte_store_dereference_binding_name (keep the SemanticFlavor::Svelte guard and BindingName::Value handling as-is, just replace the literal store_name_start with the value returned by JsFileSource::svelte_store_reference_name).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_js_semantic/src/events.rs`:
- Around line 943-960: In maybe_svelte_store_dereference_binding_name, avoid the
hard-coded TextSize::from(1) by using the result from
JsFileSource::svelte_store_reference_name instead of discarding it; update
JsFileSource::svelte_store_reference_name to return the slice start (or a
TextRange/offset) for the svelte `$` prefix and then use that returned value to
build the new BindingName in maybe_svelte_store_dereference_binding_name (keep
the SemanticFlavor::Svelte guard and BindingName::Value handling as-is, just
replace the literal store_name_start with the value returned by
JsFileSource::svelte_store_reference_name).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/biome_cli/tests/cases/handle_svelte_files.rscrates/biome_js_semantic/src/events.rscrates/biome_js_semantic/src/semantic_model/tests.rscrates/biome_service/src/file_handlers/html.rscrates/biome_service/src/file_handlers/javascript.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_cli/tests/cases/handle_svelte_files.rs
| let svelte_store_name = if is_svelte_file { | ||
| JsFileSource::svelte_store_reference_name(text) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let source_type = ctx.source_type::<JsFileSource>(); | ||
|
|
||
| if ctx.is_global(text) { | ||
| if ctx.is_global(text) | ||
| || svelte_store_name.is_some_and(|store_name| ctx.is_global(store_name)) | ||
| { | ||
| return None; | ||
| } | ||
|
|
||
| let embedded_bindings = ctx | ||
| .get_service::<EmbeddedBindings>() | ||
| .expect("embedded bindings service"); | ||
| if embedded_bindings.contains_binding(text) { | ||
| if embedded_bindings.contains_binding(text) | ||
| || svelte_store_name | ||
| .is_some_and(|store_name| embedded_bindings.contains_binding(store_name)) | ||
| { |
There was a problem hiding this comment.
It's not really clear to me why the changes to the rule are necessary if the fix is in the semantic model.
There was a problem hiding this comment.
the semantic fix covers the lexical case where $store resolves to an actual store binding in scope. But noUndeclaredVariables also has its own policy layer for things like javascript.globals config and embedded bindings, which don't flow through the semantic model end-to-end yet. So without the rule-side change those cases would misbehave
Hugo-Polloli
force-pushed
the
fix/6316-svelte-store-undeclared
branch
from
20cd81b to
4e09140
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/biome_cli/tests/cases/handle_svelte_files.rs (1)
50-59: Deduplicate identical fixture payloads
SVELTE_MODULE_STORE_DEREFERENCE_FILEandSVELTE_JS_MODULE_STORE_DEREFERENCE_FILEare byte-for-byte identical; aliasing one to the other would keep future fixture edits in one place.Suggested diff
const SVELTE_MODULE_STORE_DEREFERENCE_FILE: &str = r#"const store = 1; $store; $missing;"#; @@ -const SVELTE_JS_MODULE_STORE_DEREFERENCE_FILE: &str = r#"const store = 1; -$store; -$missing;"#; +const SVELTE_JS_MODULE_STORE_DEREFERENCE_FILE: &str = + SVELTE_MODULE_STORE_DEREFERENCE_FILE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_cli/tests/cases/handle_svelte_files.rs` around lines 50 - 59, Two fixture constants SVELTE_MODULE_STORE_DEREFERENCE_FILE and SVELTE_JS_MODULE_STORE_DEREFERENCE_FILE contain the exact same payload; to deduplicate, replace the duplicate definition by aliasing SVELTE_JS_MODULE_STORE_DEREFERENCE_FILE to SVELTE_MODULE_STORE_DEREFERENCE_FILE (i.e., assign SVELTE_JS_MODULE_STORE_DEREFERENCE_FILE = SVELTE_MODULE_STORE_DEREFERENCE_FILE) so future edits live in one place and keep SVELTE_MODULE_TYPE_ONLY_STORE_DEREFERENCE_FILE unchanged.crates/biome_js_analyze/src/lint/correctness/no_undeclared_variables.rs (1)
73-93: Optional: computeSemanticFlavoronce per run.Tiny tidy-up: flavour is file-invariant, so you can hoist it outside
filter_mapfor readability and a small perf win.♻️ Suggested patch
fn run(ctx: &RuleContext<Self>) -> Self::Signals { let model = ctx.query(); let source_type = ctx.source_type::<JsFileSource>(); + let flavor = SemanticFlavor::from(source_type); let embedded_bindings = ctx .get_service::<EmbeddedBindings>() .expect("embedded bindings service"); @@ - let store_name = SemanticFlavor::from(source_type).store_reference_name(text); + let store_name = flavor.store_reference_name(text);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/correctness/no_undeclared_variables.rs` around lines 73 - 93, Compute SemanticFlavor::from(source_type) once before calling model.all_unresolved_references() and reuse that value inside the filter_map closure instead of calling SemanticFlavor::from(source_type) repeatedly; specifically, introduce a local variable (e.g., let semantic_flavor = SemanticFlavor::from(source_type)) above the model...all_unresolved_references() chain and replace the in-closure call to SemanticFlavor::from(source_type).store_reference_name(text) with semantic_flavor.store_reference_name(text) to hoist file-invariant work out of the loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_semantic/src/semantic_model/flavor.rs`:
- Around line 29-37: The function store_reference_name currently gates Svelte
handling incorrectly: change the early check to only proceed for Svelte flavor
by flipping the condition so that you return None when NOT
self.is_svelte_rune(reference_name); then perform strip_prefix('$') and the
existing emptiness/starts_with('$') checks as before. Update the if in
store_reference_name (and keep is_svelte_rune as the selector) so only Svelte
refs lose the leading '$' and Vanilla refs are left untouched.
---
Nitpick comments:
In `@crates/biome_cli/tests/cases/handle_svelte_files.rs`:
- Around line 50-59: Two fixture constants SVELTE_MODULE_STORE_DEREFERENCE_FILE
and SVELTE_JS_MODULE_STORE_DEREFERENCE_FILE contain the exact same payload; to
deduplicate, replace the duplicate definition by aliasing
SVELTE_JS_MODULE_STORE_DEREFERENCE_FILE to SVELTE_MODULE_STORE_DEREFERENCE_FILE
(i.e., assign SVELTE_JS_MODULE_STORE_DEREFERENCE_FILE =
SVELTE_MODULE_STORE_DEREFERENCE_FILE) so future edits live in one place and keep
SVELTE_MODULE_TYPE_ONLY_STORE_DEREFERENCE_FILE unchanged.
In `@crates/biome_js_analyze/src/lint/correctness/no_undeclared_variables.rs`:
- Around line 73-93: Compute SemanticFlavor::from(source_type) once before
calling model.all_unresolved_references() and reuse that value inside the
filter_map closure instead of calling SemanticFlavor::from(source_type)
repeatedly; specifically, introduce a local variable (e.g., let semantic_flavor
= SemanticFlavor::from(source_type)) above the
model...all_unresolved_references() chain and replace the in-closure call to
SemanticFlavor::from(source_type).store_reference_name(text) with
semantic_flavor.store_reference_name(text) to hoist file-invariant work out of
the loop.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
crates/biome_cli/tests/snapshots/main_cases_handle_svelte_files/no_undeclared_variables_check_types_handles_svelte_ts_script_globals.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_handle_svelte_files/no_undeclared_variables_handles_svelte_js_module_store_dereference.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_handle_svelte_files/no_undeclared_variables_handles_svelte_module_store_dereference.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_handle_svelte_files/no_undeclared_variables_handles_svelte_module_type_only_store_dereference.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_handle_svelte_files/no_undeclared_variables_handles_svelte_store_dereference.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noConstAssign/validSvelteStoreWrite.svelte.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidBuiltinGlobalStoreDereference.svelte.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidDoubleDollarStoreDereference.svelte.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidStoreDereference.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidTypeOnlyStoreDereference.svelte.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validDollarPrefixedIdentifier.svelte.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validStoreDereference.svelte.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validSvelteGlobalStoreDereference.svelte.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/suspicious/noImportAssign/validSvelteStoreWrite.svelte.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (31)
.changeset/tasty-wings-bake.mdcrates/biome_cli/tests/cases/handle_svelte_files.rscrates/biome_js_analyze/src/lint/correctness/no_undeclared_variables.rscrates/biome_js_analyze/src/services/semantic.rscrates/biome_js_analyze/src/utils/rename.rscrates/biome_js_analyze/tests/spec_tests.rscrates/biome_js_analyze/tests/specs/correctness/noConstAssign/validSvelteStoreWrite.svelte.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidBuiltinGlobalStoreDereference.svelte.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidDoubleDollarStoreDereference.svelte.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidStoreDereference.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidTypeOnlyStoreDereference.svelte.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validDollarPrefixedIdentifier.svelte.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validStoreDereference.svelte.tscrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validSvelteGlobalStoreDereference.svelte.options.jsoncrates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validSvelteGlobalStoreDereference.svelte.tscrates/biome_js_analyze/tests/specs/suspicious/noImportAssign/validSvelteStoreWrite.svelte.tscrates/biome_js_semantic/src/events.rscrates/biome_js_semantic/src/semantic_model.rscrates/biome_js_semantic/src/semantic_model/builder.rscrates/biome_js_semantic/src/semantic_model/flavor.rscrates/biome_js_semantic/src/semantic_model/tests.rscrates/biome_js_syntax/src/file_source.rscrates/biome_service/src/file_handlers/css.rscrates/biome_service/src/file_handlers/graphql.rscrates/biome_service/src/file_handlers/html.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_service/src/file_handlers/json.rscrates/biome_service/src/file_handlers/mod.rscrates/biome_service/src/file_handlers/svelte.rscrates/biome_service/src/workspace/document/mod.rscrates/biome_service/src/workspace/server.rs
✅ Files skipped from review due to trivial changes (2)
- crates/biome_service/src/file_handlers/css.rs
- crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidBuiltinGlobalStoreDereference.svelte.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/invalidStoreDereference.ts
- crates/biome_js_syntax/src/file_source.rs
- crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validSvelteGlobalStoreDereference.svelte.options.json
- .changeset/tasty-wings-bake.md
- crates/biome_js_semantic/src/semantic_model/tests.rs
- crates/biome_js_semantic/src/semantic_model.rs
- crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validStoreDereference.svelte.ts
- crates/biome_js_analyze/tests/spec_tests.rs
- crates/biome_service/src/workspace/document/mod.rs
- crates/biome_js_analyze/tests/specs/correctness/noUndeclaredVariables/validSvelteGlobalStoreDereference.svelte.ts
- crates/biome_service/src/file_handlers/html.rs
hey so i've reworked this from the ground up and it ended up getting pretty big. I had to add some pretty important new internal plumbing, that is for now tentatively named SemanticFlavor (so that we can interpret .svelte.js and .svelte.ts modules properly) |
Hugo-Polloli
force-pushed
the
fix/6316-svelte-store-undeclared
branch
3 times, most recently
from
1ce9220 to
df95ffd
Compare
Hugo-Polloli
force-pushed
the
fix/6316-svelte-store-undeclared
branch
from
df95ffd to
f954f2a
Compare
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Still, there are many foundmental issues in the code
There was a problem hiding this comment.
Revert all the code here. We don't use this capability at the moment
There was a problem hiding this comment.
Oops... Reverted, I didnt know rename was not available!
|
|
||
| fn visit(&mut self, event: &WalkEvent<JsSyntaxNode>, _ctx: VisitorContext<JsLanguage>) { | ||
| fn visit(&mut self, event: &WalkEvent<JsSyntaxNode>, ctx: VisitorContext<JsLanguage>) { | ||
| if !self.flavor_configured { |
There was a problem hiding this comment.
I don't see the point of this flag. Even less because there are no docs. Remember to add docs strings, they help
There was a problem hiding this comment.
Thanks, I've simplified this:
I removed the flavor_configured flag and now configure flavor on root Enter and I added doc comments explaining why flavor is set there.
Is that better?
| .into(); | ||
| } else if document_source.is_javascript_like() { | ||
| } else if document_source.is_javascript_like() | ||
| && let Some(source_type) = | ||
| self.semantic_js_source_type(&path, document_source, &content) | ||
| { |
There was a problem hiding this comment.
I really don't understand what we're doing here. We already have document_source that provides all the information we need. Just use if let DocumentSource::Js() instead of is_javascript_like.
The function semantic_js_source_type isn't needed
There was a problem hiding this comment.
Thanks, I've switched to if let DocumentSource::Js() and removed the is_javascript_like() branch :)
| continue; | ||
| }; | ||
| let capabilities = self.features.get_capabilities(file_source); | ||
| let capabilities = self.features.get_capabilities(&path, file_source); |
There was a problem hiding this comment.
Again, file_source is our source of truth. Once we have it, we don't need paths anymore
There was a problem hiding this comment.
From what I can understand, today JsFileSource cannot distinguish .svelte component documents from .svelte.js/.svelte.ts source modules. ie. they can collapse to the same Svelte embedding shape
But we need different handling for source-type inference and capability routing :/
Would you be ok with me making this distinction explicit in JsFileSource (or maybe embedding metadata?) which would remove path checks entirely, or do you have another idea in mind to handle this problem?
There was a problem hiding this comment.
Yes, you should make the distinction in the JsFileSource.
There was a problem hiding this comment.
Done.
I made the distinction explicit in JsFileSource by extending Svelte embedding metadata with kind: SvelteFileKind (variants: Component and SourceModule), so .svelte and .svelte.ts/.svelte.js no longer collapse to the same shape.
Why:
.sveltecomponent documents need<script ...>extraction/inference behavior.svelte.ts/.svelte.jsare already full JS/TS source modules and should use JS capabilities directly
This in turn allowed me to remove path-based routing checks and now route from source metadata :)
SourceModule-> JS capabilitiesComponent-> Svelte handler behavior
A small nuance: for source-type inference specifically, component files are still inferred from <script ...> content (SvelteFileHandler::file_source(content)), but this is now triggered from source metadata (is_svelte_component) rather than path checks.
This keeps file_source as the single source of truth and avoids threading path everywhere.
I've also added comments for this new system along the line
Hugo-Polloli
force-pushed
the
fix/6316-svelte-store-undeclared
branch
from
8bafea7 to
3b8cd08
Compare
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
I'm wondering if it would make more sense to handle svelte stores at the parser level instead of this. We could implement a JsSyntaxFeature that can conditionally enable the syntax. Then the changes to the semantic model could be much more precise. What do you think @ematipico?
Hugo-Polloli
force-pushed
the
fix/6316-svelte-store-undeclared
branch
3 times, most recently
from
900d20f to
a7abbb9
Compare
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
I think we're getting somewhere, and it's taking a good shape. There are still a few things to address
Hugo-Polloli
force-pushed
the
fix/6316-svelte-store-undeclared
branch
from
26316b8 to
cfa91db
Compare
Absolutely no pressing matter, but I wanted to ensure we were in the same page, I think I've addressed all the existing concerns, but if you still need any follow-up from me I'm ready :) |
dyc3
force-pushed
the
fix/6316-svelte-store-undeclared
branch
from
8b1030f to
93a52b4
Compare
|
Hey @Hugo-Polloli, I rebased the branch to resolve the merge conflicts, and because we recently improved our testing infra. I believe we no longer need the CLI tests for this, we can use our normal per-rule snapshot infra to help verify this. I've added a commit 93a52b4 to explain what i mean |
3 participants
Fixes #6316
Summary
This PR enforces one invariant across the pipeline: in Svelte-flavored code,
$storemust resolve consistently to the backingstorebinding (or report consistently when missing).The fix spans semantic extraction, analyzer policy, rename, and service routing because the bug crossed those layers. Keeping any layer on old behavior reintroduced mismatches.
SemanticFlavorso Svelte-specific identifier behavior is modeled in semantic extraction/builder paths.$storeread behavior semantically while preserving unresolved diagnostics for real misses.$store = ...) in rules relying on semantic writes.noUndeclaredVariablesfallback policy:$name -> namefallback applies to configured globals and embedded bindings, but not generic env globals (for example avoids$locationfalse negatives).$and collision checks validate the effective transformed name..svelte.jsand.svelte.ts) so they use JS capabilities, while.sveltecomponent documents keep Svelte handler behavior.This also establishes reusable Svelte semantic infrastructure (flavor + routing/plumbing consistency) that future Svelte 5 rune semantics can build on without another cross-layer refactor.
Test Plan
$storeresolves to declaredstorebindings,$edge cases remain diagnostic,.svelte,.svelte.ts, and.svelte.jsstore dereference scenarios,$locationnow reports undeclared unless explicitly backed by policy/binding).noConstAssign/noImportAssignfalse positives.$prefix,.svelte.jsand.svelte.tssource modules expose JS capabilities (including rename and pull diagnostics/actions),.sveltecomponent files keep Svelte capability behavior.Docs