refactor(module_graph): preserve same-name function overloads as a set#10585
refactor(module_graph): preserve same-name function overloads as a set#10585IxxyDev wants to merge 2 commits into
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 |
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
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 |
| /// an overload set. | ||
| /// | ||
| /// It **does not** return overloads of parent scopes. | ||
| pub fn overload_sets(&self) -> Vec<Vec<BindingId>> { |
There was a problem hiding this comment.
Its a list of sets. Each item in the list is a list of all the overloads for one function, so I don't think so.
There was a problem hiding this comment.
Then why not an HashMap? Probably a dumb question but I fail to see the data structure. Considering how it's used below, the question pops up
There was a problem hiding this comment.
overloads_by_name is already a FxHashMap<name, set> internally. overload_sets() just exposes its values (filtered to len > 1), because no consumer needs the name key — only the groups. Returning the map would leak TokenText into the public API for no benefit.
The nesting (Vec<Vec<_>>) is load-bearing: each inner vec is one overload set, and flattening the sets themselves would lose which bindings belong together and break the per-set representative selection in the collector.
| // returns for the set (its last one) so a call site can select among them. | ||
| let carriers: Vec<(usize, Vec<TypeMember>)> = semantic_model | ||
| .scopes() | ||
| .flat_map(|scope| scope.overload_sets()) |
There was a problem hiding this comment.
Considering that here we're flattening overload_sets, it makes more sense that overload_sets does the flattening.
There was a problem hiding this comment.
The flattening here is across scopes, not within a set: each scope yields its own list of overload sets, and flat_map concatenates them into a single stream of sets. Each item the .map below then receives is one full overload set, which is exactly the grouping we need — it becomes one object with a call signature per overload, assigned to the set's representative (last) binding. So there's nothing to move into overload_sets(); it uplifts a different dimension.
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).