Skip to content

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

Open
ahomescu wants to merge 2 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 2 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 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/multi_namespace_import_splitting branch from bed6930 to 1398d79 Compare July 25, 2026 05:09
@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/multi_namespace_import_splitting branch from 2343a08 to 657815f Compare July 25, 2026 05:41
@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/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/multi_namespace_import_splitting branch 2 times, most recently from 04579b8 to 011e000 Compare July 25, 2026 06:26
@ahomescu
ahomescu removed the request for review from thedataking July 25, 2026 06:28
@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
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 1902a83 to 634aac3 Compare July 29, 2026 23:18
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 634aac3 to 7e7321b Compare July 30, 2026 00:19
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 7e7321b to 2f498ad Compare July 30, 2026 00:31
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 2f498ad to 6b3491e Compare July 30, 2026 22:54
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 6b3491e to 29ba8db Compare July 30, 2026 22:56
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 29ba8db to 5748cf2 Compare July 30, 2026 22:58
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch 2 times, most recently from 81850d6 to bc8c764 Compare August 5, 2026 00:10
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch 2 times, most recently from 22fd4d9 to c486129 Compare August 5, 2026 00:22
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from c486129 to 3331801 Compare August 5, 2026 00:43
thedataking
thedataking reviewed Sep 8, 2026
View reviewed changes

@thedataking thedataking left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of stack 1953: the newly generated split import needs to preserve the original binding name.

Comment thread c2rust-refactor/src/transform/reorganize_definitions.rs
self.path_mapping.get(&def_id)
{
if other_mod_id != *parent {
if !path.ast_equiv(&retained_path) {

@thedataking thedataking Sep 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve aliases when emitting the second namespace import

This broader split condition now reaches use_simple_item(path, None::<String>) when the two targets are in the same module but have different names. That generated import ignores the original as alias and can introduce a name collision.

I reproduced this at stack head using reorganize_split_renamed_import.rs: change the import in user to use crate::dest::dest_h::tick as alias;, change its type/value uses to alias, and add fn tick() {} to user. The input compiles. The output adds use crate::dest::tick; alongside that function, producing E0255 (tick defined multiple times). The earlier transform produces compiling output for this case.

The synthesized import should preserve the original binding name, so this case emits use crate::dest::tick as alias;.

@ahomescu
ahomescu removed this pull request from stack #1953 September 9, 2026 01:38
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/multi_namespace_import_splitting branch from 3331801 to dc52d84 Compare September 9, 2026 01:41
@ahomescu
ahomescu added this pull request to stack #2004 September 9, 2026 01:42
@ahomescu
refactor: add test for multi-namespace imports splitting
74b9324 
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
288f714 
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/multi_namespace_import_splitting branch from dc52d84 to 288f714 Compare September 9, 2026 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thedataking thedataking thedataking left review comments

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.

2 participants

@ahomescu @thedataking