Include all symbolic parts in structure comparison (#5247)

Co-authored-by: Dana Jansens <danakj@orodu.net>

toolchain/check/impl_lookup.cpp

@@ -540,16 +540,13 @@ static auto CollectCandidateImplsForQuery(
   }
 
   auto compare = [](auto& lhs, auto& rhs) -> bool {
-    // TODO: Allow Carbon code to provide a priority ordering explicitly. For
-    // now they have all the same priority, so the priority is the order in
-    // which they are found in code.
-
-    // Sort by their type structures. Higher value in type structure comes
-    // first, so we use `>` comparison.
-    return lhs.type_structure > rhs.type_structure;
+    return lhs.type_structure < rhs.type_structure;
   };
   // Stable sort is used so that impls that are seen first are preferred when
   // they have an equal priority ordering.
+  // TODO: Allow Carbon code to provide a priority ordering explicitly. For
+  // now they have all the same priority, so the priority is the order in
+  // which they are found in code.
   llvm::stable_sort(candidate_impls, compare);
 
   return candidate_impls;

toolchain/check/testdata/impl/lookup/min_prelude/specialization.carbon

@@ -458,3 +458,33 @@ fn G[X:! type](p: X*) -> X {
   // CHECK:STDERR:
   return H(p);
 }
+
+// --- type_structure_first_difference.carbon
+
+library "[[@TEST_NAME]]";
+
+interface Z(T:! type) {
+    let X:! type;
+    fn MakeX() -> X;
+}
+
+class C {}
+
+// Type structure: "?(?)"
+impl forall [T:! type] T as Z(T) where .X = {.less_good: ()} {
+    fn MakeX() -> {.less_good: ()} { return {.less_good = ()}; }
+}
+
+// Type structure: "?(c)". Should outrank the previous impl.
+impl forall [T:! type] T as Z(C) where .X = () {
+    fn MakeX() -> () { return (); }
+}
+
+fn F(T:! Z(C)) -> T.(Z(C).X) {
+    return T.MakeX();
+}
+
+fn G() {
+    // This won't typecheck if the first impl is selected.
+    let a: () = F(C);
+}

toolchain/check/type_structure.cpp

@@ -122,7 +122,7 @@ class TypeStructureBuilder {
            SemIR::SpecificInterface interface_constraint) -> TypeStructure {
     CARBON_CHECK(work_list_.empty());
 
-    first_symbolic_distance_ = TypeStructure::InfiniteDistance;
+    symbolic_type_indices_.clear();
     structure_.clear();
 
     // The self type comes first in the type structure, so we push it last, as
@@ -131,8 +131,10 @@ class TypeStructureBuilder {
     PushInstId(self_inst_id);
     BuildTypeStructure();
 
+    // TODO: This requires 4 SmallVector moves (two here and two in the
+    // constructor). Find a way to reduce that.
     return TypeStructure(std::exchange(structure_, {}),
-                         first_symbolic_distance_);
+                         std::exchange(symbolic_type_indices_, {}));
   }
 
  private:
@@ -391,18 +393,14 @@ class TypeStructureBuilder {
   // Append a structural element to the TypeStructure being built.
   auto AppendStructural(TypeStructure::Structural structural) -> void {
     if (structural == TypeStructure::Structural::Symbolic) {
-      // Sets the `distance` in `first_symbolic_distance_` if it does not
-      // already have a non-infinite value.
-      if (first_symbolic_distance_ == TypeStructure::InfiniteDistance) {
-        first_symbolic_distance_ = structure_.size();
-      }
+      symbolic_type_indices_.push_back(structure_.size());
     }
     structure_.push_back(structural);
   }
 
   Context* context_;
   llvm::SmallVector<WorkItem> work_list_;
-  int first_symbolic_distance_;
+  llvm::SmallVector<int> symbolic_type_indices_;
   llvm::SmallVector<TypeStructure::Structural> structure_;
 };
 

toolchain/check/type_structure.h

@@ -5,7 +5,7 @@
 #ifndef CARBON_TOOLCHAIN_CHECK_TYPE_STRUCTURE_H_
 #define CARBON_TOOLCHAIN_CHECK_TYPE_STRUCTURE_H_
 
-#include <compare>
+#include <algorithm>
 
 #include "common/ostream.h"
 #include "toolchain/check/context.h"
@@ -28,25 +28,21 @@ class TypeStructure : public Printable<TypeStructure> {
   // lookup query.
   auto IsCompatibleWith(const TypeStructure& other) const -> bool;
 
-  // Ordering of type structures. A higher value is a better match.
-  friend auto operator<=>(const TypeStructure& lhs, const TypeStructure& rhs)
-      -> std::weak_ordering {
-    // Higher distance is a better match, and `InfiniteDistance` is treated
-    // specially as the best possible match.
-    if (lhs.distance_to_first_symbolic_type_ !=
-        rhs.distance_to_first_symbolic_type_) {
-      if (lhs.distance_to_first_symbolic_type_ == InfiniteDistance) {
-        return std::weak_ordering::greater;
-      } else if (rhs.distance_to_first_symbolic_type_ == InfiniteDistance) {
-        return std::weak_ordering::less;
-      } else {
-        return lhs.distance_to_first_symbolic_type_ <=>
-               rhs.distance_to_first_symbolic_type_;
-      }
-    }
-    // TODO: If they have a symbolic in the same position, we could use further
-    // symbolic types to get an ordering.
-    return std::weak_ordering::equivalent;
+  // Ordering of type structures. A lower value is a better match.
+  // TODO: switch to operator<=> once we can depend on
+  // std::lexicographical_compare_three_way (in particular, once we can
+  // require clang-17 or newer, including in places like the GitHub test
+  // runners).
+  friend auto operator<(const TypeStructure& lhs, const TypeStructure& rhs)
+      -> bool {
+    return std::lexicographical_compare(
+        lhs.symbolic_type_indices_.begin(), lhs.symbolic_type_indices_.end(),
+        rhs.symbolic_type_indices_.begin(), rhs.symbolic_type_indices_.end(),
+        [](int lhs_index, int rhs_index) {
+          // A higher symbolic type index is a better match, so we need to
+          // reverse the order.
+          return rhs_index < lhs_index;
+        });
   }
 
   auto Print(llvm::raw_ostream& out) const -> void {
@@ -82,15 +78,15 @@ class TypeStructure : public Printable<TypeStructure> {
   static constexpr int InfiniteDistance = -1;
 
   TypeStructure(llvm::SmallVector<Structural> structure,
-                int distance_to_first_symbolic_type)
+                llvm::SmallVector<int> symbolic_type_indices)
       : structure_(std::move(structure)),
-        distance_to_first_symbolic_type_(distance_to_first_symbolic_type) {}
+        symbolic_type_indices_(std::move(symbolic_type_indices)) {}
 
   // The structural position of concrete and symbolic values in the type.
   llvm::SmallVector<Structural> structure_;
 
-  // Number of concrete types traversed before finding a symbolic type.
-  int distance_to_first_symbolic_type_;
+  // Indices of the symbolic entries in structure_.
+  llvm::SmallVector<int> symbolic_type_indices_;
 };
 
 // Constructs the TypeStructure for a self type or facet value and an interface