Remove converter hack around std::map::iterator (#523)

The old code special cased `std::map::iterator` (which is just a sugar
for `std::pair<const K, V>`) to generate `i.first()` and `i.second()`
instead of `i.0` and `i.1`.

This PR removes the hack by saving a pair in `libcc2rs::MapIter` which
can be accessed with `.0` and `.1`. It also fixes a translation bug:

```cpp
 for (auto i : m) {
    i.second = -1;
    assert(i.second == -1);
  }
  for (const auto &i : m) {
    assert(i.second != -1);
  }
```

Author: lucic71

cpp2rust/converter/converter.cpp

@@ -1498,12 +1498,8 @@ void Converter::ConvertLoopVariable(clang::VarDecl *decl,
   }
 }
 
-void Converter::ConvertForRangeBody(clang::CXXForRangeStmt *stmt,
-                                    const clang::VarDecl *map_iter_decl) {
+void Converter::ConvertForRangeBody(clang::CXXForRangeStmt *stmt) {
   PushBreakTarget push(break_target_, BreakTarget::Loop);
-  std::optional<ScopedMapIterDecl> skip;
-  if (map_iter_decl)
-    skip.emplace(*this, map_iter_decl);
   curr_for_inc_.emplace_back(nullptr);
   ConvertBodyStmts(stmt->getBody());
   curr_for_inc_.pop_back();
@@ -1529,18 +1525,19 @@ bool Converter::VisitCXXForRangeStmt(clang::CXXForRangeStmt *stmt) {
 }
 
 bool Converter::VisitCXXForRangeStmtMap(clang::CXXForRangeStmt *stmt) {
-  auto *loop_var = stmt->getLoopVariable();
-  auto loop_var_name = GetNamedDeclAsString(loop_var);
+  auto begin =
+      llvm::cast<clang::VarDecl>(stmt->getBeginStmt()->getSingleDecl());
 
   StrCat("'loop_:");
   auto map_type = Mapper::Map(*this, stmt->getRangeInit()->getType());
-  StrCat(keyword::kFor, loop_var_name, keyword::kIn,
+  StrCat(keyword::kFor, GetNamedDeclAsString(begin), keyword::kIn,
          "UnsafeMapIterator::begin(&");
   Convert(stmt->getRangeInit());
   StrCat(std::format(" as *const {})", map_type));
   {
     PushBrace brace(*this);
-    ConvertForRangeBody(stmt, loop_var);
+    Convert(stmt->getLoopVarStmt());
+    ConvertForRangeBody(stmt);
   }
 
   return false;
@@ -3239,8 +3236,7 @@ void Converter::ConvertDeclRefValue(clang::Expr *expr, clang::ValueDecl *decl) {
   auto str = ConvertDeclRef(expr, decl);
   auto type = GetDeclRefType(curr_function_, expr, decl);
 
-  if (type->getAs<clang::ReferenceType>() && !isAddrOf() &&
-      !map_iter_decls_.contains(clang::dyn_cast<clang::VarDecl>(decl))) {
+  if (type->getAs<clang::ReferenceType>() && !isAddrOf()) {
     EmitDeref(std::move(str), type.getNonReferenceType());
     SetValueFreshness(expr->getType());
     return;

cpp2rust/converter/converter.h

@@ -215,8 +215,7 @@ class Converter : public clang::RecursiveASTVisitor<Converter> {
   bool VisitCXXForRangeStmtIndexBased(clang::CXXForRangeStmt *stmt,
                                       const char *len_suffix);
 
-  void ConvertForRangeBody(clang::CXXForRangeStmt *stmt,
-                           const clang::VarDecl *map_iter_decl = nullptr);
+  void ConvertForRangeBody(clang::CXXForRangeStmt *stmt);
 
   virtual bool VisitBreakStmt(clang::BreakStmt *stmt);
 
@@ -902,8 +901,6 @@ class Converter : public clang::RecursiveASTVisitor<Converter> {
     Converter &c_;
   };
 
-  std::unordered_set<const clang::VarDecl *> map_iter_decls_;
-
   // Local variables hoisted outside a goto_block so that all labels can see and
   // use the variables.
   std::unordered_set<const clang::VarDecl *> hoisted_decls_;
@@ -954,15 +951,6 @@ class Converter : public clang::RecursiveASTVisitor<Converter> {
     operator=(const HoistMaterializedTempBindings &) = delete;
   };
 
-  struct ScopedMapIterDecl {
-    Converter &c;
-    const clang::VarDecl *decl;
-    ScopedMapIterDecl(Converter &c, const clang::VarDecl *decl)
-        : c(c), decl(decl) {
-      c.map_iter_decls_.insert(decl);
-    }
-    ~ScopedMapIterDecl() { c.map_iter_decls_.erase(decl); }
-  };
   static std::unordered_set<std::string> decl_ids_;
   static std::unordered_set<std::string> abstract_structs_;
 

cpp2rust/converter/converter_lib.cpp

@@ -1902,38 +1902,6 @@ GetStrongestIteratorCategory(clang::ASTContext &ctx, clang::QualType type) {
   return std::nullopt;
 }
 
-const clang::CXXForRangeStmt *
-GetParentForRange(clang::ASTContext &ctx, const clang::MemberExpr *member) {
-  auto base = member->getBase()->IgnoreParenImpCasts();
-  if (auto decl_ref = llvm::dyn_cast<clang::DeclRefExpr>(base)) {
-    if (auto var = llvm::dyn_cast<clang::VarDecl>(decl_ref->getDecl())) {
-      for (const auto &parent : ctx.getParents(*var)) {
-        if (auto decl_stmt = parent.get<clang::DeclStmt>()) {
-          for (const auto &grandparent : ctx.getParents(*decl_stmt)) {
-            if (auto for_range = grandparent.get<clang::CXXForRangeStmt>()) {
-              return for_range;
-            }
-          }
-        }
-      }
-    }
-  }
-  return nullptr;
-}
-
-clang::QualType
-GetForRangeIteratorType(const clang::CXXForRangeStmt *for_range) {
-  if (auto begin_stmt = for_range->getBeginStmt()) {
-    if (auto begin_decl_stmt = llvm::dyn_cast<clang::DeclStmt>(begin_stmt)) {
-      if (auto begin_var = llvm::dyn_cast<clang::VarDecl>(
-              begin_decl_stmt->getSingleDecl())) {
-        return begin_var->getType();
-      }
-    }
-  }
-  return {};
-}
-
 std::string GetClassName(clang::QualType type) {
   if (auto *record = type->getAsCXXRecordDecl())
     return record->getQualifiedNameAsString();

cpp2rust/converter/converter_lib.h

@@ -323,12 +323,6 @@ clang::Expr *BuildInitExpr(clang::Sema &sema, clang::QualType type,
                            llvm::ArrayRef<clang::Expr *> args,
                            clang::SourceLocation loc);
 
-const clang::CXXForRangeStmt *
-GetParentForRange(clang::ASTContext &ctx, const clang::MemberExpr *member);
-
-clang::QualType
-GetForRangeIteratorType(const clang::CXXForRangeStmt *for_range);
-
 std::string GetClassName(clang::QualType type);
 
 bool IsVaListType(clang::QualType type);

cpp2rust/converter/models/converter_refcount.cpp

@@ -1044,12 +1044,6 @@ void ConverterRefCount::ConvertDeclRefValue(clang::Expr *expr,
   }
 
   if (auto *ref = decl_t->getAs<clang::ReferenceType>()) {
-    if (map_iter_decls_.contains(clang::dyn_cast<clang::VarDecl>(decl))) {
-      StrCat(str);
-      SetValueFreshness(expr->getType());
-      return;
-    }
-
     if (auto pointee = ref->getPointeeType();
         isObject() && WantsElementPtr() && IsBoxedType(ctx_, pointee)) {
       StrCat(std::format("Ptr::<{}>::decay(&({}))", ToString(pointee),
@@ -2269,19 +2263,19 @@ void ConverterRefCount::EmitByValueShadow(const clang::VarDecl *loop_var,
 }
 
 bool ConverterRefCount::VisitCXXForRangeStmtMap(clang::CXXForRangeStmt *stmt) {
-  auto *loop_var = stmt->getLoopVariable();
-  auto loop_var_name = GetNamedDeclAsString(loop_var);
+  auto begin =
+      llvm::cast<clang::VarDecl>(stmt->getBeginStmt()->getSingleDecl());
+  auto begin_name = GetNamedDeclAsString(begin);
 
   StrCat("'loop_:");
-  StrCat(keyword::kFor, loop_var_name, keyword::kIn, "RefcountMapIter::begin(",
+  StrCat(keyword::kFor, begin_name, keyword::kIn, "RefcountMapIter::begin(",
          ConvertObject(stmt->getRangeInit()), ')');
   PushBrace brace(*this);
 
-  EmitByValueShadow(
-      loop_var, std::string(loop_var_name),
-      "Value<" + Mapper::Map(*this, GetForRangeIteratorType(stmt)) + '>');
+  EmitByValueShadow(begin, std::string(begin_name));
+  Convert(stmt->getLoopVarStmt());
 
-  ConvertForRangeBody(stmt, loop_var);
+  ConvertForRangeBody(stmt);
 
   return false;
 }

cpp2rust/converter/printer.cpp

@@ -371,14 +371,6 @@ std::string ToString(clang::ASTContext &ctx, const clang::Expr *expr) {
                    ToString(ctx, member_decl);
           }
         }
-      } else if (auto for_range = GetParentForRange(ctx, ME)) {
-        if (ToString(ctx, for_range->getRangeInit()->getType())
-                .starts_with("std::map<")) {
-          auto iter_type = GetForRangeIteratorType(for_range);
-          if (!iter_type.isNull()) {
-            return ToString(ctx, iter_type) + "->" + ToString(ctx, member_decl);
-          }
-        }
       }
       return ToString(ctx, member_decl);
     }

libcc2rs/src/iterators.rs

@@ -1,11 +1,9 @@
 // Copyright (c) 2022-present INESC-ID.
 // Distributed under the MIT license that can be found in the LICENSE file.
 
-use crate::{CChar, PostfixDec, PostfixInc, PrefixDec, PrefixInc, Ptr, Value};
-use std::cell::RefCell;
+use crate::{AsPointer, CChar, PostfixDec, PostfixInc, PrefixDec, PrefixInc, Ptr, Value};
 use std::collections::BTreeMap;
 use std::ops::Bound;
-use std::rc::Rc;
 
 pub trait MapAccess: Clone + Default {
     type Key: Ord + Clone;
@@ -45,8 +43,8 @@ pub struct MapIter<K, MapRef> {
     key: Option<K>,
 }
 
-pub type RefcountMapIter<K, V> = MapIter<K, Ptr<BTreeMap<K, Value<V>>>>;
-pub type UnsafeMapIterator<K, V> = MapIter<K, *const BTreeMap<K, Box<V>>>;
+pub type RefcountMapIter<K, V> = MapIter<K, Ptr<BTreeMap<K, Value<(Value<K>, Value<V>)>>>>;
+pub type UnsafeMapIterator<K, V> = MapIter<K, *const BTreeMap<K, Box<(K, V)>>>;
 
 impl<K: Clone, MapRef: Clone> Clone for MapIter<K, MapRef> {
     fn clone(&self) -> Self {
@@ -141,44 +139,31 @@ impl<K: Ord + Clone, MapRef: MapAccess<Key = K>> MapIter<K, MapRef> {
 }
 
 pub trait MapIterator {
-    type First;
-    type Second;
-    fn first(&self) -> Self::First;
-    fn second(&self) -> Self::Second;
+    type Entry;
+    fn deref(&self) -> Self::Entry;
 }
 
 impl<K: Ord + Clone + 'static, V: 'static> MapIterator for RefcountMapIter<K, V> {
-    type First = Value<K>;
-    type Second = Value<V>;
+    type Entry = Ptr<(Value<K>, Value<V>)>;
 
-    fn first(&self) -> Value<K> {
-        let key = self.key.as_ref().expect("ub: dereference of end iterator");
-        Rc::new(RefCell::new(key.clone()))
-    }
-
-    fn second(&self) -> Value<V> {
+    fn deref(&self) -> Ptr<(Value<K>, Value<V>)> {
         let key = self.key.as_ref().expect("ub: dereference of end iterator");
         self.map
-            .with(|m| m.get(key).expect("ub: key not found in map").clone())
+            .with(|m| m.get(key).expect("ub: key not found in map").as_pointer())
     }
 }
 
 impl<K: Ord + Clone, V> MapIterator for UnsafeMapIterator<K, V> {
-    type First = *const K;
-    type Second = *mut V;
-
-    fn first(&self) -> *const K {
-        self.key.as_ref().expect("ub: dereference of end iterator") as *const K
-    }
+    type Entry = *mut (K, V);
 
-    fn second(&self) -> *mut V {
+    fn deref(&self) -> *mut (K, V) {
         unsafe {
-            let map_mut = self.map as *mut BTreeMap<K, Box<V>>;
+            let map_mut = self.map as *mut BTreeMap<K, Box<(K, V)>>;
             let key = self.key.as_ref().expect("ub: dereference of end iterator");
             (*map_mut)
                 .get_mut(key)
                 .expect("ub: key not found in map")
-                .as_mut() as *mut V
+                .as_mut() as *mut (K, V)
         }
     }
 }

rules/map/src.cpp

@@ -134,3 +134,14 @@ template <typename T1, typename T2>
 std::map<T1, T2> &f26(std::map<T1, T2> &dst, const std::map<T1, T2> &src) {
   return dst.operator=(src);
 }
+
+template <typename T1, typename T2>
+std::pair<const T1, T2> &f27(typename std::map<T1, T2>::iterator it) {
+  return it.operator*();
+}
+
+template <typename T1, typename T2>
+const std::pair<const T1, T2> &
+f28(typename std::map<T1, T2>::const_iterator it) {
+  return it.operator*();
+}