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

usx95 · 2023-11-14T06:57:06Z

Re-applies #69595 with extra diff

New changes

Further relax ambiguities with a warning for member operators of a template class (primary templates of such ops do not match). Eg:

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:

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

llvmbot · 2023-11-14T06:57:34Z

@llvm/pr-subscribers-clang

Author: Utkarsh Saxena (usx95)

Changes

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+21)
(modified) clang/lib/Sema/SemaOverload.cpp (+22-10)
(modified) clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp (+73)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7a131cb520aa600..5c974eec9ed67d1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -37,6 +37,27 @@ 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:
+
+  .. code-block:: cpp
+
+    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 ambiguous(A a, B b) { return a == b; }
+
+    template<class S> bool operator!=(const P&, const S&);
+    // Ok. Found a matching operator!=.
+    bool fine(A a, B b) { return a == b; }
+
+  To reduce such widespread breakages, as an extension, Clang accepts this code
+  with an existing warning ``-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 f97e244120612e3..9d50a5ec39f817a 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -7685,7 +7685,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,
@@ -10082,11 +10082,23 @@ getImplicitObjectParamType(ASTContext &Context, const FunctionDecl *F) {
   return M->getFunctionObjectParameterReferenceType();
 }
 
-static bool haveSameParameterTypes(ASTContext &Context, const FunctionDecl *F1,
-                                   const FunctionDecl *F2) {
+// As a Clang extension, allow ambiguity among F1 and F2 if they represent
+// represent the same entity.
+static bool allowAmbiguity(ASTContext &Context, const FunctionDecl *F1,
+                           const FunctionDecl *F2) {
   if (declaresSameEntity(F1, F2))
     return true;
-
+  if (F1->isTemplateInstantiation() && F2->isTemplateInstantiation()) {
+    auto PT1 = F1->getPrimaryTemplate();
+    auto PT2 = F2->getPrimaryTemplate();
+    if (declaresSameEntity(PT1, PT2) ||
+        declaresSameEntity(PT1->getInstantiatedFromMemberTemplate(),
+                           PT2->getInstantiatedFromMemberTemplate()))
+      return true;
+  }
+  // TODO: It is not clear whether comparing parameters is necessary (i.e.
+  // different functions with same params). Consider removing this (as no test
+  // fail w/o it).
   auto NextParam = [&](const FunctionDecl *F, unsigned &I, bool First) {
     if (First) {
       if (std::optional<QualType> T = getImplicitObjectParamType(Context, F))
@@ -10271,14 +10283,14 @@ bool clang::isBetterOverloadCandidate(
     case ImplicitConversionSequence::Worse:
       if (Cand1.Function && Cand2.Function &&
           Cand1.isReversed() != Cand2.isReversed() &&
-          haveSameParameterTypes(S.Context, Cand1.Function, Cand2.Function)) {
+          allowAmbiguity(S.Context, Cand1.Function, Cand2.Function)) {
         // Work around large-scale breakage caused by considering reversed
         // forms of operator== in C++20:
         //
-        // When comparing a function against a reversed function with the same
-        // parameter types, if we have a better conversion for one argument and
-        // a worse conversion for the other, the implicit conversion sequences
-        // are treated as being equally good.
+        // When comparing a function against a reversed function, if we have a
+        // better conversion for one argument and a worse conversion for the
+        // other, the implicit conversion sequences are treated as being equally
+        // good.
         //
         // This prevents a comparison function from being considered ambiguous
         // with a reversed form that is written in the same way.
@@ -14449,7 +14461,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)) {
+                allowAmbiguity(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 d83a176ec07eec9..486172e5d10232b 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,79 @@ 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}}
+
+template<class S>
+bool operator!=(const P&, const S &);
+bool fine(A a, B b) { return a == b; } // Ok. Found a matching operator!=.
+}
+}
+
+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}}
+}
+
+namespace class_and_member_template {
+template <class T>
+struct S {
+    template <typename OtherT>
+    bool operator==(const OtherT &rhs); // expected-note {{candidate}} \
+                                        // expected-note {{reversed arguments}}
+};
+struct A : S<int> {};
+struct B : S<bool> {};
+bool x = A{} == B{}; // expected-warning {{ambiguous}}
+}
+
 namespace ADL_GH68901{
 namespace test1 {
 namespace A {

github-actions · 2023-11-14T06:59:47Z

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

clang/lib/Sema/SemaOverload.cpp

usx95 · 2023-11-15T06:37:03Z

I ran this change internally and noticed a crash for

template <typename T>
class Foo {
 public:
  template <typename U = T>
  bool operator==(const Foo& other) const;
};

bool x = Foo<int>{} == Foo<int>{};

Edit: This is fixed now.

shafik · 2023-11-17T06:33:29Z

So currently implementations differ here: https://godbolt.org/z/11331KW6e

This feels related to cwg2804 which is not live yet but can be found here. I can find the discussion on this from Kona but we ended up sending this to EWG b/c it was not clear what the right fixes for all the related issues are and we wanted design feedback.

CC @zygoloid @AaronBallman

usx95 · 2023-11-17T08:20:12Z

This PR is not completely related to cwg2804. We are already aware of the mentioned issue and are tracking it in #70210.

This PR is different, and it fixes an implementation bug in rewriting template non-member operators (the conversion seq was computed incorrectly and associated with an incorrect index). The difference in compilers (godbolt) you currently see is because of this bug, and clang would also correctly consider the example as ambiguous.

usx95 · 2023-11-17T09:10:42Z

I wanted to note down the challenges of landing this and the strategy to move forward.

Challenges

Landing this PR would make old code ambiguous (example). This is not unexpected, and it is ambiguous in C++20.
This would break LLVM in C++20 (breakage). We do not want to break LLVM at head.
But it would not be possible to resolve ambiguity also for template friend functions because of cwg2804.

In order to resolve this ambiguity, one solution is to make the operator non-friend and add a matching operator!= (P2468R2). This is done in [STLExtras] Add out-of-line definition of friend operator== for C++20 #72348. The solution would look like:

namespace N{
struct P {};
template<class S> bool operator==(const P&, const S &);
template<class S> bool operator!=(const P&, const S &);
struct A : public P {};
struct B : public P {};
}
bool x = N::A{} == N::B{};

The catch is that clang only recently fixed its (P2468R2) implementation for operators== found through ADL in 9330261.
Without the above fix, LLVM would still be broken in clang-17 (because clang-17 doesn't have 9330261).

Strategy

Wait for cherrypicking PR for llvm/llvm-project#68901 llvm-project-release-prs#778 into clang-17.0.6.
Land [STLExtras] Add out-of-line definition of friend operator== for C++20 #72348 to unbreak LLVM.
Update the latest revision to build LLVM in C++20 to 17.0.6. Also update C++20 build bots.
Land this PR [clang] Reapply Handle templated operators with reversed arguments #72213
Implement the cwg2804 proposal when it is finalized.
clang-18 would have correct behavior for template operators, i.e., correct ambiguities surfaced with the option of resolving through adding matching operator!=.

…#72348) The last attempt at #72220 was reverted by 94d6699 because it breaks C++20 build in clang-17 and before. This is a workaround of #70210 and unblocks #72213 which rectifies rewriting template operator and thus introduces new breakages. Moving the function definition out of the class makes clang find a matching `operator!=` for the `operator==`. This makes clang not rewrite the `operator==` with reversed args. Hence, the ambiguity is resolved. The final plan, when #70210 is fixed, is to move these back to inline definition or even convert to a member template operator. This should not be urgent and could even wait for a major clang release including #72213

usx95 · 2024-01-11T20:26:58Z

All the blockers to land this have been addressed. Since this is not approved, does it look fine to land @ilya-biryukov ?

ilya-biryukov

I only have minor requests to add a new test and update the release note a bit.
Otherwise LGMT

clang/docs/ReleaseNotes.rst

clang/lib/Sema/SemaOverload.cpp

ilya-biryukov · 2024-01-12T10:01:09Z

clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp

+}
+
+namespace friend_template_fixme {
+// FIXME(GH70210): This should not rewrite operator== and definitely not a hard error.


I just wanted to point out that this bug is worrying as it might break some code that's not easy to rewrite.
We worked around the problem in LLVM with 77f2ccb, but you could already see how the rewrite is complicated.

I don't think we should block on that, especially given that the issue you mention requires further progress from EWG and CWG. But I would keep a close eye for people complaining about this change.

…llvm#72348) The last attempt at llvm#72220 was reverted by llvm@94d6699 because it breaks C++20 build in clang-17 and before. This is a workaround of llvm#70210 and unblocks llvm#72213 which rectifies rewriting template operator and thus introduces new breakages. Moving the function definition out of the class makes clang find a matching `operator!=` for the `operator==`. This makes clang not rewrite the `operator==` with reversed args. Hence, the ambiguity is resolved. The final plan, when llvm#70210 is fixed, is to move these back to inline definition or even convert to a member template operator. This should not be urgent and could even wait for a major clang release including llvm#72213

…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

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

usx95 requested a review from ilya-biryukov November 14, 2023 07:58

usx95 self-assigned this Nov 14, 2023

usx95 changed the title ~~Templated equals take 2~~ [clang] Reapply Handle templated operators with reversed arguments Nov 14, 2023

ilya-biryukov mentioned this pull request Nov 14, 2023

[STLExtras] Remove incorrect hack to make indexed_accessor_range operator== compatible with C++20 #72220

Merged

ilya-biryukov reviewed Nov 14, 2023

View reviewed changes

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

usx95 mentioned this pull request Nov 15, 2023

[STLExtras] Add out-of-line definition of friend operator== for C++20 #72348

Merged

usx95 requested a review from ilya-biryukov November 15, 2023 09:53

shafik requested review from zygoloid and AaronBallman November 17, 2023 06:33

usx95 mentioned this pull request Nov 27, 2023

C++20 operator== ambiguous with derived-to-base conversion #53954

Closed

ilya-biryukov approved these changes Jan 12, 2024

View reviewed changes

usx95 requested a review from Endilll as a code owner January 12, 2024 13:35

usx95 closed this Jan 12, 2024

usx95 force-pushed the templated-equals-take-2 branch from c36e5e4 to 6c2fbc3 Compare January 12, 2024 13:37

rebase

b0183f2

usx95 removed the request for review from Endilll January 12, 2024 13:38

usx95 reopened this Jan 12, 2024

usx95 added 2 commits January 12, 2024 13:43

recover commits

ca39618

add test

570cc68

update release notes

e15df5e

usx95 merged commit 460ff58 into llvm:main Jan 12, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

usx95 commented Nov 14, 2023 •

edited

llvmbot commented Nov 14, 2023

github-actions bot commented Nov 14, 2023 •

edited

usx95 commented Nov 15, 2023 •

edited

shafik commented Nov 17, 2023

usx95 commented Nov 17, 2023 •

edited

usx95 commented Nov 17, 2023 •

edited

usx95 commented Jan 11, 2024

ilya-biryukov left a comment

ilya-biryukov Jan 12, 2024

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

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

Conversation

usx95 commented Nov 14, 2023 • edited

New changes

Description from #69595

llvmbot commented Nov 14, 2023

github-actions bot commented Nov 14, 2023 • edited

usx95 commented Nov 15, 2023 • edited

shafik commented Nov 17, 2023

usx95 commented Nov 17, 2023 • edited

usx95 commented Nov 17, 2023 • edited

Challenges

Strategy

usx95 commented Jan 11, 2024

ilya-biryukov left a comment

Choose a reason for hiding this comment

ilya-biryukov Jan 12, 2024

Choose a reason for hiding this comment

usx95 commented Nov 14, 2023 •

edited

github-actions bot commented Nov 14, 2023 •

edited

usx95 commented Nov 15, 2023 •

edited

usx95 commented Nov 17, 2023 •

edited

usx95 commented Nov 17, 2023 •

edited