[Clang] Do not assume a perfect match is a better match than a non-template non-perfect match

[cor3ntin]

This fixes a regression introduced by the "perfect match" overload resolution mechanism introduced in 8c5a307.

This does regress the performance noticeably (-0.7% for a stage 2 build), however, the original patch had a +4% performance impact, so we are only losing some of the gain, and this has
the benefit of being correct and more robust.

Fixes #147374

[llvmbot]

@llvm/pr-subscribers-clang

Author: Corentin Jabot (cor3ntin)

Changes

This fixes a regression introduced by the "perfect match" overload resolution mechanism introduced in 8c5a307.

This does regress the performance noticeably (-0.7% for a stage 2 build), however, the original patch had a +4% performance impact, so we are only losing some of the gain, and this has
the benefit of being correct and more robust.

Fixes #147374

---

Full diff: https://github.com/llvm/llvm-project/pull/149504.diff

3 Files Affected:

• (modified) clang/include/clang/Sema/Overload.h (-2)
• (modified) clang/lib/Sema/SemaOverload.cpp (+7-44)
• (modified) clang/test/SemaCXX/overload-resolution-deferred-templates.cpp (+28)

diff --git a/clang/include/clang/Sema/Overload.h b/clang/include/clang/Sema/Overload.h
index a70335bef9dd4..d34a4146ddbd6 100644
--- a/clang/include/clang/Sema/Overload.h
+++ b/clang/include/clang/Sema/Overload.h
@@ -1491,8 +1491,6 @@ class Sema;
     OverloadingResult
     BestViableFunctionImpl(Sema &S, SourceLocation Loc,
                            OverloadCandidateSet::iterator &Best);
-    void PerfectViableFunction(Sema &S, SourceLocation Loc,
-                               OverloadCandidateSet::iterator &Best);
   };
 
   bool isBetterOverloadCandidate(Sema &S, const OverloadCandidate &Cand1,
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 1b54628c5e564..cf9bd1b71ba63 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -11353,56 +11353,19 @@ OverloadingResult OverloadCandidateSet::BestViableFunction(Sema &S,
   bool TwoPhaseResolution =
       DeferredCandidatesCount != 0 && !ResolutionByPerfectCandidateIsDisabled;
 
-  if (TwoPhaseResolution) {
-
-    PerfectViableFunction(S, Loc, Best);
-    if (Best != end())
-      return ResultForBestCandidate(Best);
+  if(TwoPhaseResolution) {
+      OverloadingResult Res = BestViableFunctionImpl(S, Loc, Best);
+      if (Best != end() && Best->isPerfectMatch(S.Context)) {
+          if(!(HasDeferredTemplateConstructors &&
+               isa_and_nonnull<CXXConversionDecl>(Best->Function)))
+          return Res;
+      }
   }
 
   InjectNonDeducedTemplateCandidates(S);
   return BestViableFunctionImpl(S, Loc, Best);
 }
 
-void OverloadCandidateSet::PerfectViableFunction(
-    Sema &S, SourceLocation Loc, OverloadCandidateSet::iterator &Best) {
-
-  Best = end();
-  for (auto It = Candidates.begin(); It != Candidates.end(); ++It) {
-
-    if (!It->isPerfectMatch(S.getASTContext()))
-      continue;
-
-    // We found a suitable conversion function
-    // but if there is a template constructor in the target class
-    // we might prefer that instead.
-    if (HasDeferredTemplateConstructors &&
-        isa_and_nonnull<CXXConversionDecl>(It->Function)) {
-      Best = end();
-      break;
-    }
-
-    if (Best == end()) {
-      Best = It;
-      continue;
-    }
-    if (Best->Function && It->Function) {
-      FunctionDecl *D =
-          S.getMoreConstrainedFunction(Best->Function, It->Function);
-      if (D == nullptr) {
-        Best = end();
-        break;
-      }
-      if (D == It->Function)
-        Best = It;
-      continue;
-    }
-    // ambiguous
-    Best = end();
-    break;
-  }
-}
-
 OverloadingResult OverloadCandidateSet::BestViableFunctionImpl(
     Sema &S, SourceLocation Loc, OverloadCandidateSet::iterator &Best) {
 
diff --git a/clang/test/SemaCXX/overload-resolution-deferred-templates.cpp b/clang/test/SemaCXX/overload-resolution-deferred-templates.cpp
index 46c3670848529..135865c8450f5 100644
--- a/clang/test/SemaCXX/overload-resolution-deferred-templates.cpp
+++ b/clang/test/SemaCXX/overload-resolution-deferred-templates.cpp
@@ -283,3 +283,31 @@ void f() {
 }
 
 #endif
+
+namespace GH147374 {
+
+struct String {};
+template <typename T> void operator+(T, String &&) = delete;
+
+struct Bar {
+    void operator+(String) const; // expected-note {{candidate function}}
+    friend void operator+(Bar, String) {};  // expected-note {{candidate function}}
+};
+
+struct Baz {
+    void operator+(String); // expected-note {{candidate function}}
+    friend void operator+(Baz, String) {}; // expected-note {{candidate function}}
+};
+
+void test() {
+    Bar a;
+    String b;
+    a + b;
+    //expected-error@-1 {{use of overloaded operator '+' is ambiguous (with operand types 'Bar' and 'String')}}
+
+    Baz z;
+    z + b;
+    //expected-error@-1 {{use of overloaded operator '+' is ambiguous (with operand types 'Baz' and 'String')}}
+}
+
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.