[Sema] Change order of displayed overloads in diagnostics

Make it a strict weak order.

Fixes llvm#64121.

Current implementation uses the definition of ordering from the C++ Standard.
The definition provides only a partial order and cannot be used in sorting
algorithms.

The debug builds of libc++ are capable of detecting that problem
and this failure was found when building Clang with libc++ and
those extra checks enabled, see llvm#64121.

The new ordering is a strict weak order and still
pushes most interesting functions to the start of the list.
In some cases, it leads to better results, e.g.

```
struct Foo {
  operator int();
  operator const char*();
};

void test() { Foo() - Foo(); }
```

Now produces a list with two most relevant builtin operators at the top,
i.e. `operator-(int, int)` and `operator-(const char*, const char*)`.
Previously `operator-(const char*, const char*)` was the first element,
but `operator-(int, int)` was only the 13th element in the output.
This is a consequence of `stable_sort` now being able to compare those
two candidates, which are indistinguishable in the semantic partial order
despite being two local minimums in their respective comparable
subsets.

However, new implementation does not take into account some aspects of
C++ semantics, e.g. which function template is more specialized. This
can also lead to worse ordering sometimes.

Reviewed By: #clang-language-wg, aaron.ballman

Differential Revision: https://reviews.llvm.org/D159351

Author: ilya-biryukov

clang/docs/ReleaseNotes.rst

@@ -356,6 +356,30 @@ Improvements to Clang's diagnostics
   nonportable construct that has different semantics from a constant-sized
   array. (`#62836 <https://github.com/llvm/llvm-project/issues/62836>`_)
 
+- Clang changed the order in which it displays candidate functions on overloading failures.
+  Previously, Clang used definition of ordering from the C++ Standard. The order defined in
+  the Standard is partial and is not suited for sorting. Instead, Clang now uses a strict
+  order that still attempts to push more relevant functions to the top by comparing their
+  corresponding conversions. In some cases, this results in better order. E.g., for the
+  following code
+
+  .. code-block:: cpp
+      struct Foo {
+        operator int();
+        operator const char*();
+      };
+
+      void test() { Foo() - Foo(); }
+
+  Clang now produces a list with two most relevant builtin operators at the top,
+  i.e. ``operator-(int, int)`` and ``operator-(const char*, const char*)``.
+  Previously ``operator-(const char*, const char*)`` was the first element,
+  but ``operator-(int, int)`` was only the 13th element in the output.
+  However, new implementation does not take into account some aspects of
+  C++ semantics, e.g. which function template is more specialized. This
+  can sometimes lead to worse ordering.
+
+
 Bug Fixes in This Version
 -------------------------
 - Fixed an issue where a class template specialization whose declaration is

clang/lib/Sema/SemaOverload.cpp

@@ -38,8 +38,10 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 #include <algorithm>
+#include <cstddef>
 #include <cstdlib>
 #include <optional>
 
