Skip to content

[clang] Fix the local parameter of void type inside the Requires expression. #109831

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 1, 2024
Merged

[clang] Fix the local parameter of void type inside the Requires expression. #109831

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9c112e7
require void
c8ef Sep 24, 2024
8206940
fix
c8ef Sep 24, 2024
7af1afb
update test
c8ef Sep 30, 2024
590727f
static assert
c8ef Sep 30, 2024
a2b0e45
release note
c8ef Sep 30, 2024
fe609a4
const
c8ef Sep 30, 2024
2468551
Merge branch 'main' into require
c8ef Sep 30, 2024
c9c15de
CR
c8ef Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions clang/lib/Sema/SemaExprCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9509,6 +9509,19 @@ Sema::ActOnStartRequiresExpr(SourceLocation RequiresKWLoc,
PushDeclContext(BodyScope, Body);

for (ParmVarDecl *Param : LocalParameters) {
if (Param->getType()->isVoidType()) {
if (LocalParameters.size() > 1) {
Diag(Param->getBeginLoc(), diag::err_void_only_param);
Body->setInvalidDecl();
} else if (Param->getIdentifier()) {
Diag(Param->getBeginLoc(), diag::err_param_with_void_type);
Body->setInvalidDecl();
} else if (Param->getType().hasQualifiers()) {
Diag(Param->getBeginLoc(), diag::err_void_param_qualified);
Body->setInvalidDecl();
}
}
Copy link
Contributor

@mizvekov mizvekov Sep 24, 2024

Choose a reason for hiding this comment

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

Can we unify these with the parameter type checks in SemaType?

Also, it would make sense to perform the same error recovery here, which is to just adjust these parameter types
to int, or perhaps some other error recovery, but consistently between requires local parameters and function parameters.

I don't think it's helpful to mark the requires body as invalid, but in some scenarios we can mark the parameter itself as invalid, if needed.

Copy link
Contributor Author

@c8ef c8ef Sep 25, 2024

Choose a reason for hiding this comment

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

Are you suggesting unifying the parameter type check for void type only or for all parameters? As I understand it, the complete parameter check involves a lot more branches, such as support for objc, opencl, etc. It appears challenging to merge these two aspects seamlessly.

Copy link
Contributor

@zyn0217 zyn0217 Sep 25, 2024

Choose a reason for hiding this comment

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

I think the invalidity of the Decl should be used to indicate the "unsatisfied" flag such that the constant expression would result in a false value. That said, marking the parameters as invalid should also work.

Note that we don't seem ever to use the invalid flag. I think you need to examine that flag in RequiresExpr's constructor.

Copy link
Contributor Author

@c8ef c8ef Sep 30, 2024

Choose a reason for hiding this comment

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

Done. The required expression will now evaluate to false if it has an incorrect void parameter now.


if (Param->hasDefaultArg())
// C++2a [expr.prim.req] p4
// [...] A local parameter of a requires-expression shall not have a
Expand Down
15 changes: 15 additions & 0 deletions clang/test/SemaCXX/invalid-requirement-requires-parameter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// RUN: %clang -fsyntax-only -std=c++2a -Xclang -verify %s
Copy link
Contributor

@zyn0217 zyn0217 Sep 25, 2024

Choose a reason for hiding this comment

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

Can you move the test to clang/test/CXX/expr/expr.prim/expr.prim.req/requires-expr.cpp? I don't think this issue merits a new one

Copy link
Contributor Author

@c8ef c8ef Sep 28, 2024

Choose a reason for hiding this comment

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

I will do it later :)

Copy link
Contributor Author

@c8ef c8ef Sep 30, 2024

Choose a reason for hiding this comment

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

Done.


namespace GH109538 {
static_assert(requires(void *t) { t; });
static_assert(requires(void) { 42; });
static_assert(requires(void t) { // expected-error {{argument may not have 'void' type}}
t;
});
static_assert(requires(void t, int a) { // expected-error {{'void' must be the first and only parameter if specified}}
t;
});
static_assert(requires(const void) { // expected-error {{'void' as parameter must not have type qualifiers}}
42;
});
} // namespace GH109538