[clang] Handle templated operators with reversed arguments #69595

usx95 · 2023-10-19T12:12:54Z

#68999 correctly computed conversion sequence for reversed args to a template operators. This was a breaking change as code, previously accepted in C++17, starts to break in C++20.

Example:

struct P {};
template<class S> bool operator==(const P&, const S &);

struct A : public P {};
struct B : public P {};
bool check(A a, B b) { return a == b; }  // This is now ambiguous in C++20.

In order to minimise widespread breakages, as a clang extension, we had previously accepted such ambiguities with a warning (-Wambiguous-reversed-operator) for non-template operators. Due to the same reasons, we extend this relaxation for template operators.

Fixes #53954

…d param order (llvm#68999)" This reverts commit a3a0f59.

… operators

llvmbot · 2023-10-19T12:14:31Z

@llvm/pr-subscribers-clang

Author: Utkarsh Saxena (usx95)

Changes

#68999 correctly computed conversion sequence for reversed args to a template operators. This was a breaking change as code, previously accepted in C++17, starts to break in C++20.

Example:

struct P {};
template&lt;class S&gt; bool operator==(const P&amp;, const S &amp;);

struct A : public P {};
struct B : public P {};
bool check(A a, B b) { return a == b; }  // This is now ambiguous in C++20.

In order to minimise widespread breakages, as a clang extension, we had previously accepted such ambiguities with a warning (-Wambiguous-reversed-operator) for non-template operators. Due to the same reasons, we extend this relaxation for template operators.

Fixes #53954

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+16)
(modified) clang/lib/Sema/SemaOverload.cpp (+8-4)
(modified) clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp (+57)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index eee48431d716878..a38cd74b6165c84 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -37,6 +37,22 @@ These changes are ones which we think may surprise users when upgrading to
 Clang |release| because of the opportunity they pose for disruption to existing
 code bases.
 
