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

[c8ef]

Fixes #109538.

In this patch, we introduce diagnostic for required expression parameters in the same way as function parameters, fix the issue of handling void type parameters, and align the behavior with GCC and other compilers.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (c8ef)

Changes

Fixes #109538.

In this patch, we introduce diagnostic for required expression parameters in the same way as function parameters, fix the issue of handling void type parameters, and align the behavior with GCC and other compilers.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaExprCXX.cpp (+13)
• (added) clang/test/SemaCXX/invalid-requirement-requires-parameter.cpp (+15)

diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index ac3fe6ab8f9bd0..d0914d990be6a7 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -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();
+      }
+    }
+
     if (Param->hasDefaultArg())
       // C++2a [expr.prim.req] p4
       //     [...] A local parameter of a requires-expression shall not have a
diff --git a/clang/test/SemaCXX/invalid-requirement-requires-parameter.cpp b/clang/test/SemaCXX/invalid-requirement-requires-parameter.cpp
new file mode 100644
index 00000000000000..01aa0ca4d229b5
--- /dev/null
+++ b/clang/test/SemaCXX/invalid-requirement-requires-parameter.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang -fsyntax-only -std=c++2a -Xclang -verify %s
+
+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

[c8ef]

CC @erichkeane @zyn0217, since I do not have permission to request reviewers.

[mizvekov]

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.

[c8ef]

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.

[zyn0217]

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.

[c8ef]

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

[zyn0217]

There should be a release note :)

I think I agree with @mizvekov's idea of merging the parameter checks into SemaType; we can probably in part reuse GetTypeForDeclarator. I would appreciate it if we can see some exploration here.

Another thing is that we need to make sure the values of such require expressions are false, aligning with other compilers.

[zyn0217]

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

[c8ef]

I will do it later :)

[c8ef]

Done.

[c8ef]

I think I agree with @mizvekov's idea of merging the parameter checks into SemaType; we can probably in part reuse GetTypeForDeclarator. I would appreciate it if we can see some exploration here.

Apologies for the delayed response. The implementation I provided is actually derived from GetTypeForDeclarator, specifically GetFullTypeForDeclarator. The challenge I faced is that this function handles numerous cases. My initial idea was to extract a function from GetTypeForDeclarator to manage function parameters, but I'm uncertain if this is the best approach. I'm wondering what level of granularity we should aim for when reusing the current implementation in GetTypeForDeclarator?

[mizvekov]

I'm wondering what level of granularity we should aim for when reusing the current implementation in GetTypeForDeclarator?

You can perhaps extract just the parts that are common between both use cases into a separate function, or another possible approach would be to reuse the whole thing, but then add an extra mode parameter which turns off some checks for the requires local parameters.

[c8ef]

After spending the entire afternoon attempting to streamline the parameter check process, I still haven't found an elegant solution to consolidate it into a single location. I explored how GCC approaches this issue and discovered that while GCC does utilize a single parameter check function (specifically in the grokparm function in gcc/c/c-decl.cc), it doesn't differentiate between the two contexts. As a result, it permits a requires clause to include a parameter with a default argument: godbolt.

Upon examining the actual parameter checking process, it seems that the most crucial check related to require-expressions is for void types. Other checks are mainly for OpenCL, Objective-C, etc. Merging these checks for just a single void check might be overkill.

llvm-project/clang/lib/Sema/SemaType.cpp

Lines 5143 to 5236 in 75e08a5

	for (unsigned i = 0, e = FTI.NumParams; i != e; ++i) {
	ParmVarDecl *Param = cast<ParmVarDecl>(FTI.Params[i].Param);
	QualType ParamTy = Param->getType();
	assert(!ParamTy.isNull() && "Couldn't parse type?");
	// Look for 'void'. void is allowed only as a single parameter to a
	// function with no other parameters (C99 6.7.5.3p10). We record
	// int(void) as a FunctionProtoType with an empty parameter list.
	if (ParamTy->isVoidType()) {
	// If this is something like 'float(int, void)', reject it. 'void'
	// is an incomplete type (C99 6.2.5p19) and function decls cannot
	// have parameters of incomplete type.
	if (FTI.NumParams != 1 || FTI.isVariadic) {
	S.Diag(FTI.Params[i].IdentLoc, diag::err_void_only_param);
	ParamTy = Context.IntTy;
	Param->setType(ParamTy);
	} else if (FTI.Params[i].Ident) {
	// Reject, but continue to parse 'int(void abc)'.
	S.Diag(FTI.Params[i].IdentLoc, diag::err_param_with_void_type);
	ParamTy = Context.IntTy;
	Param->setType(ParamTy);
	} else {
	// Reject, but continue to parse 'float(const void)'.
	if (ParamTy.hasQualifiers())
	S.Diag(DeclType.Loc, diag::err_void_param_qualified);
	// Reject, but continue to parse 'float(this void)' as
	// 'float(void)'.
	if (Param->isExplicitObjectParameter()) {
	S.Diag(Param->getLocation(),
	diag::err_void_explicit_object_param);
	Param->setExplicitObjectParameterLoc(SourceLocation());
	}
	// Do not add 'void' to the list.
	break;
	}
	} else if (ParamTy->isHalfType()) {
	// Disallow half FP parameters.
	// FIXME: This really should be in BuildFunctionType.
	if (S.getLangOpts().OpenCL) {
	if (!S.getOpenCLOptions().isAvailableOption("cl_khr_fp16",
	S.getLangOpts())) {
	S.Diag(Param->getLocation(), diag::err_opencl_invalid_param)
	<< ParamTy << 0;
	D.setInvalidType();
	Param->setInvalidDecl();
	}
	} else if (!S.getLangOpts().NativeHalfArgsAndReturns &&
	!S.Context.getTargetInfo().allowHalfArgsAndReturns()) {
	S.Diag(Param->getLocation(),
	diag::err_parameters_retval_cannot_have_fp16_type) << 0;
	D.setInvalidType();
	}
	} else if (!FTI.hasPrototype) {
	if (Context.isPromotableIntegerType(ParamTy)) {
	ParamTy = Context.getPromotedIntegerType(ParamTy);
	Param->setKNRPromoted(true);
	} else if (const BuiltinType *BTy = ParamTy->getAs<BuiltinType>()) {
	if (BTy->getKind() == BuiltinType::Float) {
	ParamTy = Context.DoubleTy;
	Param->setKNRPromoted(true);
	}
	}
	} else if (S.getLangOpts().OpenCL && ParamTy->isBlockPointerType()) {
	// OpenCL 2.0 s6.12.5: A block cannot be a parameter of a function.
	S.Diag(Param->getLocation(), diag::err_opencl_invalid_param)
	<< ParamTy << 1 /*hint off*/;
	D.setInvalidType();
	}
	if (LangOpts.ObjCAutoRefCount && Param->hasAttr<NSConsumedAttr>()) {
	ExtParameterInfos[i] = ExtParameterInfos[i].withIsConsumed(true);
	HasAnyInterestingExtParameterInfos = true;
	}
	if (auto attr = Param->getAttr<ParameterABIAttr>()) {
	ExtParameterInfos[i] =
	ExtParameterInfos[i].withABI(attr->getABI());
	HasAnyInterestingExtParameterInfos = true;
	}
	if (Param->hasAttr<PassObjectSizeAttr>()) {
	ExtParameterInfos[i] = ExtParameterInfos[i].withHasPassObjectSize();
	HasAnyInterestingExtParameterInfos = true;
	}
	if (Param->hasAttr<NoEscapeAttr>()) {
	ExtParameterInfos[i] = ExtParameterInfos[i].withIsNoEscape(true);
	HasAnyInterestingExtParameterInfos = true;
	}
	ParamTys.push_back(ParamTy);
	}

