From 7a400d22b471c323aeb4e274def32f2674882a74 Mon Sep 17 00:00:00 2001
From: David Blaikie <dblaikie@gmail.com>
Date: Wed, 19 Nov 2025 10:20:49 -0800
Subject: [PATCH] Improve CHECK-failure when passing a negative id to a
 ValueStore #6370 (#6392)

Otherwise the value fails in confusing ways while untagging:

  CHECK failure at ./toolchain/base/value_store.h:71:
  index >= initial_reserved_ids_: When removing tagging bits,
  found an index that shouldn't've been tagged in the first place.

With this change:

  CHECK failure at ./toolchain/base/fixed_size_value_store.h:112:
  id.index >= 0: instFFFFFFFFFFFFFFFD
---
 toolchain/base/fixed_size_value_store.h |  6 +++---
 toolchain/base/relational_value_store.h |  2 +-
 toolchain/base/value_store.h            | 10 +++++++---
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/toolchain/base/fixed_size_value_store.h b/toolchain/base/fixed_size_value_store.h
index d357b15144bc..62a7c75268d4 100644
--- a/toolchain/base/fixed_size_value_store.h
+++ b/toolchain/base/fixed_size_value_store.h
@@ -102,22 +102,22 @@ class FixedSizeValueStore {
 
   // Sets the value for an ID.
   auto Set(IdT id, ValueType value) -> void {
+    CARBON_DCHECK(id.index >= 0, "{0}", id);
     auto index = tag_.Remove(id.index);
-    CARBON_DCHECK(index >= 0, "{0}", id);
     values_[index] = value;
   }
 
   // Returns a mutable value for an ID.
   auto Get(IdT id) -> RefType {
+    CARBON_DCHECK(id.index >= 0, "{0}", id);
     auto index = tag_.Remove(id.index);
-    CARBON_DCHECK(index >= 0, "{0}", id);
     return values_[index];
   }
 
   // Returns the value for an ID.
   auto Get(IdT id) const -> ConstRefType {
+    CARBON_DCHECK(id.index >= 0, "{0}", id);
     auto index = tag_.Remove(id.index);
-    CARBON_DCHECK(index >= 0, "{0}", id);
     return values_[index];
   }
 
diff --git a/toolchain/base/relational_value_store.h b/toolchain/base/relational_value_store.h
index 8d9660954800..e3279cc3caa1 100644
--- a/toolchain/base/relational_value_store.h
+++ b/toolchain/base/relational_value_store.h
@@ -72,8 +72,8 @@ class RelationalValueStore {
 
   // Returns a value for an ID.
   auto Get(IdT id) const -> ConstRefType {
+    CARBON_DCHECK(id.index >= 0, "{0}", id);
     auto index = related_store_->GetIdTag().Remove(id.index);
-    CARBON_DCHECK(index >= 0, "{0}", id);
     return *values_[index];
   }
 
diff --git a/toolchain/base/value_store.h b/toolchain/base/value_store.h
index ee80934d9e24..55412fb65f94 100644
--- a/toolchain/base/value_store.h
+++ b/toolchain/base/value_store.h
@@ -51,14 +51,18 @@ struct IdTag {
   }
 
   auto Apply(int32_t index) const -> int32_t {
+    CARBON_DCHECK(index >= 0, "{0}", index);
     if (index < initial_reserved_ids_) {
       return index;
     }
     // TODO: Assert that id_tag_ doesn't have the second highest bit set.
-    return index ^ id_tag_;
+    auto tagged_index = index ^ id_tag_;
+    CARBON_DCHECK(tagged_index >= 0, "{0}", tagged_index);
+    return tagged_index;
   }
 
   auto Remove(int32_t tagged_index) const -> int32_t {
+    CARBON_DCHECK(tagged_index >= 0, "{0}", tagged_index);
     if (!HasTag(tagged_index)) {
       CARBON_DCHECK(tagged_index < initial_reserved_ids_,
                     "This untagged index is outside the initial reserved ids "
@@ -226,11 +230,11 @@ class ValueStore
   auto GetWithDefault(IdType id,  //
                       ConstRefType default_value [[clang::lifetimebound]]) const
       -> ConstRefType {
+    CARBON_DCHECK(id.index >= 0, "{0}", id);
     auto index = tag_.Remove(id.index);
     if (index >= size_) {
       return default_value;
     }
-    CARBON_DCHECK(index >= 0, "{0}", id);
     auto [chunk_index, pos] = RawIndexToChunkIndices(index);
     return chunks_[chunk_index].Get(pos);
   }
@@ -325,8 +329,8 @@ class ValueStore
 
   auto GetIdTag() const -> IdTag { return tag_; }
   auto GetRawIndex(IdT id) const -> int32_t {
+    CARBON_DCHECK(id.index >= 0, "{0}", index);
     auto index = tag_.Remove(id.index);
-    CARBON_DCHECK(index >= 0, "{0}", index);
 #ifndef NDEBUG
     if (index >= size_) {
       // Attempt to decompose id.index to include extra detail in the check