Skip to content

refactor: reorganize_definitions: keep glob imports untouched - #1947

Open
ahomescu wants to merge 3 commits into
ahomescu/fix_reorganize_definitions/macro_nsfrom
ahomescu/fix_reorganize_definitions/glob_imports
Open

ahomescu wants to merge 3 commits into
ahomescu/fix_reorganize_definitions/macro_nsfrom
ahomescu/fix_reorganize_definitions/glob_imports

Conversation

@ahomescu

Copy link
Copy Markdown
Contributor

Fresh copy of #1927


Stack created with GitHub Stacks CLI • Give Feedback 💬

@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 2451609 to 78f7f9e Compare July 25, 2026 05:09
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 78f7f9e to 938632a Compare July 25, 2026 05:33
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 938632a to 28383a8 Compare July 25, 2026 05:41
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 28383a8 to e903008 Compare July 25, 2026 05:42
@ahomescu
ahomescu changed the base branch from ahomescu/fix_reorganize_definitions/use_comments to ahomescu/fix_reorganize_definitions/widen_matched_defs July 25, 2026 05:48
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from e903008 to 1ed4d4b Compare July 25, 2026 05:48
@ahomescu
ahomescu changed the base branch from ahomescu/fix_reorganize_definitions/widen_matched_defs to ahomescu/fix_reorganize_definitions/macro_ns July 25, 2026 05:53
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 1ed4d4b to 82b8149 Compare July 25, 2026 05:53
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch 2 times, most recently from e9ab8bf to e7583f6 Compare July 25, 2026 06:05
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from e7583f6 to 338d0fe Compare July 25, 2026 06:16
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch 3 times, most recently from f74837c to 85d06d6 Compare July 25, 2026 06:51
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch 2 times, most recently from 9a8867e to 57ea173 Compare July 30, 2026 00:19
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 57ea173 to 3f83eee Compare July 30, 2026 00:31
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch 2 times, most recently from 3f9bb77 to eb1ca74 Compare August 4, 2026 02:41
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from eb1ca74 to 91e1573 Compare August 5, 2026 00:10
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 91e1573 to 11cb910 Compare August 5, 2026 00:19
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 11cb910 to 46ead06 Compare August 5, 2026 00:22
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 46ead06 to d3b0e62 Compare August 5, 2026 00:43

@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 implicit-extern snapshot is reverted by this PR and fails with the corrected ABI behavior.


// =============== BEGIN first_h ================
extern "C" {
extern "Rust" {

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.

[P1] Keep the corrected implicit-extern snapshot

Commit 25efe877c restores the pre-fix expectation here, even though #1924 already changed implicit extern blocks to use the C ABI. At stack head, the transform emits one extern "C" declaration in first and redirects the second caller to it; this snapshot instead expects extern "Rust" plus a renamed compute_1.

I ran cargo test --offline -p c2rust-refactor --test snapshots -- --test-threads=1: 54 passed and only test_reorganize_implicit_extern failed on this mismatch. Please drop the snapshot-reverting commit or restore the expectation from #1924.

@ahomescu
ahomescu removed this pull request from stack #1953 September 9, 2026 01:38
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from d3b0e62 to 8a85d07 Compare September 9, 2026 01:41
@ahomescu
ahomescu added this pull request to stack #2004 September 9, 2026 01:42
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 8a85d07 to 600a813 Compare September 9, 2026 02:18
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 600a813 to 9c8ec4a Compare September 9, 2026 02:33
@ahomescu
ahomescu removed this pull request from stack #2004 September 17, 2026 00:38
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 9c8ec4a to 355e66b Compare September 17, 2026 00:39
@ahomescu
ahomescu added this pull request to stack #2016 September 17, 2026 00:42
`Regression test: `test_reorganize_implicit_extern`
(`tests/snapshots/reorganize_implicit_extern.rs`); on pre-fix code its
implicit-`extern` declaration fails to merge with the `extern "C"` duplicate,
producing an `extern "Rust"` block and a spurious `compute_1` rename.
Regression test: `test_reorganize_glob_import`
(`tests/snapshots/reorganize_glob_import.rs`); verified to reproduce the
`UseTree::ident` panic on pre-fix code.
Glob imports are now always kept in their header module: `keep_items` adds
them to the keep set, and `insert_item` gained a dedicated `Glob` arm that
returns "not collapsed" before any `tree.ident()` call. The other listed call
sites (`Extend`, `MovedDecl::ident`) are unreachable for globs once they are
never inserted into `HeaderDeclarations`.

Nested uses are split (`:427-433`), but globs survive and hit
`UseTree::ident()`, which panics for `UseTreeKind::Glob`:

- `keep_items` at `:334`
- `insert_item` at `:1794-1798` (glob falls into the `_` arm)
- `Extend for HeaderDeclarations` at `:1690`
- `MovedDecl::ident` at `:1589`
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/glob_imports branch from 355e66b to ccfd3b5 Compare September 17, 2026 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants