Fix the reorganize_definitions refactoring #1364

ahomescu · 2025-09-16T21:42:41Z

Some fixes to the reorganize_definitions transform for c2rust-refactor to make it work again.

c2rust-refactor/src/transform/reorganize_definitions.rs

fw-immunant

Several questions inline; everything else looks good.

fw-immunant · 2025-10-07T23:06:11Z

c2rust-refactor/src/ast_manip/load_modules.rs

+            ModKind::Loaded(_items, Inline::Yes, _spans) => {
+                // TODO: handle #[path="..."]
+            }
+
+            ModKind::Loaded(_items, Inline::No, _spans) => {
+                // We shouldn't be seeing any loaded modules at this point
+                panic!("unexpected loaded module: {i:?}");
+            }


The conjunction of these two comments is confusing: are inline modules not considered loaded modules (such that we might see them)? Or is it that specifically we shouldn't see any non-inline loaded modules yet?

fw-immunant · 2025-10-07T23:09:17Z

c2rust-refactor/src/context.rs

+            // TODO: we still should assert here
+            //assert!(old_id.is_none(), "span {ns:?} already has id {old_id:?} != {id:?}");


Should this TODO be addressed before merging, or is there a reason it can't be addressed yet?

fw-immunant · 2025-10-07T23:16:35Z

c2rust-refactor/src/ast_manip/span_maps.rs

+    // We need a separate kind for ExprKind::Path because
+    // the rustc macro expander emits `*foo` expressions where
+    // both the outer and inner expressions have the same span
+    // We disambiguate by assigning (span, Expr) to the outer one,
+    // and (span, PathExpr) to the inner expression.
+    PathExpr,


Is this similar to the span_fix module that driver.rs claims (in a 6-year-old comment) we should be using?

…an maps

…ions

kkysen · 2025-10-09T00:05:04Z

c2rust-refactor/src/ast_manip/span_maps.rs

+            "id {id:?} already has span {old_ns:?} != {ns:?}"
+        );
+        // Some spans can show up in multiple nodes
+        //assert!(old_id.is_none(), "span {ns:?} already has id {old_id:?} != {id:?}");


Isn't this redundant?

Oh this is old_id not old_ns, nevermind.

kkysen · 2025-10-09T00:07:20Z

c2rust-refactor/src/context.rs

+        // Intentionally skip PathSegment to avoid collisions
+        Some(Node::Ty(ty)) => Some(NodeSpan::new(ty.span, Ty)),
+        // We do not have a SpanNodeKind for TyBinding
+        // We do not have a SpanNodeKind for TraitRef
+        Some(Node::Pat(pat)) => Some(NodeSpan::new(pat.span, Pat)),
+        Some(Node::Arm(arm)) => Some(NodeSpan::new(arm.span, Arm)),
+        Some(Node::Block(block)) => Some(NodeSpan::new(block.span, Block)),
+        // We do not have a SpanNodeKind for Ctor
+        // We do not have a SpanNodeKind for Lifetime
+        // We do not have a SpanNodeKind for GenericParam
+        // We do not have a SpanNodeKind for Infer


Are the comments for other Node variants? It might make more sense then to keep the individual matches => None instead of _ => None.

kkysen · 2025-10-09T00:08:06Z

c2rust-refactor/src/context.rs

            _ => {}
        }