Therefore, I recommend adding the void parameter check directly into SemaExprCXX for an immediate fix. If there's a future need to consolidate the checks, we can consider refactoring later.

Additionally, there appears to be some overlap between the parameter checks in Sema::BuildFunctionType and GetFullTypeForDeclarator. It might be worth cleaning up this redundancy.

llvm-project/clang/lib/Sema/SemaType.cpp

Lines 2637 to 2660 in 75e08a5

	for (unsigned Idx = 0, Cnt = ParamTypes.size(); Idx < Cnt; ++Idx) {
	// FIXME: Loc is too inprecise here, should use proper locations for args.
	QualType ParamType = Context.getAdjustedParameterType(ParamTypes[Idx]);
	if (ParamType->isVoidType()) {
	Diag(Loc, diag::err_param_with_void_type);
	Invalid = true;
	} else if (ParamType->isHalfType() && !getLangOpts().NativeHalfArgsAndReturns &&
	!Context.getTargetInfo().allowHalfArgsAndReturns()) {
	// Disallow half FP arguments.
	Diag(Loc, diag::err_parameters_retval_cannot_have_fp16_type) << 0 <<
	FixItHint::CreateInsertion(Loc, "*");
	Invalid = true;
	} else if (ParamType->isWebAssemblyTableType()) {
	Diag(Loc, diag::err_wasm_table_as_function_parameter);
	Invalid = true;
	}
	// C++2a [dcl.fct]p4:
	// A parameter with volatile-qualified type is deprecated
	if (ParamType.isVolatileQualified() && getLangOpts().CPlusPlus20)
	Diag(Loc, diag::warn_deprecated_volatile_param) << ParamType;
	ParamTypes[Idx] = ParamType;
	}

CC @mizvekov @zyn0217 @erichkeane

[zyn0217]

Therefore, I recommend adding the void parameter check directly into SemaExprCXX for an immediate fix. If there's a future need to consolidate the checks, we can consider refactoring later.

That works for me if it turns out to take much more effort than our anticipation, although it's really a shame for us to end up strewing the check three times.

[mizvekov]

Same then, go ahead, but please make sure the error handling strategies don't diverge unnecessarily.

[c8ef]

I have another simple question: If an explicit-object-parameter-declaration appears in the parameter list, should the requires expression evaluate to false? All three compilers I've tested seem to evaluate it to true, but I'm unsure if this is the correct behavior.

static_assert(requires(this int) { true; });

https://gcc.godbolt.org/z/9Pajb5G9s

(This issue arises when I attempt to merge this diagnostic into Sema::ActOnStartRequiresExpr. Currently, the implementation resides in ParseDecl.)

[zyn0217]

static_assert(requires(this int) { true; });

The grammar doesn't appear to disallow the explicit object parameter in a requires expression, see https://eel.is/c++draft/expr.prim.req#nt:requirement-parameter-list.

But the usage is indeed suspicious to me. We probably need a CWG issue? @cor3ntin @mizvekov

[c8ef]

      // C++23 [dcl.fct]p6:
      //   An explicit-object-parameter-declaration is a parameter-declaration
      //   with a this specifier. An explicit-object-parameter-declaration
      //   shall appear only as the first parameter-declaration of a
      //   parameter-declaration-list of either:
      //   - a member-declarator that declares a member function, or
      //   - a lambda-declarator.
      //
      // The parameter-declaration-list of a requires-expression is not such
      // a context.

See also: #88974

[c8ef]

There should be a release note :)

Done.

[zyn0217]

Makes sense to me. Thanks

Please wait for other folks a day or two in case they have different opinions.

[mizvekov]

LGTM, Thanks!

[c8ef]

Thank you all for reviewing! If this patch looks good, could someone help me land it?