Canonicalize imported witness blocks on FacetValue (#6458)

The test has a more complete explanation of this; witnesses on the
FacetValue must have a canonical ordering.

toolchain/check/import_ref.cpp

@@ -3101,12 +3101,17 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
     return ResolveResult::Retry();
   }
 
+  auto witnesses_block_id = SemIR::InstBlockId::None;
+  if (inst.witnesses_block_id.has_value()) {
+    witnesses_block_id =
+        AddCanonicalWitnessesBlock(resolver.local_ir(), witnesses);
+  }
+
   return ResolveResult::Deduplicated<SemIR::FacetValue>(
       resolver,
       {.type_id = resolver.local_types().GetTypeIdForTypeConstantId(type_id),
        .type_inst_id = type_inst_id,
-       .witnesses_block_id = GetLocalCanonicalInstBlockId(
-           resolver, inst.witnesses_block_id, witnesses)});
+       .witnesses_block_id = witnesses_block_id});
 }
 
 static auto TryResolveTypedInst(ImportRefResolver& resolver,

toolchain/check/testdata/impl/import_canonical_witnesses.carbon

@@ -0,0 +1,82 @@
+// 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-FILE: toolchain/testing/testdata/min_prelude/full.carbon
+//
+// This test is producing a case where interface ordering will differ between
+// interfaces.carbon and use.carbon, testing that witnesses are maintained in a
+// canonical order. This tests a regression where the imported ordering would
+// be incorrectly used, resulting in witnesses getting swapped and an opaque
+// error for the `SimpleOp` lookup because `Empty`'s table is empty.
+//
+// The IR is generally not significant, but the important difference is as
+// follows (if these become the same, it means the test is no longer effective):
+//
+// SET-CHECK-SUBSET
+// CHECK:STDOUT: --- interfaces.carbon
+// CHECK:STDOUT:   %facet_type: type = facet_type <@SimpleOp & @Empty> [concrete]
+// CHECK:STDOUT: --- use.carbon
+// CHECK:STDOUT:   %facet_type: type = facet_type <@Empty & @SimpleOp> [concrete]
+//
+// This check does not strictly exclude extra facet types, although that should
+// create different instruction names. We also anticipate diagnostics should be
+// produced if there's a significant error.
+//
+// NOAUTOUPDATE
+// TIP: To test this file alone, run:
+// TIP:   bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/impl/import_canonical_witnesses.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/import_canonical_witnesses.carbon
+
+
+// --- interfaces.carbon
+//@include-in-dumps
+library "[[@TEST_NAME]]";
+
+// An interface with a trivial function and blanket impl for pointers.
+interface SimpleOp {
+  fn Op[self: Self]();
+}
+
+impl forall [T:! type] T as SimpleOp {
+  fn Op[self: Self]() {}
+}
+
+// An empty interface with blanket impl for pointers.
+interface Empty {}
+impl forall [T:! type] T as Empty {}
+
+// Class with a use of `SimpleOp.Op`.
+class InterfaceWrap(T:! SimpleOp & Empty) {
+  fn UseSimpleOp[self: Self](value:! T) {
+    value.(SimpleOp.Op)();
+  }
+}
+
+// Interface combining the above for an indirect `SimpleOp` use through `C`,
+// particularly use an associated constant.
+interface CombinedFacet {
+  let T:! SimpleOp & Empty;
+  fn GetInterfaceWrap[self: Self]() -> InterfaceWrap(T);
+}
+
+// A wrapper providing `CombinedFacet`.
+class TypeWrap(T:! type) {
+  impl as CombinedFacet where .T = T {
+    fn GetInterfaceWrap[self: Self]() -> InterfaceWrap(T) {
+      return {};
+    }
+  }
+}
+
+// --- use.carbon
+//@include-in-dumps
+library "[[@TEST_NAME]]";
+import library "interfaces";
+
+class C {}
+
+fn F(t: TypeWrap(C), c:! C) {
+  t.(CombinedFacet.GetInterfaceWrap)().UseSimpleOp(c);
+}

toolchain/lower/testdata/impl/import_facet.carbon

@@ -0,0 +1,150 @@
+// 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
+//
+// This is testing a facet type that combines two interfaces across multiple
+// import boundaries.
+//
+// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/full.carbon
+//
+// AUTOUPDATE
+// TIP: To test this file alone, run:
+// TIP:   bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/lower/testdata/impl/import_facet.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/lower/testdata/impl/import_facet.carbon
+
+// --- a.carbon
+library "[[@TEST_NAME]]";
+
+class C(T:! Core.Copy & Core.Destroy) {
+  fn GetC[self: Self]() -> T {
+    var value: T;
+    return value;
+  }
+}
+
+// --- b.carbon
+library "[[@TEST_NAME]]";
+export import library "a";
+
+interface I {
+  let T:! Core.Copy & Core.Destroy;
+  fn GetI[self: Self]() -> C(T);
+}
+
+// --- c.carbon
+library "[[@TEST_NAME]]";
+export import library "b";
+
+class D(T:! type) {
+  impl as I where .T = T* {
+    fn GetI[self: Self]() -> C(T*) {
+      return {};
+    }
+  }
+}
+
+// --- d.carbon
+library "[[@TEST_NAME]]";
+import library "c";
+
+fn F(d: D(i32)) -> i32* {
+  return d.(I.GetI)().GetC();
+}
+
+// CHECK:STDOUT: ; ModuleID = 'a.carbon'
+// CHECK:STDOUT: source_filename = "a.carbon"
+// CHECK:STDOUT:
+// CHECK:STDOUT: !llvm.module.flags = !{!0, !1}
+// CHECK:STDOUT: !llvm.dbg.cu = !{!2}
+// CHECK:STDOUT:
+// CHECK:STDOUT: !0 = !{i32 7, !"Dwarf Version", i32 5}
+// CHECK:STDOUT: !1 = !{i32 2, !"Debug Info Version", i32 3}
+// CHECK:STDOUT: !2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "carbon", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
+// CHECK:STDOUT: !3 = !DIFile(filename: "a.carbon", directory: "")
+// CHECK:STDOUT: ; ModuleID = 'b.carbon'
+// CHECK:STDOUT: source_filename = "b.carbon"
+// CHECK:STDOUT:
+// CHECK:STDOUT: !llvm.module.flags = !{!0, !1}
+// CHECK:STDOUT: !llvm.dbg.cu = !{!2}
+// CHECK:STDOUT:
+// CHECK:STDOUT: !0 = !{i32 7, !"Dwarf Version", i32 5}
+// CHECK:STDOUT: !1 = !{i32 2, !"Debug Info Version", i32 3}
+// CHECK:STDOUT: !2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "carbon", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
+// CHECK:STDOUT: !3 = !DIFile(filename: "b.carbon", directory: "")
+// CHECK:STDOUT: ; ModuleID = 'c.carbon'
+// CHECK:STDOUT: source_filename = "c.carbon"
+// CHECK:STDOUT:
+// CHECK:STDOUT: !llvm.module.flags = !{!0, !1}
+// CHECK:STDOUT: !llvm.dbg.cu = !{!2}
+// CHECK:STDOUT:
+// CHECK:STDOUT: !0 = !{i32 7, !"Dwarf Version", i32 5}
+// CHECK:STDOUT: !1 = !{i32 2, !"Debug Info Version", i32 3}
+// CHECK:STDOUT: !2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "carbon", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
+// CHECK:STDOUT: !3 = !DIFile(filename: "c.carbon", directory: "")
+// CHECK:STDOUT: ; ModuleID = 'd.carbon'
+// CHECK:STDOUT: source_filename = "d.carbon"
+// CHECK:STDOUT:
+// CHECK:STDOUT: @C.val.136.anon = internal constant {} zeroinitializer
+// CHECK:STDOUT:
+// CHECK:STDOUT: ; Function Attrs: nounwind
+// CHECK:STDOUT: define ptr @_CF.Main(ptr %d) #0 !dbg !4 {
+// CHECK:STDOUT: entry:
+// CHECK:STDOUT:   %.loc5_21.1.temp = alloca {}, align 8, !dbg !10
+// CHECK:STDOUT:   call void @llvm.lifetime.start.p0(ptr %.loc5_21.1.temp), !dbg !10
+// CHECK:STDOUT:   call void @"_CGetI.D.Main:I.Main.b88d1103f417c6d4"(ptr %.loc5_21.1.temp, ptr %d), !dbg !10
+// CHECK:STDOUT:   %C.GetC.call = call ptr @_CGetC.C.Main.6d3edfd8901fffe5(ptr %.loc5_21.1.temp), !dbg !10
+// CHECK:STDOUT:   ret ptr %C.GetC.call, !dbg !11
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: ; Function Attrs: nocallback nofree nosync nounwind willreturn memory(argmem: readwrite)
+// CHECK:STDOUT: declare void @llvm.lifetime.start.p0(ptr captures(none)) #1
+// CHECK:STDOUT:
+// CHECK:STDOUT: ; Function Attrs: nounwind
+// CHECK:STDOUT: define linkonce_odr void @"_CGetI.D.Main:I.Main.b88d1103f417c6d4"(ptr sret({}) %return, ptr %self) #0 !dbg !12 {
+// CHECK:STDOUT:   call void @llvm.memcpy.p0.p0.i64(ptr align 1 %return, ptr align 1 @C.val.136.anon, i64 0, i1 false), !dbg !16
+// CHECK:STDOUT:   ret void, !dbg !16
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: ; Function Attrs: nounwind
+// CHECK:STDOUT: define linkonce_odr ptr @_CGetC.C.Main.6d3edfd8901fffe5(ptr %self) #0 !dbg !17 {
+// CHECK:STDOUT:   %1 = alloca ptr, align 8, !dbg !21
+// CHECK:STDOUT:   call void @llvm.lifetime.start.p0(ptr %1), !dbg !21
+// CHECK:STDOUT:   %2 = load ptr, ptr %1, align 8, !dbg !22
+// CHECK:STDOUT:   ret ptr %2, !dbg !23
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: ; Function Attrs: nocallback nofree nounwind willreturn memory(argmem: readwrite)
+// CHECK:STDOUT: declare void @llvm.memcpy.p0.p0.i64(ptr noalias writeonly captures(none), ptr noalias readonly captures(none), i64, i1 immarg) #2
+// CHECK:STDOUT:
+// CHECK:STDOUT: attributes #0 = { nounwind }
+// CHECK:STDOUT: attributes #1 = { nocallback nofree nosync nounwind willreturn memory(argmem: readwrite) }
+// CHECK:STDOUT: attributes #2 = { nocallback nofree nounwind willreturn memory(argmem: readwrite) }
+// CHECK:STDOUT:
+// CHECK:STDOUT: !llvm.module.flags = !{!0, !1}
+// CHECK:STDOUT: !llvm.dbg.cu = !{!2}
+// CHECK:STDOUT:
+// CHECK:STDOUT: !0 = !{i32 7, !"Dwarf Version", i32 5}
+// CHECK:STDOUT: !1 = !{i32 2, !"Debug Info Version", i32 3}
+// CHECK:STDOUT: !2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "carbon", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
+// CHECK:STDOUT: !3 = !DIFile(filename: "d.carbon", directory: "")
+// CHECK:STDOUT: !4 = distinct !DISubprogram(name: "F", linkageName: "_CF.Main", scope: null, file: !3, line: 4, type: !5, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !8)
+// CHECK:STDOUT: !5 = !DISubroutineType(types: !6)
+// CHECK:STDOUT: !6 = !{!7, !7}
+// CHECK:STDOUT: !7 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 8)
+// CHECK:STDOUT: !8 = !{!9}
+// CHECK:STDOUT: !9 = !DILocalVariable(arg: 1, scope: !4, type: !7)
+// CHECK:STDOUT: !10 = !DILocation(line: 5, column: 10, scope: !4)
+// CHECK:STDOUT: !11 = !DILocation(line: 5, column: 3, scope: !4)
+// CHECK:STDOUT: !12 = distinct !DISubprogram(name: "GetI", linkageName: "_CGetI.D.Main:I.Main.b88d1103f417c6d4", scope: null, file: !13, line: 6, type: !5, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !14)
+// CHECK:STDOUT: !13 = !DIFile(filename: "c.carbon", directory: "")
+// CHECK:STDOUT: !14 = !{!15}
+// CHECK:STDOUT: !15 = !DILocalVariable(arg: 1, scope: !12, type: !7)
+// CHECK:STDOUT: !16 = !DILocation(line: 7, column: 7, scope: !12)
+// CHECK:STDOUT: !17 = distinct !DISubprogram(name: "GetC", linkageName: "_CGetC.C.Main.6d3edfd8901fffe5", scope: null, file: !18, line: 4, type: !5, spFlags: DISPFlagDefinition, unit: !2, retainedNodes: !19)
+// CHECK:STDOUT: !18 = !DIFile(filename: "a.carbon", directory: "")
+// CHECK:STDOUT: !19 = !{!20}
+// CHECK:STDOUT: !20 = !DILocalVariable(arg: 1, scope: !17, type: !7)
+// CHECK:STDOUT: !21 = !DILocation(line: 5, column: 5, scope: !17)
+// CHECK:STDOUT: !22 = !DILocation(line: 6, column: 12, scope: !17)
+// CHECK:STDOUT: !23 = !DILocation(line: 6, column: 5, scope: !17)

toolchain/sem_ir/facet_type_info.cpp

@@ -6,7 +6,10 @@
 
 #include <tuple>
 
+#include "toolchain/base/kind_switch.h"
+#include "toolchain/sem_ir/file.h"
 #include "toolchain/sem_ir/ids.h"
+#include "toolchain/sem_ir/typed_insts.h"
 
 namespace Carbon::SemIR {
 
@@ -232,4 +235,50 @@ IdentifiedFacetType::IdentifiedFacetType(
   SortAndDeduplicate(required_interfaces_, RequiredLess);
 }
 
+auto AddCanonicalWitnessesBlock(File& sem_ir,
+                                llvm::SmallVector<InstId>& witnesses)
+    -> InstBlockId {
+  // Small blocks don't need to be sorted.
+  if (witnesses.size() <= 1) {
+    return sem_ir.inst_blocks().AddCanonical(witnesses);
+  }
+
+  llvm::SmallVector<std::pair<SpecificInterface, InstId>> sortable;
+  sortable.reserve(witnesses.size());
+
+  // Produce the sorted order based on the witness's SpecificInterface.
+  for (auto witness_id : witnesses) {
+    auto inst = sem_ir.insts().Get(witness_id);
+    CARBON_KIND_SWITCH(inst) {
+      case CARBON_KIND(ImplWitness witness): {
+        auto table =
+            sem_ir.insts().GetAs<ImplWitnessTable>(witness.witness_table_id);
+        sortable.push_back(
+            {sem_ir.impls().Get(table.impl_id).interface, witness_id});
+        break;
+      }
+      case CARBON_KIND(LookupImplWitness witness): {
+        sortable.push_back({sem_ir.specific_interfaces().Get(
+                                witness.query_specific_interface_id),
+                            witness_id});
+        break;
+      }
+      default:
+        CARBON_FATAL("Unhandled inst: {0}", inst);
+    }
+  }
+  llvm::sort(sortable, [](auto& lhs, auto& rhs) {
+    return ImplsLess(lhs.first, rhs.first);
+  });
+
+  // Update the original list with the new order (reusing to avoid an
+  // allocation).
+  for (auto [witness_id, sortable_entry] :
+       llvm::zip_equal(witnesses, sortable)) {
+    witness_id = sortable_entry.second;
+  }
+
+  return sem_ir.inst_blocks().AddCanonical(witnesses);
+}
+
 }  // namespace Carbon::SemIR

toolchain/sem_ir/facet_type_info.h

@@ -15,6 +15,8 @@
 
 namespace Carbon::SemIR {
 
+class File;
+
 #define CARBON_BUILTIN_CONSTRAINT_MASK(X)                  \
   /* Verifies types can use the builtin `type.destroy`. */ \
   X(TypeCanDestroy)
@@ -211,8 +213,12 @@ inline auto CarbonHashValue(const FacetTypeInfo& value, uint64_t seed)
   return static_cast<HashCode>(hasher);
 }
 
-// Declared:
-// (Carbon::HashCode)  (value_ = 12557349131240970624)
+// Given an array of witnesses, sorts them to match the FacetTypeInfo ordering
+// and returns the resulting block ID. This assumes witnesses have already been
+// deduplicated, because it's mainly for imports.
+auto AddCanonicalWitnessesBlock(File& sem_ir,
+                                llvm::SmallVector<InstId>& witnesses)
+    -> InstBlockId;
 
 }  // namespace Carbon::SemIR