Skip to content

refactor: reorganize_definitions: fix multi-namespace imports splitting - #1925

Open
ahomescu wants to merge 3 commits into
ahomescu/fix_reorganize_definitions/compare_extern_fn_sigsfrom
ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting
Open

refactor: reorganize_definitions: fix multi-namespace imports splitting#1925
ahomescu wants to merge 3 commits into
ahomescu/fix_reorganize_definitions/compare_extern_fn_sigsfrom
ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting

Conversation

@ahomescu

@ahomescu ahomescu commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Stack created with GitHub Stacks CLI • Give Feedback 💬

@ahomescu
ahomescu changed the base branch from ahomescu/fix_reorganize_definitions/extern_abi_fallback to ahomescu/fix_reorganize_definitions/preserve_extern_link_name July 25, 2026 01:00
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from e47e2b3 to 072a330 Compare July 25, 2026 01:40
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/preserve_extern_link_name branch from 1cbedd1 to 248139d Compare July 25, 2026 01:40
@ahomescu
ahomescu changed the base branch from ahomescu/fix_reorganize_definitions/preserve_extern_link_name to ahomescu/fix_reorganize_definitions/extern_abi_fallback July 25, 2026 01:57
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 072a330 to bed6930 Compare July 25, 2026 01:57
@ahomescu
ahomescu requested a review from thedataking July 25, 2026 02:00
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/extern_abi_fallback branch from 401842b to d17fbe9 Compare July 25, 2026 05:09
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from bed6930 to 1398d79 Compare July 25, 2026 05:09
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/extern_abi_fallback branch from d17fbe9 to cc214be Compare July 25, 2026 05:33
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 1398d79 to 2343a08 Compare July 25, 2026 05:33
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/extern_abi_fallback branch from cc214be to ddc9bbd Compare July 25, 2026 05:41
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 2343a08 to 657815f Compare July 25, 2026 05:41
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/extern_abi_fallback branch from ddc9bbd to d8aec07 Compare July 25, 2026 05:42
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 657815f to a33e792 Compare July 25, 2026 05:42
@ahomescu
ahomescu changed the base branch from ahomescu/fix_reorganize_definitions/extern_abi_fallback to ahomescu/fix_reorganize_definitions/compare_extern_fn_sigs July 25, 2026 05:47
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from a33e792 to f9b5346 Compare July 25, 2026 05:47
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/compare_extern_fn_sigs branch from 0380037 to 4e3b45a Compare July 25, 2026 05:59
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch 2 times, most recently from d908672 to f058f77 Compare July 25, 2026 06:05
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/compare_extern_fn_sigs branch 2 times, most recently from f423c43 to 8e87153 Compare July 25, 2026 06:16
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch 2 times, most recently from 04579b8 to 011e000 Compare July 25, 2026 06:26
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/compare_extern_fn_sigs branch from 8e87153 to 15e6ee0 Compare July 25, 2026 06:26
@ahomescu
ahomescu removed the request for review from thedataking July 25, 2026 06:28
ahomescu added 3 commits July 24, 2026 23:43
@ahomescu
refactor: fix foreign declarations merged across an arity mismatch
36bfb3f 
`compatible_fn_prototypes` zipped the two parameter lists and compared the
pairs, but `zip` stops at the shorter list, so it never noticed that one
declaration had parameters the other did not. Any declaration was therefore
compatible with one that merely extends it, and `find_foreign_item` reported
the two as equivalent: the longer declaration was dropped and its callers
were rewritten to call the shorter one.

Compare the lengths first. `compatible_fn_sigs`, the resolved-type
counterpart, already did this. A trailing `...` is a `CVarArgs` parameter
rather than a flag on `FnDecl`, so the length check also rejects pairing a
variadic declaration with a non-variadic one.

Both declarations now survive, the second is renamed to avoid the collision,
and each call site keeps calling the function it was written against, so the
output of the test compiles and `new_expect_compile_error` is dropped.

Renaming the second one does not yet keep it linking against the original
symbol; that is fixed next.
@ahomescu
refactor: add test for multi-namespace imports splitting
df343d0 
Regression test: `test_reorganize_split_renamed_import`
(`tests/snapshots/reorganize_split_renamed_import.rs`). The original
two-external-crates scenario cannot be built in the single-file snapshot
harness, so the test exercises the equivalent local case the old parent-id
comparison also got wrong: a collision-renamed type target (`tick_1`) and a
value target (`tick`) in the *same* destination module. Pre-fix output omits
the value-namespace `use crate::dest::tick;`.
@ahomescu
refactor: reorganize_definitions: fix multi-namespace imports splitting
1902a83 
The split loop now reconstructs the path the retained import was rewritten to
and compares target paths with `ast_equiv` instead of comparing parent module
NodeIds, so external targets (all `DUMMY_NODE_ID`) are handled correctly.

Regression test: `test_reorganize_split_renamed_import`
(`tests/snapshots/reorganize_split_renamed_import.rs`). The original
two-external-crates scenario cannot be built in the single-file snapshot
harness, so the test exercises the equivalent local case the old parent-id
comparison also got wrong: a collision-renamed type target (`tick_1`) and a
value target (`tick`) in the *same* destination module. Pre-fix output omits
the value-namespace `use crate::dest::tick;`.

Follow-on bug found while writing the test: the split-import insertion had
never actually fired in any test before, and when the synthesized `use` is
inserted next to the rewritten retained import *with a comment attached
directly above it*, the rewriter panics with "conflicting rewrites" — the
sequence splice absorbs the comment into its span and overlaps the item's own
path rewrite. The test sidesteps it by keeping comments away from the import;
the rewriter/seq-edit interplay still needs a real fix.

`:1144-1172`. `match_exports` stores `parent: DUMMY_NODE_ID` for all external
replacements (`:673`), and `other_mod_id` falls back to `DUMMY_NODE_ID`
(`:1153`). So when a single `use` resolves in two namespaces to targets in two
*different* external crates/modules, `other_mod_id != *parent` compares
`DUMMY == DUMMY`, concludes "same module", and skips emitting the second
import — the retained import is rewritten to the first namespace's path and
the other binding is lost.

The redundancy check further down explicitly documents this exact pitfall
("DUMMY_NODE_ID represents every external module and cannot prove path
equality", `:1286-1295`) but this earlier check doesn't apply the same rule.
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/compare_extern_fn_sigs branch from 15e6ee0 to 7bb56b6 Compare July 25, 2026 06:43
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 011e000 to 1902a83 Compare July 25, 2026 06:43
@ahomescu
ahomescu requested a review from thedataking July 25, 2026 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thedataking thedataking Awaiting requested review from thedataking

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ahomescu