Skip to content

[Clang][Sema] Reject array prvalue operands #140702

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Clang][Sema] Reject array prvalue operands #140702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7639,6 +7639,8 @@ def warn_param_mismatched_alignment : Warning<

def err_objc_object_assignment : Error<
"cannot assign to class object (%0 invalid)">;
def err_typecheck_array_prvalue_operand : Error<
"array prvalue is not permitted">;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"array prvalue is not permitted">;
"operand of '%0' cannot be an array prvalue">;

I think it’s a bit clearer if we phrase it like this and also print out the operator rather than just saying ‘X is not permitted’.

Copy link
Author

@languagelawyer languagelawyer May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

But the operator is pointed at by ^, like

test.cxx:13:23: error: array prvalue is not permitted
   13 |         ((int []){ 1, 2, 3}) + 0;
      |         ~~~~~~~~~~~~~~~~~~~~ ^

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, in that case, maybe just ‘operand cannot be an array prvalue’. I would prefer at least including the word ‘operand’ so it’s clear that the problem is that you’re passing an array prvalue to this operator, because at the moment the diagnostic makes it sound like array prvalues aren’t permitted at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the operator is pointed at by ^

This part can be removed by using -fno-caret-diagnostics command line option. In this case, the diagnostic is as followed:

test.cxx:13:23: error: array prvalue is not permitted

Users may get confused.

Copy link
Author

Choose a reason for hiding this comment

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

@zwuis is "operand cannot be an array prvalue" ok? Comparing to an existing error:

	IA{ 1, 2, 3 } + 0.;
	IA{ 1, 2, 3 } + 0;
test.cxx:5:16: error: invalid operands to binary expression ('int[3]' and 'double')
test.cxx:6:16: error: operand cannot be an array prvalue

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you're coming from a C background
What about Python/Java background?

They're not particularly relevant because you don't directly mix Java and C++ code in the same way you do with C and C++ as happens in header files.

IIRC there's no rvalue conversion on the statement expression result so I think that ends up being a prvalue of array type
This is not correct, statement expressions (their "result") undergo array and function pointer decay (independently of the context where they appear), so you're getting a "dangling pointer"

I was incorrectly remembering the behavior from returning a char and not having it promote to an int. I can confirm we decay the array: https://godbolt.org/z/zhW1fnsej but you can hit the same concern I had via a typedef or using, where the type information is actually slightly helpful in understanding the issue: using foo = int[10]; foo{} + 0; (keeping in mind that foo{} could also be behind a macro where it's not easy for the user to spot the {} and realize there's a temporary involved). So I still find a formulation that includes the type information a bit more user-friendly, but that could be included in a new diagnostic that talks about temporary arrays.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @AaronBallman and @Sirraide that using err_typecheck_invalid_operands is the better approach here.
The "prvalue" bit is only relevant as far as array decay is concerned (eg : array-to-pointer does not apply).
But ultimately, int[] + int is what we should diagnose, not how we got here.

Copy link
Author

Choose a reason for hiding this comment

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

keeping in mind that foo{} could also be behind a macro

I doubt that presenting exact array type vs. just saying "array" would help much in this case. There is note: expanded from macro to point that something comes from macro expansion. (Not shown for err_typecheck_invalid_operands, BTW!)

err_typecheck_invalid_operands is the better approach here

int main()
{
	using IA = int[];
	IA ia = { 1, 2, 3 };

	ia + 0.; // error: invalid operands to binary expression ('int[3]' and 'double')
	         // Where is the problem? in 'int[3]' or in 'double'?

	ia + 0; // no error means the issue was in 'double'?

	IA{ 1, 2, 3 } + 0; // error: invalid operands to binary expression ('int[3]' and 'int')
	                   // Huh? What about now?
}

But ultimately, int[] + int is what we should diagnose

The types are not the issue, why give misleading diagnostics?

Copy link
Author

Choose a reason for hiding this comment

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

invalid operands to binary expression

BTW, I think it is either "in binary expression" or "to binary operator"

Copy link
Member

Choose a reason for hiding this comment

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

