Run element destructors on refcount map iterator erase

docs/src/rules/writing-rules.md

@@ -102,9 +102,19 @@ std::string f9(std::size_t n, char ch) { return std::string(n, ch); }
 Overloads that differ in value category are distinct rules too: `rules/vector`
 has separate rules for `push_back(const T1 &)` and `push_back(T1 &&)`.
 
-No destructor rules exist so far: the STL and libc APIs covered by the current
-rules have not needed any, since their types map to Rust types whose `Drop`
-implementations already do the right thing.
+Destruction of a translated record uses the explicit `Destructor` trait, not
+Rust's `Drop`. Container rules must therefore distinguish releasing Rust storage
+from calling a contained record's C++ destructor. For example, the refcount map
+iterator-erase rules (`f3` and `f27` in
+[`rules/map/tgt_refcount.rs`](https://github.com/Cpp2Rust/cpp2rust/blob/master/rules/map/tgt_refcount.rs))
+pass a finalizer to `Map::erase`. The runtime unlinks the entry and removes its
+key from the index, then calls the finalizer with pointers to the stored key and
+mapped value, without holding a borrow of the map. The rules reuse the
+`__swap::Destroy` dispatch to call each type's destructor if it has one, and do
+nothing for trivial types. This dispatch is in the expanded rule, where the
+concrete types are known; a `Destructor` bound on the generic runtime method
+would exclude trivial types. Iterator copies may retain the Rust storage, but do
+not delay or repeat these explicit destructor calls.
 
 ## Operators
 

rules/map/shim.rs

@@ -255,18 +255,23 @@ impl<K: CppLess + 'static, V: 'static> Map<K, Entry<K, V>> {
     pub fn erase(
         map: Ptr<Self>,
         it: &BidirectionalIterator<Entry<K, V>>,
+        destroy: impl FnOnce(Ptr<K>, Ptr<V>),
     ) -> BidirectionalIterator<Entry<K, V>> {
         let BidirectionalIterator::Map(node) = it;
-        let key = node
-            .borrow()
-            .value
-            .as_ref()
-            .expect("ub: erase of end iterator")
-            .borrow()
-            .0
-            .as_pointer();
+        let (key, value) = {
+            let node = node.borrow();
+            let entry = node
+                .value
+                .as_ref()
+                .expect("ub: erase of end iterator")
+                .borrow();
+            (entry.0.as_pointer(), entry.1.as_pointer())
+        };
         let next = Self::unlink(node);
-        map.with_mut(|map| map.nodes.remove(&MapKey(key)));
+        map.with_mut(|map| map.nodes.remove(&MapKey(key.clone())));
+        // Run C++ destructors after removing the key from the index and ending
+        // the map borrow. Iterator copies may still keep the node alive.
+        destroy(key, value);
         BidirectionalIterator::Map(next)
     }
 

rules/map/tgt_refcount.rs

@@ -24,7 +24,11 @@ fn f3<T1: CppLess + 'static, T2: 'static>(
     a0: Ptr<Map<T1, (Value<T1>, Value<T2>)>>,
     a1: BidirectionalIterator<(Value<T1>, Value<T2>)>,
 ) -> BidirectionalIterator<(Value<T1>, Value<T2>)> {
-    Map::erase(a0, &a1)
+    Map::erase(a0, &a1, |__key, __value| {
+        use libcc2rs::__swap::{Destroy, DestroyNone, DestroyWithDtor};
+        (&&Destroy(&__value)).destroy();
+        (&&Destroy(&__key)).destroy();
+    })
 }
 
 fn f5<T1, T2>() -> Map<T1, (Value<T1>, Value<T2>)> {
@@ -87,7 +91,11 @@ fn f27<T1: CppLess + 'static, T2: 'static>(
     a0: Ptr<Map<T1, (Value<T1>, Value<T2>)>>,
     a1: BidirectionalIterator<(Value<T1>, Value<T2>)>,
 ) -> BidirectionalIterator<(Value<T1>, Value<T2>)> {
-    Map::erase(a0, &a1)
+    Map::erase(a0, &a1, |__key, __value| {
+        use libcc2rs::__swap::{Destroy, DestroyNone, DestroyWithDtor};
+        (&&Destroy(&__value)).destroy();
+        (&&Destroy(&__key)).destroy();
+    })
 }
 
 fn f28<T1: CppLess + 'static, T2: 'static>(a0: Ptr<Map<T1, (Value<T1>, Value<T2>)>>, a1: Ptr<T1>) -> usize {

tests/unit/map_erase_destructor.cpp

@@ -0,0 +1,78 @@
+// XFAIL: unsafe
+#include <cassert>
+#include <map>
+
+int destroyed[32];
+
+struct Tracked {
+  int id;
+  Tracked() : id(0) {}
+  Tracked(int id) : id(id) {}
+  ~Tracked() { ++destroyed[id]; }
+};
+
+bool operator<(const Tracked &a, const Tracked &b) { return a.id < b.id; }
+
+template <typename K, typename V>
+void check_erase(bool key_destructor, bool value_destructor) {
+  K one(1), two(2), three(3);
+  std::map<K, V> m;
+  m[one] = V(11);
+  m[two] = V(12);
+  m[three] = V(13);
+  for (int &n : destroyed)
+    n = 0;
+
+  {
+    auto first = m.find(one);
+    auto middle = m.find(two);
+    auto last = m.find(three);
+    auto retained = middle;
+    typename std::map<K, V>::const_iterator retained_const = middle;
+    auto *first_value = &first->second;
+    auto *last_value = &last->second;
+    auto end = m.end();
+
+    auto next = m.erase(middle);
+    assert(destroyed[2] == key_destructor);
+    assert(destroyed[12] == value_destructor);
+    assert(destroyed[1] == 0 && destroyed[3] == 0);
+    assert(destroyed[11] == 0 && destroyed[13] == 0);
+    assert(m.size() == 2 && m.find(two) == end);
+    assert(next == last && first == m.begin());
+    assert(&first->second == first_value && &last->second == last_value);
+    auto walking = first;
+    ++walking;
+    assert(walking == last);
+    ++walking;
+    assert(walking == end);
+
+    typename std::map<K, V>::const_iterator const_first = first;
+    next = m.erase(const_first);
+    assert(destroyed[1] == key_destructor);
+    assert(destroyed[11] == value_destructor);
+    assert(m.size() == 1 && next == last && m.begin() == last);
+    assert(&last->second == last_value);
+
+    next = m.erase(last);
+    assert(destroyed[3] == key_destructor);
+    assert(destroyed[13] == value_destructor);
+    assert(next == end && m.size() == 0 && m.begin() == end);
+    // The copies of erased iterators are retained, but never used.
+  }
+  for (int i = 0; i < 32; ++i) {
+    int expected = 0;
+    if (i >= 1 && i <= 3)
+      expected = key_destructor;
+    if (i >= 11 && i <= 13)
+      expected = value_destructor;
+    assert(destroyed[i] == expected);
+  }
+}
+
+int main() {
+  check_erase<Tracked, Tracked>(true, true);
+  check_erase<Tracked, int>(true, false);
+  check_erase<int, Tracked>(false, true);
+  check_erase<int, int>(false, false);
+}

tests/unit/out/refcount/map.rs

@@ -315,7 +315,15 @@ fn main_0() -> i32 {
             .as_pointer()
             .with(|__m: &Map<i16, (Value<i16>, Value<u32>)>| __m.end())
     );
-    Map::erase((r).clone(), &({ (*it4.borrow()).clone() }));
+    Map::erase(
+        (r).clone(),
+        &({ (*it4.borrow()).clone() }),
+        |__key, __value| {
+            use libcc2rs::__swap::{Destroy, DestroyNone, DestroyWithDtor};
+            (&&Destroy(&__value)).destroy();
+            (&&Destroy(&__key)).destroy();
+        },
+    );
     assert!(({ ((*r.upgrade().deref()).len() == 3_usize) }));
     assert!(
         ({
@@ -370,9 +378,16 @@ fn main_0() -> i32 {
             }),
         ),
     ));
-    let next: Value<BidirectionalIterator<(Value<i16>, Value<u32>)>> = Rc::new(RefCell::new(
-        Map::erase(m.as_pointer(), &({ (*cit.borrow()).clone() })),
-    ));
+    let next: Value<BidirectionalIterator<(Value<i16>, Value<u32>)>> =
+        Rc::new(RefCell::new(Map::erase(
+            m.as_pointer(),
+            &({ (*cit.borrow()).clone() }),
+            |__key, __value| {
+                use libcc2rs::__swap::{Destroy, DestroyNone, DestroyWithDtor};
+                (&&Destroy(&__value)).destroy();
+                (&&Destroy(&__key)).destroy();
+            },
+        )));
     assert!(
         (*next.borrow())
             == ({
@@ -383,7 +398,15 @@ fn main_0() -> i32 {
                     })
             })
     );
-    (*next.borrow_mut()) = Map::erase(m.as_pointer(), &({ (*next.borrow()).clone() }));
+    (*next.borrow_mut()) = Map::erase(
+        m.as_pointer(),
+        &({ (*next.borrow()).clone() }),
+        |__key, __value| {
+            use libcc2rs::__swap::{Destroy, DestroyNone, DestroyWithDtor};
+            (&&Destroy(&__value)).destroy();
+            (&&Destroy(&__key)).destroy();
+        },
+    );
     assert!(
         (*next.borrow())
             == m.as_pointer()