fix(inference): improved inference for assignment types #8119
Conversation
🦋 Changeset detectedLatest commit: 7b39a52 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-Project
WalkthroughThis pull request enhances Biome's type inference engine to better handle variables assigned multiple values. It introduces a new public API method Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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_js_type_info/src/resolver.rs (1)
725-731: union_with helper is fine, but note the “naive” union growthThe implementation matches
optionaland does what the PR describes: it always creates a fresh two‑memberUnionfromcurrent_typeandtyand registers it.That’s perfectly OK for the current “naive” widening, but it does mean repeated widening will keep allocating new union types without flattening or deduplication. If unions ever start to get large or performance becomes an issue, this might be a good place to introduce flattening/reuse.
.changeset/upset-cameras-walk.md (1)
5-5: Tiny wording tweak (optional)Pure style, but this reads a bit smoother without the comma:
-Improved the type inference engine, by resolving types for variables that are assigned to multiple values. +Improved the type inference engine by resolving types for variables that are assigned to multiple values.crates/biome_module_graph/tests/spec_tests.rs (1)
2132-2163: New widening snapshot test is consistent and focusedThis is a clean minimal case for “boolean literal + reassignment” widening; setup mirrors other snapshot tests and the relative
"index.ts"path is used consistently, so behaviour should be stable.If you want to align style with nearby tests, you could optionally write:
let snapshot = ModuleGraphSnapshot::new(module_graph.as_ref(), &fs).with_resolver(resolver.as_ref());but the current
&module_graphform is perfectly fine thanks to deref‑coercion.crates/biome_module_graph/src/js_module_info/collector.rs (1)
609-648: Code duplication in the if-else branches.Lines 613–616 and 643–648 perform identical lookups from
typed_bindings. The else branch is redundant; you can simplify by initialisingtyonce (lines 613–616), applying the writable-reference union conditionally, then returningtyat the end.Apply this refactor to eliminate duplication:
- let ty = if let Some(typed_bindings) = decl + let mut ty = if let Some(typed_bindings) = decl .as_js_variable_declaration() .and_then(|decl| self.variable_declarations.get(decl.syntax())) { - let mut ty = typed_bindings + typed_bindings .iter() .find_map(|(name, ty)| (name == binding_name).then(|| ty.clone())) - .unwrap_or_default(); - + .unwrap_or_default() + } else { + let data = TypeData::from_any_js_declaration(self, scope_id, &decl); + self.reference_to_owned_data(data) + }; + if self.has_writable_reference(&binding) { let references = self.get_writable_references(&binding); for reference in references { let Some(node) = self.binding_node_by_start.get(&reference.range_start) else { continue; }; for ancestor in node.ancestors().skip(1) { if let Some(assignment) = JsAssignmentExpression::cast_ref(&ancestor) && let Ok(right) = assignment.right() { let data = TypeData::from_any_js_expression(self, scope_id, &right); let assigned_type = self.reference_to_owned_data(data); ty = ResolvedTypeId::new( self.level(), self.union_with(ty.clone(), assigned_type).into(), ) .into(); } } } - - ty - } else { - typed_bindings - .iter() - .find_map(|(name, ty)| (name == binding_name).then(|| ty.clone())) - .unwrap_or_default() } - } else { - let data = TypeData::from_any_js_declaration(self, scope_id, &decl); - self.reference_to_owned_data(data) - };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/validAssignment.ts.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_widening_via_assignment.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (7)
.changeset/lovely-sloths-chew.md(1 hunks).changeset/upset-cameras-walk.md(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/validAssignment.ts(1 hunks)crates/biome_js_type_info/src/resolver.rs(1 hunks)crates/biome_module_graph/src/js_module_info/collector.rs(2 hunks)crates/biome_module_graph/tests/snapshots/test_widening_via_assignment_multiple_values.snap.new(1 hunks)crates/biome_module_graph/tests/spec_tests.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T16:24:59.781Z
Learnt from: arendjr
Repo: biomejs/biome PR: 7266
File: crates/biome_js_type_info/src/type.rs:94-102
Timestamp: 2025-08-20T16:24:59.781Z
Learning: In crates/biome_js_type_info/src/type.rs, the flattened_union_variants() method returns TypeReference instances that already have the correct module IDs applied to them. These references should be used directly with resolver.resolve_reference() without applying additional module ID transformations, as variant references may originate from nested unions in different modules.
Applied to files:
crates/biome_js_type_info/src/resolver.rscrates/biome_module_graph/src/js_module_info/collector.rscrates/biome_module_graph/tests/snapshots/test_widening_via_assignment_multiple_values.snap.new
🧬 Code graph analysis (2)
crates/biome_module_graph/tests/spec_tests.rs (3)
crates/biome_fs/src/fs/memory.rs (1)
default(37-49)crates/biome_test_utils/src/lib.rs (1)
get_added_paths(169-190)crates/biome_module_graph/src/js_module_info/module_resolver.rs (1)
for_module(76-91)
crates/biome_module_graph/src/js_module_info/collector.rs (3)
crates/biome_js_type_info/src/type_data.rs (8)
ty(545-550)index(45-47)index(1431-1433)index(1472-1476)reference(343-345)new(39-43)new(1425-1429)new(1460-1470)crates/biome_module_graph/src/js_module_info.rs (1)
binding(264-266)crates/biome_js_type_info/src/local_inference.rs (3)
from_any_js_expression(425-602)from_any_js_expression(2173-2180)from_any_js_expression(2343-2425)
🪛 LanguageTool
.changeset/lovely-sloths-chew.md
[uncategorized] ~5-~5: Possible missing comma found.
Context: ... of the rule noUnnecessaryConditions. Now the rule doesn't isn't triggered for va...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~5-~5: Two consecutive contractions are very uncommon. Did you maybe just mean “doesn't” or “isn't”?
Context: ...noUnnecessaryConditions. Now the rule doesn't isn't triggered for variables that are mutate...
(DON_T_AREN_T)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: autofix
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Test Node.js API
- GitHub Check: Documentation
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: End-to-end tests
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
🔇 Additional comments (7)
crates/biome_js_analyze/tests/specs/nursery/noUnnecessaryConditions/validAssignment.ts (1)
1-9: Targeted regression fixture looks goodThis neatly captures the “mutated in the same module before the condition” case for
noUnnecessaryConditions. No changes needed from my side.crates/biome_module_graph/tests/spec_tests.rs (1)
2165-2200: Good coverage for multi‑value wideningNice to see a dedicated case where a binding starts as
undefinedand is then assigned both a string and a number in separate functions; that should exercise the “multiple assignments → union” path nicely. Snapshot wiring matches the previous test and looks correct.Nothing I’d change here.
crates/biome_module_graph/tests/snapshots/test_widening_via_assignment_multiple_values.snap.new (1)
1-99: Snapshot looks good!The progressive type widening from
undefined→undefined | string→undefined | string | numbercorrectly reflects the multiple assignment behavior. The structure is clear and validates the new inference logic.crates/biome_module_graph/src/js_module_info/collector.rs (4)
6-9: LGTM!Import changes appropriately include
JsAssignmentExpressionfor the new assignment-tracking logic.
578-578: Clone is acceptable here.Cloning the binding provides access to metadata needed for the improved inference. Since this occurs once per binding during collection, the performance cost is reasonable.
584-598: Helper methods look solid.Clean, focused implementations for detecting and collecting writable references. These support the new type-widening logic nicely.
654-654: Early return is appropriate.Returning after resolving the type from variable declarations prevents unnecessary traversal of further ancestors and maintains the existing control-flow pattern.
CodSpeed Performance ReportMerging #8119 will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_module_graph/tests/snapshots/test_widening_via_assignment_multiple_values.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (2)
.changeset/lovely-sloths-chew.md(1 hunks)crates/biome_module_graph/src/js_module_info/collector.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: arendjr
Repo: biomejs/biome PR: 7266
File: crates/biome_js_type_info/src/type.rs:94-102
Timestamp: 2025-08-20T16:24:59.781Z
Learning: In crates/biome_js_type_info/src/type.rs, the flattened_union_variants() method returns TypeReference instances that already have the correct module IDs applied to them. These references should be used directly with resolver.resolve_reference() without applying additional module ID transformations, as variant references may originate from nested unions in different modules.
📚 Learning: 2025-08-20T16:24:59.781Z
Learnt from: arendjr
Repo: biomejs/biome PR: 7266
File: crates/biome_js_type_info/src/type.rs:94-102
Timestamp: 2025-08-20T16:24:59.781Z
Learning: In crates/biome_js_type_info/src/type.rs, the flattened_union_variants() method returns TypeReference instances that already have the correct module IDs applied to them. These references should be used directly with resolver.resolve_reference() without applying additional module ID transformations, as variant references may originate from nested unions in different modules.
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
📚 Learning: 2025-08-05T14:43:29.581Z
Learnt from: dyc3
Repo: biomejs/biome PR: 7081
File: packages/@biomejs/biome/configuration_schema.json:7765-7781
Timestamp: 2025-08-05T14:43:29.581Z
Learning: The file `packages/biomejs/biome/configuration_schema.json` is auto-generated and should not be manually edited or reviewed for schema issues; any changes should be made at the code generation source.
Applied to files:
.changeset/lovely-sloths-chew.md
🧬 Code graph analysis (1)
crates/biome_module_graph/src/js_module_info/collector.rs (2)
crates/biome_module_graph/src/js_module_info.rs (1)
binding(264-266)crates/biome_js_type_info/src/local_inference.rs (3)
from_any_js_expression(425-602)from_any_js_expression(2173-2180)from_any_js_expression(2343-2425)
🪛 LanguageTool
.changeset/lovely-sloths-chew.md
[uncategorized] ~5-~5: Possible missing comma found.
Context: ... of the rule noUnnecessaryConditions. Now the rule isn't triggered for variables ...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: autofix
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
🔇 Additional comments (2)
crates/biome_module_graph/src/js_module_info/collector.rs (2)
584-598: Clean helper methods.These helper methods are well-structured and serve their purpose clearly.
618-641: Theunion_withmethod exists and is correctly used; however, scope handling requires architectural clarification.The call to
self.union_with(ty.clone(), assigned_type)on line 635 is valid—the method exists on theTypeResolvertrait thatJsModuleInfoCollectorimplements.However, the scope concern remains: line 631 uses the binding's declaration scope (
scope_id) to infer the assignment's RHS type, even when the assignment occurs in a nested scope (e.g., inside a function). Sincescope_by_rangeis available elsewhere in the collector, consider whether you should look up the assignment's actual scope rather than using the binding's declaration scope. This matters especially for resolving identifiers referenced in complex RHS expressions within different scopes.
| } else { | ||
| typed_bindings | ||
| .iter() | ||
| .find_map(|(name, ty)| (name == binding_name).then(|| ty.clone())) | ||
| .unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate logic.
The else branch duplicates the exact logic from lines 613-616. Since ty is already computed, simply return it directly instead of re-computing.
Apply this diff:
} else {
- typed_bindings
- .iter()
- .find_map(|(name, ty)| (name == binding_name).then(|| ty.clone()))
- .unwrap_or_default()
+ ty
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/biome_module_graph/src/js_module_info/collector.rs around lines
643-648, the else branch re-runs the same find_map logic already used at lines
613-616; replace the duplicated lookup with returning the previously computed ty
value (use ty or ty.clone() as appropriate for ownership) instead of recomputing
the binding type.
Summary
Part of #6611
This PR improves the inference engine by updating the type of those bindings that are assigned to multiple values.
The implementation is quite naive for now, so if there more than one assignments, we create new types for each one of them. You can see it from the snapshots.
cc @arendjr
Test Plan
Added new tests
Docs