Correctly compute conversion seq for args to fn with reversed param order #68999

usx95 · 2023-10-13T15:23:04Z

We associated conversion seq for args (when reversed) to the wrong index.
This lead to clang believing reversed operator== a worse overload candidate than the operator== without reversed args when both these candidate were ambiguous.

Fixes #53954

…order

llvmbot · 2023-10-13T15:24:09Z

@llvm/pr-subscribers-clang

Author: Utkarsh Saxena (usx95)

Changes

We associated conversion seq for args (when reversed) to the wrong index.
This lead to clang believing reversed operator== a worse overload candidate than the operator== without reversed args when both these candidate were ambiguous.

Fixes #53954

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

3 Files Affected:

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

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2d918967e7f0b02..07cedeabcdbd9b7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -113,6 +113,8 @@ C++ Language Changes
 
 C++20 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
+- Fix a bug in conversion sequence of arguments to a function with reversed parameter order.
+  Fixes `GH <https://github.com/llvm/llvm-project/issues/53954>`_.
 
 C++23 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index ce78994e6553814..b00adb00eab01aa 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,
@@ -10631,7 +10631,7 @@ OverloadCandidateSet::BestViableFunction(Sema &S, SourceLocation Loc,
   // Find the best viable function.
   Best = end();
   for (auto *Cand : Candidates) {
-    Cand->Best = false;
+        Cand->Best = false;
     if (Cand->Viable) {
       if (Best == end() ||
           isBetterOverloadCandidate(S, *Cand, *Best, Loc, Kind))
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..8975706e721cff1 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,41 @@ bool x = X() == X(); // expected-warning {{ambiguous}}
 }
 } // namespace P2468R2
 
+namespace GH53954{
+namespace test1 {
+struct P {
+    template <class S>
+    friend bool operator==(const P &, const S &); // expected-note {{candidate}} \
+                                                  // expected-note {{reversed parameter order}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-error {{ '==' is ambiguous}}
+}
+
+namespace test2 {
+struct P {
+    template <class S>
+    friend bool operator==(const S &, const P &); // expected-note {{candidate}} \
+                                                  // expected-note {{reversed parameter order}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-error {{ '==' is ambiguous}}
+}
+
+namespace test3 {
+struct P {
+  template<class S>
+  bool operator==(const S &) const; // expected-note {{candidate}} \
+                                    // expected-note {{reversed parameter order}}
+};
+struct A : public P {};
+struct B : public P {};
+bool check(A a, B b) { return a == b; } // expected-error {{ '==' is ambiguous}}
+}
+}
+
 #else // NO_ERRORS
 
 namespace problem_cases {

github-actions · 2023-10-13T15:36:05Z

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

usx95 · 2023-10-17T15:13:15Z

Friendly ping.

zygoloid

Thanks, looks good.

Our overall approach here seems error-prone, and I wonder if there's a better way to model the reversal of the conversion sequence. For example, perhaps we could change ConversionSequenceList into a class that tracks its OverloadCandidateParamOrder, and provides both accessors in candidate order (optionally the object parameter, then the remaining declared parameters in order) and in comparison order (potentially reversed). But let's land this as-is before thinking about that :)

usx95 · 2023-10-17T18:53:24Z

+1 I agree that this is confusing and error prone.

usx95 · 2023-10-18T14:07:52Z

This breaks new code (expected) but does not respect -Wno-ambiguous-reversed-operator. https://godbolt.org/z/oMsGeK1nc
@zygoloid IIUC this should be silenced by this warning but somehow does not. Will investigate further.
Reverting to put out large unsuppresable breakages

… param order (#68999)" This reverts commit e6d0b12. See PR for reason #68999 (comment)

usx95 · 2023-10-18T14:43:35Z

Ah. I see, this is because this is not a warning but hard errors in C++20.

usx95 · 2023-10-19T10:11:49Z

I think this is important that clang chooses not to error but only warn here as a clang extension (it already chooses to do so in cases when it the can match the function params(1 and 2)
Now that we correctly handle templated operator== in this PR, should we relax the error here as well to a warning ? WDYT @zygoloid ?

GCC does the same.

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

#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: ```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

…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

Correctly compute conversion seq for args to fn with reveresed param …

01ba05e

…order

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

usx95 requested review from zygoloid, ilya-biryukov, jyknight and royjacobson October 13, 2023 15:23

usx95 added the c++20 label Oct 13, 2023

usx95 added 2 commits October 13, 2023 17:24

add more tests

7471202

revert unintentional change

ea00a3c

zygoloid approved these changes Oct 17, 2023

View reviewed changes

usx95 merged commit e6d0b12 into llvm:main Oct 17, 2023

steelannelida pushed a commit that referenced this pull request Oct 18, 2023

Revert "Correctly compute conversion seq for args to fn with reversed…

a3a0f59

… param order (#68999)" This reverts commit e6d0b12. See PR for reason #68999 (comment)

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

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

4460b02

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

usx95 mentioned this pull request Oct 19, 2023

[clang] Handle templated operators with reversed arguments #69595

Merged

madhur13490 mentioned this pull request Oct 20, 2023

Revert commit ba8565fbcb975e2d067ce3ae5a7dbaae4953edd3 madhur13490/llvm-project#3

Closed

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!

Correctly compute conversion seq for args to fn with reversed param order #68999

Correctly compute conversion seq for args to fn with reversed param order #68999

Uh oh!

usx95 commented Oct 13, 2023

Uh oh!

llvmbot commented Oct 13, 2023

Uh oh!

github-actions bot commented Oct 13, 2023 •

edited

Loading

Uh oh!

usx95 commented Oct 17, 2023

Uh oh!

zygoloid left a comment

Uh oh!

usx95 commented Oct 17, 2023

Uh oh!

usx95 commented Oct 18, 2023

Uh oh!

usx95 commented Oct 18, 2023

Uh oh!

usx95 commented Oct 19, 2023 •

edited

Loading

Uh oh!

Uh oh!

Correctly compute conversion seq for args to fn with reversed param order #68999

Correctly compute conversion seq for args to fn with reversed param order #68999

Uh oh!

Conversation

usx95 commented Oct 13, 2023

Uh oh!

llvmbot commented Oct 13, 2023

Uh oh!

github-actions bot commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

usx95 commented Oct 17, 2023

Uh oh!

zygoloid left a comment

Choose a reason for hiding this comment

Uh oh!

usx95 commented Oct 17, 2023

Uh oh!

usx95 commented Oct 18, 2023

Uh oh!

usx95 commented Oct 18, 2023

Uh oh!

usx95 commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 13, 2023 •

edited

Loading

usx95 commented Oct 19, 2023 •

edited

Loading