Run element destructors on refcount map iterator erase

docs/src/rules/writing-rules.md

@@ -102,9 +102,24 @@ 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))
+use `Map::erase` to unlink the entry and remove its key from the index. The
+runtime returns the successor and a strong owner of the erased entry. After its
+borrows have ended, the rule calls the key and mapped-value destructors before
+yielding the successor. Keeping that owner alive during destruction is necessary
+even if no iterator copy retains the erased node.
+
+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. No finalizer is passed to the
+runtime or stored in the map, so default construction, copying and moving do not
+need to carry destructor hooks. Iterator copies may retain the Rust storage, but
+do not delay or repeat these explicit destructor calls.
 
 ## Operators
 

rules/map/shim.rs

@@ -145,6 +145,7 @@ impl<K, T> Map<K, T> {
 
 type Entry<K, V> = (Value<K>, Value<V>);
 type EntryNode<K, V> = Value<MapNode<Entry<K, V>>>;
+type EraseResult<K, V> = (BidirectionalIterator<Entry<K, V>>, Value<Entry<K, V>>);
 
 impl<K: CppLess + 'static, V> Map<K, Entry<K, V>> {
     fn link_after(
@@ -252,22 +253,23 @@ impl<K: CppLess + 'static, V: 'static> Map<K, Entry<K, V>> {
         })
     }
 
+    // Return ownership of the erased entry so the rule can call its C++
+    // destructors with concrete types, after all map borrows have ended.
     pub fn erase(
         map: Ptr<Self>,
         it: &BidirectionalIterator<Entry<K, V>>,
-    ) -> BidirectionalIterator<Entry<K, V>> {
+    ) -> EraseResult<K, V> {
         let BidirectionalIterator::Map(node) = it;
-        let key = node
+        let entry = node
             .borrow()
             .value
             .as_ref()
             .expect("ub: erase of end iterator")
-            .borrow()
-            .0
-            .as_pointer();
+            .clone();
+        let key = entry.borrow().0.as_pointer();
         let next = Self::unlink(node);
         map.with_mut(|map| map.nodes.remove(&MapKey(key)));
-        BidirectionalIterator::Map(next)
+        (BidirectionalIterator::Map(next), entry)
     }
 
     pub fn erase_key(map: Ptr<Self>, key: &Ptr<K>) -> usize {

rules/map/tgt_refcount.rs

@@ -24,7 +24,15 @@ 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)
+    let (__next, __entry) = Map::erase(a0, &a1);
+    let (__key, __value) = {
+        let __entry_ref = __entry.borrow();
+        (__entry_ref.0.as_pointer(), __entry_ref.1.as_pointer())
+    };
+    use libcc2rs::__swap::{Destroy, DestroyNone, DestroyWithDtor};
+    (&&Destroy(&__value)).destroy();
+    (&&Destroy(&__key)).destroy();
+    __next
 }
 
 fn f5<T1, T2>() -> Map<T1, (Value<T1>, Value<T2>)> {
@@ -87,7 +95,15 @@ 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)
+    let (__next, __entry) = Map::erase(a0, &a1);
+    let (__key, __value) = {
+        let __entry_ref = __entry.borrow();
+        (__entry_ref.0.as_pointer(), __entry_ref.1.as_pointer())
+    };
+    use libcc2rs::__swap::{Destroy, DestroyNone, DestroyWithDtor};
+    (&&Destroy(&__value)).destroy();
+    (&&Destroy(&__key)).destroy();
+    __next
 }
 
 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,102 @@
+// XFAIL: unsafe
+#include <cassert>
+#include <map>
+#include <utility>
+
+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);
+  }
+}
+
+struct MoveOnly {
+  int id;
+  MoveOnly(int id) : id(id) {}
+  MoveOnly(const MoveOnly &) = delete;
+  MoveOnly(MoveOnly &&other) : id(other.id) {}
+  ~MoveOnly() { ++destroyed[id]; }
+};
+
+bool operator<(const MoveOnly &a, const MoveOnly &b) { return a.id < b.id; }
+
+void check_nested_move_only() {
+  // The inner map is default-constructed by operator[], not a constructor rule.
+  std::map<int, std::map<MoveOnly, Tracked>> outer;
+  auto &m = outer[0];
+  MoveOnly key(4);
+  m[std::move(key)] = Tracked(14);
+  destroyed[4] = destroyed[14] = 0;
+  auto next = m.erase(m.begin());
+  assert(destroyed[4] == 1 && destroyed[14] == 1);
+  assert(m.size() == 0 && next == m.end());
+}
+
+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);
+  check_nested_move_only();
+}

tests/unit/out/refcount/map.rs

@@ -315,7 +315,17 @@ fn main_0() -> i32 {
             .as_pointer()
             .with(|__m: &Map<i16, (Value<i16>, Value<u32>)>| __m.end())
     );
-    Map::erase((r).clone(), &({ (*it4.borrow()).clone() }));
+    {
+        let (__next, __entry) = Map::erase((r).clone(), &({ (*it4.borrow()).clone() }));
+        let (__key, __value) = {
+            let __entry_ref = __entry.borrow();
+            (__entry_ref.0.as_pointer(), __entry_ref.1.as_pointer())
+        };
+        use libcc2rs::__swap::{Destroy, DestroyNone, DestroyWithDtor};
+        (&&Destroy(&__value)).destroy();
+        (&&Destroy(&__key)).destroy();
+        __next
+    };
     assert!(({ ((*r.upgrade().deref()).len() == 3_usize) }));
     assert!(
         ({
@@ -370,9 +380,17 @@ 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({
+        let (__next, __entry) = Map::erase(m.as_pointer(), &({ (*cit.borrow()).clone() }));
+        let (__key, __value) = {
+            let __entry_ref = __entry.borrow();
+            (__entry_ref.0.as_pointer(), __entry_ref.1.as_pointer())
+        };
+        use libcc2rs::__swap::{Destroy, DestroyNone, DestroyWithDtor};
+        (&&Destroy(&__value)).destroy();
+        (&&Destroy(&__key)).destroy();
+        __next
+    }));
     assert!(
         (*next.borrow())
             == ({
@@ -383,7 +401,17 @@ fn main_0() -> i32 {
                     })
             })
     );
-    (*next.borrow_mut()) = Map::erase(m.as_pointer(), &({ (*next.borrow()).clone() }));
+    (*next.borrow_mut()) = {
+        let (__next, __entry) = Map::erase(m.as_pointer(), &({ (*next.borrow()).clone() }));
+        let (__key, __value) = {
+            let __entry_ref = __entry.borrow();
+            (__entry_ref.0.as_pointer(), __entry_ref.1.as_pointer())
+        };
+        use libcc2rs::__swap::{Destroy, DestroyNone, DestroyWithDtor};
+        (&&Destroy(&__value)).destroy();
+        (&&Destroy(&__key)).destroy();
+        __next
+    };
     assert!(
         (*next.borrow())
             == m.as_pointer()