Special-case api/impl implicit imports and verify relevant redeclarat…

…ions. (#3843)

Adds ImportIRId::ApiForImpl to reserve a specific slot for the `api`
import, so that the code can trivially determine whether an import is
from the same library. This is then used for merging function
declarations, because the rules for redeclarations in the same library
slightly differ as compared to other imports (note they're also not
identical to same-file rules).

The main thing this leaves from the recent #3762 is verifying that
entities forward declared in the `impl` file are also defined, but
that's not in-scope for merging; it's moreso post-checking validation.

Note, a lot of our `invalid <entity> ID` comments in ids.h were
incorrectly copy-pasted, so I've cut `<entity>`.

toolchain/check/check.cpp

@@ -184,10 +184,10 @@ struct UnitInfo {
   ErrorTrackingDiagnosticConsumer err_tracker;
   DiagnosticEmitter<Parse::NodeLoc> emitter;
 
-  // A map of package names to outgoing imports. If the
-  // import's target isn't available, the unit will be nullptr to assist with
-  // name lookup. Invalid imports (for example, `import Main;`) aren't added
-  // because they won't add identifiers to name lookup.
+  // A map of package names to outgoing imports. If a package includes
+  // unavailable library imports, it has an entry with has_load_error set.
+  // Invalid imports (for example, `import Main;`) aren't added because they
+  // won't add identifiers to name lookup.
   llvm::DenseMap<IdentifierId, PackageImports> package_imports_map;
 
   // The remaining number of imports which must be checked before this unit can
@@ -197,6 +197,10 @@ struct UnitInfo {
   // A list of incoming imports. This will be empty for `impl` files, because
   // imports only touch `api` files.
   llvm::SmallVector<UnitInfo*> incoming_imports;
+
+  // True if this is an `impl` file and an `api` implicit import has
+  // successfully been added. Used for determining the number of import IRs.
+  bool has_api_for_impl = false;
 };
 
 // Add imports to the root block.
@@ -208,6 +212,10 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info)
   for (auto& [_, package_imports] : unit_info.package_imports_map) {
     num_irs += package_imports.imports.size();
   }
+  if (unit_info.has_api_for_impl) {
+    // One of the IRs replaces ImportIRId::ApiForImpl.
+    --num_irs;
+  }
   context.import_ir_constant_values().resize(
       num_irs, SemIR::ConstantValueStore(SemIR::ConstantId::Invalid));
 
@@ -232,10 +240,15 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info)
   auto self_import = unit_info.package_imports_map.find(IdentifierId::Invalid);
   if (self_import != unit_info.package_imports_map.end()) {
     bool error_in_import = self_import->second.has_load_error;
+    const auto& packaging = context.parse_tree().packaging_directive();
+    auto current_library_id =
+        packaging ? packaging->names.library_id : StringLiteralValueId::Invalid;
     for (const auto& import : self_import->second.imports) {
+      bool is_api_for_impl = current_library_id == import.names.library_id;
       const auto& import_sem_ir = **import.unit_info->unit->sem_ir;
       ImportLibraryFromCurrentPackage(context, namespace_type_id,
-                                      import.names.node_id, import_sem_ir);
+                                      import.names.node_id, is_api_for_impl,
+                                      import_sem_ir);
       error_in_import |= import_sem_ir.name_scopes()
                              .Get(SemIR::NameScopeId::Package)
                              .has_error;
@@ -271,7 +284,8 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info)
   }
 
   CARBON_CHECK(context.import_irs().size() == num_irs)
-      << "Created an unexpected number of IRs";
+      << "Created an unexpected number of IRs: expected " << num_irs
+      << ", have " << context.import_irs().size();
 }
 
 namespace {
@@ -901,6 +915,12 @@ static auto TrackImport(
     package_imports_it->second.imports.push_back({import, api->second});
     ++unit_info.imports_remaining;
     api->second->incoming_imports.push_back(&unit_info);
+
+    // If this is the implicit import, note it was successfully imported.
+    if (!explicit_import_map) {
+      CARBON_CHECK(!import.package_id.is_valid());
+      unit_info.has_api_for_impl = true;
+    }
   } else {
     // The imported api is missing.
     package_imports_it->second.has_load_error = true;
@@ -1067,6 +1087,7 @@ auto CheckParseTrees(const SemIR::File& builtin_ir,
     // on a part of it.
     // TODO: Better identify cycles, maybe try to untangle them.
     for (auto& unit_info : unit_infos) {
+      const auto& packaging = unit_info.unit->parse_tree->packaging_directive();
       if (unit_info.imports_remaining > 0) {
         for (auto& [package_id, package_imports] :
              unit_info.package_imports_map) {
@@ -1084,6 +1105,10 @@ auto CheckParseTrees(const SemIR::File& builtin_ir,
                                      ImportCycleDetected);
               // Make this look the same as an import which wasn't found.
               package_imports.has_load_error = true;
+              if (packaging && !package_id.is_valid() &&
+                  packaging->names.library_id == import_it->names.library_id) {
+                unit_info.has_api_for_impl = false;
+              }
               import_it = package_imports.imports.erase(import_it);
             }
           }

toolchain/check/function.cpp

@@ -6,9 +6,12 @@
 
 #include "toolchain/check/merge.h"
 #include "toolchain/check/subst.h"
+#include "toolchain/sem_ir/ids.h"
 
 namespace Carbon::Check {
 
+CARBON_DIAGNOSTIC(FunctionPreviousDecl, Note, "Previously declared here.");
+
 // Returns true if there was an error in declaring the function, which will have
 // previously been diagnosed.
 static auto FunctionDeclHasError(Context& context, const SemIR::Function& fn)
@@ -192,47 +195,46 @@ auto CheckFunctionTypeMatches(Context& context,
                      context.functions().Get(prev_function_id), substitutions);
 }
 
+// Emits a redundant redeclaration diagnostic.
+static auto EmitRedundantRedecl(Context& context, SemIR::LocId loc_id,
+                                const SemIR::Function& prev_function) {
+  CARBON_DIAGNOSTIC(FunctionRedecl, Error,
+                    "Redundant redeclaration of function {0}.", SemIR::NameId);
+  context.emitter()
+      .Build(loc_id, FunctionRedecl, prev_function.name_id)
+      .Note(prev_function.decl_id, FunctionPreviousDecl)
+      .Emit();
+}
+
+// Emits a redefinition diagnostic.
+static auto EmitRedefinition(Context& context, SemIR::LocId loc_id,
+                             const SemIR::Function& prev_function) {
+  CARBON_DIAGNOSTIC(FunctionRedefinition, Error,
+                    "Redefinition of function {0}.", SemIR::NameId);
+  CARBON_DIAGNOSTIC(FunctionPreviousDefinition, Note,
+                    "Previously defined here.");
+  context.emitter()
+      .Build(loc_id, FunctionRedefinition, prev_function.name_id)
+      .Note(prev_function.definition_id, FunctionPreviousDefinition)
+      .Emit();
+}
+
 // Checks to see if a structurally valid redeclaration is allowed in context.
 // These all still merge.
 static auto CheckIsAllowedRedecl(Context& context, SemIR::LocId loc_id,
-                                 SemIR::Function& new_function,
+                                 const SemIR::Function& new_function,
                                  bool new_is_definition,
-                                 SemIR::Function& prev_function,
-                                 bool prev_is_import) -> void {
-  CARBON_DIAGNOSTIC(FunctionPreviousDecl, Note, "Previously declared here.");
-  if (prev_is_import) {
-    // TODO: Allow non-extern declarations in the same library.
-    if (!new_function.is_extern && !prev_function.is_extern) {
-      CARBON_DIAGNOSTIC(
-          FunctionNonExternRedecl, Error,
-          "Only one library can declare function {0} without `extern`.",
-          SemIR::NameId);
-      context.emitter()
-          .Build(loc_id, FunctionNonExternRedecl, prev_function.name_id)
-          .Note(prev_function.decl_id, FunctionPreviousDecl)
-          .Emit();
-      return;
-    }
-  } else {
+                                 const SemIR::Function& prev_function,
+                                 SemIR::ImportIRInstId prev_import_ir_inst_id)
+    -> void {
+  if (!prev_import_ir_inst_id.is_valid()) {
+    // Check for disallowed redeclarations in the same file.
     if (!new_is_definition) {
-      CARBON_DIAGNOSTIC(FunctionRedecl, Error,
-                        "Redundant redeclaration of function {0}.",
-                        SemIR::NameId);
-      context.emitter()
-          .Build(loc_id, FunctionRedecl, prev_function.name_id)
-          .Note(prev_function.decl_id, FunctionPreviousDecl)
-          .Emit();
+      EmitRedundantRedecl(context, loc_id, prev_function);
       return;
     }
     if (prev_function.definition_id.is_valid()) {
-      CARBON_DIAGNOSTIC(FunctionRedefinition, Error,
-                        "Redefinition of function {0}.", SemIR::NameId);
-      CARBON_DIAGNOSTIC(FunctionPreviousDefinition, Note,
-                        "Previously defined here.");
-      context.emitter()
-          .Build(loc_id, FunctionRedefinition, prev_function.name_id)
-          .Note(prev_function.definition_id, FunctionPreviousDefinition)
-          .Emit();
+      EmitRedefinition(context, loc_id, prev_function);
       return;
     }
     // `extern` definitions are prevented in handle_function.cpp; this is only
@@ -249,22 +251,62 @@ static auto CheckIsAllowedRedecl(Context& context, SemIR::LocId loc_id,
           .Emit();
       return;
     }
+    return;
+  }
+
+  auto import_ir_id =
+      context.import_ir_insts().Get(prev_import_ir_inst_id).ir_id;
+  if (import_ir_id == SemIR::ImportIRId::ApiForImpl) {
+    // Check for disallowed redeclarations in the same library. Note that a
+    // forward declaration in the impl is allowed.
+    if (prev_function.definition_id.is_valid()) {
+      if (new_function.definition_id.is_valid()) {
+        EmitRedefinition(context, loc_id, prev_function);
+      } else {
+        EmitRedundantRedecl(context, loc_id, prev_function);
+      }
+      return;
+    }
+    if (prev_function.is_extern != new_function.is_extern) {
+      CARBON_DIAGNOSTIC(
+          FunctionExternMismatch, Error,
+          "Redeclarations in the same library must match use of `extern`.");
+      context.emitter()
+          .Build(loc_id, FunctionExternMismatch)
+          .Note(prev_function.decl_id, FunctionPreviousDecl)
+          .Emit();
+      return;
+    }
+    return;
+  }
+
+  // Check for disallowed redeclarations cross-library.
+  if (!new_function.is_extern && !prev_function.is_extern) {
+    CARBON_DIAGNOSTIC(
+        FunctionNonExternRedecl, Error,
+        "Only one library can declare function {0} without `extern`.",
+        SemIR::NameId);
+    context.emitter()
+        .Build(loc_id, FunctionNonExternRedecl, prev_function.name_id)
+        .Note(prev_function.decl_id, FunctionPreviousDecl)
+        .Emit();
+    return;
   }
 }
 
 auto MergeFunctionRedecl(Context& context, SemIR::LocId loc_id,
                          SemIR::Function& new_function, bool new_is_import,
                          bool new_is_definition,
                          SemIR::FunctionId prev_function_id,
-                         bool prev_is_import) -> bool {
+                         SemIR::ImportIRInstId prev_import_ir_inst_id) -> bool {
   auto& prev_function = context.functions().Get(prev_function_id);
 
   if (!CheckRedecl(context, new_function, prev_function, {})) {
     return false;
   }
 
   CheckIsAllowedRedecl(context, loc_id, new_function, new_is_definition,
-                       prev_function, prev_is_import);
+                       prev_function, prev_import_ir_inst_id);
 
   if (new_is_definition) {
     // Track the signature from the definition, so that IDs in the body
@@ -275,7 +317,7 @@ auto MergeFunctionRedecl(Context& context, SemIR::LocId loc_id,
     prev_function.return_type_id = new_function.return_type_id;
     prev_function.return_slot_id = new_function.return_slot_id;
   }
-  if ((prev_is_import && !new_is_import) ||
+  if ((prev_import_ir_inst_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;

toolchain/check/function.h

@@ -46,7 +46,7 @@ auto MergeFunctionRedecl(Context& context, SemIR::LocId loc_id,
                          SemIR::Function& new_function, bool new_is_import,
                          bool new_is_definition,
                          SemIR::FunctionId prev_function_id,
-                         bool prev_is_imported) -> bool;
+                         SemIR::ImportIRInstId prev_import_ir_inst_id) -> bool;
 
 }  // namespace Carbon::Check
 

toolchain/check/handle_function.cpp

@@ -169,14 +169,15 @@ static auto BuildFunctionDecl(Context& context,
   auto prev_id =
       context.decl_name_stack().LookupOrAddName(name_context, lookup_result_id);
   if (prev_id.is_valid()) {
-    auto prev_inst = ResolvePrevInstForMerge(context, node_id, prev_id);
+    auto prev_inst_for_merge =
+        ResolvePrevInstForMerge(context, node_id, prev_id);
 
     if (auto existing_function_decl =
-            prev_inst.inst.TryAs<SemIR::FunctionDecl>()) {
+            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_inst.is_import)) {
+                              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;
       }

toolchain/check/import.cpp

@@ -210,9 +210,19 @@ static auto CopyEnclosingNameScopesFromImportIR(
 auto ImportLibraryFromCurrentPackage(Context& context,
                                      SemIR::TypeId namespace_type_id,
                                      Parse::ImportDirectiveId node_id,
+                                     bool is_api_for_impl,
                                      const SemIR::File& import_sem_ir) -> void {
-  auto ir_id =
-      context.import_irs().Add({.node_id = node_id, .sem_ir = &import_sem_ir});
+  auto ir_id = SemIR::ImportIRId::Invalid;
+  if (is_api_for_impl) {
+    ir_id = SemIR::ImportIRId::ApiForImpl;
+    auto& import_ir = context.import_irs().Get(ir_id);
+    CARBON_CHECK(import_ir.sem_ir == nullptr) << "ApiForImpl is only set once";
+    import_ir = {.node_id = node_id, .sem_ir = &import_sem_ir};
+  } else {
+    ir_id = context.import_irs().Add(
+        {.node_id = node_id, .sem_ir = &import_sem_ir});
+  }
+
   context.import_ir_constant_values()[ir_id.index].Set(
       SemIR::InstId::PackageNamespace,
       context.constant_values().Get(SemIR::InstId::PackageNamespace));

toolchain/check/import.h

@@ -17,6 +17,7 @@ namespace Carbon::Check {
 auto ImportLibraryFromCurrentPackage(Context& context,
                                      SemIR::TypeId namespace_type_id,
                                      Parse::ImportDirectiveId node_id,
+                                     bool is_api_for_impl,
                                      const SemIR::File& import_sem_ir) -> void;
 
 // Adds another package's imports to name lookup, with all libraries together.

toolchain/check/import_ref.cpp

@@ -700,9 +700,12 @@ class ImportRefResolver {
     auto function_decl = SemIR::FunctionDecl{
         context_.GetTypeIdForTypeConstant(type_const_id),
         SemIR::FunctionId::Invalid, SemIR::InstBlockId::Empty};
-    auto function_decl_id =
-        context_.AddPlaceholderInstInNoBlock(SemIR::LocIdAndInst::Untyped(
-            AddImportIRInst(function.decl_id), function_decl));
+    // Prefer pointing diagnostics towards a definition.
+    auto imported_loc_id = AddImportIRInst(function.definition_id.is_valid()
+                                               ? function.definition_id
+                                               : function.decl_id);
+    auto function_decl_id = context_.AddPlaceholderInstInNoBlock(
+        SemIR::LocIdAndInst::Untyped(imported_loc_id, function_decl));
 
     auto new_return_type_id =
         return_type_const_id.is_valid()
@@ -727,7 +730,10 @@ class ImportRefResolver {
          .return_type_id = new_return_type_id,
          .return_slot_id = new_return_slot,
          .is_extern = function.is_extern,
-         .builtin_kind = function.builtin_kind});
+         .builtin_kind = function.builtin_kind,
+         .definition_id = function.definition_id.is_valid()
+                              ? function_decl_id
+                              : SemIR::InstId::Invalid});
     // Write the function ID into the FunctionDecl.
     context_.ReplaceInstBeforeConstantUse(function_decl_id, function_decl);
     return {context_.constant_values().Get(function_decl_id)};