Improve ClangDeclStore ergonomics (#7311)

Change `Lookup` by InstId to return a ClangDecl pointer. All callers
were immediately calling `Get` anyway, so this makes call sites a little
shorter. The other `Lookup` method, by ClangDeclKey, is sometimes called
without calling `Get`, so left that as-is, but renamed to `LookupId`.

Also add a `decl` method to ClangDecl so that the commonly repeated
`clang_decl->key.decl` can be written `clang_decl->decl()`.

toolchain/check/cpp/call.cpp

@@ -140,7 +140,7 @@ static auto ConvertArgToTemplateArg(
             context.types().TryGetAs<SemIR::CppTemplateNameType>(
                 inst.type_id())) {
       clang::TemplateName name(cast<clang::TemplateDecl>(
-          context.clang_decls().Get(template_name_type->decl_id).key.decl));
+          context.clang_decls().Get(template_name_type->decl_id).decl()));
       return clang::TemplateArgumentLoc(
           context.ast_context(), clang::TemplateArgument(name),
           /*TemplateKWLoc=*/clang::SourceLocation(),
@@ -346,7 +346,7 @@ auto PerformCallToCppTemplateName(Context& context, SemIR::LocId loc_id,
                                   llvm::ArrayRef<SemIR::InstId> arg_ids)
     -> SemIR::InstId {
   auto* template_decl = dyn_cast<clang::TemplateDecl>(
-      context.clang_decls().Get(template_decl_id).key.decl);
+      context.clang_decls().Get(template_decl_id).decl());
   auto loc = GetCppLocation(context, loc_id);
 
   // Form a template argument list for this template.

toolchain/check/cpp/constant.cpp

@@ -219,13 +219,11 @@ static auto ConvertArgToExpr(Context& context, SemIR::InstId arg_inst_id,
 }
 
 auto EvalCppCall(Context& context, SemIR::LocId loc_id,
-                 SemIR::ClangDeclId clang_decl_id, SemIR::InstBlockId args_id)
+                 const SemIR::ClangDecl& clang_decl, SemIR::InstBlockId args_id)
     -> SemIR::ConstantId {
   const auto& args = context.inst_blocks().Get(args_id);
 
-  auto* decl = context.clang_decls().Get(clang_decl_id).GetAsKey().decl;
-
-  auto* function_decl = cast<clang::FunctionDecl>(decl);
+  auto* function_decl = cast<clang::FunctionDecl>(clang_decl.decl());
 
   // Create expr for the function declaration.
   auto* decl_ref_expr = clang::DeclRefExpr::Create(
@@ -305,10 +303,8 @@ auto MaybeModifyCppThunkCallForConstEval(Context& context, SemIR::Call* call)
 
     function_decl = cast<clang::FunctionDecl>(
         context.clang_decls()
-            .Get(context.clang_decls().Lookup(
-                thunk_callee_function.first_decl_id()))
-            .GetAsKey()
-            .decl);
+            .Lookup(thunk_callee_function.first_decl_id())
+            ->decl());
 
     if (!(function_decl->isConstexpr() || function_decl->isConsteval())) {
       return;

toolchain/check/cpp/constant.h

@@ -31,7 +31,7 @@ auto EvalCppVarDecl(Context& context, SemIR::LocId loc_id,
 // Attempt to evaluate a call to a C++ constexpr/consteval function as a
 // Carbon constant.
 auto EvalCppCall(Context& context, SemIR::LocId loc_id,
-                 SemIR::ClangDeclId clang_decl_id, SemIR::InstBlockId args_id)
+                 const SemIR::ClangDecl& clang_decl, SemIR::InstBlockId args_id)
     -> SemIR::ConstantId;
 
 // If the callee is a C++ thunk, modify `call` to directly call the

toolchain/check/cpp/export.cpp

@@ -38,7 +38,7 @@ static auto GetClangDeclContextForScope(Context& context,
   if (!clang_decl_context_id.has_value()) {
     return nullptr;
   }
-  auto* decl = context.clang_decls().Get(clang_decl_context_id).key.decl;
+  auto* decl = context.clang_decls().Get(clang_decl_context_id).decl();
   return cast<clang::DeclContext>(decl);
 }
 
@@ -137,11 +137,9 @@ auto ExportClassToCpp(Context& context, SemIR::LocId loc_id,
   // If this class was produced by importing a C++ declaration or has
   // already been exported to C++, return the corresponding Clang declaration.
   // That could either be a CXXRecordDecl or an EnumDecl.
-  if (auto clang_decl_id =
-          context.clang_decls().Lookup(class_info.first_decl_id());
-      clang_decl_id.has_value()) {
-    return cast<clang::TagDecl>(
-        context.clang_decls().Get(clang_decl_id).key.decl);
+  if (const auto* clang_decl =
+          context.clang_decls().Lookup(class_info.first_decl_id())) {
+    return cast<clang::TagDecl>(clang_decl->decl());
   }
 
   auto* identifier_info = GetClangIdentifierInfo(context, class_info.name_id);
@@ -308,12 +306,10 @@ auto ExportFieldToCpp(Context& context, SemIR::InstId field_inst_id,
   ExportAllFieldsToCpp(context, class_info);
 
   // Get the exported `clang::FieldDecl`.
-  auto clang_decl_id = context.clang_decls().Lookup(field_inst_id);
-  if (clang_decl_id == SemIR::ClangDeclId::None) {
-    return nullptr;
+  if (const auto* clang_decl = context.clang_decls().Lookup(field_inst_id)) {
+    return cast<clang::FieldDecl>(clang_decl->decl());
   }
-  return cast<clang::FieldDecl>(
-      context.clang_decls().Get(clang_decl_id).key.decl);
+  return nullptr;
 }
 
 auto CalculateCppFieldOffsets(
@@ -957,10 +953,9 @@ auto ExportVarToCpp(Context& context, SemIR::InstId inst_id,
   // Check if the variable was already exported and return the existing
   // `VarDecl` if so. Note that the `pattern_id` is used as the key
   // rather than the `InstId` for the `VarStorage`.
-  auto clang_decl_id = context.clang_decls().Lookup(var_storage.pattern_id);
-  if (clang_decl_id.has_value()) {
-    return cast<clang::VarDecl>(
-        context.clang_decls().Get(clang_decl_id).key.decl);
+  if (const auto* clang_decl =
+          context.clang_decls().Lookup(var_storage.pattern_id)) {
+    return cast<clang::VarDecl>(clang_decl->decl());
   }
 
   // Look up the entity name and check the scope.

toolchain/check/cpp/generate_ast.cpp

@@ -447,10 +447,9 @@ auto CarbonExternalASTSource::GetOrExportFunctionToCpp(
     SemIR::InstId target_inst_id, SemIR::FunctionId function_id)
     -> clang::FunctionDecl* {
   SemIR::Function& function = context_->functions().Get(function_id);
-  auto clang_decl_id = context_->clang_decls().Lookup(function.first_decl_id());
-  if (clang_decl_id.has_value()) {
-    return cast<clang::FunctionDecl>(
-        context_->clang_decls().Get(clang_decl_id).key.decl);
+  if (const auto* clang_decl =
+          context_->clang_decls().Lookup(function.first_decl_id())) {
+    return cast<clang::FunctionDecl>(clang_decl->decl());
   }
 
   auto* clang_function_decl =
@@ -510,7 +509,7 @@ auto CarbonExternalASTSource::FindExternalVisibleDeclsByName(
   auto* decl = cast<clang::Decl>(
       const_cast<clang::DeclContext*>(decl_context->getPrimaryContext()));
   auto key = SemIR::ClangDeclKey::ForNonFunctionDecl(decl);
-  auto decl_id = context_->clang_decls().Lookup(key);
+  auto decl_id = context_->clang_decls().LookupId(key);
   CARBON_CHECK(
       decl_id.has_value(),
       "The DeclContext should already be associated with a Carbon InstId.");
@@ -591,7 +590,7 @@ static auto GetAsCarbonOwnedClass(Context& context,
 
   auto key = SemIR::ClangDeclKey::ForNonFunctionDecl(
       const_cast<clang::TagDecl*>(tag_decl->getFirstDecl()));
-  auto clang_decl_id = context.clang_decls().Lookup(key);
+  auto clang_decl_id = context.clang_decls().LookupId(key);
   if (!clang_decl_id.has_value()) {
     return std::nullopt;
   }

toolchain/check/cpp/impl_lookup.cpp

@@ -64,7 +64,7 @@ static auto TypeAsTagDecl(Context& context,
     return nullptr;
   }
 
-  return dyn_cast<clang::TagDecl>(context.clang_decls().Get(decl_id).key.decl);
+  return dyn_cast<clang::TagDecl>(context.clang_decls().Get(decl_id).decl());
 }
 
 // If the given type is a C++ class type, returns the corresponding class

toolchain/check/cpp/import.cpp

@@ -378,7 +378,7 @@ static auto GetDeclContext(Context& context, SemIR::NameScopeId scope_id)
   auto scope_clang_decl_context_id =
       context.name_scopes().Get(scope_id).clang_decl_context_id();
   return dyn_cast<clang::DeclContext>(
-      context.clang_decls().Get(scope_clang_decl_context_id).key.decl);
+      context.clang_decls().Get(scope_clang_decl_context_id).decl());
 }
 
 // Returns true if the given Clang declaration is the implicit injected class
@@ -399,8 +399,7 @@ static auto IsDeclInjectedClassName(Context& context,
 
   const SemIR::ClangDecl& clang_decl = context.clang_decls().Get(
       context.name_scopes().Get(scope_id).clang_decl_context_id());
-  const auto* scope_record_decl =
-      cast<clang::CXXRecordDecl>(clang_decl.key.decl);
+  const auto* scope_record_decl = cast<clang::CXXRecordDecl>(clang_decl.decl());
 
   const clang::ASTContext& ast_context = context.ast_context();
   CARBON_CHECK(ast_context.getCanonicalTagType(scope_record_decl) ==
@@ -441,7 +440,7 @@ static auto ClangLookupName(Context& context, SemIR::NameScopeId scope_id,
 // Returns whether `decl` already mapped to an instruction.
 static auto IsClangDeclImported(Context& context, SemIR::ClangDeclKey key)
     -> bool {
-  return context.clang_decls().Lookup(key).has_value();
+  return context.clang_decls().LookupId(key).has_value();
 }
 
 // If `decl` already mapped to an instruction, returns that instruction.
@@ -449,7 +448,7 @@ static auto IsClangDeclImported(Context& context, SemIR::ClangDeclKey key)
 static auto LookupClangDeclInstId(Context& context, SemIR::ClangDeclKey key)
     -> SemIR::InstId {
   const auto& clang_decls = context.clang_decls();
-  if (auto context_clang_decl_id = clang_decls.Lookup(key);
+  if (auto context_clang_decl_id = clang_decls.LookupId(key);
       context_clang_decl_id.has_value()) {
     return clang_decls.Get(context_clang_decl_id).inst_id;
   }
@@ -2602,7 +2601,7 @@ auto ImportClassDefinitionForClangDecl(Context& context,
   CARBON_CHECK(cpp_file);
 
   auto* clang_decl =
-      cast<clang::TagDecl>(context.clang_decls().Get(clang_decl_id).key.decl);
+      cast<clang::TagDecl>(context.clang_decls().Get(clang_decl_id).decl());
   auto class_inst_id = context.types().GetAsTypeInstId(
       context.classes().Get(class_id).first_owning_decl_id);
 
@@ -2637,10 +2636,9 @@ auto GetAsClangVarDecl(Context& context, SemIR::InstId inst_id)
     -> clang::VarDecl* {
   if (const auto& var_storage =
           context.insts().TryGetAs<SemIR::VarStorage>(inst_id)) {
-    auto clang_decl_id = context.clang_decls().Lookup(var_storage->pattern_id);
-    if (clang_decl_id.has_value()) {
-      return cast<clang::VarDecl>(
-          context.clang_decls().Get(clang_decl_id).key.decl);
+    if (const auto* clang_decl =
+            context.clang_decls().Lookup(var_storage->pattern_id)) {
+      return cast<clang::VarDecl>(clang_decl->decl());
     }
   }
 

toolchain/check/cpp/operators.cpp

@@ -732,12 +732,10 @@ static auto GetAsCppFunctionDecl(Context& context, SemIR::InstId inst_id)
   if (!function_type) {
     return nullptr;
   }
-  SemIR::ClangDeclId clang_decl_id = context.clang_decls().Lookup(
+  const auto* clang_decl = context.clang_decls().Lookup(
       context.functions().Get(function_type->function_id).first_decl_id());
-  return clang_decl_id.has_value()
-             ? dyn_cast<clang::FunctionDecl>(
-                   context.clang_decls().Get(clang_decl_id).key.decl)
-             : nullptr;
+  return clang_decl ? dyn_cast<clang::FunctionDecl>(clang_decl->decl())
+                    : nullptr;
 }
 
 auto IsCppOperatorMethod(Context& context, SemIR::InstId inst_id) -> bool {

toolchain/check/cpp/thunk.cpp

@@ -245,20 +245,19 @@ struct CalleeFunctionInfo {
 
 auto IsCppThunkRequired(Context& context, const SemIR::Function& function)
     -> bool {
-  auto clang_decl_id = context.clang_decls().Lookup(function.first_decl_id());
-  if (!clang_decl_id.has_value()) {
+  const auto* clang_decl =
+      context.clang_decls().Lookup(function.first_decl_id());
+  if (!clang_decl) {
     return false;
   }
 
-  const auto& decl_info = context.clang_decls().Get(clang_decl_id);
-
-  if (!decl_info.is_imported) {
+  if (!clang_decl->is_imported) {
     return false;
   }
 
   const auto& signature =
-      context.clang_decl_signatures().Get(decl_info.key.signature_id);
-  auto* decl = cast<clang::FunctionDecl>(decl_info.key.decl);
+      context.clang_decl_signatures().Get(clang_decl->key.signature_id);
+  auto* decl = cast<clang::FunctionDecl>(clang_decl->decl());
   if (signature.kind != SemIR::ClangDeclSignature::Normal ||
       signature.num_params != static_cast<int>(decl->getNumNonObjectParams())) {
     // We require a thunk if the number of parameters we want isn't all of them.
@@ -652,9 +651,7 @@ static auto BuildThunkBody(CppContext& cpp_context, clang::Sema& sema,
 auto BuildCppThunk(Context& context, const SemIR::Function& callee_function)
     -> clang::FunctionDecl* {
   auto clang_decl_key =
-      context.clang_decls()
-          .Get(context.clang_decls().Lookup(callee_function.first_decl_id()))
-          .key;
+      context.clang_decls().Lookup(callee_function.first_decl_id())->key;
   clang::FunctionDecl* callee_function_decl =
       clang_decl_key.decl->getAsFunction();
   CARBON_CHECK(callee_function_decl);

toolchain/check/eval.cpp

@@ -3452,13 +3452,10 @@ static auto TryEvalCall(EvalContext& outer_eval_context, SemIR::LocId loc_id,
                         const SemIR::Function& function,
                         SemIR::SpecificId specific_id,
                         SemIR::InstBlockId args_id) -> SemIR::ConstantId {
-  auto clang_decl_id = outer_eval_context.sem_ir().clang_decls().Lookup(
+  const auto* clang_decl = outer_eval_context.sem_ir().clang_decls().Lookup(
       function.first_decl_id());
-  if (clang_decl_id.has_value() && outer_eval_context.sem_ir()
-                                       .clang_decls()
-                                       .Get(clang_decl_id)
-                                       .is_imported) {
-    return EvalCppCall(outer_eval_context.context(), loc_id, clang_decl_id,
+  if (clang_decl && clang_decl->is_imported) {
+    return EvalCppCall(outer_eval_context.context(), loc_id, *clang_decl,
                        args_id);
   } else if (function.body_block_ids.empty()) {
     // TODO: Diagnose this.

toolchain/lower/file_context.cpp

@@ -351,15 +351,12 @@ auto FileContext::GetOrCreateLLVMFunction(
   // decl_id so it doesn't fall out naturally from the handling below)
   if (function_id != sem_ir().global_ctor_id()) {
     const auto& function = sem_ir().functions().Get(function_id);
-    auto clang_decl_id =
-        sem_ir().clang_decls().Lookup(function.first_decl_id());
-    if (clang_decl_id.has_value()) {
-      const auto& clang_decl = sem_ir().clang_decls().Get(clang_decl_id);
-      if (clang_decl.is_imported) {
+    if (const auto* clang_decl =
+            sem_ir().clang_decls().Lookup(function.first_decl_id())) {
+      if (clang_decl->is_imported) {
         CARBON_CHECK(!specific_id.has_value(),
                      "Specific functions cannot have C++ definitions");
-        return HandleReferencedCppFunction(
-            clang_decl.key.decl->getAsFunction());
+        return HandleReferencedCppFunction(clang_decl->decl()->getAsFunction());
       }
     }
   }
@@ -720,11 +717,10 @@ auto FileContext::BuildGlobalVariableDecl(SemIR::VarStorage var_storage)
   // happens when a Carbon variable is exported and used from C++; code
   // generation for the C++ code may have already created an
   // llvm::GlobalVariable.
-  auto clang_decl_id = sem_ir().clang_decls().Lookup(var_storage.pattern_id);
-  if (clang_decl_id.has_value()) {
-    auto* decl = sem_ir().clang_decls().Get(clang_decl_id).key.decl;
+  if (const auto* clang_decl =
+          sem_ir().clang_decls().Lookup(var_storage.pattern_id)) {
     auto* constant = cpp_code_generator_->GetAddrOfGlobal(
-        CreateGlobalDecl(cast<clang::NamedDecl>(decl)),
+        CreateGlobalDecl(cast<clang::NamedDecl>(clang_decl->decl())),
         /*isForDefinition=*/false);
     if (constant) {
       return constant;

toolchain/lower/handle_call.cpp

@@ -637,10 +637,9 @@ static auto HandleVirtualCall(FunctionContext& context,
   auto* pointer_type =
       llvm::PointerType::get(context.llvm_context(), /* address space */ 0);
   llvm::Value* virtual_fn;
-  auto clang_decl_id =
+  const auto* clang_decl =
       context.sem_ir().clang_decls().Lookup(function.first_decl_id());
-  if (clang_decl_id.has_value() &&
-      context.sem_ir().clang_decls().Get(clang_decl_id).is_imported) {
+  if (clang_decl && clang_decl->is_imported) {
     // Use absolute vtables for clang interop - the itanium vtable contains
     // function pointers.
     auto* virtual_function_pointer_address = context.builder().CreateGEP(

toolchain/sem_ir/clang_decl.cpp

@@ -81,28 +81,28 @@ auto ClangDecl::Print(llvm::raw_ostream& out) const -> void {
 ClangDeclStore::ClangDeclStore(CheckIRId check_ir_id) : values_(check_ir_id) {}
 
 auto ClangDeclStore::Add(ClangDecl value) -> ClangDeclId {
-  CARBON_CHECK(!isa<clang::VarDecl>(value.key.decl));
+  CARBON_CHECK(!isa<clang::VarDecl>(value.decl()));
   auto id = values_.Add(value);
   inst_id_to_clang_decl_id_.Insert(value.inst_id, id);
   return id;
 }
 
 auto ClangDeclStore::AddVar(ClangDecl value, InstId pattern_id) -> ClangDeclId {
-  CARBON_CHECK(isa<clang::VarDecl>(value.key.decl));
+  CARBON_CHECK(isa<clang::VarDecl>(value.decl()));
   auto id = values_.Add(value);
   inst_id_to_clang_decl_id_.Insert(pattern_id, id);
   return id;
 }
 
-auto ClangDeclStore::Lookup(ClangDeclKey key) const -> ClangDeclId {
+auto ClangDeclStore::LookupId(ClangDeclKey key) const -> ClangDeclId {
   return values_.Lookup(key);
 }
 
-auto ClangDeclStore::Lookup(InstId inst_id) const -> ClangDeclId {
+auto ClangDeclStore::Lookup(InstId inst_id) const -> const ClangDecl* {
   if (auto result = inst_id_to_clang_decl_id_.Lookup(inst_id)) {
-    return result.value();
+    return &Get(result.value());
   }
-  return ClangDeclId::None;
+  return nullptr;
 }
 
 auto ClangDeclStore::OutputYaml() const -> Yaml::OutputMapping {

toolchain/sem_ir/clang_decl.h

@@ -184,6 +184,9 @@ struct ClangDecl : public Printable<ClangDecl> {
   // created by exporting some Carbon declaration to C++.
   bool is_imported = false;
 
+  // Get the `clang::Decl` pointer.
+  auto decl() const -> clang::Decl* { return key.decl; }
+
   auto GetAsKey() const -> ClangDeclKey { return key; }
 };
 
@@ -212,10 +215,10 @@ class ClangDeclStore {
   auto Get(ClangDeclId id) const -> const ClangDecl& { return values_.Get(id); }
 
   // Looks up a `ClangDeclId` by `ClangDeclKey`.
-  auto Lookup(ClangDeclKey key) const -> ClangDeclId;
+  auto LookupId(ClangDeclKey key) const -> ClangDeclId;
 
-  // Looks up a `ClangDeclId` by `InstId`.
-  auto Lookup(InstId inst_id) const -> ClangDeclId;
+  // Looks up a `ClangDecl` by `InstId`. Returns nullptr if not found.
+  auto Lookup(InstId inst_id) const -> const ClangDecl*;
 
   auto OutputYaml() const -> Yaml::OutputMapping;
 

toolchain/sem_ir/file.cpp

@@ -241,7 +241,7 @@ auto File::AppendCppMangledTypeName(ClassId class_id,
   }
   // A C++ class's scope always maps to a Clang tag declaration.
   auto* tag_decl =
-      clang::cast<clang::TagDecl>(clang_decls().Get(clang_decl_id).key.decl);
+      clang::cast<clang::TagDecl>(clang_decls().Get(clang_decl_id).decl());
   cpp_file_->mangle_context().mangleCanonicalTypeName(
       cpp_file_->ast_context().getCanonicalTagType(tag_decl), out);
   return true;

toolchain/sem_ir/mangler.cpp

@@ -193,10 +193,9 @@ auto Mangler::Mangle(SemIR::FunctionId function_id,
 
   // Clang should emit C++ function declarations for us.
   if (function_id != sem_ir().global_ctor_id()) {
-    auto clang_decl_id =
+    const auto* clang_decl =
         sem_ir().clang_decls().Lookup(function.first_decl_id());
-    CARBON_CHECK(!clang_decl_id.has_value() ||
-                     !sem_ir().clang_decls().Get(clang_decl_id).is_imported,
+    CARBON_CHECK(!clang_decl || !clang_decl->is_imported,
                  "Shouldn't mangle C++ function");
   }
 
@@ -276,10 +275,8 @@ auto Mangler::MangleGlobalVariable(SemIR::InstId pattern_id) -> std::string {
     return std::string();
   }
 
-  auto clang_decl_id = sem_ir().clang_decls().Lookup(pattern_id);
-  if (clang_decl_id.has_value()) {
-    CARBON_CHECK(!sem_ir().clang_decls().Get(clang_decl_id).is_imported,
-                 "Mangling a C++ variable");
+  if (const auto* clang_decl = sem_ir().clang_decls().Lookup(pattern_id)) {
+    CARBON_CHECK(!clang_decl->is_imported, "Mangling a C++ variable");
   }
 
   RawStringOstream os;