refactor(module_graph): preserve same-name function overloads as a set - #10585
Conversation
🦋 Changeset detectedLatest commit: df46d90 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
|
Caution Review failedFailed to post review comments WalkthroughThis PR bundles release notes documenting numerous bug fixes for existing rules (Vue attribute handling, ARIA props detection, TypeScript imports, Svelte parsing, CSS Modules), introduces a new nursery rule for restricted dependencies, promotes 73 nursery rules to stable groups with four rule renames, adds support for a standalone Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (1 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
🤖 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_module_graph/src/js_module_info/scope.rs`:
- Around line 154-167: The current match arms in union_with collapse
FunctionValue/Overloaded into ValueType(last) and lose overloads; change them to
preserve overload lists when merging with a value/type/namespace by producing a
carrier that keeps the full overload set plus the value-binding (e.g., add a new
variant like OverloadedWithValue(binding_id) or extend Self::Overloaded to hold
Option<binding_id>) instead of converting to Self::ValueType, and update
union_with handling so (Self::FunctionValue(own_binding_id), other), (this,
Self::FunctionValue(other_binding_id)), (Self::Overloaded(set), other) and
(this, Self::Overloaded(set)) forward to create/merge into that
combined-overload carrier rather than calling
Self::ValueType(last).union_with(...), ensuring bindings_by_name retains the
overload list for later call-site resolution.
🪄 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: f4bb6a86-36ae-4f72-ace6-bd7bdab2efc3
📒 Files selected for processing (3)
crates/biome_module_graph/src/js_module_info/binding.rscrates/biome_module_graph/src/js_module_info/collector.rscrates/biome_module_graph/src/js_module_info/scope.rs
| // Any other combination degrades to a plain value via the existing rules. | ||
| (Self::FunctionValue(own_binding_id), other) => { | ||
| Self::ValueType(own_binding_id).union_with(other) | ||
| } | ||
| (this, Self::FunctionValue(other_binding_id)) => { | ||
| this.union_with(Self::ValueType(other_binding_id)) | ||
| } | ||
| (Self::Overloaded(set), other) => { | ||
| let last = *set.last().expect("overload set is never empty"); | ||
| Self::ValueType(last).union_with(other) | ||
| } | ||
| (this, Self::Overloaded(set)) => { | ||
| let last = *set.last().expect("overload set is never empty"); | ||
| this.union_with(Self::ValueType(last)) |
There was a problem hiding this comment.
Don't throw the overload set away when another declaration space joins in.
If an overloaded function later merges with a same-name type/interface/namespace, these arms first squash Overloaded down to ValueType(last) and only then apply the existing merge rules. That means valid TS like overloaded function f(...) plus type f = ... loses the overload list in bindings_by_name, so later call-site selection can only ever see the last binding. A larger carrier is needed here for “overloads + type/namespace”, otherwise the groundwork falls over at the first declaration merge.
🤖 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_module_graph/src/js_module_info/scope.rs` around lines 154 -
167, The current match arms in union_with collapse FunctionValue/Overloaded into
ValueType(last) and lose overloads; change them to preserve overload lists when
merging with a value/type/namespace by producing a carrier that keeps the full
overload set plus the value-binding (e.g., add a new variant like
OverloadedWithValue(binding_id) or extend Self::Overloaded to hold
Option<binding_id>) instead of converting to Self::ValueType, and update
union_with handling so (Self::FunctionValue(own_binding_id), other), (this,
Self::FunctionValue(other_binding_id)), (Self::Overloaded(set), other) and
(this, Self::Overloaded(set)) forward to create/merge into that
combined-overload carrier rather than calling
Self::ValueType(last).union_with(...), ensuring bindings_by_name retains the
overload list for later call-site resolution.
If that's the case, send the PR against |
a7a7f0c to
505ac00
Compare
github-actions
Bot
added
the
L-JavaScript
505ac00 to
b265aef
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
@ematipico It depends on |
|
@IxxyDev we recently merged |
Same-name `function` / `declare function` declarations in one scope collapsed via last-wins in `TsBindingReference::union_with`, so only one signature survived before reaching any consumer. Split `JsDeclarationKind::Function` out of `HoistedValue` (in both the semantic event extractor and `from_node`) and add `TsBindingReference::FunctionValue` and `Overloaded`, so functions are marked distinctly and accumulate into an ordered set on merge. `var` / `const` redeclaration keeps its previous last-wins behaviour, and every non-overload read path degrades to the set's last binding, so resolution and exports are unchanged. This is the type-system groundwork for biomejs#9568; call-site overload selection follows separately.
Record overload sets only on name collisions and resolve them through the scope instead of storing them on every binding, restoring analyzer performance to baseline.
b265aef to
df46d90
Compare
@ematipico done |
3 participants
Summary
Groundwork for #9568 (part 1 of 2). This change has no user-facing effect on its own.
Same-name
function/declare functiondeclarations in one scope collapsed via last-wins inTsBindingReference::union_with, so only one signature survived before reaching any consumer — the overload set never existed. This splitsJsDeclarationKind::Functionout ofHoistedValueand addsTsBindingReference::FunctionValue(a single function) andOverloaded(two or more, in source order), so functions are marked distinctly and accumulate into an ordered set on merge.var/const/letredeclaration keeps its previous last-wins behaviour.Every non-overload read path (type resolution, exports, qualifier lookup) degrades to the set's last binding, so resolution and exports are unchanged.
Part 2 selects the matching overload at the call site and is what actually fixes #9568.
How to review
scope.rs— the newTsBindingReferencevariants and the merge arms inunion_with(the core), plus the read-path handling.binding.rs—Functionsplit out ofHoistedValue.collector.rs— theCopy -> Clonefallout at the two move sites.Test Plan
scope.rscover overload accumulation into an ordered set and confirmconst/varredeclaration is unaffected.Docs
N/A — internal change, no changeset (no user-facing behaviour difference).