Add HasId method to ValueStore for debug checks

[clavin]

Small addition to ValueStore to check if an ID can be safely used to access values from the store. This change iterates on the DCHECK bounds check that currently exists in ValueStore::Get.

When working with multiple ValueStores, IDs can become mixed up and misused to access the wrong ValueStore. This always indicates a bug in the code, so this method is added for the convenience of debug checks to identify bugs.

In many situations there is only one ValueStore instance available with a unique ID type, so it's guaranteed that those IDs came from ValueStore::Add; thus, this check is usually unnecessary. To deter overly-cautious code, this method is documented as a debug tool.

I plan to use this method in a follow-up PR to harden Dump functions against out-of-bounds IDs.

[jonmeow]

What's it look like if you just check against size() in debug code?

[clavin]

@jonmeow I was writing size() checks in Dump functions, but found it to be a little noisy when working with ValueStore. Here's an example of my first sketch:

toolchain/check/dump.cpp:48-55:

+   if (static_cast<size_t>(loc_id.import_ir_inst_id().index) >=
+        context.sem_ir().import_ir_insts().size()) {
+      llvm::errs() << "LocId(<out of bounds import instruction>)";
+      return;
+    }
     auto import_ir_id = context.sem_ir()
                             .import_ir_insts()
                             .Get(loc_id.import_ir_inst_id())
                             .ir_id;
 
+    if (static_cast<size_t>(import_ir_id.index) >=
+        context.sem_ir().import_irs().size()) {
+      llvm::errs() << "LocId(<out of bounds import file>)";
+      return;
+    }
     const auto* import_file =
         context.sem_ir().import_irs().Get(import_ir_id).sem_ir;
     llvm::errs() << "LocId(import from \""
                  << FormatEscaped(import_file->filename()) << "\")";

For Lex & Parse, each having only one small Dump function, checking against size() is relatively quiet and works well.

Looking now at toolchain/sem_ir/dump.cpp, I realize that many ValueStore types there are wrappers. 😅 So HasId won't be as widely available as I originally thought.

[clavin]

One alternative would be a template function like MemUsage::Collect for types that have an array_ref() or size() member. What do you think?

[jonmeow]

I'm not sure whether this is what you're thinking about with the Collect example, but how about:

• Add support to ErrorOr to support printing if T is printable.
• Add a SafeGet helper somewhere like toolchain/base/dump.h (i.e., separating the dump-specific functions from regular code) that looks something like template<ValueStoreT, IdT> auto SafeGet(ValueStoreT value_store, IdT id) -> ErrorOr<ValueStoreT::ConstRefType> [if any value store-ish things are missing size, I think it'd be fair to add)

Note, I'm suggesting this to allow changes like:

static auto DumpNoNewline(const File& file, InstId inst_id) -> void {
  llvm::errs() << inst_id;
  if (inst_id.has_value()) {
-     llvm::errs() << ": " << file.insts().Get(inst_id);
+     llvm::errs() << ": " << SafeGet(file.insts(), inst_id);
  }
}

The example you're looking at with LocId is a little more complex and would need to examine the ErrorOr, but for the SemIR case it should be easier to replace.

[jonmeow]

Tiny sidenote, SmallVector has a assert(idx < size());, which already covers what's changing here.

[jonmeow]

I'm not sure whether this is what you're thinking about with the Collect example, but how about:

• Add support to ErrorOr to support printing if T is printable.
• Add a SafeGet helper somewhere like toolchain/base/dump.h (i.e., separating the dump-specific functions from regular code) that looks something like template<ValueStoreT, IdT> auto SafeGet(ValueStoreT value_store, IdT id) -> ErrorOr<ValueStoreT::ConstRefType> [if any value store-ish things are missing size, I think it'd be fair to add)

Note, I'm suggesting this to allow changes like:

static auto DumpNoNewline(const File& file, InstId inst_id) -> void {
  llvm::errs() << inst_id;
  if (inst_id.has_value()) {
-     llvm::errs() << ": " << file.insts().Get(inst_id);
+     llvm::errs() << ": " << SafeGet(file.insts(), inst_id);
  }
}

The example you're looking at with LocId is a little more complex and would need to examine the ErrorOr, but for the SemIR case it should be easier to replace.