Suppress clone in iterator conversion (#112)

lucic71 · web-flow · commit 08a11c21803b · 2026-05-18T19:40:36.000+01:00
From, tests/unit/redundant_copy_in_conversion:

```cpp
  // Comparing const_iterator with iterator forces an implicit conversion of
  // `end` to const_iterator. The AST shape differs between platforms:
  //
  //   Linux: const_it == ConvertingCtor(end)
  //   macOS: const_it == ConvertingCtor(CopyCtor(end))
  //
  // The extra inner CopyCtor on macOS would emit a redundant .clone() in the
  // generated Rust. cpp2rust suppresses it so the output matches Linux.
  const_it == end ? 0 : 1;
```

This only happens on iterator types because the distinction between
iterator and const_iterator. Other containers don't have this issue.
diff --git a/cpp2rust/converter/converter.cpp b/cpp2rust/converter/converter.cpp
@@ -2754,6 +2754,8 @@ void Converter::ConvertCXXConstructExprArgs(clang::CXXConstructExpr *expr) {
 }
 
 bool Converter::VisitCXXConstructExpr(clang::CXXConstructExpr *expr) {
+  PushSuppressIteratorClone push(*this, expr);
+
   if (auto str = GetMappedAsString(expr, expr->getArgs(), expr->getNumArgs());
       !str.empty()) {
     StrCat(str);
@@ -2764,8 +2766,10 @@ bool Converter::VisitCXXConstructExpr(clang::CXXConstructExpr *expr) {
   if (ctor->isCopyOrMoveConstructor() ||
       (ctor->isConvertingConstructor(false) && ctor->getNumParams() == 1 &&
        ctor->getParamDecl(0)->getType()->isRValueReferenceType())) {
+    // Take supress before recursing into the child.
+    bool suppress = PushSuppressIteratorClone::take(*this);
     Convert(expr->getArg(0));
-    if (ctor->isCopyConstructor() && !IsRedundantCopyInConversion(ctx_, expr)) {
+    if (ctor->isCopyConstructor() && !suppress) {
       StrCat(".clone()");
     }
     return false;
diff --git a/cpp2rust/converter/converter.h b/cpp2rust/converter/converter.h
@@ -13,6 +13,7 @@
 #include <string>
 #include <unordered_map>
 #include <unordered_set>
+#include <utility>
 #include <vector>
 
 #include "converter/lex.h"
@@ -529,6 +530,7 @@ class Converter : public clang::RecursiveASTVisitor<Converter> {
   bool in_function_formals_ = false;
   bool in_const_initializer_ = false;
   std::optional<bool> autoref_mut_;
+  bool suppress_iterator_clone_ = false;
 
   struct PushExplicitAutoref {
     Converter &c;
@@ -540,6 +542,41 @@ class Converter : public clang::RecursiveASTVisitor<Converter> {
     ~PushExplicitAutoref() { c.autoref_mut_ = prev; }
   };
 
+  struct PushSuppressIteratorClone {
+    Converter &c;
+    bool prev;
+    PushSuppressIteratorClone(Converter &c, clang::CXXConstructExpr *expr)
+        : c(c), prev(c.suppress_iterator_clone_) {
+      auto *ctor = expr->getConstructor();
+      if (!ctor->isCopyOrMoveConstructor() &&
+          ctor->isConvertingConstructor(/*AllowExplicit=*/false) &&
+          ctor->getNumParams() == 1 && IsIteratorType(expr->getType())) {
+        c.suppress_iterator_clone_ = true;
+      }
+    }
+    ~PushSuppressIteratorClone() { c.suppress_iterator_clone_ = prev; }
+    PushSuppressIteratorClone(const PushSuppressIteratorClone &) = delete;
+    PushSuppressIteratorClone &
+    operator=(const PushSuppressIteratorClone &) = delete;
+
+    static bool take(Converter &c) {
+      return std::exchange(c.suppress_iterator_clone_, false);
+    }
+
+  private:
+    static bool IsIteratorType(clang::QualType qt) {
+      if (auto *record = qt->getAsCXXRecordDecl()) {
+        for (auto *d : record->decls()) {
+          if (auto *tnd = llvm::dyn_cast<clang::TypedefNameDecl>(d)) {
+            if (tnd->getName() == "iterator_category")
+              return true;
+          }
+        }
+      }
+      return false;
+    }
+  };
+
   struct PushConstInitializer {
     Converter &c;
     bool prev;
diff --git a/cpp2rust/converter/converter_lib.cpp b/cpp2rust/converter/converter_lib.cpp
@@ -644,16 +644,6 @@ std::string GetClassName(clang::QualType type) {
   return {};
 }
 
-bool IsRedundantCopyInConversion(clang::ASTContext &ctx,
-                                 const clang::CXXConstructExpr *expr) {
-  auto parents = ctx.getParentMapContext().getParents(*expr);
-  if (parents.empty()) {
-    return false;
-  }
-  auto *parent = parents[0].get<clang::CXXConstructExpr>();
-  return parent && parent->getConstructor()->isConvertingConstructor(false);
-}
-
 bool IsVaListType(clang::QualType type) {
   for (auto t = type; !t.isNull();) {
     if (auto *adjusted = t->getAs<clang::AdjustedType>()) {
diff --git a/cpp2rust/converter/converter_lib.h b/cpp2rust/converter/converter_lib.h
@@ -145,9 +145,6 @@ GetForRangeIteratorType(const clang::CXXForRangeStmt *for_range);
 
 std::string GetClassName(clang::QualType type);
 
-bool IsRedundantCopyInConversion(clang::ASTContext &ctx,
-                                 const clang::CXXConstructExpr *expr);
-
 bool IsVaListType(clang::QualType type);
 
 bool IsBuiltinVaStart(const clang::CallExpr *expr);
diff --git a/cpp2rust/converter/models/converter_refcount.cpp b/cpp2rust/converter/models/converter_refcount.cpp
@@ -1541,6 +1541,7 @@ std::string ConverterRefCount::ConvertStream(clang::Expr *expr) {
 
 bool ConverterRefCount::VisitCXXConstructExpr(clang::CXXConstructExpr *expr) {
   PushConversionKind push(*this, ConversionKind::Unboxed);
+  PushSuppressIteratorClone push_suppress(*this, expr);
 
   if (auto str = GetMappedAsString(expr, expr->getArgs(), expr->getNumArgs());
       !str.empty()) {
@@ -1564,7 +1565,9 @@ bool ConverterRefCount::VisitCXXConstructExpr(clang::CXXConstructExpr *expr) {
   }
 
   if (ctor->isCopyConstructor()) {
-    StrCat(ConvertRValue(expr->getArg(0)));
+    StrCat(PushSuppressIteratorClone::take(*this)
+               ? ConvertRValue(expr->getArg(0))
+               : ConvertFreshRValue(expr->getArg(0)));
     return false;
   }
 
diff --git a/rules/map/ir_refcount.json b/rules/map/ir_refcount.json
@@ -462,14 +462,27 @@
   "f19": {
     "body": [
       {
-        "placeholder": {
-          "arg": 0,
-          "access": "read"
+        "method_call": {
+          "receiver": [
+            {
+              "placeholder": {
+                "arg": 0,
+                "access": "read"
+              }
+            }
+          ],
+          "body": [
+            {
+              "text": ".clone()"
+            }
+          ]
         }
       }
     ],
     "generics": {
-      "T1": [],
+      "T1": [
+        "Clone"
+      ],
       "T2": []
     },
     "params": {
diff --git a/rules/map/ir_unsafe.json b/rules/map/ir_unsafe.json
@@ -466,14 +466,27 @@
   "f19": {
     "body": [
       {
-        "placeholder": {
-          "arg": 0,
-          "access": "read"
+        "method_call": {
+          "receiver": [
+            {
+              "placeholder": {
+                "arg": 0,
+                "access": "read"
+              }
+            }
+          ],
+          "body": [
+            {
+              "text": ".clone()"
+            }
+          ]
         }
       }
     ],
     "generics": {
-      "T1": [],
+      "T1": [
+        "Clone"
+      ],
       "T2": []
     },
     "params": {
diff --git a/rules/map/tgt_refcount.rs b/rules/map/tgt_refcount.rs
@@ -120,8 +120,8 @@ fn f17<T1: Ord + Clone + 'static, T2: 'static>(
     RefcountMapIter::find_key(a0, &a1)
 }
 
-fn f19<T1, T2>(a0: RefcountMapIter<T1, T2>) -> RefcountMapIter<T1, T2> {
-    a0
+fn f19<T1: Clone, T2>(a0: RefcountMapIter<T1, T2>) -> RefcountMapIter<T1, T2> {
+    a0.clone()
 }
 
 fn f20<T1: Ord + Clone + 'static, T2: 'static>(a0: RefcountMapIter<T1, T2>) -> Value<T1> {
diff --git a/rules/map/tgt_unsafe.rs b/rules/map/tgt_unsafe.rs
@@ -83,8 +83,8 @@ unsafe fn f17<T1: Ord + Clone, T2>(a0: BTreeMap<T1, Box<T2>>, a1: T1) -> UnsafeM
     UnsafeMapIterator::find_key(&a0 as *const BTreeMap<T1, Box<T2>>, &a1)
 }
 
-unsafe fn f19<T1, T2>(a0: UnsafeMapIterator<T1, T2>) -> UnsafeMapIterator<T1, T2> {
-    a0
+unsafe fn f19<T1: Clone, T2>(a0: UnsafeMapIterator<T1, T2>) -> UnsafeMapIterator<T1, T2> {
+    a0.clone()
 }
 
 unsafe fn f20<T1: Ord + Clone, T2>(a0: UnsafeMapIterator<T1, T2>) -> *const T1 {
diff --git a/tests/unit/initializer_list.cpp b/tests/unit/initializer_list.cpp
@@ -0,0 +1,16 @@
+#include <cassert>
+#include <cstddef>
+#include <initializer_list>
+#include <vector>
+
+size_t f(std::initializer_list<int> bytes) {
+  std::vector<int> *buf = new std::vector<int>(bytes);
+  size_t n = bytes.size();
+  delete buf;
+  return n;
+}
+
+int main() {
+  assert(f({1, 2, 3}) == 3);
+  return 0;
+}
diff --git a/tests/unit/out/refcount/initializer_list.rs b/tests/unit/out/refcount/initializer_list.rs
@@ -0,0 +1,27 @@
+extern crate libcc2rs;
+use libcc2rs::*;
+use std::cell::RefCell;
+use std::collections::BTreeMap;
+use std::io::prelude::*;
+use std::io::{Read, Seek, Write};
+use std::os::fd::AsFd;
+use std::rc::{Rc, Weak};
+pub fn f_0(bytes: Vec<i32>) -> u64 {
+    let bytes: Value<Vec<i32>> = Rc::new(RefCell::new(bytes));
+    let buf: Value<Ptr<Vec<i32>>> = Rc::new(RefCell::new(Ptr::alloc((*bytes.borrow()).clone())));
+    let n: Value<u64> = Rc::new(RefCell::new((*bytes.borrow()).len() as u64));
+    (*buf.borrow()).delete();
+    return (*n.borrow());
+}
+pub fn main() {
+    std::process::exit(main_0());
+}
+fn main_0() -> i32 {
+    assert!(
+        (({
+            let _bytes: Vec<i32> = vec![1, 2, 3];
+            f_0(_bytes)
+        }) == 3_u64)
+    );
+    return 0;
+}
diff --git a/tests/unit/out/refcount/map.rs b/tests/unit/out/refcount/map.rs
@@ -192,19 +192,22 @@ fn main_0() -> i32 {
         &1_i16,
     )));
     let const_it: Value<RefcountMapIter<i16, u32>> = Rc::new(RefCell::new(
-        RefcountMapIter::find_key((m.as_pointer() as Ptr<BTreeMap<i16, Value<u32>>>), &10_i16),
+        RefcountMapIter::find_key((m.as_pointer() as Ptr<BTreeMap<i16, Value<u32>>>), &10_i16)
+            .clone(),
     ));
     let x1: Value<u32> = Rc::new(RefCell::new(if (*it.borrow()) == (*end.borrow()) {
         0_u32
     } else {
         (*(*it.borrow()).second().borrow())
     }));
     assert!(((*x1.borrow()) == 4_u32));
-    let x2: Value<u32> = Rc::new(RefCell::new(if (*const_it.borrow()) == (*end.borrow()) {
-        0_u32
-    } else {
-        (*(*const_it.borrow()).second().borrow())
-    }));
+    let x2: Value<u32> = Rc::new(RefCell::new(
+        if (*const_it.borrow()) == (*end.borrow()).clone() {
+            0_u32
+        } else {
+            (*(*const_it.borrow()).second().borrow())
+        },
+    ));
     assert!(((*x2.borrow()) == 0_u32));
     let x3: Value<u32> = Rc::new(RefCell::new(
         if (*it.borrow())
@@ -218,7 +221,7 @@ fn main_0() -> i32 {
     assert!(((*x3.borrow()) == 4_u32));
     let x4: Value<u32> = Rc::new(RefCell::new(
         if (*const_it.borrow())
-            == RefcountMapIter::end((m.as_pointer() as Ptr<BTreeMap<i16, Value<u32>>>))
+            == RefcountMapIter::end((m.as_pointer() as Ptr<BTreeMap<i16, Value<u32>>>)).clone()
         {
             0_u32
         } else {
@@ -274,7 +277,7 @@ fn main_0() -> i32 {
     );
     RefcountMapIter::erase(
         ((r).clone() as Ptr<BTreeMap<i16, Value<u32>>>),
-        &(*it4.borrow()),
+        &(*it4.borrow()).clone(),
     );
     assert!(((*r.upgrade().deref()).len() as u64 == 3_u64));
     assert!(
diff --git a/tests/unit/out/refcount/redundant_copy_in_conversion.rs b/tests/unit/out/refcount/redundant_copy_in_conversion.rs
@@ -0,0 +1,55 @@
+extern crate libcc2rs;
+use libcc2rs::*;
+use std::cell::RefCell;
+use std::collections::BTreeMap;
+use std::io::prelude::*;
+use std::io::{Read, Seek, Write};
+use std::os::fd::AsFd;
+use std::rc::{Rc, Weak};
+pub fn sink_0(it: RefcountMapIter<i32, i32>) -> i32 {
+    let it: Value<RefcountMapIter<i32, i32>> = Rc::new(RefCell::new(it));
+    let cit: Value<RefcountMapIter<i32, i32>> = Rc::new(RefCell::new((*it.borrow()).clone()));
+    return if (*cit.borrow()) == (*it.borrow()).clone() {
+        (*(*it.borrow()).second().borrow())
+    } else {
+        0
+    };
+}
+pub fn main() {
+    std::process::exit(main_0());
+}
+fn main_0() -> i32 {
+    let m: Value<BTreeMap<i32, Value<i32>>> = Rc::new(RefCell::new(BTreeMap::new()));
+    (m.as_pointer() as Ptr<BTreeMap<i32, Value<i32>>>)
+        .with_mut(|__v: &mut BTreeMap<i32, Value<i32>>| {
+            __v.entry(0.clone())
+                .or_insert_with(|| Rc::new(RefCell::new(<i32>::default())))
+                .as_pointer()
+        })
+        .write(1);
+    let end: Value<RefcountMapIter<i32, i32>> = Rc::new(RefCell::new(RefcountMapIter::end(
+        (m.as_pointer() as Ptr<BTreeMap<i32, Value<i32>>>),
+    )));
+    let it0: Value<RefcountMapIter<i32, i32>> = Rc::new(RefCell::new(RefcountMapIter::find_key(
+        (m.as_pointer() as Ptr<BTreeMap<i32, Value<i32>>>),
+        &0,
+    )));
+    let const_it: Value<RefcountMapIter<i32, i32>> = Rc::new(RefCell::new((*it0.borrow()).clone()));
+    let r: Value<i32> = Rc::new(RefCell::new(
+        if (*const_it.borrow()) == (*end.borrow()).clone() {
+            0
+        } else {
+            1
+        },
+    ));
+    (*r.borrow_mut()) += ({
+        let _it: RefcountMapIter<i32, i32> = (*it0.borrow()).clone();
+        sink_0(_it)
+    });
+    (*r.borrow_mut()) += if (*end.borrow()) == (*end.borrow()) {
+        0
+    } else {
+        1
+    };
+    return (*r.borrow());
+}
diff --git a/tests/unit/out/unsafe/initializer_list.rs b/tests/unit/out/unsafe/initializer_list.rs
@@ -0,0 +1,28 @@
+extern crate libc;
+use libc::*;
+extern crate libcc2rs;
+use libcc2rs::*;
+use std::collections::BTreeMap;
+use std::io::{Read, Seek, Write};
+use std::os::fd::{AsFd, FromRawFd, IntoRawFd};
+use std::rc::Rc;
+pub unsafe fn f_0(mut bytes: Vec<i32>) -> u64 {
+    let mut buf: *mut Vec<i32> = (Box::leak(Box::new(bytes.clone())) as *mut Vec<i32>);
+    let mut n: u64 = bytes.len() as u64;
+    ::std::mem::drop(Box::from_raw(buf));
+    return n;
+}
+pub fn main() {
+    unsafe {
+        std::process::exit(main_0() as i32);
+    }
+}
+unsafe fn main_0() -> i32 {
+    assert!(
+        ((unsafe {
+            let _bytes: Vec<i32> = vec![1, 2, 3];
+            f_0(_bytes)
+        }) == (3_u64))
+    );
+    return 0;
+}
diff --git a/tests/unit/out/unsafe/map.rs b/tests/unit/out/unsafe/map.rs
diff --git a/tests/unit/out/unsafe/redundant_copy_in_conversion.rs b/tests/unit/out/unsafe/redundant_copy_in_conversion.rs
diff --git a/tests/unit/redundant_copy_in_conversion.cpp b/tests/unit/redundant_copy_in_conversion.cpp

Original file line number	Diff line number	Diff line change
`@@ -1541,6 +1541,7 @@ std::string ConverterRefCount::ConvertStream(clang::Expr *expr) {`
`1541`	`1541`
`1542`	`1542`	`bool ConverterRefCount::VisitCXXConstructExpr(clang::CXXConstructExpr *expr) {`
`1543`	`1543`	`PushConversionKind push(*this, ConversionKind::Unboxed);`
	`1544`	`+ PushSuppressIteratorClone push_suppress(*this, expr);`
`1544`	`1545`
`1545`	`1546`	`if (auto str = GetMappedAsString(expr, expr->getArgs(), expr->getNumArgs());`
`1546`	`1547`	`!str.empty()) {`
`@@ -1564,7 +1565,9 @@ bool ConverterRefCount::VisitCXXConstructExpr(clang::CXXConstructExpr *expr) {`
`1564`	`1565`	`}`
`1565`	`1566`
`1566`	`1567`	`if (ctor->isCopyConstructor()) {`
`1567`		`- StrCat(ConvertRValue(expr->getArg(0)));`
	`1568`	`+ StrCat(PushSuppressIteratorClone::take(*this)`
	`1569`	`+ ? ConvertRValue(expr->getArg(0))`
	`1570`	`+ : ConvertFreshRValue(expr->getArg(0)));`
`1568`	`1571`	`return false;`
`1569`	`1572`	`}`
`1570`	`1573`
Original file line number	Diff line number	Diff line change
`@@ -120,8 +120,8 @@ fn f17<T1: Ord + Clone + 'static, T2: 'static>(`
`120`	`120`	`RefcountMapIter::find_key(a0, &a1)`
`121`	`121`	`}`
`122`	`122`
`123`		`-fn f19<T1, T2>(a0: RefcountMapIter<T1, T2>) -> RefcountMapIter<T1, T2> {`
`124`		`- a0`
	`123`	`+fn f19<T1: Clone, T2>(a0: RefcountMapIter<T1, T2>) -> RefcountMapIter<T1, T2> {`
	`124`	`+ a0.clone()`
`125`	`125`	`}`
`126`	`126`
`127`	`127`	`fn f20<T1: Ord + Clone + 'static, T2: 'static>(a0: RefcountMapIter<T1, T2>) -> Value<T1> {`
Original file line number	Diff line number	Diff line change
`@@ -83,8 +83,8 @@ unsafe fn f17<T1: Ord + Clone, T2>(a0: BTreeMap<T1, Box<T2>>, a1: T1) -> UnsafeM`
`83`	`83`	`UnsafeMapIterator::find_key(&a0 as *const BTreeMap<T1, Box<T2>>, &a1)`
`84`	`84`	`}`
`85`	`85`
`86`		`-unsafe fn f19<T1, T2>(a0: UnsafeMapIterator<T1, T2>) -> UnsafeMapIterator<T1, T2> {`
`87`		`- a0`
	`86`	`+unsafe fn f19<T1: Clone, T2>(a0: UnsafeMapIterator<T1, T2>) -> UnsafeMapIterator<T1, T2> {`
	`87`	`+ a0.clone()`
`88`	`88`	`}`
`89`	`89`
`90`	`90`	`unsafe fn f20<T1: Ord + Clone, T2>(a0: UnsafeMapIterator<T1, T2>) -> *const T1 {`