Repository: carbon-language/carbon-lang
File: toolchain/check/handle_function.cpp (C++)

Code before:
}

// Build a FunctionDecl describing the signature of a function. This
// handles the common logic shared by function declaration syntax and function
// definition syntax.
static auto BuildFunctionDecl(Context& context,
                              Parse::AnyFunctionDeclId node_id,
                              bool is_definition)
    -> std::pair<SemIR::FunctionId, SemIR::InstId> {
  auto return_slot_pattern_id = SemIR::InstId::None;
  if (auto [return_node, maybe_return_slot_pattern_id] =
          context.node_stack().PopWithNodeIdIf<Parse::NodeKind::ReturnType>();
      maybe_return_slot_pattern_id) {
    return_slot_pattern_id = *maybe_return_slot_pattern_id;
  }

  auto name = PopNameComponent(context, return_slot_pattern_id);
  if (name.param_patterns_id.has_value() ||
      !name.pattern_block_id.has_value()) {
    // If there were parameters then pattern stack entries were added from the
    // parameters, and we can drop the entries from the FunctionIntroducer. If
    // there were no parameters, but the pattern block was not used for a return
    // type, then we can also drop them.
    context.pattern_block_stack().PopAndDiscard();
    context.full_pattern_stack().PopFullPattern();
  }
  if (!name.param_patterns_id.has_value()) {
    context.TODO(node_id, "function with positional parameters");
    name.param_patterns_id = SemIR::InstBlockId::Empty;
  }

  auto name_context = context.decl_name_stack().FinishName(name);
  context.node_stack()
      .PopAndDiscardSoloNodeId<Parse::NodeKind::FunctionIntroducer>();

  // Process modifiers.
  auto [parent_scope_inst_id, parent_scope_inst] =
      context.name_scopes().GetInstIfValid(name_context.parent_scope_id);
  auto introducer =
      context.decl_introducer_state_stack().Pop<Lex::TokenKind::Fn>();
  DiagnoseModifiers(context, introducer, is_definition, parent_scope_inst_id,
                    parent_scope_inst);
  bool is_extern = introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extern);
  auto virtual_modifier =
      introducer.modifier_set.ToEnum<SemIR::Function::VirtualModifier>()
          .Case(KeywordModifierSet::Virtual,
                SemIR::Function::VirtualModifier::Virtual)
          .Case(KeywordModifierSet::Abstract,
                SemIR::Function::VirtualModifier::Abstract)
          .Case(KeywordModifierSet::Impl,
                SemIR::Function::VirtualModifier::Impl)

Review comment (suggestion):
It's unclear to me whether this pops the placeholder block in exactly the cases where `PopNameComponent` didn't pop it, and I'm worried that the coupling between this code and `PopNameComponent` is a bit subtle. For example, you're checking `param_patterns_id` but not `implicit_param_patterns_id` here, and I'm not sure whether that's correct or not.

I wonder if there's a different approach we could use here, that would also avoid the speculative push: when we handle the `ReturnTypeId` node in `handle_function.cpp`, could we look at the top of the node stack to determine if we have parameters, and push the new entries if not? I think that wouldn't entirely eliminate the subtlety, but this might be a bit easier to reason about if the rule is that we have a pattern block if and only if we have either parameters or a return type.

Code after:

  if (MergeFunctionRedecl(context, node_id, function_info, is_definition,
                          prev_function_id, prev_import_ir_id)) {
    // When merging, use the existing function rather than adding a new one.
    function_decl.function_id = prev_function_id;
  }
}

// Build a FunctionDecl describing the signature of a function. This
// handles the common logic shared by function declaration syntax and function
// definition syntax.
static auto BuildFunctionDecl(Context& context,
                              Parse::AnyFunctionDeclId node_id,
                              bool is_definition)
    -> std::pair<SemIR::FunctionId, SemIR::InstId> {
  auto return_slot_pattern_id = SemIR::InstId::None;
  if (auto [return_node, maybe_return_slot_pattern_id] =
          context.node_stack().PopWithNodeIdIf<Parse::NodeKind::ReturnType>();
      maybe_return_slot_pattern_id) {
    return_slot_pattern_id = *maybe_return_slot_pattern_id;
  }

  auto name = PopNameComponent(context, return_slot_pattern_id);
  if (!name.param_patterns_id.has_value()) {
    context.TODO(node_id, "function with positional parameters");
    name.param_patterns_id = SemIR::InstBlockId::Empty;
  }

  auto name_context = context.decl_name_stack().FinishName(name);
  context.node_stack()
      .PopAndDiscardSoloNodeId<Parse::NodeKind::FunctionIntroducer>();

  // Process modifiers.
  auto [parent_scope_inst_id, parent_scope_inst] =
      context.name_scopes().GetInstIfValid(name_context.parent_scope_id);
  auto introducer =
      context.decl_introducer_state_stack().Pop<Lex::TokenKind::Fn>();
  DiagnoseModifiers(context, introducer, is_definition, parent_scope_inst_id,
                    parent_scope_inst);
  bool is_extern = introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extern);
  auto virtual_modifier =
      introducer.modifier_set.ToEnum<SemIR::Function::VirtualModifier>()
          .Case(KeywordModifierSet::Virtual,
                SemIR::Function::VirtualModifier::Virtual)
          .Case(KeywordModifierSet::Abstract,
                SemIR::Function::VirtualModifier::Abstract)
          .Case(KeywordModifierSet::Impl,
                SemIR::Function::VirtualModifier::Impl)
          .Default(SemIR::Function::VirtualModifier::None);
  SemIR::Class* virtual_class_info = nullptr;
  if (virtual_modifier != SemIR::Function::VirtualModifier::None &&