-        match (self.cx.try_resolve_ty(ty1), self.cx.try_resolve_ty(ty1)) {
+        match (self.cx.try_resolve_ty(ty1), self.cx.try_resolve_ty(ty2)) {


Was this just a typo before?

kkysen · 2025-10-09T00:09:51Z

c2rust-refactor/src/macros.rs

+#[macro_export]
+macro_rules! match_or_else {
+    ([$e:expr] $($arm_pat:pat => $arm_body:expr),*; $or_else:expr) => {
+        match $e {
+            $( $arm_pat => $arm_body, )*
+            ref x @ _ => $or_else(x),
+        }
+    };
+}
+
 #[macro_export]
 macro_rules! expect {
    ([$e:expr] $arm_pat:pat => $arm_body:expr) => {
-        $crate::match_or!([$e] $arm_pat => $arm_body;
-            panic!("expected {}", stringify!($arm_pat)))
+        $crate::match_or_else!([$e] $arm_pat => $arm_body;
+            |x| panic!("expected {}, got {:?}", stringify!($arm_pat), x))
    };
    ([$e:expr] $($arm_pat:pat => $arm_body:expr),*) => {
-        $crate::match_or!([$e] $($arm_pat => $arm_body),*;
-            panic!("expected one of: {}", stringify!($($arm_pat),*)))
+        $crate::match_or_else!([$e] $($arm_pat => $arm_body),*;
+            |x| panic!("expected one of: {}, got {:?}", stringify!($($arm_pat),*), x))
    };
 }


What do we need this for? Especially if it's not used, I'd rather not add to expect! given that assert_matches! does the same and is much more standardized.

kkysen · 2025-10-09T00:27:11Z

c2rust-refactor/src/ast_manip/load_modules.rs

+        let Item {
+            ref mut attrs,
+            ref span,
+            ref ident,
+            kind: ItemKind::Mod(_, ref mut mod_kind),
+            ..
+        } = *i else {


Suggested change

let Item {

ref mut attrs,

ref span,

ref ident,

kind: ItemKind::Mod(_, ref mut mod_kind),

..

} = *i else {

let Item {

attrs,

span,

ident,

kind: ItemKind::Mod(_, mod_kind),

..

} = &mut *i else {

kkysen · 2025-10-09T00:31:07Z

c2rust-refactor/src/ast_manip/load_modules.rs

+                // Look for dir_path/foo.rs, then try dir_path/foo/mod.rs
+                let mut mod_file_path = self.dir_path.join(ident.as_str()).with_extension("rs");
+                if !self.source_map.file_exists(&mod_file_path) {
+                    mod_file_path = self.dir_path.join(ident.as_str()).with_file_name("mod.rs");


This doesn't do ${dir_path}/${ident}/mod.rs, it does ${dir_path}/mod.rs.

kkysen · 2025-10-09T00:34:00Z

c2rust-refactor/src/ast_manip/span_maps.rs

+    pub node_id_to_span_map: HashMap<NodeId, NodeSpan>,
+    pub span_to_node_id_map: HashMap<NodeSpan, NodeId>,


Why do we use HashMap sometimes and FxHashMap other times?

ahomescu changed the base branch from master to ahomescu/restore_refactor September 16, 2025 21:47

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch from f0c9f2a to 5d6a751 Compare September 17, 2025 03:58

ahomescu force-pushed the ahomescu/restore_refactor branch from b0a8be1 to a3464ac Compare September 17, 2025 03:58

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch 3 times, most recently from e527293 to 7968d29 Compare September 18, 2025 02:08

ahomescu force-pushed the ahomescu/restore_refactor branch from cae38e2 to da612cb Compare September 18, 2025 02:09

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch from 7968d29 to 17261ed Compare September 18, 2025 02:11

ahomescu force-pushed the ahomescu/restore_refactor branch from da612cb to 073e366 Compare September 18, 2025 02:11

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch 3 times, most recently from 1720b7c to 5c20a5e Compare September 18, 2025 02:46

ahomescu force-pushed the ahomescu/restore_refactor branch from 4a917b1 to b998b55 Compare September 18, 2025 02:46

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch 2 times, most recently from 657816b to 7f25d95 Compare September 18, 2025 05:32

ahomescu force-pushed the ahomescu/restore_refactor branch from 36fd226 to c351a8f Compare September 18, 2025 05:32

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch from 7f25d95 to 28e3158 Compare September 18, 2025 05:42

ahomescu force-pushed the ahomescu/restore_refactor branch from c351a8f to 71e9d3d Compare September 18, 2025 05:43

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch from 28e3158 to 1fb194a Compare September 18, 2025 06:07

ahomescu force-pushed the ahomescu/restore_refactor branch from 71e9d3d to c42bdd0 Compare September 18, 2025 06:08

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch 3 times, most recently from 3b6b204 to 0f281b4 Compare September 19, 2025 02:54

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch from 0f281b4 to e715943 Compare October 3, 2025 21:22

ahomescu requested a review from fw-immunant October 7, 2025 18:20

ahomescu commented Oct 7, 2025

View reviewed changes

c2rust-refactor/src/transform/reorganize_definitions.rs Outdated Show resolved Hide resolved

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch from e715943 to 6aa13a0 Compare October 7, 2025 22:26

ahomescu force-pushed the ahomescu/restore_refactor branch from e3fb41a to c717e17 Compare October 7, 2025 22:26

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch from 6aa13a0 to 6d527b1 Compare October 7, 2025 22:46

fw-immunant reviewed Oct 7, 2025

View reviewed changes

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch from 6d527b1 to 8edb5f0 Compare October 8, 2025 23:03

ahomescu force-pushed the ahomescu/restore_refactor branch from c717e17 to 2ec9dfd Compare October 8, 2025 23:03

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch from 8edb5f0 to b6d2c9e Compare October 8, 2025 23:10

ahomescu force-pushed the ahomescu/restore_refactor branch from 2ec9dfd to 7bd3b4e Compare October 8, 2025 23:10

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch from b6d2c9e to cd518be Compare October 8, 2025 23:14

ahomescu force-pushed the ahomescu/restore_refactor branch from 7bd3b4e to a77dce1 Compare October 8, 2025 23:14

ahomescu added 10 commits October 8, 2025 16:20

c2rust-refactor: Use sym module items instead of Symbol::intern()

a64c3c6

c2rust-refactor: Update target edition to 2021

513b76d

c2rust-refactor: Load ModKind::Unloaded modules manually

b00d291

c2rust-refactor: Map NodeIds to HirIds using a set of intermediate Sp…

f148365

…an maps

c2rust-refactor: Print actual value on expect failure

ec90a40

c2rust-refactor: Peel references when editing type paths in path_edit.rs

5ed7076

c2rust-refactor: Fix typo in structural_eq_ast_tys

5c65c97

c2rust-refactor: Add separate span NodeKind for Path nodes in express…

0970fd0

…ions

c2rust-refactor: Outline modules by default

68f670b

reorganize_definitions: Temporarily inline new modules

9804b07

ahomescu force-pushed the ahomescu/fix_reorganize_definitions branch from cd518be to 9804b07 Compare October 8, 2025 23:20

ahomescu force-pushed the ahomescu/restore_refactor branch from a77dce1 to 51fbbcd Compare October 8, 2025 23:20

Base automatically changed from ahomescu/restore_refactor to master October 8, 2025 23:57

kkysen reviewed Oct 9, 2025

View reviewed changes

		// TODO: we still should assert here
		//assert!(old_id.is_none(), "span {ns:?} already has id {old_id:?} != {id:?}");

		pub node_id_to_span_map: HashMap<NodeId, NodeSpan>,
		pub span_to_node_id_map: HashMap<NodeSpan, NodeId>,

Fix the reorganize_definitions refactoring #1364

Are you sure you want to change the base?

Fix the reorganize_definitions refactoring #1364

Uh oh!

Conversation

ahomescu commented Sep 16, 2025

Uh oh!

Uh oh!

fw-immunant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkysen Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kkysen Oct 9, 2025 •

edited

Loading