invalid operands to binary expression

BTW, I think it is either "in binary expression" or "to binary operator"

‘an operand to sth.’ is a common turn of phrase that’s been around for a long time (not just in Clang).

def err_typecheck_invalid_operands : Error<
"invalid operands to binary expression (%0 and %1)">, Deferrable;
def note_typecheck_invalid_operands_converted : Note<
Expand Down
24 changes: 24 additions & 0 deletions clang/lib/Sema/SemaExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11333,6 +11333,13 @@ QualType Sema::CheckAdditionOperands(ExprResult &LHS, ExprResult &RHS,
if (!IExp->getType()->isIntegerType())
return InvalidOperands(Loc, LHS, RHS);

if (OriginalOperand Orig(PExp);
Orig.getType()->isArrayType() && Orig.Orig->isPRValue()) {
Diag(Loc, diag::err_typecheck_array_prvalue_operand)
<< PExp->getSourceRange();
return QualType();
}

// Adding to a null pointer results in undefined behavior.
if (PExp->IgnoreParenCasts()->isNullPointerConstant(
Context, Expr::NPC_ValueDependentIsNotNull)) {
Expand Down Expand Up @@ -11429,6 +11436,18 @@ QualType Sema::CheckSubtractionOperands(ExprResult &LHS, ExprResult &RHS,
return compType;
}

OriginalOperand OrigLHS(LHS.get()), OrigRHS(RHS.get());
bool LHSArrP = OrigLHS.getType()->isArrayType() && OrigLHS.Orig->isPRValue();
bool RHSArrP = OrigRHS.getType()->isArrayType() && OrigRHS.Orig->isPRValue();
if (LHSArrP || RHSArrP) {
auto &&diag = Diag(Loc, diag::err_typecheck_array_prvalue_operand);
if (LHSArrP)
diag << LHS.get()->getSourceRange();
if (RHSArrP)
diag << RHS.get()->getSourceRange();
return QualType();
}

// Either ptr - int or ptr - ptr.
if (LHS.get()->getType()->isAnyPointerType()) {
QualType lpointee = LHS.get()->getType()->getPointeeType();
Expand Down Expand Up @@ -15840,6 +15859,11 @@ ExprResult Sema::CreateBuiltinUnaryOp(SourceLocation OpLoc,
InputExpr->getType()->isSpecificBuiltinType(BuiltinType::Dependent)) {
resultType = Context.DependentTy;
} else {
if (Opc == UO_Deref || Opc == UO_Plus) {
Copy link
Member

Choose a reason for hiding this comment

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

It feels a bit weird to use an if statement here when there’s a switch statement on the same variable right after. I’d probably make this a lambda and then call it in the cases for * and + below (the + case would then also need a [[fallthrough]] annotation).

if (InputExpr->getType()->isArrayType() && InputExpr->isPRValue())
return ExprError(Diag(OpLoc, diag::err_typecheck_array_prvalue_operand)
<< InputExpr->getSourceRange());
}
switch (Opc) {
case UO_PreInc:
case UO_PreDec:
Expand Down
12 changes: 10 additions & 2 deletions clang/test/CXX/expr/p8.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
// expected-no-diagnostics
// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11

int a0;
const volatile int a1 = 2;
Expand All @@ -16,4 +15,13 @@ int main()
f0(a1);
f1(a2);
f2(a3);

using IA = int[];
void(+IA{ 1, 2, 3 }); // expected-error {{array prvalue}}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some tests here to make sure that we don’t complain about array glvalues?

void(*IA{ 1, 2, 3 }); // expected-error {{array prvalue}}
void(IA{ 1, 2, 3 } + 0); // expected-error {{array prvalue}}
void(IA{ 1, 2, 3 } - 0); // expected-error {{array prvalue}}
void(0 + IA{ 1, 2, 3 }); // expected-error {{array prvalue}}
void(0 - IA{ 1, 2, 3 }); // expected-error {{array prvalue}}
void(IA{ 1, 2, 3 } - IA{ 1, 2, 3 }); // expected-error {{array prvalue}}
}