Improve error messages when dereferencing invalid swisstable iterators.

- Separate the failure cases into different assertions: end/default constructed vs rehashed or erased.
- Update the assertion error for AssertIsValid to not mention the end iterator case because end iterators are considered valid by AssertIsValid.
- Also fix an out-of-date comment for skip_empty_or_deleted.

PiperOrigin-RevId: 485402559
Change-Id: I593056abdc6c3565d0396fb885923fef643bf4e4

2 files changed, 64 insertions, 23 deletions

diff --git a/absl/container/internal/raw_hash_set.h b/absl/container/internal/raw_hash_set.h
index 93de2221..676cebd7 100644
--- a/absl/container/internal/raw_hash_set.h
+++ b/absl/container/internal/raw_hash_set.h
@@ -797,15 +797,22 @@ size_t SelectBucketCountForIterRange(InputIter first, InputIter last,
   return 0;
 }
 
-#define ABSL_INTERNAL_ASSERT_IS_FULL(ctrl, msg) \
-  ABSL_HARDENING_ASSERT((ctrl != nullptr && IsFull(*ctrl)) && msg)
+#define ABSL_INTERNAL_ASSERT_IS_FULL(ctrl, operation)                         \
+  do {                                                                        \
+    ABSL_HARDENING_ASSERT(                                                    \
+        (ctrl != nullptr) && operation                                        \
+        " called on invalid iterator. The iterator might be an end() "        \
+        "iterator or may have been default constructed.");                    \
+    ABSL_HARDENING_ASSERT(                                                    \
+        (IsFull(*ctrl)) && operation                                          \
+        " called on invalid iterator. The element might have been erased or " \
+        "the table might have rehashed.");                                    \
+  } while (0)
 
 inline void AssertIsValid(ctrl_t* ctrl) {
-  ABSL_HARDENING_ASSERT(
-      (ctrl == nullptr || IsFull(*ctrl)) &&
-      "Invalid operation on iterator. The element might have "
-      "been erased, the table might have rehashed, or this may "
-      "be an end() iterator.");
+  ABSL_HARDENING_ASSERT((ctrl == nullptr || IsFull(*ctrl)) &&
+                        "Invalid operation on iterator. The element might have "
+                        "been erased or the table might have rehashed.");
 }
 
 struct FindInfo {
@@ -1034,22 +1041,19 @@ class raw_hash_set {
 
     // PRECONDITION: not an end() iterator.
     reference operator*() const {
-      ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_,
-                                   "operator*() called on invalid iterator.");
+      ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_, "operator*()");
       return PolicyTraits::element(slot_);
     }
 
     // PRECONDITION: not an end() iterator.
     pointer operator->() const {
-      ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_,
-                                   "operator-> called on invalid iterator.");
+      ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_, "operator->");
       return &operator*();
     }
 
     // PRECONDITION: not an end() iterator.
     iterator& operator++() {
-      ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_,
-                                   "operator++ called on invalid iterator.");
+      ABSL_INTERNAL_ASSERT_IS_FULL(ctrl_, "operator++");
       ++ctrl_;
       ++slot_;
       skip_empty_or_deleted();
