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

Code before:
static auto AddImportRefOrMerge(Context& context, SemIR::ImportIRId ir_id,
                                const SemIR::File& import_sem_ir,
                                SemIR::InstId import_inst_id,
                                SemIR::NameScopeId parent_scope_id,
                                SemIR::NameId name_id) -> void {
  // Leave a placeholder that the inst comes from the other IR.
  auto& parent_scope = context.name_scopes().Get(parent_scope_id);
  auto [inserted, entry_id] = parent_scope.LookupOrAdd(
      name_id,
      // This InstId is temporary and would be overridden if used.
      SemIR::InstId::None, SemIR::AccessKind::Public);
  auto& entry = parent_scope.GetEntry(entry_id);
  if (inserted) {
    auto entity_name_id = context.entity_names().Add(
      {.name_id = name_id, .parent_scope_id = parent_scope_id});
    auto import_ref = AddImportRef(context, {.ir_id = ir_id, .inst_id = import_inst_id},
      entity_name_id);
    entry.result = SemIR::ScopeLookupResult::MakeFound( import_ref,
      SemIR::AccessKind::Public);

    // Import references for non-owning declarations that match current library
    const auto functions = import_sem_ir.get_non_owning_functions();
    auto current_id = context.sem_ir().library_id();
    bool is_current_default = current_id == SemIR::LibraryNameId::Default;
    for (auto function_id : functions) {
      const auto& function = import_sem_ir.functions().Get(function_id);
      auto lib_id = function.extern_library_id;
      bool is_lib_default = lib_id == SemIR::LibraryNameId::Default;

      if ((is_lib_default && is_current_default) ||
          (!is_lib_default && !is_current_default)) {
        if (is_lib_default) {  // both libraries are default, import ref.
          LoadImportRef(context, import_ref);
        } else {
          // both libraries are non-default, check if they're the same named
          // library, import ref if yes.
          StringLiteralValueId owner_library =
              current_id.AsStringLiteralValueId();
          auto str_owner_library =
              context.string_literal_values().Get(owner_library);
          StringLiteralValueId decl_library = lib_id.AsStringLiteralValueId();
          auto str_decl_library =
              import_sem_ir.string_literal_values().Get(decl_library);
          if (str_owner_library.compare(str_decl_library) == 0) {
            LoadImportRef(context, import_ref);
          }
        }
      }
    }
    return;
  }

Review comment (refactor):
This doesn't look like it's verifying the found function matches the current ImportRef. I think what this is going to do is it will load the import ref if there is _any_ non-owning function in the library. For contrast, we should probably be checking that this `ImportRef` is _specifically_ a non-owning function.

Maybe there should be a test that includes a non-`extern` declaration, and the textual semir should show that it's not loaded? e.g.:

```
// --- unloaded_decl_extern.carbon

library "[[@TEST_NAME]]";

extern library "unloaded_decl" fn ExternDecl();
fn NonExternDecl();

// --- unloaded_decl.carbon

library "[[@TEST_NAME]]";

import library "unloaded_decl_extern";

extern fn ExternDecl();
```

Note in the above example `NonExternDecl` has no references, so will show in name lookup but should not be loaded. Note it'd really require examining the semir to see what got loaded here, I think it's subtle without that.

Had you considered fetching the instruction being pointed to and seeing if it's a function declaration? e.g. `import_sem_ir.insts().TryGetAs(SemIR::FunctionDecl)` (I *think* `FunctionDecl` is appropriate there, but you could verify by first loading all `FunctionDecl`s), then inspect the FunctionId that appears there. Note this would be _instead_ of `non_owning_functions`, since it'd be a direct query instead of the lookup.

Note, on a skim, most of the rest would probably mirror over (library stuff still applies), though it might make sense to move into a helper function to keep `AddImportRefOrMerge` small.

Code after:
            .name_scope_id;
  }

  return scope_cursor;
}

// Imports the function if it's a non-owning declaration with the current file
// as owner.
static auto LoadImportForOwningFunction(Context& context,
                                        const SemIR::File& import_sem_ir,
                                        const SemIR::Function& function,
                                        SemIR::InstId import_ref) {
  if (!function.extern_library_id.has_value()) {
    return;
  }
  CARBON_CHECK(function.is_extern && "Expected extern functions");
  auto lib_id = function.extern_library_id;
  bool is_lib_default = lib_id == SemIR::LibraryNameId::Default;

  auto current_id = context.sem_ir().library_id();
  bool is_current_default = current_id == SemIR::LibraryNameId::Default;

  if (is_lib_default == is_current_default) {
    if (is_lib_default) {
      // Both libraries are default, import ref.
      LoadImportRef(context, import_ref);
    } else {
      // Both libraries are non-default: check if they're the same named
      // library, import ref if yes.
      auto str_owner_library = context.string_literal_values().Get(
          current_id.AsStringLiteralValueId());
      auto str_decl_library = import_sem_ir.string_literal_values().Get(
          lib_id.AsStringLiteralValueId());
      if (str_owner_library == str_decl_library) {
        LoadImportRef(context, import_ref);
      }
    }
  }
}

// Adds an ImportRef for an entity, handling merging if needed.
static auto AddImportRefOrMerge(Context& context, SemIR::ImportIRId ir_id,
                                const SemIR::File& import_sem_ir,
                                SemIR::InstId import_inst_id,
                                SemIR::NameScopeId parent_scope_id,
                                SemIR::NameId name_id) -> void {
  // Leave a placeholder that the inst comes from the other IR.
  auto& parent_scope = context.name_scopes().Get(parent_scope_id);
  auto [inserted, entry_id] = parent_scope.LookupOrAdd(
      name_id,
      // This InstId is temporary and would be overridden if used.