refactor: `--reorganize-definitions` generates invalid Rust code due to stripped parens

[NWPlayer123]

I've been toying around with using c2rust to transpile x264 for a while now, and I've struggled to get reorganize-definitions to work since that's tons and tons of manual labor otherwise, and I finally got it to succeed after fixing or bypassing several bugs. I'm including my compilation steps here so y'all can replicate it locally.

Note that this was all tested off master/the latest commit (4df3811), using cargo build --release and manually copying the binaries into my ~/.cargo/bin.

Compilation/Transpilation Steps

• Creating a tmpfile due to c2rust not supporting some glibc stuff

common/base.c:908:21: warning: c2rust: Encountered unsupported generic selection expression
  908 |         while( (c = strchr( name_buf, '_' )) )
      |                     ^~~~~~~~~~~~~~~~~~~~~~~
/usr/include/string.h:265:3: note: expanded from macro 'strchr'
  265 |   __glibc_const_generic (S, const char *, strchr (S, C))
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/sys/cdefs.h:838:3: note: expanded from macro '__glibc_const_generic'
  838 |   _Generic (0 ? (PTR) : (void *) 1,                     \
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  839 |             const void *: (CTYPE) (CALL),               \
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  840 |             default: CALL)
      |             ~~~~~~~~~~~~~

mkdir -p /tmp/c2rust-fix && printf '#include_next <string.h>\n#undef strchr\n#undef strrchr\n#undef strstr\n#undef strpbrk\n#undef memchr\n' > /tmp/c2rust-fix/string.h

• Configuring x264 with a bunch of additional flags (static library + CLI, OpenCL and bashcompletion were a mess when cleaning up last time, I don't need the manual assembly, no idea if -g and fPIC actually help c2rust but I turned those on, plus including the override header from above, and unsetting __SSE__ because x264 still emits some types that c2rust doesn't currently support, I forget why I turned on fno-inline, I think it helped with the transpilation output)

error: Skipping declaration Some(Typedef { name: "v4si", typ: CQualTypeId { qualifiers: Qualifiers { is_const: false, is_restrict: false, is_volatile: false }, ctype: CTypeId(21552) }, is_implicit: false, target_dependent_macro: None }) due to error: Unsupported type Vector(CQualTypeId { qualifiers: Qualifiers { is_const: false, is_restrict: false, is_volatile: false }, ctype: CTypeId(8031) }, 4)

error: Failed to translate x264_macroblock_cache_rect: Unknown vector init list: (Elaborated(CTypeId(8032)), 4)

./configure --enable-static --disable-bashcompletion --disable-opencl --disable-asm --enable-debug --enable-pic --extra-cflags="-fno-inline -isystem /tmp/c2rust-fix -U__SSE__"

• Running bear -- make -j$(nproc)

• Modifying compile_commands.json to remove all -Wno-maybe-uninitialized and -mpreferred-stack-boundary=6 since they pop up as errors/warnings when transpiling

• Attempting to invoke c2rust, with another bypass (rustfmt is unable to strip some whitespace due to how insanely nested some of the functions are, or something, so I just bypass rustfmt)

error[internal]: left behind trailing whitespace
    --> .../src/encoder/macroblock.rs:1843:1843:1
     |
1843 |
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |

RUSTFMT=/usr/bin/true c2rust transpile -e -r -o ../x264-rs --overwrite-existing compile_commands.json

The Actual Issue

After doing all that, I still run into an issue that I can't fix with command line workarounds, which seems to be a c2rust-refactor bug stripping semantically-necessary parentheses.

error: `<` is interpreted as a start of generic arguments for `::core::ffi::c_int`, not a comparison
  --> <anon>:60:85
   |
60 |                                   4 as ::core::ffi::c_int) + 12 as ::core::ffi::c_int < bcost
   |  _____________________________________________________________________________________^_-
   | |                                                                                     |
   | |                                                                                     not interpreted as comparison
61 | |                     {
   | |_____________________- interpreted as generic arguments
   |
help: try comparing the cast value
   |
60 |                                 4 as ::core::ffi::c_int) + (12 as ::core::ffi::c_int) < bcost
   |                                                            +                        +

I did manage to get AI (Claude Opus) to generate a patch that seems* to have fixed it on my end, but I'm obviously not going to open a PR to submit it since I'm unfamiliar with the codebase, and I'm not going to submit patches if I don't understand if this is the best way to fix it, hence why I'm opening an issue. I've included it here in the hopes that it helps with narrowing down what's wrong, though.

diff --git a/c2rust-refactor/src/ast_manip/mod.rs b/c2rust-refactor/src/ast_manip/mod.rs
index 56bdf4644..62959478f 100644
--- a/c2rust-refactor/src/ast_manip/mod.rs
+++ b/c2rust-refactor/src/ast_manip/mod.rs
@@ -33,7 +33,7 @@ pub use self::get_span::GetSpan;
 pub use self::list_node_ids::ListNodeIds;
 pub use self::load_modules::load_modules;
 pub use self::output_exprs::fold_output_exprs;