@@ -1081,7 +1085,7 @@ class raw_hash_set {
     // Fixes up `ctrl_` to point to a full by advancing it and `slot_` until
     // they reach one.
     //
-    // If a sentinel is reached, we null both of them out instead.
+    // If a sentinel is reached, we null `ctrl_` out instead.
     void skip_empty_or_deleted() {
       while (IsEmptyOrDeleted(*ctrl_)) {
         uint32_t shift = Group{ctrl_}.CountLeadingEmptyOrDeleted();
@@ -1601,8 +1605,7 @@ class raw_hash_set {
   // This overload is necessary because otherwise erase<K>(const K&) would be
   // a better match if non-const iterator is passed as an argument.
   void erase(iterator it) {
-    ABSL_INTERNAL_ASSERT_IS_FULL(it.ctrl_,
-                                 "erase() called on invalid iterator.");
+    ABSL_INTERNAL_ASSERT_IS_FULL(it.ctrl_, "erase()");
     PolicyTraits::destroy(&alloc_ref(), it.slot_);
     erase_meta_only(it);
   }
@@ -1636,8 +1639,7 @@ class raw_hash_set {
   }
 
   node_type extract(const_iterator position) {
-    ABSL_INTERNAL_ASSERT_IS_FULL(position.inner_.ctrl_,
-                                 "extract() called on invalid iterator.");
+    ABSL_INTERNAL_ASSERT_IS_FULL(position.inner_.ctrl_, "extract()");
     auto node =
         CommonAccess::Transfer<node_type>(alloc_ref(), position.inner_.slot_);
     erase_meta_only(position);
diff --git a/absl/container/internal/raw_hash_set_test.cc b/absl/container/internal/raw_hash_set_test.cc
index eec9da43..6478d3fc 100644
--- a/absl/container/internal/raw_hash_set_test.cc
+++ b/absl/container/internal/raw_hash_set_test.cc
@@ -2036,20 +2036,59 @@ TEST(Table, UnstablePointers) {
   EXPECT_NE(old_ptr, addr(0));
 }
 
-// Confirm that we assert if we try to erase() end().
-TEST(TableDeathTest, EraseOfEndAsserts) {
+bool IsAssertEnabled() {
   // Use an assert with side-effects to figure out if they are actually enabled.
   bool assert_enabled = false;
   assert([&]() {  // NOLINT
     assert_enabled = true;
     return true;
   }());
-  if (!assert_enabled) return;
+  return assert_enabled;
+}
+
+TEST(TableDeathTest, InvalidIteratorAsserts) {
+  if (!IsAssertEnabled()) GTEST_SKIP() << "Assertions not enabled.";
+
+  IntTable t;
+  // Extra simple "regexp" as regexp support is highly varied across platforms.
+  EXPECT_DEATH_IF_SUPPORTED(
+      t.erase(t.end()),
+      "erase.* called on invalid iterator. The iterator might be an "
+      "end.*iterator or may have been default constructed.");
+  typename IntTable::iterator iter;
+  EXPECT_DEATH_IF_SUPPORTED(
+      ++iter,
+      "operator.* called on invalid iterator. The iterator might be an "
+      "end.*iterator or may have been default constructed.");
+  t.insert(0);
+  iter = t.begin();
+  t.erase(iter);
+  EXPECT_DEATH_IF_SUPPORTED(
+      ++iter,
+      "operator.* called on invalid iterator. The element might have been "
+      "erased or .*the table might have rehashed.");
+}
+
+TEST(TableDeathTest, IteratorInvalidAssertsEqualityOperator) {
+  if (!IsAssertEnabled()) GTEST_SKIP() << "Assertions not enabled.";
 
   IntTable t;
+  t.insert(1);
+  t.insert(2);
+  t.insert(3);
+  auto iter1 = t.begin();
+  auto iter2 = std::next(iter1);
+  ASSERT_NE(iter1, t.end());
+  ASSERT_NE(iter2, t.end());
+  t.erase(iter1);
   // Extra simple "regexp" as regexp support is highly varied across platforms.
-  constexpr char kDeathMsg[] = "erase.. called on invalid iterator";
-  EXPECT_DEATH_IF_SUPPORTED(t.erase(t.end()), kDeathMsg);
+  const char* const kDeathMessage =
+      "Invalid operation on iterator. The element might have .*been erased or "
+      "the table might have rehashed.";
+  EXPECT_DEATH_IF_SUPPORTED(void(iter1 == iter2), kDeathMessage);
+  EXPECT_DEATH_IF_SUPPORTED(void(iter2 != iter1), kDeathMessage);
+  t.erase(iter2);
+  EXPECT_DEATH_IF_SUPPORTED(void(iter1 == iter2), kDeathMessage);
 }
 
 #if defined(ABSL_INTERNAL_HASHTABLEZ_SAMPLE)