+- Fix a bug in reversed argument for templated operators.
+  This breaks code in C++20 which was previously accepted in C++17. Eg:
+  ```
+  struct P {};
+  template<class S> bool operator==(const P&, const S &);
+
+  struct A : public P {};
+  struct B : public P {};
+  // This equality is now ambiguous in C++20.
+  bool check(A a, B b) { return a == b; } 
+  ```
+  To reduce widespread breakages, as an extension, clang would accept this with a
+  `-Wambiguous-reversed-operator` warning.
+  Fixes `GH <https://github.com/llvm/llvm-project/issues/53954>`_.
+
+
 
 C/C++ Language Potentially Breaking Changes
 -------------------------------------------
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index ce78994e6553814..53de7cb783a6ced 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -7688,7 +7688,7 @@ bool Sema::CheckNonDependentConversions(
     QualType ParamType = ParamTypes[I + Offset];
     if (!ParamType->isDependentType()) {
       unsigned ConvIdx = PO == OverloadCandidateParamOrder::Reversed
-                             ? 0
+                             ? Args.size() - 1 - (ThisConversions + I)
                              : (ThisConversions + I);
       Conversions[ConvIdx]
         = TryCopyInitialization(*this, Args[I], ParamType,
@@ -10085,10 +10085,14 @@ getImplicitObjectParamType(ASTContext &Context, const FunctionDecl *F) {
   return M->getFunctionObjectParameterReferenceType();
 }
 
-static bool haveSameParameterTypes(ASTContext &Context, const FunctionDecl *F1,
+static bool allowAmbiguityWithSelf(ASTContext &Context, const FunctionDecl *F1,
                                    const FunctionDecl *F2) {
   if (declaresSameEntity(F1, F2))
     return true;
+  if (F1->isTemplateInstantiation() && F2->isTemplateInstantiation() &&
+      declaresSameEntity(F1->getPrimaryTemplate(), F2->getPrimaryTemplate())) {
+    return true;
+  }
 
   auto NextParam = [&](const FunctionDecl *F, unsigned &I, bool First) {
     if (First) {
@@ -10274,7 +10278,7 @@ bool clang::isBetterOverloadCandidate(
     case ImplicitConversionSequence::Worse:
       if (Cand1.Function && Cand2.Function &&
           Cand1.isReversed() != Cand2.isReversed() &&
-          haveSameParameterTypes(S.Context, Cand1.Function, Cand2.Function)) {
+          allowAmbiguityWithSelf(S.Context, Cand1.Function, Cand2.Function)) {
         // Work around large-scale breakage caused by considering reversed
         // forms of operator== in C++20:
         //
@@ -14421,7 +14425,7 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation OpLoc,
           llvm::SmallVector<FunctionDecl*, 4> AmbiguousWith;
           for (OverloadCandidate &Cand : CandidateSet) {
             if (Cand.Viable && Cand.Function && Cand.isReversed() &&
-                haveSameParameterTypes(Context, Cand.Function, FnDecl)) {
+                allowAmbiguityWithSelf(Context, Cand.Function, FnDecl)) {
               for (unsigned ArgIdx = 0; ArgIdx < 2; ++ArgIdx) {
                 if (CompareImplicitConversionSequences(
                         *this, OpLoc, Cand.Conversions[ArgIdx],
diff --git a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
index 5c6804eb7726b5f..e075580828f42e5 100644
--- a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
+++ b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
@@ -324,6 +324,63 @@ bool x = X() == X(); // expected-warning {{ambiguous}}
 }
 } // namespace P2468R2
 
+namespace GH53954{
+namespace friend_template_1 {
+struct P {
+    template <class T>
+    friend bool operator==(const P&, const T&); // expected-note {{candidate}} \
+                                                // expected-note {{ambiguous candidate function with reversed arguments}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-warning {{use of overloaded operator '==' (with operand types 'A' and 'B') to be ambiguous}}
+}
+
+namespace friend_template_2 {
+struct P {
+    template <class T>
+    friend bool operator==(const T&, const P&); // expected-note {{candidate}} \
+                                                // expected-note {{ambiguous candidate function with reversed arguments}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-warning {{use of overloaded operator '==' (with operand types 'A' and 'B') to be ambiguous}}
+}
+
+namespace member_template {
+struct P {
+  template<class S>
+  bool operator==(const S &) const; // expected-note {{candidate}} \
+                                    // expected-note {{ambiguous candidate function with reversed arguments}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-warning {{use of overloaded operator '==' (with operand types 'A' and 'B') to be ambiguous}}
+}
+
+namespace non_member_template_1 {
+struct P {};
+template<class S>
+bool operator==(const P&, const S &); // expected-note {{candidate}} \
+                                      // expected-note {{ambiguous candidate function with reversed arguments}}
+
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-warning {{use of overloaded operator '==' (with operand types 'A' and 'B') to be ambiguous}}
+}
+}
+
+namespace non_member_template_2 {
+struct P {};
+template<class S>
+bool operator==(const S&, const P&); // expected-note {{candidate}} \
+                                     // expected-note {{ambiguous candidate function with reversed arguments}}
+
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-warning {{use of overloaded operator '==' (with operand types 'A' and 'B') to be ambiguous}}
+}
+
 #else // NO_ERRORS
 
 namespace problem_cases {

clang/docs/ReleaseNotes.rst

clang/lib/Sema/SemaOverload.cpp

ilya-biryukov

The change looks good, I believe the comments about potentially redundant checks and naming of the function could be addressed in a follow-up.

…lvm#69595)" This reverts commit 47747da.

…72213) Re-applies #69595 with extra [diff](79181ef) ### New changes Further relax ambiguities with a warning for member operators of a template class (primary templates of such ops do not match). Eg: ```cpp template <class T> struct S { template <typename OtherT> bool operator==(const OtherT &rhs); }; struct A : S<int> {}; struct B : S<bool> {}; bool x = A{} == B{}; // accepted with a warning. ``` This is important for making llvm build using previous clang versions in C++20 mode (eg: this makes the commit e558be5 keep working with a warning instead of an error). ### Description from #69595 #68999 correctly computed conversion sequence for reversed args to a template operator. This was a breaking change as code, previously accepted in C++17, starts to break in C++20. Example: ```cpp struct P {}; template<class S> bool operator==(const P&, const S &); struct A : public P {}; struct B : public P {}; bool check(A a, B b) { return a == b; } // This is now ambiguous in C++20. ``` In order to minimise widespread breakages, as a clang extension, we had previously accepted such ambiguities with a warning (`-Wambiguous-reversed-operator`) for non-template operators. Due to the same reasons, we extend this relaxation for template operators. Fixes #53954

…lvm#72213) Re-applies llvm#69595 with extra [diff](llvm@79181ef) ### New changes Further relax ambiguities with a warning for member operators of a template class (primary templates of such ops do not match). Eg: ```cpp template <class T> struct S { template <typename OtherT> bool operator==(const OtherT &rhs); }; struct A : S<int> {}; struct B : S<bool> {}; bool x = A{} == B{}; // accepted with a warning. ``` This is important for making llvm build using previous clang versions in C++20 mode (eg: this makes the commit e558be5 keep working with a warning instead of an error). ### Description from llvm#69595 llvm#68999 correctly computed conversion sequence for reversed args to a template operator. This was a breaking change as code, previously accepted in C++17, starts to break in C++20. Example: ```cpp struct P {}; template<class S> bool operator==(const P&, const S &); struct A : public P {}; struct B : public P {}; bool check(A a, B b) { return a == b; } // This is now ambiguous in C++20. ``` In order to minimise widespread breakages, as a clang extension, we had previously accepted such ambiguities with a warning (`-Wambiguous-reversed-operator`) for non-template operators. Due to the same reasons, we extend this relaxation for template operators. Fixes llvm#53954

usx95 added 2 commits October 19, 2023 12:33

Reapply "Correctly compute conversion seq for args to fn with reverse…

4460b02

…d param order (llvm#68999)" This reverts commit a3a0f59.

Do not hard error for ambiguity with reversed arguments for templated…

12f5b5e

… operators

usx95 requested review from zygoloid and ilya-biryukov October 19, 2023 12:12

usx95 self-assigned this Oct 19, 2023

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 19, 2023

usx95 added 2 commits October 19, 2023 14:49

Add more test and update docs

7b7a53d

fix release notes

ab3a6d8

ilya-biryukov reviewed Oct 20, 2023

View reviewed changes

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

ilya-biryukov reviewed Oct 20, 2023

View reviewed changes

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

clang/lib/Sema/SemaOverload.cpp Show resolved Hide resolved

clang/lib/Sema/SemaOverload.cpp Outdated Show resolved Hide resolved

usx95 added 2 commits October 20, 2023 13:24

address comments

5cb02ab

fix line wrapping

b6ddecd

ilya-biryukov approved these changes Oct 20, 2023

View reviewed changes

usx95 merged commit 47747da into llvm:main Oct 20, 2023

usx95 added a commit to usx95/llvm-project that referenced this pull request Oct 23, 2023

Revert "[clang] Handle templated operators with reversed arguments (l…

6942679

…lvm#69595)" This reverts commit 47747da.

spupyrev mentioned this pull request Oct 23, 2023

[BOLT] Rename cds to cdsort #69966

Merged

banach-space mentioned this pull request Oct 24, 2023

[mlir][vector] Add scalable vectors to tests for vector.contract #70039

Merged

usx95 mentioned this pull request Nov 14, 2023

[clang] Reapply Handle templated operators with reversed arguments #72213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] Handle templated operators with reversed arguments #69595

[clang] Handle templated operators with reversed arguments #69595

Uh oh!

usx95 commented Oct 19, 2023

Uh oh!

llvmbot commented Oct 19, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilya-biryukov left a comment

Uh oh!

Uh oh!

[clang] Handle templated operators with reversed arguments #69595

[clang] Handle templated operators with reversed arguments #69595

Uh oh!

Conversation

usx95 commented Oct 19, 2023

Uh oh!

llvmbot commented Oct 19, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilya-biryukov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!