-pub use self::remove_paren::remove_paren;
+pub use self::remove_paren::{fix_cast_parens, remove_paren};
 pub use self::seq_edit::{fold_blocks, fold_modules};
 pub use self::span_maps::{
     child_slot, AstSpanMaps, NodeContextKey, NodeSpan, SpanNodeKind, StructuralContext,
diff --git a/c2rust-refactor/src/ast_manip/remove_paren.rs b/c2rust-refactor/src/ast_manip/remove_paren.rs
index ef98b1140..2ea3f3b83 100644
--- a/c2rust-refactor/src/ast_manip/remove_paren.rs
+++ b/c2rust-refactor/src/ast_manip/remove_paren.rs
@@ -1,4 +1,5 @@
 //! `remove_paren` function, for removing unnecessary `ExprKind::Paren` nodes.
+//! `fix_cast_parens` function, for re-inserting parens stripped by `remove_paren`.
 use rustc_ast::mut_visit::{self, MutVisitor};
 use rustc_ast::ptr::P;
 use rustc_ast::*;
@@ -35,3 +36,64 @@ impl MutVisitor for RemoveParen {
 pub fn remove_paren<T: MutVisit>(x: &mut T) {
     x.visit(&mut RemoveParen)
 }
+
+/// Check whether the rightmost leaf of an expression tree is a cast (`as`).
+///
+/// When such an expression appears as the LHS of `<` or `<<`, the parser
+/// interprets the `<` as the start of generic arguments for the cast's
+/// target type (e.g. `x as c_int < y` is parsed as `x as c_int<y>`).
+fn has_rightmost_cast(expr: &Expr) -> bool {
+    match &expr.kind {
+        ExprKind::Cast(..) => true,
+        ExprKind::Unary(_, ref arg) => has_rightmost_cast(arg),
+        ExprKind::Binary(_, _, ref rhs) => has_rightmost_cast(rhs),
+        _ => false,
+    }
+}
+
+/// AST pass that re-inserts `ExprKind::Paren` nodes around cast expressions
+/// that would be ambiguous after `remove_paren` strips all parentheses.
+///
+/// Specifically, when a binary `<` or `<<` has an LHS whose rightmost
+/// subexpression is an `as` cast, the LHS must be parenthesized so that the
+/// parser does not interpret `<` as the start of generic type arguments.
+///
+/// This mirrors the `has_rightmost_cast` check already present in both
+/// `c2rust-ast-builder/src/builder.rs` and
+/// `c2rust-refactor/src/ast_builder/builder.rs`, but applied as a fixup pass
+/// rather than at AST construction time.
+struct FixCastParens;
+
+impl MutVisitor for FixCastParens {
+    fn visit_expr(&mut self, e: &mut P<Expr>) {
+        // Recurse first so inner expressions are fixed before we inspect them.
+        mut_visit::noop_visit_expr(e, self);
+
+        if let ExprKind::Binary(ref op, ref mut lhs, _) = e.kind {
+            if matches!(op.node, BinOpKind::Lt | BinOpKind::Shl)
+                && has_rightmost_cast(lhs)
+            {
+                let inner = lhs.clone();
+                *lhs = P(Expr {
+                    id: DUMMY_NODE_ID,
+                    kind: ExprKind::Paren(inner),
+                    span: lhs.span,
+                    attrs: AttrVec::new(),
+                    tokens: None,
+                });
+            }
+        }
+    }
+
+    fn visit_mac_call(&mut self, mac: &mut MacCall) {
+        mut_visit::noop_visit_mac(mac, self)
+    }
+}
+
+/// Re-insert parentheses around `as` casts that would be ambiguous when
+/// pretty-printed before `<` or `<<` operators. Should be called after
+/// `remove_paren`.
+#[cfg_attr(feature = "profile", flame)]
+pub fn fix_cast_parens<T: MutVisit>(x: &mut T) {
+    x.visit(&mut FixCastParens)
+}
\ No newline at end of file
diff --git a/c2rust-refactor/src/command.rs b/c2rust-refactor/src/command.rs
index b418bff57..c88cb734d 100644
--- a/c2rust-refactor/src/command.rs
+++ b/c2rust-refactor/src/command.rs
@@ -29,7 +29,7 @@ use crate::ast_manip::number_nodes::{
 };
 use crate::ast_manip::AstSpanMaps;
 use crate::ast_manip::{collect_comments, Comment, CommentMap};
-use crate::ast_manip::{load_modules, remove_paren, ListNodeIds, MutVisit, Visit};
+use crate::ast_manip::{load_modules, fix_cast_parens, remove_paren, ListNodeIds, MutVisit, Visit};
 use crate::collapse::CollapseInfo;
 use crate::driver::{self, Phase};
 use crate::file_io::FileIO;
@@ -299,6 +299,7 @@ impl RefactorState {
                 // since rustc folded that operation into expansion
                 load_modules(&mut krate, &session.parse_sess, source_map);
                 remove_paren(&mut krate);
+                fix_cast_parens(&mut krate);
                 number_nodes(&mut krate);
 
                 DiskState::new(krate, source_map, session, node_map)
@@ -357,6 +358,7 @@ impl RefactorState {
                     );
                     profile_end!("Expand crate");
                     remove_paren(cs.krate.get_mut());
+                    fix_cast_parens(cs.krate.get_mut());
                 }
             }
 

[NWPlayer123]

Upon actually setting up the transpiled repo, looks like even after fixing the parens issue, it still fails to properly resolve a handful of types (x264_zone_t, x264_frame, x264_mb_analysis_t but that might be an x264 includes issue) and produce a repo that still compiles, but it still gets me 98% of the way there. That was happening independent of the parens issue though, if I remember correctly (I've done this transpilation probably a dozen times across both master and 0.20/0.21/0.22, so I've seen most of the issues already, this is just the newest one).

Also if there's something else I should be doing for all these other misc issues with c2rust supporting x264, let me know, this was just the deal-breaker that caused me to open this first issue, the rest I could hack around.