Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] Fix assertion failure with deleted overloaded unary operators #78316

Merged
merged 7 commits into from
Jan 22, 2024

Conversation

Fznamznon
Copy link
Contributor

When emitting notes related to wrong number of arguments do not consider implicit object argument.

Fixes #78314

When emitting notes related to wrong number of arguments do not consider
implicit object argument.

Fixes llvm#78314
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 16, 2024
@llvmbot
Copy link

llvmbot commented Jan 16, 2024

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

When emitting notes related to wrong number of arguments do not consider implicit object argument.

Fixes #78314


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+2-2)
  • (modified) clang/test/SemaCXX/overloaded-operator.cpp (+27)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 6e31849ce16dd40..8382e5d55f6c6e5 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -750,6 +750,8 @@ Bug Fixes in This Version
   Fixes (`#77583 <https://github.com/llvm/llvm-project/issues/77583>`_)
 - Fix an issue where CTAD fails for function-type/array-type arguments.
   Fixes (`#51710 <https://github.com/llvm/llvm-project/issues/51710>`_)
+- Fixed assertion failure with deleted overloaded unary operators.
+  Fixes (`#78314 <https://github.com/llvm/llvm-project/issues/78314>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 37c62b306b3cd3f..83ab7cb0f3411be 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14310,8 +14310,8 @@ Sema::CreateOverloadedUnaryOp(SourceLocation OpLoc, UnaryOperatorKind Opc,
         PartialDiagnosticAt(OpLoc, PDiag(diag::err_ovl_deleted_oper)
                                        << UnaryOperator::getOpcodeStr(Opc)
                                        << Input->getSourceRange()),
-        *this, OCD_AllCandidates, ArgsArray, UnaryOperator::getOpcodeStr(Opc),
-        OpLoc);
+        *this, OCD_AllCandidates, ArgsArray.slice(1),
+        UnaryOperator::getOpcodeStr(Opc), OpLoc);
     return ExprError();
   }
 
diff --git a/clang/test/SemaCXX/overloaded-operator.cpp b/clang/test/SemaCXX/overloaded-operator.cpp
index 83a7e65b43dd012..60332019f516cf0 100644
--- a/clang/test/SemaCXX/overloaded-operator.cpp
+++ b/clang/test/SemaCXX/overloaded-operator.cpp
@@ -598,3 +598,30 @@ namespace B {
 }
 void g(B::X x) { A::f(x); }
 }
+
+namespace GH78314 {
+
+class a {
+public:
+  void operator--() = delete; // expected-note {{candidate function has been explicitly deleted}} \
+                              // expected-note {{candidate function not viable: requires 0 arguments, but 1 was provided}}
+  void operator--(int) = delete; // expected-note {{candidate function has been explicitly deleted}} \
+                                 // expected-note {{candidate function not viable: requires 1 argument, but 0 were provided}}
+};
+
+void foo() {
+  a aa;
+  --aa; // expected-error {{overload resolution selected deleted operator '--'}}
+  aa--; // expected-error {{overload resolution selected deleted operator '--'}}
+}
+
+class b {
+  void operator++() = delete; // expected-note {{candidate function has been explicitly deleted}}
+  template <class> void operator++(int) { // expected-note {{function template not viable: requires 1 argument, but 0 were provided}}
+    b bb;
+    ++bb; // expected-error {{overload resolution selected deleted operator '++'}}
+  }
+};
+
+
+}

@@ -14310,8 +14310,8 @@ Sema::CreateOverloadedUnaryOp(SourceLocation OpLoc, UnaryOperatorKind Opc,
PartialDiagnosticAt(OpLoc, PDiag(diag::err_ovl_deleted_oper)
<< UnaryOperator::getOpcodeStr(Opc)
<< Input->getSourceRange()),
*this, OCD_AllCandidates, ArgsArray, UnaryOperator::getOpcodeStr(Opc),
OpLoc);
*this, OCD_AllCandidates, ArgsArray.slice(1),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why ArgArray.slice(1) fixes the problem? It is not obvious to me from looking at NoteCandidates

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem ArgArray.slice(1) solves is that CreateOverloadedUnaryOp fills first element ArgsArray with the object whose method was called. Later in NoteCandidates size of ArgsArray is passed further and it eventually ends up compared to number of function candidate parameters which never includes implicit object parameter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add that as a comment so that future us don't get confused? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add that as a comment so that future us don't get confused?

Sure, done.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, ArgsArray.drop_front() makes more sense to me here. It does the same thing, but is, IMO, more 'clear'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, ArgsArray.drop_front() makes more sense to me here.

Okay, done.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (modulo nit)

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 suggestion, not necessary, but I think it is beneficial.

@@ -14310,8 +14310,8 @@ Sema::CreateOverloadedUnaryOp(SourceLocation OpLoc, UnaryOperatorKind Opc,
PartialDiagnosticAt(OpLoc, PDiag(diag::err_ovl_deleted_oper)
<< UnaryOperator::getOpcodeStr(Opc)
<< Input->getSourceRange()),
*this, OCD_AllCandidates, ArgsArray, UnaryOperator::getOpcodeStr(Opc),
OpLoc);
*this, OCD_AllCandidates, ArgsArray.slice(1),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, ArgsArray.drop_front() makes more sense to me here. It does the same thing, but is, IMO, more 'clear'.

@Fznamznon
Copy link
Contributor Author

Driver failure seems unrelated.

@Fznamznon Fznamznon merged commit 11d1310 into llvm:main Jan 22, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang] Assertion failure when prefix increment is deleted and postfix increment is overloaded
5 participants