Initial, extern-ignoring support for extern class decls. (#3891)

This doesn't actually track whether a declaration is `extern`. It does,
however:

- Factor out and expand merge support for classes, sharing handling with
functions.
- This makes the ClassRedefinition diagnostic redundant, as the
redeclaration checking overlaps.
- Add partial `extern` handling to class handling; just some
verifications of correct use.
- Factor out `extern` on member handling for sharing with `fn`.
- Fixes a bug in import_ref where a class's definition_id wasn't
assigned when defining.

This changes how a redefinition is handled (replaced, rather than
merged). I don't know whether that's ideal, but I think it results in
easy-to-understand consequences, and it's more consistent with how `fn`
works.

There's enough work here that this felt like a decent cut point,
particularly as the amount of work to actually add `extern` tracking
will be significant.

toolchain/check/BUILD

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

toolchain/check/class.cpp

@@ -0,0 +1,57 @@
+// 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::ImportIRInstId prev_import_ir_inst_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;
+
+  // TODO: Check that the generic parameter list agrees with the prior
+  // declaration.
+
+  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_inst_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.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;
+  }
+
+  return true;
+}
+
+}  // namespace Carbon::Check

toolchain/check/class.h

@@ -0,0 +1,28 @@
+// 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::ImportIRInstId prev_import_ir_inst_id) -> bool;
+
+}  // namespace Carbon::Check
+
+#endif  // CARBON_TOOLCHAIN_CHECK_CLASS_H_

toolchain/check/function.h

@@ -40,8 +40,8 @@ auto CheckFunctionTypeMatches(Context& context,
 // have a definition even if one is upcoming, set is_definition to indicate the
 // planned result.
 //
-// If merging is successful, updates the FunctionId on new_function and returns
-// true. Otherwise, returns false. Prints a diagnostic when appropriate.
+// 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,

toolchain/check/handle_class.cpp

@@ -2,8 +2,10 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
+#include "toolchain/check/class.h"
 #include "toolchain/check/context.h"
 #include "toolchain/check/convert.h"
+#include "toolchain/check/merge.h"
 #include "toolchain/check/modifiers.h"
 #include "toolchain/sem_ir/typed_insts.h"
 
@@ -33,7 +35,8 @@ auto HandleClassIntroducer(Context& context, Parse::ClassIntroducerId node_id)
   return true;
 }
 
-static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id)
+static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id,
+                           bool is_definition)
     -> std::tuple<SemIR::ClassId, SemIR::InstId> {
   if (context.node_stack().PopIf<Parse::NodeKind::TuplePattern>()) {
     context.TODO(node_id, "generic class");
@@ -50,15 +53,20 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id)
   CheckAccessModifiersOnDecl(context, Lex::TokenKind::Class,
                              name_context.target_scope_id);
   LimitModifiersOnDecl(context,
-                       KeywordModifierSet::Class | KeywordModifierSet::Access,
+                       KeywordModifierSet::Class | KeywordModifierSet::Access |
+                           KeywordModifierSet::Extern,
                        Lex::TokenKind::Class);
+  RestrictExternModifierOnDecl(context, Lex::TokenKind::Class,
+                               name_context.target_scope_id, is_definition);
 
   auto modifiers = context.decl_state_stack().innermost().modifier_set;
   if (!!(modifiers & KeywordModifierSet::Access)) {
     context.TODO(context.decl_state_stack().innermost().modifier_node_id(
                      ModifierOrder::Access),
                  "access modifier");
   }
+
+  bool is_extern = !!(modifiers & KeywordModifierSet::Extern);
   auto inheritance_kind =
       !!(modifiers & KeywordModifierSet::Abstract) ? SemIR::Class::Abstract
       : !!(modifiers & KeywordModifierSet::Base)   ? SemIR::Class::Base
@@ -72,34 +80,35 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id)
                                      SemIR::ClassId::Invalid, decl_block_id};
   auto class_decl_id = context.AddPlaceholderInst({node_id, class_decl});
 
+  SemIR::Class class_info = {
+      .name_id = name_context.name_id_for_new_inst(),
+      .enclosing_scope_id = name_context.enclosing_scope_id_for_new_inst(),
+      // `.self_type_id` depends on the ClassType, so is set below.
+      .self_type_id = SemIR::TypeId::Invalid,
+      .decl_id = class_decl_id,
+      .inheritance_kind = inheritance_kind};
+
   // Check whether this is a redeclaration.
-  auto existing_id =
+  auto prev_id =
       context.decl_name_stack().LookupOrAddName(name_context, class_decl_id);
