Skip to content

refactor: handle impls correctly by moving instead of dropping them - #1929

Open
ahomescu wants to merge 2 commits into
ahomescu/fix_reorganize_definitions/canonicalize_moved_pathsfrom
ahomescu/fix_reorganize_definitions/move_impls
Open

refactor: handle impls correctly by moving instead of dropping them#1929
ahomescu wants to merge 2 commits into
ahomescu/fix_reorganize_definitions/canonicalize_moved_pathsfrom
ahomescu/fix_reorganize_definitions/move_impls

Conversation

@ahomescu

@ahomescu ahomescu commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Stack created with GitHub Stacks CLI • Give Feedback 💬

@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from a7914dd to ec26c55 Compare July 25, 2026 01:40
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from ec26c55 to 1eeef4c Compare July 25, 2026 01:57
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 1eeef4c to 1a14511 Compare July 25, 2026 02:07
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 1a14511 to 8e0fdc6 Compare July 25, 2026 02:11
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 8e0fdc6 to bc21f43 Compare July 25, 2026 02:16
@ahomescu
ahomescu changed the base branch from ahomescu/fix_reorganize_definitions/canonicalize_moved_paths to ahomescu/fix_reorganize_definitions/widen_matched_defs July 25, 2026 02:16
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from bc21f43 to 9b9adb2 Compare July 25, 2026 02:44
@ahomescu
ahomescu changed the base branch from ahomescu/fix_reorganize_definitions/widen_matched_defs to ahomescu/fix_reorganize_definitions/canonicalize_moved_paths July 25, 2026 03:17
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 9b9adb2 to 96ca5e6 Compare July 25, 2026 03:17
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 96ca5e6 to 0739482 Compare July 25, 2026 03:26
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 0739482 to 2fca703 Compare July 25, 2026 05:09
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 2fca703 to 52cf39f Compare July 25, 2026 05:33
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 52cf39f to 1ff0704 Compare July 25, 2026 05:41
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 1ff0704 to 1f4904f Compare July 25, 2026 05:42
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 1f4904f to f675e52 Compare July 25, 2026 05:48
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from f675e52 to 90c2469 Compare July 25, 2026 05:53
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch 2 times, most recently from af56551 to dd20965 Compare July 25, 2026 06:05
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from dd20965 to 8a12703 Compare July 25, 2026 06:16
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch 2 times, most recently from 9f50992 to ac05fb9 Compare July 25, 2026 06:43
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from ac05fb9 to 8257d5e Compare July 25, 2026 06:51
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch 2 times, most recently from 0633465 to 9857a1f Compare July 30, 2026 00:19
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 9857a1f to 7d232db Compare July 30, 2026 00:31
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 7d232db to 7e42a9d Compare July 30, 2026 22:54
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 7e42a9d to 15af7d6 Compare July 30, 2026 22:56
ahomescu added 2 commits July 30, 2026 15:57
@ahomescu
refactor: add test for impl blocks
f23773f 
Regression test (documents the current broken behavior):
`test_reorganize_orphaned_impls`
(`tests/snapshots/reorganize_orphaned_impls.rs`). It covers both loss paths —
a const-only impl whose self type is defined outside the headers (orphaned
`impls` entry) and an impl containing a method (never saved) — and asserts
via snapshot that both vanish; the output no longer compiles, marked with
`new_expect_compile_error(true)`. When this finding is fixed, drop that flag
and update the snapshot to show the impls surviving.
@ahomescu
refactor: handle impls correctly by moving instead of dropping them
ad6ed94 
At most one const-only inherent impl per self-type `DefId` can be carried
alongside a moved declaration (through `MovedDecl::r#impl`, unchanged from
before). Everything else that `remove_header_items` pulls out of a header
module — trait impls, impls with non-`const` items, extra impls beyond the
first for a given type, and const-only impls whose self type is never itself
individually processed as a moved declaration (e.g. because it's defined
outside any header module) — is now collected into a new
`Reorganizer::pending_impls: Vec<(DefId, MovedDeclImpl)>` field instead of
being dropped. An impl whose self type doesn't resolve to a plain path to a
definition at all (generic, reference, etc.) is left exactly where it was,
with a warning, since there's nothing to key it on.

`move_items` resolves each pending impl's destination once `path_mapping` is
complete: if the self type itself moved, its destination module is already
recorded there; otherwise the self type is a pre-existing definition, and the
module it's already defined in is looked up via
`parent_module_from_def_id`. The impl's item is then run through the same
`canonicalize_moved_decl_paths` used for #5 (now taking a `NodeId` instead of
a borrowed `&ModuleInfo`, so it also works for destinations outside
`self.modules`) and appended directly into that module's items — at both
places a destination module's items can end up populated (the "module
already exists in the crate" sweep and the "create a new module" fallback),
so a pending impl can land correctly regardless of which path its
destination module takes.

Note this fix does *not* leave non-qualifying impls physically in place in
their original header submodule: an earlier version of the fix did that, but
it collides with `find_destination_id`, which independently creates a
same-named *new* destination module for any other content from the same
header that still needs relocating (since `unique_ident` doesn't know about
the surviving header submodule) — two modules end up with the same name.
Reattaching every impl to its self type's actual destination avoids this.

Regression test: `test_reorganize_orphaned_impls`
(`tests/snapshots/reorganize_orphaned_impls.rs`) covers both loss paths — a
const-only impl whose self type is defined outside the headers, and an impl
containing a non-const method — attached to a struct defined in an ordinary,
non-header module. Verified against the pre-fix transform (this session's
starting commit) that removing the fix reproduces the original failure
(`E0599: no associated item named DEFAULT_X` / `no method named reset`); with
the fix, both impls are reattached next to their self type and the output
compiles.

`:345-404`. The `retain` closure returns `false` for *every*
`ItemKind::Impl`, but only const-only inherent impls with a resolvable simple
self-type path get saved into the `impls` map. Trait impls, impls containing
anything but consts, and impls whose self type can't be resolved are dropped
with no warning.

Additionally, saved impls are only re-attached when the matching decl flows
through `insert_item` (`impls.remove(&new_def_id)` at `:469`). If the impl's
self type resolves to a def that is not itself a moved header item (e.g.
resolved through a `use` to a type defined in a source module, or the decl was
dropped as an unused non-exported import at `:447-461`), the entry is never
consumed and the impl — including its constants — vanishes from the crate.
@ahomescu
ahomescu force-pushed the ahomescu/fix_reorganize_definitions/move_impls branch from 15af7d6 to ad6ed94 Compare July 30, 2026 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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