Switch CalleeFunction to a variant (#6104)

Trying to make it easier to see what's intended to be present/correct on
`CalleeFunction` in its various modes.

toolchain/check/call.cpp

@@ -202,11 +202,10 @@ static auto CheckCalleeFunctionReturnType(Context& context, SemIR::LocId loc_id,
 }
 
 // Performs a call where the callee is a function.
-static auto PerformCallToFunction(Context& context, SemIR::LocId loc_id,
-                                  SemIR::InstId callee_id,
-                                  const SemIR::CalleeFunction& callee_function,
-                                  llvm::ArrayRef<SemIR::InstId> arg_ids)
-    -> SemIR::InstId {
+static auto PerformCallToFunction(
+    Context& context, SemIR::LocId loc_id, SemIR::InstId callee_id,
+    const SemIR::CalleeFunction::Function& callee_function,
+    llvm::ArrayRef<SemIR::InstId> arg_ids) -> SemIR::InstId {
   // If the callee is a generic function, determine the generic argument values
   // for the call.
   auto callee_specific_id = ResolveCalleeInCall(
@@ -293,33 +292,12 @@ static auto PerformCallToFunction(Context& context, SemIR::LocId loc_id,
   }
 }
 
-auto PerformCall(Context& context, SemIR::LocId loc_id, SemIR::InstId callee_id,
-                 llvm::ArrayRef<SemIR::InstId> arg_ids) -> SemIR::InstId {
-  // Try treating the callee as a function first.
-  auto callee_function = GetCalleeFunction(context.sem_ir(), callee_id);
-  if (callee_function.is_error) {
-    return SemIR::ErrorInst::InstId;
-  }
-  if (callee_function.cpp_overload_set_id.has_value()) {
-    auto self_id = callee_function.self_id;
-    callee_id = PerformCppOverloadResolution(
-        context, loc_id, callee_function.cpp_overload_set_id, self_id, arg_ids);
-    callee_function = GetCalleeFunction(context.sem_ir(), callee_id);
-    if (callee_function.is_error) {
-      return SemIR::ErrorInst::InstId;
-    }
-    CARBON_CHECK(!callee_function.cpp_overload_set_id.has_value());
-
-    // Preserve the `self` argument from the original callee.
-    CARBON_CHECK(!callee_function.self_id.has_value());
-    callee_function.self_id = self_id;
-  }
-  if (callee_function.function_id.has_value()) {
-    return PerformCallToFunction(context, loc_id, callee_id, callee_function,
-                                 arg_ids);
-  }
-
-  // Callee isn't a function, so try treating it as a generic type.
+// Performs a call where the callee is a generic type. If it's not a generic
+// type, produces a diagnostic.
+static auto PerformCallToNonFunction(Context& context, SemIR::LocId loc_id,
+                                     SemIR::InstId callee_id,
+                                     llvm::ArrayRef<SemIR::InstId> arg_ids)
+    -> SemIR::InstId {
   auto type_inst =
       context.types().GetAsInst(context.insts().Get(callee_id).type_id());
   CARBON_KIND_SWITCH(type_inst) {
@@ -342,4 +320,45 @@ auto PerformCall(Context& context, SemIR::LocId loc_id, SemIR::InstId callee_id,
   }
 }
 
+auto PerformCall(Context& context, SemIR::LocId loc_id, SemIR::InstId callee_id,
+                 llvm::ArrayRef<SemIR::InstId> arg_ids) -> SemIR::InstId {
+  // Try treating the callee as a function first.
+  auto callee_function = GetCalleeFunction(context.sem_ir(), callee_id);
+  CARBON_KIND_SWITCH(callee_function.info) {
+    case CARBON_KIND(SemIR::CalleeFunction::Error _): {
+      return SemIR::ErrorInst::InstId;
+    }
+    case CARBON_KIND(SemIR::CalleeFunction::Function fn): {
+      return PerformCallToFunction(context, loc_id, callee_id, fn, arg_ids);
+    }
+    case CARBON_KIND(SemIR::CalleeFunction::NonFunction _): {
+      return PerformCallToNonFunction(context, loc_id, callee_id, arg_ids);
+    }
+
+    case CARBON_KIND(SemIR::CalleeFunction::CppOverloadSet overload): {
+      callee_id = PerformCppOverloadResolution(context, loc_id,
+                                               overload.cpp_overload_set_id,
+                                               overload.self_id, arg_ids);
+      auto overload_result = GetCalleeFunction(context.sem_ir(), callee_id);
+      CARBON_KIND_SWITCH(overload_result.info) {
+        case CARBON_KIND(SemIR::CalleeFunction::Error _): {
+          return SemIR::ErrorInst::InstId;
+        }
+        case CARBON_KIND(SemIR::CalleeFunction::Function fn): {
+          // Preserve the `self` argument from the original callee.
+          CARBON_CHECK(!fn.self_id.has_value());
+          fn.self_id = overload.self_id;
+          return PerformCallToFunction(context, loc_id, callee_id, fn, arg_ids);
+        }
+        case CARBON_KIND(SemIR::CalleeFunction::CppOverloadSet _): {
+          CARBON_FATAL("overloads can't be recursive");
+        }
+        case CARBON_KIND(SemIR::CalleeFunction::NonFunction _): {
+          CARBON_FATAL("overloads should produce functions");
+        }
+      }
+    }
+  }
+}
+
 }  // namespace Carbon::Check

toolchain/check/cpp/thunk.cpp

@@ -16,6 +16,7 @@
 #include "toolchain/check/literal.h"
 #include "toolchain/check/type.h"
 #include "toolchain/check/type_completion.h"
+#include "toolchain/sem_ir/function.h"
 #include "toolchain/sem_ir/ids.h"
 #include "toolchain/sem_ir/typed_insts.h"
 
@@ -552,7 +553,8 @@ auto PerformCppThunkCall(Context& context, SemIR::LocId loc_id,
   auto callee_function_params =
       context.inst_blocks().Get(callee_function.call_params_id);
 
-  auto thunk_callee = GetCalleeFunction(context.sem_ir(), thunk_callee_id);
+  auto thunk_callee =
+      GetCalleeFunctionAsFunction(context.sem_ir(), thunk_callee_id);
   auto& thunk_function = context.functions().Get(thunk_callee.function_id);
   auto thunk_function_params =
       context.inst_blocks().Get(thunk_function.call_params_id);

toolchain/check/eval.cpp

@@ -1942,11 +1942,11 @@ static auto MakeConstantForCall(EvalContext& eval_context,
   auto callee_function =
       SemIR::GetCalleeFunction(eval_context.sem_ir(), call.callee_id);
   auto builtin_kind = SemIR::BuiltinFunctionKind::None;
-  if (callee_function.function_id.has_value()) {
+  if (auto* fn =
+          std::get_if<SemIR::CalleeFunction::Function>(&callee_function.info)) {
     // Calls to builtins might be constant.
-    builtin_kind = eval_context.functions()
-                       .Get(callee_function.function_id)
-                       .builtin_function_kind();
+    builtin_kind =
+        eval_context.functions().Get(fn->function_id).builtin_function_kind();
     if (builtin_kind == SemIR::BuiltinFunctionKind::None) {
       // TODO: Eventually we'll want to treat some kinds of non-builtin
       // functions as producing constants.
@@ -1977,12 +1977,12 @@ static auto MakeConstantForCall(EvalContext& eval_context,
                         "non-constant call to compile-time-only function");
       CARBON_DIAGNOSTIC(CompTimeOnlyFunctionHere, Note,
                         "compile-time-only function declared here");
+      const auto& function = eval_context.functions().Get(
+          std::get<SemIR::CalleeFunction::Function>(callee_function.info)
+              .function_id);
       eval_context.emitter()
           .Build(inst_id, NonConstantCallToCompTimeOnlyFunction)
-          .Note(eval_context.functions()
-                    .Get(callee_function.function_id)
-                    .latest_decl_id(),
-                CompTimeOnlyFunctionHere)
+          .Note(function.latest_decl_id(), CompTimeOnlyFunctionHere)
           .Emit();
     }
     return SemIR::ConstantId::NotConstant;

toolchain/check/eval_inst.cpp

@@ -501,7 +501,7 @@ auto EvalConstantInst(Context& context, SemIR::InstId inst_id,
 auto EvalConstantInst(Context& context, SemIR::InstId inst_id,
                       SemIR::SpecificFunction inst) -> ConstantEvalResult {
   auto callee_function =
-      SemIR::GetCalleeFunction(context.sem_ir(), inst.callee_id);
+      SemIR::GetCalleeFunctionAsFunction(context.sem_ir(), inst.callee_id);
   const auto& fn = context.functions().Get(callee_function.function_id);
   if (!callee_function.self_type_id.has_value() &&
       fn.builtin_function_kind() != SemIR::BuiltinFunctionKind::NoOp &&

toolchain/check/member_access.cpp

@@ -53,24 +53,37 @@ static auto IsInstanceMethod(const SemIR::File& sem_ir,
   return function.self_param_id.has_value();
 }
 
-// Returns whether the callee function is an instance method, either because
-// it's a Carbon instance method or because it's a C++ overload set that might
-// contain an instance method.
-static auto IsInstanceMethod(const SemIR::File& sem_ir,
-                             const SemIR::CalleeFunction& callee) -> bool {
-  if (callee.function_id.has_value()) {
-    return IsInstanceMethod(sem_ir, callee.function_id);
-  }
-  if (callee.cpp_overload_set_id.has_value()) {
-    // For now, treat all C++ overload sets as potentially containing instance
-    // methods. Overload resolution will handle the case where we actually
-    // found a static method.
-    // TODO: Consider returning `false` if there are no non-instance methods in
-    // the overload set. This would cause us to reject
-    // `instance.(Class.StaticMethod)()` like we do in pure Carbon code.
-    return true;
+// For callee functions which are instance methods, returns the `self_id` (which
+// may be `None`). This may be an instance method either because it's a Carbon
+// instance method or because it's a C++ overload set that might contain an
+// instance method.
+static auto GetSelfIfInstanceMethod(const SemIR::File& sem_ir,
+                                    const SemIR::CalleeFunction& callee)
+    -> std::optional<SemIR::InstId> {
+  CARBON_KIND_SWITCH(callee.info) {
+    case CARBON_KIND(SemIR::CalleeFunction::Function fn): {
+      if (IsInstanceMethod(sem_ir, fn.function_id)) {
+        return fn.self_id;
+      }
+      return std::nullopt;
+    }
+    case CARBON_KIND(SemIR::CalleeFunction::CppOverloadSet overload): {
+      // For now, treat all C++ overload sets as potentially containing instance
+      // methods. Overload resolution will handle the case where we actually
+      // found a static method.
+      // TODO: Consider returning `None` if there are no non-instance methods
+      // in the overload set. This would cause us to reject
+      // `instance.(Class.StaticMethod)()` like we do in pure Carbon code.
+      return overload.self_id;
+    }
+
+    case CARBON_KIND(SemIR::CalleeFunction::Error _): {
+      return std::nullopt;
+    }
+    case CARBON_KIND(SemIR::CalleeFunction::NonFunction _): {
+      return std::nullopt;
+    }
   }
-  return false;
 }
 
 // Return whether `type_id`, the type of an associated entity, is for an
@@ -391,9 +404,10 @@ static auto PerformInstanceBinding(Context& context, SemIR::LocId loc_id,
                                    SemIR::InstId base_id,
                                    SemIR::InstId member_id) -> SemIR::InstId {
   // If the member is a function, check whether it's an instance method.
-  if (auto callee = SemIR::GetCalleeFunction(context.sem_ir(), member_id);
-      IsInstanceMethod(context.sem_ir(), callee)) {
-    if (callee.self_id.has_value()) {
+  if (auto self_id = GetSelfIfInstanceMethod(
+          context.sem_ir(),
+          SemIR::GetCalleeFunction(context.sem_ir(), member_id))) {
+    if (self_id->has_value()) {
       // Found an already-bound method.
       return member_id;
     }

toolchain/check/thunk.cpp

@@ -227,7 +227,7 @@ static auto HasDeclaredReturnType(Context& context,
 auto BuildThunk(Context& context, SemIR::FunctionId signature_id,
                 SemIR::SpecificId signature_specific_id,
                 SemIR::InstId callee_id) -> SemIR::InstId {
-  auto callee = SemIR::GetCalleeFunction(context.sem_ir(), callee_id);
+  auto callee = SemIR::GetCalleeFunctionAsFunction(context.sem_ir(), callee_id);
 
   // Check whether we can use the given function without a thunk.
   // TODO: For virtual functions, we want different rules for checking `self`.

toolchain/lower/handle_call.cpp

@@ -10,6 +10,7 @@
 #include "llvm/IR/Value.h"
 #include "toolchain/lower/function_context.h"
 #include "toolchain/sem_ir/builtin_function_kind.h"
+#include "toolchain/sem_ir/function.h"
 #include "toolchain/sem_ir/ids.h"
 #include "toolchain/sem_ir/typed_insts.h"
 
@@ -476,12 +477,10 @@ static auto HandleBuiltinCall(FunctionContext& context, SemIR::InstId inst_id,
   CARBON_FATAL("Unsupported builtin call.");
 }
 
-static auto HandleVirtualCall(FunctionContext& context,
-                              llvm::ArrayRef<llvm::Value*> args,
-                              const SemIR::File* callee_file,
-                              const SemIR::Function& function,
-                              const SemIR::CalleeFunction& callee_function)
-    -> llvm::CallInst* {
+static auto HandleVirtualCall(
+    FunctionContext& context, llvm::ArrayRef<llvm::Value*> args,
+    const SemIR::File* callee_file, const SemIR::Function& function,
+    const SemIR::CalleeFunction::Function& callee_function) -> llvm::CallInst* {
   CARBON_CHECK(!args.empty(),
                "Virtual functions must have at least one parameter");
   auto* ptr_type =
@@ -558,8 +557,8 @@ auto HandleInst(FunctionContext& context, SemIR::InstId inst_id,
     CARBON_CHECK(callee_id.has_value());
   }
 
-  auto callee_function = SemIR::GetCalleeFunction(*callee_file, callee_id);
-  CARBON_CHECK(callee_function.function_id.has_value());
+  auto callee_function =
+      SemIR::GetCalleeFunctionAsFunction(*callee_file, callee_id);
 
   const SemIR::Function& function =
       callee_file->functions().Get(callee_function.function_id);

toolchain/sem_ir/function.cpp

@@ -6,6 +6,7 @@
 
 #include <optional>
 
+#include "toolchain/base/kind_switch.h"
 #include "toolchain/sem_ir/file.h"
 #include "toolchain/sem_ir/generic.h"
 #include "toolchain/sem_ir/ids.h"
@@ -13,17 +14,43 @@
 
 namespace Carbon::SemIR {
 
+auto CalleeFunction::Print(llvm::raw_ostream& out) const -> void {
+  out << "{";
+  CARBON_KIND_SWITCH(info) {
+    case CARBON_KIND(SemIR::CalleeFunction::Function fn): {
+      out << "Function{function_id: " << fn.function_id
+          << ", enclosing_specific_id: " << fn.enclosing_specific_id
+          << ", resolved_specific_id: " << fn.resolved_specific_id
+          << ", self_type_id: " << fn.self_type_id
+          << ", self_id: " << fn.self_id << "}";
+      break;
+    }
+    case CARBON_KIND(SemIR::CalleeFunction::CppOverloadSet overload): {
+      out << "CppOverload{cpp_overload_set_id: " << overload.cpp_overload_set_id
+          << ", self_id: " << overload.self_id << "}";
+      break;
+    }
+    case CARBON_KIND(SemIR::CalleeFunction::Error _): {
+      out << "Error{}";
+      break;
+    }
+    case CARBON_KIND(SemIR::CalleeFunction::NonFunction _): {
+      out << "Other{}";
+      break;
+    }
+  }
+  out << "}";
+}
+
 auto GetCalleeFunction(const File& sem_ir, InstId callee_id,
                        SpecificId specific_id) -> CalleeFunction {
-  CalleeFunction result = {.function_id = FunctionId::None,
-                           .cpp_overload_set_id = CppOverloadSetId::None,
-                           .enclosing_specific_id = SpecificId::None,
-                           .resolved_specific_id = SpecificId::None,
-                           .self_type_id = InstId::None,
-                           .self_id = InstId::None,
-                           .is_error = false};
+  CalleeFunction::Function fn = {.function_id = FunctionId::None,
+                                 .enclosing_specific_id = SpecificId::None,
+                                 .resolved_specific_id = SpecificId::None,
+                                 .self_type_id = InstId::None,
+                                 .self_id = InstId::None};
   if (auto bound_method = sem_ir.insts().TryGetAs<BoundMethod>(callee_id)) {
-    result.self_id = bound_method->object_id;
+    fn.self_id = bound_method->object_id;
     callee_id = bound_method->function_decl_id;
   }
 
@@ -36,39 +63,51 @@ auto GetCalleeFunction(const File& sem_ir, InstId callee_id,
 
   if (auto specific_function =
           sem_ir.insts().TryGetAs<SpecificFunction>(callee_id)) {
-    result.resolved_specific_id = specific_function->specific_id;
+    fn.resolved_specific_id = specific_function->specific_id;
     callee_id = specific_function->callee_id;
   }
 
   // Identify the function we're calling by its type.
   auto val_id = sem_ir.constant_values().GetConstantInstId(callee_id);
   if (!val_id.has_value()) {
-    return result;
+    return {.info = CalleeFunction::NonFunction()};
   }
   auto fn_type_inst =
       sem_ir.types().GetAsInst(sem_ir.insts().Get(val_id).type_id());
 
   if (auto cpp_overload_set_type = fn_type_inst.TryAs<CppOverloadSetType>()) {
-    result.cpp_overload_set_id = cpp_overload_set_type->overload_set_id;
-    return result;
+    CARBON_CHECK(!fn.resolved_specific_id.has_value(),
+                 "Only `SpecificFunction` will be resolved, not C++ overloads");
+    return {.info = CalleeFunction::CppOverloadSet{
+                .cpp_overload_set_id = cpp_overload_set_type->overload_set_id,
+                .self_id = fn.self_id}};
   }
 
   if (auto impl_fn_type = fn_type_inst.TryAs<FunctionTypeWithSelfType>()) {
     // Combine the associated function's `Self` with the interface function
     // data.
-    result.self_type_id = impl_fn_type->self_id;
+    fn.self_type_id = impl_fn_type->self_id;
     fn_type_inst = sem_ir.insts().Get(impl_fn_type->interface_function_type_id);
   }
 
   auto fn_type = fn_type_inst.TryAs<FunctionType>();
   if (!fn_type) {
-    result.is_error = fn_type_inst.Is<ErrorInst>();
-    return result;
+    if (fn_type_inst.Is<ErrorInst>()) {
+      return {.info = CalleeFunction::Error()};
+    }
+    return {.info = CalleeFunction::NonFunction()};
   }
 
-  result.function_id = fn_type->function_id;
-  result.enclosing_specific_id = fn_type->specific_id;
-  return result;
+  fn.function_id = fn_type->function_id;
+  fn.enclosing_specific_id = fn_type->specific_id;
+  return {.info = fn};
+}
+
+auto GetCalleeFunctionAsFunction(const File& sem_ir, InstId callee_id,
+                                 SpecificId specific_id)
+    -> CalleeFunction::Function {
+  return std::get<CalleeFunction::Function>(
+      GetCalleeFunction(sem_ir, callee_id, specific_id).info);
 }
 
 auto DecomposeVirtualFunction(const File& sem_ir, InstId fn_decl_id,

toolchain/sem_ir/function.h

@@ -186,34 +186,38 @@ using FunctionStore = ValueStore<FunctionId, Function>;
 class File;
 
 struct CalleeFunction : public Printable<CalleeFunction> {
-  // The function. `None` if not a function.
-  FunctionId function_id;
-  // The overload set, if this is a C++ overload set rather than a function.
-  CppOverloadSetId cpp_overload_set_id;
-  // The specific that contains the function.
-  SpecificId enclosing_specific_id;
-  // The specific for the callee itself, in a resolved call.
-  SpecificId resolved_specific_id;
-  // The bound `Self` type or facet value. `None` if not a bound interface
-  // member.
-  InstId self_type_id;
-  // The bound `self` parameter. `None` if not a method.
-  InstId self_id;
-  // True if an error instruction was found.
-  bool is_error;
+  // This is a C++ overload set.
+  struct CppOverloadSet {
+    // The overload set.
+    CppOverloadSetId cpp_overload_set_id;
+    // The bound `self` parameter. `None` if not a method.
+    InstId self_id;
+  };
 
-  auto Print(llvm::raw_ostream& out) const -> void {
-    out << "{";
-    if (cpp_overload_set_id.has_value()) {
-      out << "cpp_overload_set_id: " << cpp_overload_set_id;
-    } else {
-      out << "function_id: " << function_id;
-    }
-    out << ", enclosing_specific_id: " << enclosing_specific_id
-        << ", resolved_specific_id: " << resolved_specific_id
-        << ", self_type_id: " << self_type_id << ", self_id: " << self_id
-        << ", is_error: " << is_error << "}";
-  }
+  // An error instruction was found.
+  struct Error {};
+
+  // This is a function.
+  struct Function {
+    // The function.
+    FunctionId function_id;
+    // The specific that contains the function.
+    SpecificId enclosing_specific_id;
+    // The specific for the callee itself, in a resolved call.
+    SpecificId resolved_specific_id;
+    // The bound `Self` type or facet value. `None` if not a bound interface
+    // member.
+    InstId self_type_id;
+    // The bound `self` parameter. `None` if not a method.
+    InstId self_id;
+  };
+
+  // This may be a generic type, or could be an invalid callee.
+  struct NonFunction {};
+
+  std::variant<CppOverloadSet, Error, Function, NonFunction> info;
+
+  auto Print(llvm::raw_ostream& out) const -> void;
 };
 
 // Returns information for the function corresponding to callee_id.
@@ -221,6 +225,11 @@ auto GetCalleeFunction(const File& sem_ir, InstId callee_id,
                        SpecificId specific_id = SpecificId::None)
     -> CalleeFunction;
 
+// Like `GetCalleeFunction`, but restricts to the `Function` callee kind.
+auto GetCalleeFunctionAsFunction(const File& sem_ir, InstId callee_id,
+                                 SpecificId specific_id = SpecificId::None)
+    -> CalleeFunction::Function;
+
 struct DecomposedVirtualFunction {
   // The canonical instruction from the `fn_decl_const_id`.
   InstId fn_decl_id;

toolchain/sem_ir/inst_namer.cpp

@@ -760,11 +760,12 @@ auto InstNamer::NamingContext::NameInst() -> void {
     }
     case CARBON_KIND(Call inst): {
       auto callee_function = GetCalleeFunction(sem_ir(), inst.callee_id);
-      if (!callee_function.function_id.has_value()) {
-        AddInstName("");
+      if (auto* fn =
+              std::get_if<CalleeFunction::Function>(&callee_function.info)) {
+        AddEntityNameAndMaybePush(fn->function_id, ".call");
         return;
       }
-      AddEntityNameAndMaybePush(callee_function.function_id, ".call");
+      AddInstName("");
       return;
     }
     case CARBON_KIND(ClassDecl inst): {

toolchain/sem_ir/stringify.cpp

@@ -570,25 +570,25 @@ class Stringifier {
 
   auto StringifyInst(InstId /*inst_id*/, SpecificFunction inst) -> void {
     auto callee = GetCalleeFunction(*sem_ir_, inst.callee_id);
-    if (callee.function_id.has_value()) {
-      step_stack_->PushEntityName(sem_ir_->functions().Get(callee.function_id),
+    if (auto* fn = std::get_if<CalleeFunction::Function>(&callee.info)) {
+      step_stack_->PushEntityName(sem_ir_->functions().Get(fn->function_id),
                                   inst.specific_id);
-    } else {
-      step_stack_->PushString("<invalid specific function>");
+      return;
     }
+    step_stack_->PushString("<invalid specific function>");
   }
 
   auto StringifyInst(InstId /*inst_id*/, SpecificImplFunction inst) -> void {
     auto callee = GetCalleeFunction(*sem_ir_, inst.callee_id);
-    if (callee.function_id.has_value()) {
+    if (auto* fn = std::get_if<CalleeFunction::Function>(&callee.info)) {
       // TODO: The specific_id here is for the interface member, but the
       // entity we're passing is the impl member. This might result in
       // strange output once we render specific arguments properly.
-      step_stack_->PushEntityName(sem_ir_->functions().Get(callee.function_id),
+      step_stack_->PushEntityName(sem_ir_->functions().Get(fn->function_id),
                                   inst.specific_id);
-    } else {
-      step_stack_->PushString("<invalid specific function>");
+      return;
     }
+    step_stack_->PushString("<invalid specific function>");
   }
 
   auto StringifyInst(InstId /*inst_id*/, StructType inst) -> void {