Only produce a name scope for a namespace when not merged (#4168)

Addresses the comment on
https://github.com/carbon-language/carbon-lang/pull/4153#discussion_r1690292168
(although maybe with somewhat quirky results, since contents get printed
each time)

toolchain/check/handle_namespace.cpp

@@ -34,10 +34,6 @@ auto HandleParseNode(Context& context, Parse::NamespaceId node_id) -> bool {
       SemIR::NameScopeId::Invalid, SemIR::InstId::Invalid};
   auto namespace_id =
       context.AddPlaceholderInst(SemIR::LocIdAndInst(node_id, namespace_inst));
-  namespace_inst.name_scope_id = context.name_scopes().Add(
-      namespace_id, name_context.name_id_for_new_inst(),
-      name_context.parent_scope_id_for_new_inst());
-  context.ReplaceInstBeforeConstantUse(namespace_id, namespace_inst);
 
   auto existing_inst_id = context.decl_name_stack().LookupOrAddName(
       name_context, namespace_id, SemIR::AccessKind::Public);
@@ -46,6 +42,9 @@ auto HandleParseNode(Context& context, Parse::NamespaceId node_id) -> bool {
     // previous declaration. Otherwise, diagnose the issue.
     if (auto existing =
             context.insts().TryGetAs<SemIR::Namespace>(existing_inst_id)) {
+      // Point at the other namespace.
+      namespace_inst.name_scope_id = existing->name_scope_id;
+
       if (context.name_scopes().Get(existing->name_scope_id).is_closed_import) {
         // The existing name is a package name, so this is a name conflict.
         context.DiagnoseDuplicateName(namespace_id, existing_inst_id);
@@ -65,6 +64,17 @@ auto HandleParseNode(Context& context, Parse::NamespaceId node_id) -> bool {
     }
   }
 
+  // If we weren't able to merge namespaces, add a new name scope. Note this
+  // occurs even for duplicates where we discard the namespace, because we want
+  // to produce a valid constant.
+  if (!namespace_inst.name_scope_id.is_valid()) {
+    namespace_inst.name_scope_id = context.name_scopes().Add(
+        namespace_id, name_context.name_id_for_new_inst(),
+        name_context.parent_scope_id_for_new_inst());
+  }
+
+  context.ReplaceInstBeforeConstantUse(namespace_id, namespace_inst);
+
   context.decl_name_stack().PopScope();
   return true;
 }

toolchain/check/testdata/namespace/fail_conflict_after_merge.carbon

@@ -105,9 +105,9 @@ fn NS();
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %Core.import = import Core
 // CHECK:STDOUT:   %default.import = import <invalid>
-// CHECK:STDOUT:   %.loc8: <namespace> = namespace [template] {}
+// CHECK:STDOUT:   %NS.loc8: <namespace> = namespace [template] {}
 // CHECK:STDOUT:   %.decl.loc20: %.type.1 = fn_decl @.1 [template = constants.%.2] {}
-// CHECK:STDOUT:   %.loc24: <namespace> = namespace [template] {}
+// CHECK:STDOUT:   %NS.loc24: <namespace> = namespace [template] {}
 // CHECK:STDOUT:   %.decl.loc35: %.type.2 = fn_decl @.2 [template = constants.%.3] {}
 // CHECK:STDOUT: }
 // CHECK:STDOUT:

toolchain/check/testdata/namespace/merging.carbon

@@ -108,15 +108,18 @@ fn Run() {
 // CHECK:STDOUT: file {
 // CHECK:STDOUT:   package: <namespace> = namespace [template] {
 // CHECK:STDOUT:     .Core = imports.%Core
-// CHECK:STDOUT:     .NS = %NS
+// CHECK:STDOUT:     .NS = %NS.loc4
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %Core.import = import Core
-// CHECK:STDOUT:   %NS: <namespace> = namespace [template] {
+// CHECK:STDOUT:   %NS.loc4: <namespace> = namespace [template] {
 // CHECK:STDOUT:     .B1 = %B1.decl
 // CHECK:STDOUT:     .B2 = %B2.decl
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %B1.decl: %B1.type = fn_decl @B1 [template = constants.%B1] {}
-// CHECK:STDOUT:   %.loc8: <namespace> = namespace [template] {}
+// CHECK:STDOUT:   %NS.loc8: <namespace> = namespace [template] {
+// CHECK:STDOUT:     .B1 = %B1.decl
+// CHECK:STDOUT:     .B2 = %B2.decl
+// CHECK:STDOUT:   }
 // CHECK:STDOUT:   %B2.decl: %B2.type = fn_decl @B2 [template = constants.%B2] {}
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
@@ -176,7 +179,12 @@ fn Run() {
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %Core.import = import Core
 // CHECK:STDOUT:   %default.import = import <invalid>
-// CHECK:STDOUT:   %.loc7: <namespace> = namespace [template] {}
+// CHECK:STDOUT:   %NS: <namespace> = namespace [template] {
+// CHECK:STDOUT:     .A = imports.%import_ref.2
+// CHECK:STDOUT:     .B1 = imports.%import_ref.3
+// CHECK:STDOUT:     .B2 = imports.%import_ref.4
+// CHECK:STDOUT:     .C = %C.decl
+// CHECK:STDOUT:   }
 // CHECK:STDOUT:   %C.decl: %C.type = fn_decl @C [template = constants.%C] {}
 // CHECK:STDOUT:   %Run.decl: %Run.type = fn_decl @Run [template = constants.%Run] {}
 // CHECK:STDOUT: }

toolchain/check/testdata/namespace/merging_with_indirections.carbon

@@ -117,7 +117,10 @@ fn Run() {
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %Core.import = import Core
 // CHECK:STDOUT:   %default.import = import <invalid>
-// CHECK:STDOUT:   %.loc5: <namespace> = namespace [template] {}
+// CHECK:STDOUT:   %NS1: <namespace> = namespace [template] {
+// CHECK:STDOUT:     .A = imports.%import_ref.2
+// CHECK:STDOUT:     .B = %B.decl
+// CHECK:STDOUT:   }
 // CHECK:STDOUT:   %B.decl: type = class_decl @B [template = constants.%B] {}
 // CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [template = constants.%F] {
 // CHECK:STDOUT:     %NS1.ref: <namespace> = name_ref NS1, imports.%NS1 [template = imports.%NS1]

toolchain/check/testdata/packages/no_prelude/cross_package_import.carbon

@@ -533,7 +533,10 @@ fn UseF() { Other.F(); }
 // CHECK:STDOUT:     .Other = imports.%Other
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %Other.import = import Other
-// CHECK:STDOUT:   %.loc13: <namespace> = namespace [template] {}
+// CHECK:STDOUT:   %Other: <namespace> = namespace [template] {
+// CHECK:STDOUT:     .G = %G.decl
+// CHECK:STDOUT:     import Other//fn
+// CHECK:STDOUT:   }
 // CHECK:STDOUT:   %G.decl: %G.type = fn_decl @G [template = constants.%G] {}
 // CHECK:STDOUT: }
 // CHECK:STDOUT: