Canonicalize imported witness blocks

Author: jonmeow

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 =
+        GetCanonicalWitnessesBlock(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 checks 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/sem_ir/facet_type_info.cpp

@@ -6,7 +6,9 @@
 
 #include <tuple>
 
+#include "toolchain/sem_ir/file.h"
 #include "toolchain/sem_ir/ids.h"
+#include "toolchain/sem_ir/typed_insts.h"
 
 namespace Carbon::SemIR {
 
@@ -232,4 +234,36 @@ IdentifiedFacetType::IdentifiedFacetType(
   SortAndDeduplicate(required_interfaces_, RequiredLess);
 }
 
+auto GetCanonicalWitnessesBlock(File& sem_ir,
+                                llvm::SmallVector<SemIR::InstId>& witnesses)
+    -> SemIR::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, SemIR::InstId>> sortable;
+  sortable.reserve(witnesses.size());
+
+  // Produce the sorted order based on the witness's SpecificInterface.
+  for (auto witness_id : witnesses) {
+    auto witness = sem_ir.insts().GetAs<SemIR::LookupImplWitness>(witness_id);
+    sortable.push_back(
+        {sem_ir.specific_interfaces().Get(witness.query_specific_interface_id),
+         witness_id});
+  }
+  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,11 @@ 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.
+auto GetCanonicalWitnessesBlock(File& sem_ir,
+                                llvm::SmallVector<SemIR::InstId>& witnesses)
+    -> SemIR::InstBlockId;
 
 }  // namespace Carbon::SemIR