perf(inference): reduce type widening #8374
Conversation
|
CodSpeed Performance ReportMerging #8374 will not alter performanceComparing Summary
Footnotes
|
WalkthroughAdds a public UnionCollector and uses it in the JS module info collector to accumulate union types from writable references instead of merging per reference. Introduces get_scope_by_range(range) to locate the innermost scope for a reference and skips widening when the reference scope equals the binding scope. Refactors assignment extraction to obtain the right‑hand expression under the new scope checks, finalises the union via UnionCollector, registers it, and updates the binding type to the resulting ResolvedTypeId. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ 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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/biome_module_graph/src/js_module_info/collector.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use thedbg!()macro for debugging output during testing, and pass the--show-outputflag tocargoto view debug output
Usecargo torcargo testto run tests; for a single test, pass the test name after thetestcommand
Use snapshot testing with theinstacrate; runcargo insta accept,cargo insta reject, orcargo insta reviewto manage snapshot changes
Write doctests as doc comments with code blocks; the code inside code blocks will be run during the testing phase
Usejust f(alias forjust format) to format Rust and TOML files before committing
Files:
crates/biome_module_graph/src/js_module_info/collector.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/js_module_info/collector.rs : Implement module-level (thin) inference to resolve `TypeReference::Qualifier` variants by looking up declarations in module scopes and handling import statements
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/js_module_info/collector.rs : Implement module-level (thin) inference to resolve `TypeReference::Qualifier` variants by looking up declarations in module scopes and handling import statements
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/scoped_resolver.rs : Implement full inference to resolve `TypeReference::Import` variants across the entire module graph
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Use `TypeReference` instead of `Arc` for types that reference other types to avoid stale cache issues when modules are replaced
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/local_inference.rs : Implement local inference in dedicated modules to derive type definitions from expressions without context of surrounding scopes
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Check if a variable is global before banning it to avoid false positives when the variable is redeclared in local scope; use the semantic model to verify global scope
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
📚 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-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Use `TypeReference` variants (`Qualifier`, `Resolved`, `Import`, `Unknown`) to represent different phases of type resolution
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Store type data in linear vectors instead of using recursive data structures with `Arc` for improved data locality and performance
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : No module may copy or clone data from another module in the module graph, not even behind an `Arc`
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Distinguish between `TypeData::Unknown` and `TypeData::UnknownKeyword` to measure inference effectiveness versus explicit user-provided unknown types
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
🧬 Code graph analysis (1)
crates/biome_module_graph/src/js_module_info/collector.rs (1)
crates/biome_module_graph/src/js_module_info.rs (1)
binding(264-266)
⏰ 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). (10)
- GitHub Check: Test Node.js API
- GitHub Check: autofix
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: End-to-end tests
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Bench (biome_module_graph)
github-actions
bot
added
L-JavaScript
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_module_graph/src/js_module_info/collector.rs (1)
685-716: Include the original type in the union and remove the unused assignment.The initial
ty.clone()on line 692 is never read before being overwritten on line 719 (hence the compiler warning). More importantly, theUnionCollectoronly accumulates assigned types from cross-scope writes but never includes the original declared type. This means:
- If there are no cross-scope writes, an empty union is created (uninhabitable).
- The declared type is lost entirely, which contradicts the intent of "widening."
Apply this diff to fix both issues:
let references = self.get_writable_references(binding); - let mut ty = ty.clone(); let mut union_collector = UnionCollector::new(); + union_collector.add(ty.clone()); for reference in references {This ensures the union starts with the declared type and grows with assigned types.
🧹 Nitpick comments (1)
crates/biome_module_graph/src/js_module_info/collector.rs (1)
668-682: Consider refactoringget_scope_by_rangeto use the Lapper-based approach for better performance.The assumption that higher
ScopeIdvalues represent more deeply nested scopes is correct—scopes are created sequentially asScopeId::new(self.scopes.len()), so later creation means deeper nesting. However,get_scope_by_rangeperforms a linear scan through all scope intervals for each writable reference during type widening, which is inefficient.The codebase already uses a Lapper-based approach via
scope_id_for_range()ininfer_all_types(). To improve performance here, passscope_by_rangeas a parameter through the method chain (infer_type→widen_binding_from_writable_references) and use the samescope_id_for_range()function instead of manual iteration.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/biome_js_type_info/src/resolver.rs(1 hunks)crates/biome_module_graph/src/js_module_info/collector.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use thedbg!()macro for debugging output during testing, and pass the--show-outputflag tocargoto view debug output
Usecargo torcargo testto run tests; for a single test, pass the test name after thetestcommand
Use snapshot testing with theinstacrate; runcargo insta accept,cargo insta reject, orcargo insta reviewto manage snapshot changes
Write doctests as doc comments with code blocks; the code inside code blocks will be run during the testing phase
Usejust f(alias forjust format) to format Rust and TOML files before committing
Files:
crates/biome_module_graph/src/js_module_info/collector.rscrates/biome_js_type_info/src/resolver.rs
crates/biome_js_type_info/**/*.rs
📄 CodeRabbit inference engine (crates/biome_js_type_info/CONTRIBUTING.md)
crates/biome_js_type_info/**/*.rs: No module may copy or clone data from another module in the module graph, not even behind anArc
UseTypeReferenceinstead ofArcfor types that reference other types to avoid stale cache issues when modules are replaced
Store type data in linear vectors instead of using recursive data structures withArcfor improved data locality and performance
UseTypeReferencevariants (Qualifier,Resolved,Import,Unknown) to represent different phases of type resolution
UseTypeData::Unknownto indicate when type inference falls short or is not implemented
Distinguish betweenTypeData::UnknownandTypeData::UnknownKeywordto measure inference effectiveness versus explicit user-provided unknown types
When usingResolvedTypeData, track theResolverIdto ensure subsequent resolver calls use the correct context
Always apply the correctResolverIdwhen retrieving raw type data fromResolvedTypeData.as_raw_data()to prevent panics during subsequent resolution calls
Files:
crates/biome_js_type_info/src/resolver.rs
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/js_module_info/collector.rs : Implement module-level (thin) inference to resolve `TypeReference::Qualifier` variants by looking up declarations in module scopes and handling import statements
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/js_module_info/collector.rs : Implement module-level (thin) inference to resolve `TypeReference::Qualifier` variants by looking up declarations in module scopes and handling import statements
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rscrates/biome_js_type_info/src/resolver.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/scoped_resolver.rs : Implement full inference to resolve `TypeReference::Import` variants across the entire module graph
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rscrates/biome_js_type_info/src/resolver.rs
📚 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.rscrates/biome_js_type_info/src/resolver.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Use `TypeReference` instead of `Arc` for types that reference other types to avoid stale cache issues when modules are replaced
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rscrates/biome_js_type_info/src/resolver.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Use `TypeReference` variants (`Qualifier`, `Resolved`, `Import`, `Unknown`) to represent different phases of type resolution
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rscrates/biome_js_type_info/src/resolver.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/local_inference.rs : Implement local inference in dedicated modules to derive type definitions from expressions without context of surrounding scopes
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Distinguish between `TypeData::Unknown` and `TypeData::UnknownKeyword` to measure inference effectiveness versus explicit user-provided unknown types
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rscrates/biome_js_type_info/src/resolver.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : When using `ResolvedTypeData`, track the `ResolverId` to ensure subsequent resolver calls use the correct context
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rscrates/biome_js_type_info/src/resolver.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Use `TypeData::Unknown` to indicate when type inference falls short or is not implemented
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rscrates/biome_js_type_info/src/resolver.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Store type data in linear vectors instead of using recursive data structures with `Arc` for improved data locality and performance
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/*.rs : Always apply the correct `ResolverId` when retrieving raw type data from `ResolvedTypeData.as_raw_data()` to prevent panics during subsequent resolution calls
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rscrates/biome_js_type_info/src/resolver.rs
📚 Learning: 2025-11-24T18:05:42.356Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:42.356Z
Learning: Applies to crates/biome_js_type_info/**/flattening.rs : Implement type flattening to simplify `TypeofExpression` variants once all component types are resolved
Applied to files:
crates/biome_module_graph/src/js_module_info/collector.rscrates/biome_js_type_info/src/resolver.rs
🪛 GitHub Actions: Pull request
crates/biome_module_graph/src/js_module_info/collector.rs
[warning] 692-692: value assigned to ty is never read. maybe it is overwritten before being read? (note: #[warn(unused_assignments)])
crates/biome_js_type_info/src/resolver.rs
[error] 745-747: Clippy: you should consider adding a Default implementation for UnionCollector. This lint was treated as an error due to -D warnings, causing the build to fail while running 'cargo lint'.
| pub struct UnionCollector { | ||
| types: Vec<TypeReference>, | ||
| } | ||
|
|
||
| impl UnionCollector { | ||
| pub fn new() -> Self { | ||
| Self { types: Vec::new() } | ||
| } | ||
|
|
||
| pub fn add(&mut self, ty: TypeReference) { | ||
| self.types.push(ty); | ||
| } | ||
|
|
||
| pub fn finish(self) -> Cow<'static, TypeData> { | ||
| Cow::Owned(TypeData::Union(Box::new(Union( | ||
| self.types.into_boxed_slice(), | ||
| )))) | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement Default trait and handle the empty-union edge case.
The Clippy lint correctly identifies that new() should be replaced with a Default implementation—this is causing the build to fail. Additionally, finish() will create an empty union if no types were added, which is likely incorrect (an empty union is uninhabitable).
Apply this diff to fix the Clippy error:
+impl Default for UnionCollector {
+ fn default() -> Self {
+ Self { types: Vec::new() }
+ }
+}
+
impl UnionCollector {
- pub fn new() -> Self {
- Self { types: Vec::new() }
- }
-
pub fn add(&mut self, ty: TypeReference) {
self.types.push(ty);
}
pub fn finish(self) -> Cow<'static, TypeData> {
+ if self.types.is_empty() {
+ return Cow::Owned(TypeData::unknown());
+ }
Cow::Owned(TypeData::Union(Box::new(Union(
self.types.into_boxed_slice(),
))))
}
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Pull request
[error] 745-747: Clippy: you should consider adding a Default implementation for UnionCollector. This lint was treated as an error due to -D warnings, causing the build to fail while running 'cargo lint'.
🤖 Prompt for AI Agents
In crates/biome_js_type_info/src/resolver.rs around lines 740 to 758, replace
the manual new() with a Default impl for UnionCollector to satisfy Clippy and
change finish() to handle the empty-union edge case: implement Default for
UnionCollector (initializing types = Vec::new()), update or remove new() to call
Default if kept, and modify finish() so that if self.types.is_empty() it returns
Cow::Owned(TypeData::Never) (or the crate's uninhabitable variant) instead of
constructing an empty Union; otherwise return the Union as before.
Summary
It should partially address #8204
We widen types only for assignments that are inside a different scope from where the variable was declared.
Test Plan
Green CI. It should improve the performance of some benchmarks
Docs