-  if (existing_id.is_valid()) {
-    if (auto existing_class_decl =
-            context.insts().Get(existing_id).TryAs<SemIR::ClassDecl>()) {
-      // This is a redeclaration of an existing class.
-      class_decl.class_id = existing_class_decl->class_id;
-      auto& class_info = context.classes().Get(class_decl.class_id);
-
-      // The introducer kind must match the previous declaration.
-      // TODO: The rule here is not yet decided. See #3384.
-      if (class_info.inheritance_kind != inheritance_kind) {
-        CARBON_DIAGNOSTIC(ClassRedeclarationDifferentIntroducer, Error,
-                          "Class redeclared with different inheritance kind.");
-        CARBON_DIAGNOSTIC(ClassRedeclarationDifferentIntroducerPrevious, Note,
-                          "Previously declared here.");
-        context.emitter()
-            .Build(node_id, ClassRedeclarationDifferentIntroducer)
-            .Note(existing_id, ClassRedeclarationDifferentIntroducerPrevious)
-            .Emit();
+  if (prev_id.is_valid()) {
+    auto prev_inst_for_merge =
+        ResolvePrevInstForMerge(context, node_id, prev_id);
+
+    if (auto prev_class_decl =
+            prev_inst_for_merge.inst.TryAs<SemIR::ClassDecl>()) {
+      // TODO: Fix prev_is_extern.
+      if (MergeClassRedecl(context, node_id, class_info,
+                           /*new_is_import=*/false, is_definition, is_extern,
+                           prev_class_decl->class_id,
+                           /*prev_is_extern=*/false,
+                           prev_inst_for_merge.import_ir_inst_id)) {
+        // When merging, use the existing entity rather than adding a new one.
+        class_decl.class_id = prev_class_decl->class_id;
       }
-
-      // TODO: Check that the generic parameter list agrees with the prior
-      // declaration.
     } else {
       // This is a redeclaration of something other than a class.
-      context.DiagnoseDuplicateName(class_decl_id, existing_id);
+      context.DiagnoseDuplicateName(class_decl_id, prev_id);
     }
   }
 
@@ -109,13 +118,7 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id)
     // TODO: If this is an invalid redeclaration of a non-class entity or there
     // was an error in the qualifier, we will have lost track of the class name
     // here. We should keep track of it even if the name is invalid.
-    class_decl.class_id = context.classes().Add(
-        {.name_id = name_context.name_id_for_new_inst(),
-         .enclosing_scope_id = name_context.enclosing_scope_id_for_new_inst(),
-         // `.self_type_id` depends on the ClassType, so is set below.
-         .self_type_id = SemIR::TypeId::Invalid,
-         .decl_id = class_decl_id,
-         .inheritance_kind = inheritance_kind});
+    class_decl.class_id = context.classes().Add(class_info);
   }
 
   // Write the class ID into the ClassDecl.
@@ -131,27 +134,19 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id)
 }
 
 auto HandleClassDecl(Context& context, Parse::ClassDeclId node_id) -> bool {
-  BuildClassDecl(context, node_id);
+  BuildClassDecl(context, node_id, /*is_definition=*/false);
   context.decl_name_stack().PopScope();
   return true;
 }
 
 auto HandleClassDefinitionStart(Context& context,
                                 Parse::ClassDefinitionStartId node_id) -> bool {
-  auto [class_id, class_decl_id] = BuildClassDecl(context, node_id);
+  auto [class_id, class_decl_id] =
+      BuildClassDecl(context, node_id, /*is_definition=*/true);
   auto& class_info = context.classes().Get(class_id);
 
   // Track that this declaration is the definition.
-  if (class_info.is_defined()) {
-    CARBON_DIAGNOSTIC(ClassRedefinition, Error, "Redefinition of class {0}.",
-                      SemIR::NameId);
-    CARBON_DIAGNOSTIC(ClassPreviousDefinition, Note,
-                      "Previous definition was here.");
-    context.emitter()
-        .Build(node_id, ClassRedefinition, class_info.name_id)
-        .Note(class_info.definition_id, ClassPreviousDefinition)
-        .Emit();
-  } else {
+  if (!class_info.is_defined()) {
     class_info.definition_id = class_decl_id;
     class_info.scope_id = context.name_scopes().Add(
         class_decl_id, SemIR::NameId::Invalid, class_info.enclosing_scope_id);

toolchain/check/handle_function.cpp

@@ -47,22 +47,13 @@ static auto DiagnoseModifiers(Context& context, bool is_definition,
     -> KeywordModifierSet {
   const Lex::TokenKind decl_kind = Lex::TokenKind::Fn;
   CheckAccessModifiersOnDecl(context, decl_kind, target_scope_id);
-  if (is_definition) {
-    ForbidExternModifierOnDefinition(context, decl_kind);
-  }
-  if (target_scope_id.is_valid()) {
-    auto target_id = context.name_scopes().Get(target_scope_id).inst_id;
-    if (target_id.is_valid() &&
-        !context.insts().Is<SemIR::Namespace>(target_id)) {
-      ForbidModifiersOnDecl(context, KeywordModifierSet::Extern, decl_kind,
-                            " that is a member");
-    }
-  }
   LimitModifiersOnDecl(context,
                        KeywordModifierSet::Access | KeywordModifierSet::Extern |
                            KeywordModifierSet::Method |
                            KeywordModifierSet::Interface,
                        decl_kind);
+  RestrictExternModifierOnDecl(context, decl_kind, target_scope_id,
+                               is_definition);
   CheckMethodModifiersOnFunction(context, target_scope_id);
   RequireDefaultFinalOnlyInInterfaces(context, decl_kind, target_scope_id);
 
@@ -163,14 +154,14 @@ static auto BuildFunctionDecl(Context& context,
     auto prev_inst_for_merge =
         ResolvePrevInstForMerge(context, node_id, prev_id);
 
-    if (auto existing_function_decl =
+    if (auto prev_function_decl =
             prev_inst_for_merge.inst.TryAs<SemIR::FunctionDecl>()) {
       if (MergeFunctionRedecl(context, node_id, function_info,
                               /*new_is_import=*/false, is_definition,
-                              existing_function_decl->function_id,
+                              prev_function_decl->function_id,
                               prev_inst_for_merge.import_ir_inst_id)) {
         // When merging, use the existing function rather than adding a new one.
-        function_decl.function_id = existing_function_decl->function_id;
+        function_decl.function_id = prev_function_decl->function_id;
       }
     } else {
       // This is a redeclaration of something other than a function. This

toolchain/check/import_ref.cpp

@@ -571,6 +571,8 @@ class ImportRefResolver {
                           SemIR::Class& new_class,
                           SemIR::ConstantId object_repr_const_id,
                           SemIR::ConstantId base_const_id) -> void {
+    new_class.definition_id = new_class.decl_id;
+
     new_class.object_repr_id =
         context_.GetTypeIdForTypeConstant(object_repr_const_id);
 

toolchain/check/merge.cpp

@@ -5,6 +5,7 @@
 #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"
@@ -232,6 +233,20 @@ auto MergeImportRef(Context& context, SemIR::InstId new_inst_id,
                           prev_inst->import_ir_inst_id);
       return;
     }
+    case CARBON_KIND(SemIR::ClassType new_type): {
+      auto prev_type = prev_inst->inst.TryAs<SemIR::ClassType>();
+      if (!prev_type) {
+        break;
+      }
+
+      auto new_class = context.classes().Get(new_type.class_id);
+      // TODO: Fix new_is_extern and prev_is_extern.
+      MergeClassRedecl(context, new_inst_id, new_class,
+                       /*new_is_import=*/true, new_class.is_defined(),
+                       /*new_is_extern=*/false, prev_type->class_id,
+                       /*prev_is_extern=*/false, prev_inst->import_ir_inst_id);
+      return;
+    }
     default:
       context.TODO(new_inst_id, llvm::formatv("Merging {0} not yet supported.",
                                               new_inst->inst.kind()));

toolchain/check/modifiers.cpp

@@ -136,10 +136,21 @@ auto CheckMethodModifiersOnFunction(Context& context,
                         " outside of a class");
 }
 
-auto ForbidExternModifierOnDefinition(Context& context,
-                                      Lex::TokenKind decl_kind) -> void {
-  ForbidModifiersOnDecl(context, KeywordModifierSet::Extern, decl_kind,
-                        " that provides a definition");
+auto RestrictExternModifierOnDecl(Context& context, Lex::TokenKind decl_kind,
+                                  SemIR::NameScopeId target_scope_id,
+                                  bool is_definition) -> void {
+  if (is_definition) {
+    ForbidModifiersOnDecl(context, KeywordModifierSet::Extern, decl_kind,
+                          " that provides a definition");
+  }
+  if (target_scope_id.is_valid()) {
+    auto target_id = context.name_scopes().Get(target_scope_id).inst_id;
+    if (target_id.is_valid() &&
+        !context.insts().Is<SemIR::Namespace>(target_id)) {
+      ForbidModifiersOnDecl(context, KeywordModifierSet::Extern, decl_kind,
+                            " that is a member");
+    }
+  }
 }
 
 auto RequireDefaultFinalOnlyInInterfaces(Context& context,

toolchain/check/modifiers.h

@@ -43,10 +43,13 @@ inline auto LimitModifiersOnDecl(Context& context, KeywordModifierSet allowed,
   ForbidModifiersOnDecl(context, ~allowed, decl_kind, "");
 }
 
-// If the `extern` modifier is present, diagnoses and updates the declaration
-// state to remove it. Only called for declarations with definitions.
-auto ForbidExternModifierOnDefinition(Context& context,
-                                      Lex::TokenKind decl_kind) -> void;
+// Restricts the `extern` modifier to only be used on namespace-scoped
+// declarations, diagnosing and removing it on:
+// - `extern` on a definition.
+// - `extern` on a scoped entity.
+auto RestrictExternModifierOnDecl(Context& context, Lex::TokenKind decl_kind,
+                                  SemIR::NameScopeId target_scope_id,
+                                  bool is_definition) -> void;
 
 // Report a diagonostic if `default` and `final` modifiers are used on
 // declarations where they are not allowed. Right now they are only allowed