Stop passing a lambda as a non-type template argument. (#6229)

This creates ODR issues, as the "same" value store type in different
translation units can end up being treated as different types. In some
build configurations, such as `-c dbg` with Clang 19.1, this is
currently resulting in link-time errors.

Instead, make the customization mechanism for mapping keys to values be
a member function on the value type.

Author: zygoloid

toolchain/base/canonical_value_store.h

@@ -20,15 +20,9 @@ namespace Carbon {
 // `ValueT` represents the type being stored.
 //
 // `KeyT` can optionally be different from `ValueT`, and if so is used for the
-// argument to `Lookup`. It must be valid to use both `KeyT` and `ValueT` as
-// lookup types in the underlying `Set`.
-template <typename IdT, typename KeyT, typename ValueT = KeyT,
-          // Parentheses around the lambda to help clang-format.
-          auto ValueToKeyFn =
-              ([](typename ValueStoreTypes<ValueT>::ConstRefType value) ->
-               typename ValueStoreTypes<ValueT>::ConstRefType {
-                 return value;
-               })>
+// argument to `Lookup`. In this case, `ValueT` must provide a `GetAsKey` member
+// function that returns the corresponding key.
+template <typename IdT, typename KeyT, typename ValueT = KeyT>
 class CanonicalValueStore {
  public:
   using KeyType = std::remove_cvref_t<KeyT>;
@@ -79,47 +73,55 @@ class CanonicalValueStore {
  private:
   class KeyContext;
 
+  static auto GetAsKey(ConstRefType value) -> ConstRefType
+    requires std::same_as<KeyT, ValueT>
+  {
+    return value;
+  }
+
+  template <typename T>
+  static auto GetAsKey(T&& value) -> decltype(value.GetAsKey()) {
+    return value.GetAsKey();
+  }
+
   ValueStore<IdT, ValueType> values_;
   Set<IdT, /*SmallSize=*/0, KeyContext> set_;
 };
 
-template <typename IdT, typename KeyT, typename ValueT, auto ValueToKeyFn>
-class CanonicalValueStore<IdT, KeyT, ValueT, ValueToKeyFn>::KeyContext
+template <typename IdT, typename KeyT, typename ValueT>
+class CanonicalValueStore<IdT, KeyT, ValueT>::KeyContext
     : public TranslatingKeyContext<KeyContext> {
  public:
   explicit KeyContext(const ValueStore<IdT, ValueType>* values)
       : values_(values) {}
 
-  // Note that it is safe to return a `const` reference here as the underlying
-  // object's lifetime is provided by the `ValueStore`.
+  // Note that it is safe to return a reference here as the underlying object's
+  // lifetime is provided by the `ValueStore`.
   auto TranslateKey(IdT id) const
-      -> llvm::function_traits<decltype(ValueToKeyFn)>::result_t {
-    return ValueToKeyFn(values_->Get(id));
+      -> decltype(GetAsKey(std::declval<ConstRefType>())) {
+    return GetAsKey(values_->Get(id));
   }
 
  private:
   const ValueStore<IdT, ValueType>* values_;
 };
 
-template <typename IdT, typename KeyT, typename ValueT, auto ValueToKeyFn>
-auto CanonicalValueStore<IdT, KeyT, ValueT, ValueToKeyFn>::Add(ValueType value)
-    -> IdT {
+template <typename IdT, typename KeyT, typename ValueT>
+auto CanonicalValueStore<IdT, KeyT, ValueT>::Add(ValueType value) -> IdT {
   auto make_key = [&] { return IdT(values_.Add(std::move(value))); };
-  return set_.Insert(ValueToKeyFn(value), make_key, KeyContext(&values_)).key();
+  return set_.Insert(GetAsKey(value), make_key, KeyContext(&values_)).key();
 }
 
-template <typename IdT, typename KeyT, typename ValueT, auto ValueToKeyFn>
-auto CanonicalValueStore<IdT, KeyT, ValueT, ValueToKeyFn>::Lookup(
-    KeyType key) const -> IdT {
+template <typename IdT, typename KeyT, typename ValueT>
+auto CanonicalValueStore<IdT, KeyT, ValueT>::Lookup(KeyType key) const -> IdT {
   if (auto result = set_.Lookup(key, KeyContext(&values_))) {
     return result.key();
   }
   return IdT::None;
 }
 
-template <typename IdT, typename KeyT, typename ValueT, auto ValueToKeyFn>
-auto CanonicalValueStore<IdT, KeyT, ValueT, ValueToKeyFn>::Reserve(size_t size)
-    -> void {
+template <typename IdT, typename KeyT, typename ValueT>
+auto CanonicalValueStore<IdT, KeyT, ValueT>::Reserve(size_t size) -> void {
   // Compute the resulting new insert count using the size of values -- the
   // set doesn't have a fast to compute current size.
   if (size > values_.size()) {

toolchain/sem_ir/clang_decl.h

@@ -90,6 +90,8 @@ struct ClangDecl : public Printable<ClangDecl> {
 
   // The instruction the Clang declaration is mapped to.
   InstId inst_id;
+
+  auto GetAsKey() const -> ClangDeclKey { return key; }
 };
 
 // The ID of a `ClangDecl`.
@@ -108,10 +110,7 @@ struct ClangDeclId : public IdBase<ClangDeclId> {
 
 // Use the AST node pointer directly when doing `Lookup` to find an ID.
 using ClangDeclStore =
-    CanonicalValueStore<ClangDeclId, ClangDeclKey, ClangDecl,
-                        [](const ClangDecl& value) -> const ClangDeclKey& {
-                          return value.key;
-                        }>;
+    CanonicalValueStore<ClangDeclId, ClangDeclKey, ClangDecl>;
 
 }  // namespace Carbon::SemIR
 

toolchain/sem_ir/cpp_global_var.h

@@ -44,13 +44,13 @@ struct CppGlobalVar : public Printable<CppGlobalVar> {
   // given key, in order to store it in `CanonicalValueStore` and allow lookup
   // by `CppGlobalVarKey`.
   ClangDeclId clang_decl_id;
+
+  auto GetAsKey() const -> CppGlobalVarKey { return key; }
 };
 
 // Use the name of a C++ global variable when doing `Lookup` to find an ID.
 using CppGlobalVarStore =
-    CanonicalValueStore<CppGlobalVarId, CppGlobalVarKey, CppGlobalVar,
-                        [](const CppGlobalVar& value)
-                            -> const CppGlobalVarKey& { return value.key; }>;
+    CanonicalValueStore<CppGlobalVarId, CppGlobalVarKey, CppGlobalVar>;
 
 }  // namespace Carbon::SemIR