fix(core): partition duplicate-export groups by common-importer before value/type suppression#858
Closed
BartWaardenburg wants to merge 3 commits into
Closed
fix(core): partition duplicate-export groups by common-importer before value/type suppression#858BartWaardenburg wants to merge 3 commits into
BartWaardenburg wants to merge 3 commits into
Conversation
…e value/type suppression Before this fix, has_common_importer was a whole-group boolean. A cross-package member sharing no importer with the rest of the name-group (e.g. a backend `class Label` that no frontend module imports) stayed in the group, inflating value_modules so the value+type self-suppression check was defeated, manufacturing a false duplicate-export finding. The fix replaces the whole-group has_common_importer call with partition_by_common_importer, a union-find that splits the independent entries into connected components by shared importer. Value+type self-suppression now applies per component, so an isolated backend class does not inflate the frontend component's value count. Two regression tests added: - duplicate_exports_cross_package_backend_does_not_defeat_value_type_suppression: backend `class Label` (no shared importer with frontend) must not prevent the frontend value+type pair from self-suppressing. - duplicate_exports_genuine_value_duplicate_still_flagged_despite_unrelated_backend: two frontend values sharing a common importer must still report as a genuine duplicate even when an unrelated backend module also exports the same name. Fixes #848
BartWaardenburg
force-pushed
the
fix/issue-848-duplicate-export-partition
branch
from
f9cf622 to
4aec65d
Compare
Collaborator
Author
|
Landed on main via maintainer direct-to-main flow, squashed into e8175df. The linked issue is fixed on main; closing this PR as redundant. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaced the whole-group
has_common_importerboolean withpartition_by_common_importer(union-find). Independent entries for a name-group are now split into connected components by shared importer before the value+type self-suppression check runs. A cross-package member sharing no importer with the rest (e.g. a backend classLabel) is isolated into a singleton component and dropped, so it cannot inflatevalue_modulesand defeat the suppression that should apply to the connected frontend value+type pair. Two focused regression tests were added: one for the false-positive scenario from #848 and a companion test confirming genuine duplicates among connected modules still surface.Review
Verdict: SHIP
value_count/type_count) rather than distinct-module counts as the old global check did (value_modules/type_modules). Only diverges if a single module contributes 2+ value entries of the same name, which is pathological (TS overload dedup merges these earlier); no real-world regression. The direct-import union usesentries.iter().position(first match by file_id), so a second same-file entry is only joined transitively via shared importers; incompleteness is in the singleton-drop (conservative) direction, not a false-positive risk.FxHashMap/FxHashSetused throughout (no std collections); test module already carries#[expect(deprecated)]covering thefind_duplicate_exportscalls; no#[cfg_attr(miri, ignore)]needed since the tests build an in-memoryModuleGraphwith no tempdir/fs IO, matching sibling duplicate-export tests. Diff is single-file and minimal.Closes #848