@@ -12017,6 +12019,7 @@ static unsigned RankDeductionFailure(const DeductionFailureInfo &DFI) {
 }
 
 namespace {
+
 struct CompareOverloadCandidatesForDisplay {
   Sema &S;
   SourceLocation Loc;
@@ -12054,13 +12057,9 @@ struct CompareOverloadCandidatesForDisplay {
     if (L->Viable) {
       if (!R->Viable) return true;
 
-      // TODO: introduce a tri-valued comparison for overload
-      // candidates.  Would be more worthwhile if we had a sort
-      // that could exploit it.
-      if (isBetterOverloadCandidate(S, *L, *R, SourceLocation(), CSK))
-        return true;
-      if (isBetterOverloadCandidate(S, *R, *L, SourceLocation(), CSK))
-        return false;
+      if (int Ord = CompareConversions(*L, *R))
+        return Ord < 0;
+      // Use other tie breakers.
     } else if (R->Viable)
       return false;
 
@@ -12112,30 +12111,8 @@ struct CompareOverloadCandidatesForDisplay {
         }
 
         // If there's any ordering between the defined conversions...
-        // FIXME: this might not be transitive.
-        assert(L->Conversions.size() == R->Conversions.size());
-
-        int leftBetter = 0;
-        unsigned I = (L->IgnoreObjectArgument || R->IgnoreObjectArgument);
-        for (unsigned E = L->Conversions.size(); I != E; ++I) {
-          switch (CompareImplicitConversionSequences(S, Loc,
-                                                     L->Conversions[I],
-                                                     R->Conversions[I])) {
-          case ImplicitConversionSequence::Better:
-            leftBetter++;
-            break;
-
-          case ImplicitConversionSequence::Worse:
-            leftBetter--;
-            break;
-
-          case ImplicitConversionSequence::Indistinguishable:
-            break;
-          }
-        }
-        if (leftBetter > 0) return true;
-        if (leftBetter < 0) return false;
-
+        if (int Ord = CompareConversions(*L, *R))
+          return Ord < 0;
       } else if (RFailureKind == ovl_fail_bad_conversion)
         return false;
 
@@ -12157,10 +12134,66 @@ struct CompareOverloadCandidatesForDisplay {
     SourceLocation RLoc = GetLocationForCandidate(R);
 
     // Put candidates without locations (e.g. builtins) at the end.
-    if (LLoc.isInvalid()) return false;
-    if (RLoc.isInvalid()) return true;
+    if (LLoc.isValid() && RLoc.isValid())
+      return S.SourceMgr.isBeforeInTranslationUnit(LLoc, RLoc);
+    if (LLoc.isValid() && !RLoc.isValid())
+      return true;
+    if (RLoc.isValid() && !LLoc.isValid())
+      return false;
+    assert(!LLoc.isValid() && !RLoc.isValid());
+    // For builtins and other functions without locations, fallback to the order
+    // in which they were added into the candidate set.
+    return L < R;
+  }
 
-    return S.SourceMgr.isBeforeInTranslationUnit(LLoc, RLoc);
+private:
+  struct ConversionSignals {
+    unsigned KindRank = 0;
+    ImplicitConversionRank Rank = ICR_Exact_Match;
+
+    static ConversionSignals ForSequence(ImplicitConversionSequence &Seq) {
+      ConversionSignals Sig;
+      Sig.KindRank = Seq.getKindRank();
+      if (Seq.isStandard())
+        Sig.Rank = Seq.Standard.getRank();
+      else if (Seq.isUserDefined())
+        Sig.Rank = Seq.UserDefined.After.getRank();
+      // We intend StaticObjectArgumentConversion to compare the same as
+      // StandardConversion with ICR_ExactMatch rank.
+      return Sig;
+    }
+
+    static ConversionSignals ForObjectArgument() {
+      // We intend StaticObjectArgumentConversion to compare the same as
+      // StandardConversion with ICR_ExactMatch rank. Default give us that.
+      return {};
+    }
+  };
+
+  // Returns -1 if conversions in L are considered better.
+  //          0 if they are considered indistinguishable.
+  //          1 if conversions in R are better.
+  int CompareConversions(const OverloadCandidate &L,
+                         const OverloadCandidate &R) {
+    // We cannot use `isBetterOverloadCandidate` because it is defined
+    // according to the C++ standard and provides a partial order, but we need
+    // a total order as this function is used in sort.
+    assert(L.Conversions.size() == R.Conversions.size());
+    for (unsigned I = 0, N = L.Conversions.size(); I != N; ++I) {
+      auto LS = L.IgnoreObjectArgument && I == 0
+                    ? ConversionSignals::ForObjectArgument()
+                    : ConversionSignals::ForSequence(L.Conversions[I]);
+      auto RS = R.IgnoreObjectArgument
+                    ? ConversionSignals::ForObjectArgument()
+                    : ConversionSignals::ForSequence(R.Conversions[I]);
+      if (std::tie(LS.KindRank, LS.Rank) != std::tie(RS.KindRank, RS.Rank))
+        return std::tie(LS.KindRank, LS.Rank) < std::tie(RS.KindRank, RS.Rank)
+                   ? -1
+                   : 1;
+    }
+    // FIXME: find a way to compare templates for being more or less
+    // specialized that provides a strict weak ordering.
+    return 0;
   }
 };
 }

clang/test/SemaCXX/overloaded-operators-display-order-crash.cpp

@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: not %clang_cc1 -fsyntax-only %s 2>&1 | FileCheck %s
+
+namespace ambig {
+  struct Foo {
+    operator int();
+    operator const char *();
+  };
+
+
+  void func(const char*, long);
+  void func(const char*, const char*);
+  void func(int, int);
+
+  bool doit(Foo x) {
+    func(x, x); // expected-error {{call to 'func' is ambiguous}}
+                // expected-note@* 3{{candidate}}
+    // Check that two functions with best conversions are at the top.
+    // CHECK: error: call to 'func' is ambiguous
+    // CHECK-NEXT: func(x, x)
+    // CHECK-NEXT: ^~~~
+    // CHECK-NEXT: note: candidate function
+    // CHECK-NEXT: void func(const char*, const char*)
+    // CHECK-NEXT: ^
+    // CHECK-NEXT: note: candidate function
+    // CHECK-NEXT: void func(int, int)
+  }
+}
+
+namespace bad_conversion {
+  struct Foo {
+    operator int();
+    operator const char *();
+  };
+
+
+  void func(double*, const char*, long);
+  void func(double*, const char*, const char*);
+  void func(double*, int, int);
+
+  bool doit(Foo x) {
+    func((int*)0, x, x); // expected-error {{no matching function for call to 'func'}}
+                         // expected-note@* 3{{candidate}}
+    // Check that two functions with best conversions are at the top.
+    // CHECK: error: no matching function for call to 'func'
+    // CHECK-NEXT: func((int*)0, x, x)
+    // CHECK-NEXT: ^~~~
+    // CHECK-NEXT: note: candidate function
+    // CHECK-NEXT: void func(double*, const char*, const char*)
+    // CHECK-NEXT: ^
+    // CHECK-NEXT: note: candidate function
+    // CHECK-NEXT: void func(double*, int, int)
+  }
+}