Move decl-specific merge logic back to respective handlers. (#3937)

I'd split out the decl-specific handlers so that import_ref.cpp could
also call them. However, with the current model for imports and
constants, we shouldn't use them there. As a consequence, merge them
back into individual files for greater consistency with other helper
functions (and better visibility when making related code changes).

toolchain/check/BUILD

@@ -50,7 +50,6 @@ cc_library(
 cc_library(
     name = "context",
     srcs = [
-        "class.cpp",
         "context.cpp",
         "convert.cpp",
         "decl_name_stack.cpp",
@@ -64,7 +63,6 @@ cc_library(
         "subst.cpp",
     ],
     hdrs = [
-        "class.h",
         "context.h",
         "convert.h",
         "decl_name_stack.h",

toolchain/check/class.cpp

@@ -1,69 +0,0 @@
-// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
-// Exceptions. See /LICENSE for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-
-#include "toolchain/check/class.h"
-
-#include "toolchain/check/merge.h"
-
-namespace Carbon::Check {
-
-auto MergeClassRedecl(Context& context, SemIRLoc new_loc,
-                      SemIR::Class& new_class, bool new_is_import,
-                      bool new_is_definition, bool new_is_extern,
-                      SemIR::ClassId prev_class_id, bool prev_is_extern,
-                      SemIR::ImportIRId prev_import_ir_id) -> bool {
-  auto& prev_class = context.classes().Get(prev_class_id);
-  SemIRLoc prev_loc =
-      prev_class.is_defined() ? prev_class.definition_id : prev_class.decl_id;
-
-  // Check the generic parameters match, if they were specified.
-  if (!CheckRedeclParamsMatch(context, DeclParams(new_class),
-                              DeclParams(prev_class), {})) {
-    return false;
-  }
-
-  CheckIsAllowedRedecl(context, Lex::TokenKind::Class, prev_class.name_id,
-                       {.loc = new_loc,
-                        .is_definition = new_is_definition,
-                        .is_extern = new_is_extern},
-                       {.loc = prev_loc,
-                        .is_definition = prev_class.is_defined(),
-                        .is_extern = prev_is_extern},
-                       prev_import_ir_id);
-
-  // The introducer kind must match the previous declaration.
-  // TODO: The rule here is not yet decided. See #3384.
-  if (prev_class.inheritance_kind != new_class.inheritance_kind) {
-    CARBON_DIAGNOSTIC(ClassRedeclarationDifferentIntroducer, Error,
-                      "Class redeclared with different inheritance kind.");
-    CARBON_DIAGNOSTIC(ClassRedeclarationDifferentIntroducerPrevious, Note,
-                      "Previously declared here.");
-    context.emitter()
-        .Build(new_loc, ClassRedeclarationDifferentIntroducer)
-        .Note(prev_loc, ClassRedeclarationDifferentIntroducerPrevious)
-        .Emit();
-  }
-
-  if (new_is_definition) {
-    prev_class.implicit_param_refs_id = new_class.implicit_param_refs_id;
-    prev_class.param_refs_id = new_class.param_refs_id;
-    prev_class.definition_id = new_class.definition_id;
-    prev_class.scope_id = new_class.scope_id;
-    prev_class.body_block_id = new_class.body_block_id;
-    prev_class.adapt_id = new_class.adapt_id;
-    prev_class.base_id = new_class.base_id;
-    prev_class.object_repr_id = new_class.object_repr_id;
-  }
-
-  if ((prev_import_ir_id.is_valid() && !new_is_import) ||
-      (prev_is_extern && !new_is_extern)) {
-    prev_class.decl_id = new_class.decl_id;
-    ReplacePrevInstForMerge(
-        context, prev_class.enclosing_scope_id, prev_class.name_id,
-        new_is_import ? new_loc.inst_id : new_class.decl_id);
-  }
-  return true;
-}
-
-}  // namespace Carbon::Check

toolchain/check/class.h

@@ -1,28 +0,0 @@
-// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
-// Exceptions. See /LICENSE for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-
-#ifndef CARBON_TOOLCHAIN_CHECK_CLASS_H_
-#define CARBON_TOOLCHAIN_CHECK_CLASS_H_
-
-#include "toolchain/check/context.h"
-#include "toolchain/sem_ir/class.h"
-#include "toolchain/sem_ir/ids.h"
-
-namespace Carbon::Check {
-
-// Tries to merge new_class into prev_class_id. Since new_class won't have a
-// definition even if one is upcoming, set is_definition to indicate the planned
-// result.
-//
-// If merging is successful, returns true and may update the previous class.
-// Otherwise, returns false. Prints a diagnostic when appropriate.
-auto MergeClassRedecl(Context& context, SemIRLoc new_loc,
-                      SemIR::Class& new_class, bool new_is_import,
-                      bool new_is_definition, bool new_is_extern,
-                      SemIR::ClassId prev_class_id, bool prev_is_extern,
-                      SemIR::ImportIRId prev_import_ir_id) -> bool;
-
-}  // namespace Carbon::Check
-
-#endif  // CARBON_TOOLCHAIN_CHECK_CLASS_H_

toolchain/check/function.cpp

@@ -10,10 +10,10 @@
 
 namespace Carbon::Check {
 
-// Returns false if the provided function declarations differ.
-static auto CheckRedecl(Context& context, const SemIR::Function& new_function,
-                        const SemIR::Function& prev_function,
-                        Substitutions substitutions) -> bool {
+auto CheckFunctionTypeMatches(Context& context,
+                              const SemIR::Function& new_function,
+                              const SemIR::Function& prev_function,
+                              Substitutions substitutions) -> bool {
   if (!CheckRedeclParamsMatch(context, DeclParams(new_function),
                               DeclParams(prev_function), substitutions)) {
     return false;
@@ -61,80 +61,6 @@ static auto CheckRedecl(Context& context, const SemIR::Function& new_function,
   return true;
 }
 
-auto CheckFunctionTypeMatches(Context& context,
-                              SemIR::FunctionId new_function_id,
-                              SemIR::FunctionId prev_function_id,
-                              Substitutions substitutions) -> bool {
-  return CheckRedecl(context, context.functions().Get(new_function_id),
-                     context.functions().Get(prev_function_id), substitutions);
-}
-
-// Returns the return slot usage for a function given the computed usage for two
-// different declarations of the function.
-static auto MergeReturnSlot(SemIR::Function::ReturnSlot a,
-                            SemIR::Function::ReturnSlot b)
-    -> SemIR::Function::ReturnSlot {
-  if (a == SemIR::Function::ReturnSlot::NotComputed) {
-    return b;
-  }
-  if (b == SemIR::Function::ReturnSlot::NotComputed) {
-    return a;
-  }
-  if (a == SemIR::Function::ReturnSlot::Error) {
-    return b;
-  }
-  if (b == SemIR::Function::ReturnSlot::Error) {
-    return a;
-  }
-  CARBON_CHECK(a == b)
-      << "Different return slot usage computed for the same function.";
-  return a;
-}
-
-auto MergeFunctionRedecl(Context& context, SemIRLoc new_loc,
-                         SemIR::Function& new_function, bool new_is_import,
-                         bool new_is_definition,
-                         SemIR::FunctionId prev_function_id,
-                         SemIR::ImportIRId prev_import_ir_id) -> bool {
-  auto& prev_function = context.functions().Get(prev_function_id);
-
-  if (!CheckRedecl(context, new_function, prev_function, {})) {
-    return false;
-  }
-
-  CheckIsAllowedRedecl(context, Lex::TokenKind::Fn, prev_function.name_id,
-                       {.loc = new_loc,
-                        .is_definition = new_is_definition,
-                        .is_extern = new_function.is_extern},
-                       {.loc = prev_function.definition_id.is_valid()
-                                   ? prev_function.definition_id
-                                   : prev_function.decl_id,
-                        .is_definition = prev_function.definition_id.is_valid(),
-                        .is_extern = prev_function.is_extern},
-                       prev_import_ir_id);
-
-  if (new_is_definition) {
-    // Track the signature from the definition, so that IDs in the body
-    // match IDs in the signature.
-    prev_function.definition_id = new_function.definition_id;
-    prev_function.implicit_param_refs_id = new_function.implicit_param_refs_id;
-    prev_function.param_refs_id = new_function.param_refs_id;
-    prev_function.return_type_id = new_function.return_type_id;
-    prev_function.return_storage_id = new_function.return_storage_id;
-  }
-  // The new function might have return slot information if it was imported.
-  prev_function.return_slot =
-      MergeReturnSlot(prev_function.return_slot, new_function.return_slot);
-  if ((prev_import_ir_id.is_valid() && !new_is_import) ||
-      (prev_function.is_extern && !new_function.is_extern)) {
-    prev_function.is_extern = new_function.is_extern;
-    prev_function.decl_id = new_function.decl_id;
-    ReplacePrevInstForMerge(context, prev_function.enclosing_scope_id,
-                            prev_function.name_id, new_function.decl_id);
-  }
-  return true;
-}
-
 auto CheckFunctionReturnType(Context& context, SemIRLoc loc,
                              SemIR::Function& function) -> void {
   // If we have already checked the return type, we have nothing to do.

toolchain/check/function.h

@@ -26,28 +26,16 @@ struct SuspendedFunction {
   DeclNameStack::SuspendedName saved_name_state;
 };
 
-// Checks that `new_function_id` has the same parameter types and return type as
-// `prev_function_id`, applying the specified set of substitutions to the
+// Checks that `new_function` has the same parameter types and return type as
+// `prev_function`, applying the specified set of substitutions to the
 // previous function. Prints a suitable diagnostic and returns false if not.
 // Note that this doesn't include the syntactic check that's performed for
 // redeclarations.
 auto CheckFunctionTypeMatches(Context& context,
-                              SemIR::FunctionId new_function_id,
-                              SemIR::FunctionId prev_function_id,
+                              const SemIR::Function& new_function,
+                              const SemIR::Function& prev_function,
                               Substitutions substitutions) -> bool;
 
-// Tries to merge new_function into prev_function_id. Since new_function won't
-// have a definition even if one is upcoming, set is_definition to indicate the
-// planned result.
-//
-// If merging is successful, returns true and may update the previous function.
-// Otherwise, returns false. Prints a diagnostic when appropriate.
-auto MergeFunctionRedecl(Context& context, SemIRLoc new_loc,
-                         SemIR::Function& new_function, bool new_is_import,
-                         bool new_is_definition,
-                         SemIR::FunctionId prev_function_id,
-                         SemIR::ImportIRId prev_import_ir_id) -> bool;
-
 // Checks that the return type of the specified function is complete, issuing an
 // error if not. This computes the return slot usage for the function if
 // necessary.

toolchain/check/handle_class.cpp

@@ -3,7 +3,6 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 #include "toolchain/base/kind_switch.h"
-#include "toolchain/check/class.h"
 #include "toolchain/check/context.h"
 #include "toolchain/check/convert.h"
 #include "toolchain/check/decl_name_stack.h"
@@ -39,6 +38,70 @@ auto HandleClassIntroducer(Context& context, Parse::ClassIntroducerId node_id)
   return true;
 }
 
+// Tries to merge new_class into prev_class_id. Since new_class won't have a
+// definition even if one is upcoming, set is_definition to indicate the planned
+// result.
+//
+// If merging is successful, returns true and may update the previous class.
+// Otherwise, returns false. Prints a diagnostic when appropriate.
+static auto MergeClassRedecl(Context& context, SemIRLoc new_loc,
+                             SemIR::Class& new_class, bool new_is_import,
+                             bool new_is_definition, bool new_is_extern,
+                             SemIR::ClassId prev_class_id, bool prev_is_extern,
+                             SemIR::ImportIRId prev_import_ir_id) -> bool {
+  auto& prev_class = context.classes().Get(prev_class_id);
+  SemIRLoc prev_loc =
+      prev_class.is_defined() ? prev_class.definition_id : prev_class.decl_id;
+
+  // Check the generic parameters match, if they were specified.
+  if (!CheckRedeclParamsMatch(context, DeclParams(new_class),
+                              DeclParams(prev_class), {})) {
+    return false;
+  }
+
+  CheckIsAllowedRedecl(context, Lex::TokenKind::Class, prev_class.name_id,
+                       {.loc = new_loc,
+                        .is_definition = new_is_definition,
+                        .is_extern = new_is_extern},
+                       {.loc = prev_loc,
+                        .is_definition = prev_class.is_defined(),
+                        .is_extern = prev_is_extern},
+                       prev_import_ir_id);
+
+  // The introducer kind must match the previous declaration.
+  // TODO: The rule here is not yet decided. See #3384.
+  if (prev_class.inheritance_kind != new_class.inheritance_kind) {
+    CARBON_DIAGNOSTIC(ClassRedeclarationDifferentIntroducer, Error,
+                      "Class redeclared with different inheritance kind.");
+    CARBON_DIAGNOSTIC(ClassRedeclarationDifferentIntroducerPrevious, Note,
+                      "Previously declared here.");
+    context.emitter()
+        .Build(new_loc, ClassRedeclarationDifferentIntroducer)
+        .Note(prev_loc, ClassRedeclarationDifferentIntroducerPrevious)
+        .Emit();
+  }
+
+  if (new_is_definition) {
+    prev_class.implicit_param_refs_id = new_class.implicit_param_refs_id;
+    prev_class.param_refs_id = new_class.param_refs_id;
+    prev_class.definition_id = new_class.definition_id;
+    prev_class.scope_id = new_class.scope_id;
+    prev_class.body_block_id = new_class.body_block_id;
+    prev_class.adapt_id = new_class.adapt_id;
+    prev_class.base_id = new_class.base_id;
+    prev_class.object_repr_id = new_class.object_repr_id;
+  }
+
+  if ((prev_import_ir_id.is_valid() && !new_is_import) ||
+      (prev_is_extern && !new_is_extern)) {
+    prev_class.decl_id = new_class.decl_id;
+    ReplacePrevInstForMerge(
+        context, prev_class.enclosing_scope_id, prev_class.name_id,
+        new_is_import ? new_loc.inst_id : new_class.decl_id);
+  }
+  return true;
+}
+
 // Adds the name to name lookup. If there's a conflict, tries to merge. May
 // update class_decl and class_info when merging.
 static auto MergeOrAddName(Context& context, Parse::AnyClassDeclId node_id,

toolchain/check/handle_function.cpp

@@ -61,6 +61,78 @@ static auto DiagnoseModifiers(Context& context, bool is_definition,
   return context.decl_state_stack().innermost().modifier_set;
 }
 
+// Returns the return slot usage for a function given the computed usage for two
+// different declarations of the function.
+static auto MergeReturnSlot(SemIR::Function::ReturnSlot a,
+                            SemIR::Function::ReturnSlot b)
+    -> SemIR::Function::ReturnSlot {
+  if (a == SemIR::Function::ReturnSlot::NotComputed) {
+    return b;
+  }
+  if (b == SemIR::Function::ReturnSlot::NotComputed) {
+    return a;
+  }
+  if (a == SemIR::Function::ReturnSlot::Error) {
+    return b;
+  }
+  if (b == SemIR::Function::ReturnSlot::Error) {
+    return a;
+  }
+  CARBON_CHECK(a == b)
+      << "Different return slot usage computed for the same function.";
+  return a;
+}
+
+// Tries to merge new_function into prev_function_id. Since new_function won't
+// have a definition even if one is upcoming, set is_definition to indicate the
+// planned result.
+//
+// If merging is successful, returns true and may update the previous function.
+// Otherwise, returns false. Prints a diagnostic when appropriate.
+static auto MergeFunctionRedecl(Context& context, SemIRLoc new_loc,
+                                SemIR::Function& new_function,
+                                bool new_is_import, bool new_is_definition,
+                                SemIR::FunctionId prev_function_id,
+                                SemIR::ImportIRId prev_import_ir_id) -> bool {
+  auto& prev_function = context.functions().Get(prev_function_id);
+
+  if (!CheckFunctionTypeMatches(context, new_function, prev_function, {})) {
+    return false;
+  }
+
+  CheckIsAllowedRedecl(context, Lex::TokenKind::Fn, prev_function.name_id,
+                       {.loc = new_loc,
+                        .is_definition = new_is_definition,
+                        .is_extern = new_function.is_extern},
+                       {.loc = prev_function.definition_id.is_valid()
+                                   ? prev_function.definition_id
+                                   : prev_function.decl_id,
+                        .is_definition = prev_function.definition_id.is_valid(),
+                        .is_extern = prev_function.is_extern},
+                       prev_import_ir_id);
+
+  if (new_is_definition) {
+    // Track the signature from the definition, so that IDs in the body
+    // match IDs in the signature.
+    prev_function.definition_id = new_function.definition_id;
+    prev_function.implicit_param_refs_id = new_function.implicit_param_refs_id;
+    prev_function.param_refs_id = new_function.param_refs_id;
+    prev_function.return_type_id = new_function.return_type_id;
+    prev_function.return_storage_id = new_function.return_storage_id;
+  }
+  // The new function might have return slot information if it was imported.
+  prev_function.return_slot =
+      MergeReturnSlot(prev_function.return_slot, new_function.return_slot);
+  if ((prev_import_ir_id.is_valid() && !new_is_import) ||
+      (prev_function.is_extern && !new_function.is_extern)) {
+    prev_function.is_extern = new_function.is_extern;
+    prev_function.decl_id = new_function.decl_id;
+    ReplacePrevInstForMerge(context, prev_function.enclosing_scope_id,
+                            prev_function.name_id, new_function.decl_id);
+  }
+  return true;
+}
+
 // Check whether this is a redeclaration, merging if needed.
 static auto TryMergeRedecl(Context& context, Parse::AnyFunctionDeclId node_id,
                            SemIR::InstId prev_id,

toolchain/check/impl.cpp

@@ -52,8 +52,9 @@ static auto CheckAssociatedFunctionImplementation(
   // TODO: This should be a semantic check rather than a syntactic one. The
   // functions should be allowed to have different signatures as long as we can
   // synthesize a suitable thunk.
-  if (!CheckFunctionTypeMatches(context, impl_function_decl->function_id,
-                                interface_function_id, substitutions)) {
+  if (!CheckFunctionTypeMatches(
+          context, context.functions().Get(impl_function_decl->function_id),
+          context.functions().Get(interface_function_id), substitutions)) {
     return SemIR::InstId::BuiltinError;
   }
   return impl_decl_id;

toolchain/check/merge.cpp

@@ -5,8 +5,6 @@
 #include "toolchain/check/merge.h"
 
 #include "toolchain/base/kind_switch.h"
-#include "toolchain/check/class.h"
-#include "toolchain/check/function.h"
 #include "toolchain/check/import_ref.h"
 #include "toolchain/sem_ir/ids.h"
 #include "toolchain/sem_ir/typed_insts.h"