From 27ea0c4e7234f3b15cbbb696e6c408af7141f342 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Thu, 11 Nov 2021 15:27:10 +0100 Subject: [PATCH] [Parse] Use empty RecoveryExpr when if/while/do/switch conditions fail to parse This allows the body to be parsed. An special-case that would replace a missing if condition with OpaqueValueExpr was removed as it's now redundant (unless recovery-expr is disabled). For loops are not handled at this point, as the parsing is more complicated. Differential Revision: https://reviews.llvm.org/D113752 --- clang/include/clang/Parse/Parser.h | 5 +- clang/include/clang/Sema/Sema.h | 7 +- clang/lib/AST/ExprConstant.cpp | 9 ++- clang/lib/Parse/ParseExprCXX.cpp | 21 +++--- clang/lib/Parse/ParseStmt.cpp | 54 ++++++++++----- clang/lib/Sema/SemaExpr.cpp | 10 +-- clang/lib/Sema/SemaStmt.cpp | 8 +-- clang/lib/Sema/TreeTransform.h | 3 +- clang/test/AST/ast-dump-invalid.cpp | 2 +- clang/test/AST/loop-recovery.cpp | 65 +++++++++++++++++++ clang/test/Parser/cxx0x-attributes.cpp | 1 + clang/test/Sema/complex-int.c | 4 +- clang/test/SemaCXX/condition.cpp | 2 + .../constexpr-function-recovery-crash.cpp | 22 +++++++ 14 files changed, 168 insertions(+), 45 deletions(-) create mode 100644 clang/test/AST/loop-recovery.cpp diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h index d266a0b37265..b651929fa9da 100644 --- a/clang/include/clang/Parse/Parser.h +++ b/clang/include/clang/Parse/Parser.h @@ -1975,6 +1975,7 @@ private: Sema::ConditionResult ParseCXXCondition(StmtResult *InitStmt, SourceLocation Loc, Sema::ConditionKind CK, + bool MissingOK, ForRangeInfo *FRI = nullptr, bool EnterForConditionScope = false); DeclGroupPtrTy @@ -2079,8 +2080,8 @@ private: bool ParseParenExprOrCondition(StmtResult *InitStmt, Sema::ConditionResult &CondResult, SourceLocation Loc, Sema::ConditionKind CK, - SourceLocation *LParenLoc = nullptr, - SourceLocation *RParenLoc = nullptr); + bool MissingOK, SourceLocation *LParenLoc, + SourceLocation *RParenLoc); StmtResult ParseIfStatement(SourceLocation *TrailingElseLoc); StmtResult ParseSwitchStatement(SourceLocation *TrailingElseLoc); StmtResult ParseWhileStatement(SourceLocation *TrailingElseLoc); diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 9521b24e44a7..9b6d9d20ca43 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -12080,9 +12080,12 @@ public: ConstexprIf, ///< A constant boolean condition from 'if constexpr'. Switch ///< An integral condition for a 'switch' statement. }; + QualType PreferredConditionType(ConditionKind K) const { + return K == ConditionKind::Switch ? Context.IntTy : Context.BoolTy; + } - ConditionResult ActOnCondition(Scope *S, SourceLocation Loc, - Expr *SubExpr, ConditionKind CK); + ConditionResult ActOnCondition(Scope *S, SourceLocation Loc, Expr *SubExpr, + ConditionKind CK, bool MissingOK = false); ConditionResult ActOnConditionVariable(Decl *ConditionVar, SourceLocation StmtLoc, diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp index 51e8a55f2e4b..f9416e8e215d 100644 --- a/clang/lib/AST/ExprConstant.cpp +++ b/clang/lib/AST/ExprConstant.cpp @@ -4933,8 +4933,13 @@ static EvalStmtResult EvaluateSwitch(StmtResult &Result, EvalInfo &Info, if (SS->getConditionVariable() && !EvaluateDecl(Info, SS->getConditionVariable())) return ESR_Failed; - if (!EvaluateInteger(SS->getCond(), Value, Info)) - return ESR_Failed; + if (SS->getCond()->isValueDependent()) { + if (!EvaluateDependentExpr(SS->getCond(), Info)) + return ESR_Failed; + } else { + if (!EvaluateInteger(SS->getCond(), Value, Info)) + return ESR_Failed; + } if (!CondScope.destroy()) return ESR_Failed; } diff --git a/clang/lib/Parse/ParseExprCXX.cpp b/clang/lib/Parse/ParseExprCXX.cpp index 6685c0e89702..cd600c4247a7 100644 --- a/clang/lib/Parse/ParseExprCXX.cpp +++ b/clang/lib/Parse/ParseExprCXX.cpp @@ -1953,6 +1953,9 @@ Parser::ParseAliasDeclarationInInitStatement(DeclaratorContext Context, /// \param Loc The location of the start of the statement that requires this /// condition, e.g., the "for" in a for loop. /// +/// \param MissingOK Whether an empty condition is acceptable here. Otherwise +/// it is considered an error to be recovered from. +/// /// \param FRI If non-null, a for range declaration is permitted, and if /// present will be parsed and stored here, and a null result will be returned. /// @@ -1960,11 +1963,10 @@ Parser::ParseAliasDeclarationInInitStatement(DeclaratorContext Context, /// appropriate moment for a 'for' loop. /// /// \returns The parsed condition. -Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt, - SourceLocation Loc, - Sema::ConditionKind CK, - ForRangeInfo *FRI, - bool EnterForConditionScope) { +Sema::ConditionResult +Parser::ParseCXXCondition(StmtResult *InitStmt, SourceLocation Loc, + Sema::ConditionKind CK, bool MissingOK, + ForRangeInfo *FRI, bool EnterForConditionScope) { // Helper to ensure we always enter a continue/break scope if requested. struct ForConditionScopeRAII { Scope *S; @@ -2019,7 +2021,7 @@ Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt, } ConsumeToken(); *InitStmt = Actions.ActOnNullStmt(SemiLoc); - return ParseCXXCondition(nullptr, Loc, CK); + return ParseCXXCondition(nullptr, Loc, CK, MissingOK); } // Parse the expression. @@ -2031,10 +2033,11 @@ Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt, WarnOnInit(); *InitStmt = Actions.ActOnExprStmt(Expr.get()); ConsumeToken(); - return ParseCXXCondition(nullptr, Loc, CK); + return ParseCXXCondition(nullptr, Loc, CK, MissingOK); } - return Actions.ActOnCondition(getCurScope(), Loc, Expr.get(), CK); + return Actions.ActOnCondition(getCurScope(), Loc, Expr.get(), CK, + MissingOK); } case ConditionOrInitStatement::InitStmtDecl: { @@ -2048,7 +2051,7 @@ Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt, DG = ParseSimpleDeclaration(DeclaratorContext::SelectionInit, DeclEnd, attrs, /*RequireSemi=*/true); *InitStmt = Actions.ActOnDeclStmt(DG, DeclStart, DeclEnd); - return ParseCXXCondition(nullptr, Loc, CK); + return ParseCXXCondition(nullptr, Loc, CK, MissingOK); } case ConditionOrInitStatement::ForRangeDecl: { diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp index cc0a9d1a5b88..ee07775b6346 100644 --- a/clang/lib/Parse/ParseStmt.cpp +++ b/clang/lib/Parse/ParseStmt.cpp @@ -1191,22 +1191,24 @@ StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) { bool Parser::ParseParenExprOrCondition(StmtResult *InitStmt, Sema::ConditionResult &Cond, SourceLocation Loc, - Sema::ConditionKind CK, + Sema::ConditionKind CK, bool MissingOK, SourceLocation *LParenLoc, SourceLocation *RParenLoc) { BalancedDelimiterTracker T(*this, tok::l_paren); T.consumeOpen(); + SourceLocation Start = Tok.getLocation(); - if (getLangOpts().CPlusPlus) - Cond = ParseCXXCondition(InitStmt, Loc, CK); - else { + if (getLangOpts().CPlusPlus) { + Cond = ParseCXXCondition(InitStmt, Loc, CK, MissingOK); + } else { ExprResult CondExpr = ParseExpression(); // If required, convert to a boolean value. if (CondExpr.isInvalid()) Cond = Sema::ConditionError(); else - Cond = Actions.ActOnCondition(getCurScope(), Loc, CondExpr.get(), CK); + Cond = Actions.ActOnCondition(getCurScope(), Loc, CondExpr.get(), CK, + MissingOK); } // If the parser was confused by the condition and we don't have a ')', try to @@ -1220,7 +1222,16 @@ bool Parser::ParseParenExprOrCondition(StmtResult *InitStmt, return true; } - // Otherwise the condition is valid or the rparen is present. + if (Cond.isInvalid()) { + ExprResult CondExpr = Actions.CreateRecoveryExpr( + Start, Tok.getLocation() == Start ? Start : PrevTokLocation, {}, + Actions.PreferredConditionType(CK)); + if (!CondExpr.isInvalid()) + Cond = Actions.ActOnCondition(getCurScope(), Loc, CondExpr.get(), CK, + MissingOK); + } + + // Either the condition is valid or the rparen is present. T.consumeClose(); if (LParenLoc != nullptr) { @@ -1404,7 +1415,7 @@ StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc) { if (ParseParenExprOrCondition(&InitStmt, Cond, IfLoc, IsConstexpr ? Sema::ConditionKind::ConstexprIf : Sema::ConditionKind::Boolean, - &LParen, &RParen)) + /*MissingOK=*/false, &LParen, &RParen)) return StmtError(); if (IsConstexpr) @@ -1599,7 +1610,8 @@ StmtResult Parser::ParseSwitchStatement(SourceLocation *TrailingElseLoc) { SourceLocation LParen; SourceLocation RParen; if (ParseParenExprOrCondition(&InitStmt, Cond, SwitchLoc, - Sema::ConditionKind::Switch, &LParen, &RParen)) + Sema::ConditionKind::Switch, + /*MissingOK=*/false, &LParen, &RParen)) return StmtError(); StmtResult Switch = Actions.ActOnStartOfSwitchStmt( @@ -1689,7 +1701,8 @@ StmtResult Parser::ParseWhileStatement(SourceLocation *TrailingElseLoc) { SourceLocation LParen; SourceLocation RParen; if (ParseParenExprOrCondition(nullptr, Cond, WhileLoc, - Sema::ConditionKind::Boolean, &LParen, &RParen)) + Sema::ConditionKind::Boolean, + /*MissingOK=*/false, &LParen, &RParen)) return StmtError(); // C99 6.8.5p5 - In C99, the body of the while statement is a scope, even if @@ -1780,10 +1793,18 @@ StmtResult Parser::ParseDoStatement() { // A do-while expression is not a condition, so can't have attributes. DiagnoseAndSkipCXX11Attributes(); + SourceLocation Start = Tok.getLocation(); ExprResult Cond = ParseExpression(); // Correct the typos in condition before closing the scope. if (Cond.isUsable()) Cond = Actions.CorrectDelayedTyposInExpr(Cond); + else { + if (!Tok.isOneOf(tok::r_paren, tok::r_square, tok::r_brace)) + SkipUntil(tok::semi); + Cond = Actions.CreateRecoveryExpr( + Start, Start == Tok.getLocation() ? Start : PrevTokLocation, {}, + Actions.getASTContext().BoolTy); + } T.consumeClose(); DoScope.Exit(); @@ -2038,10 +2059,11 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) { // for-range-declaration next. bool MightBeForRangeStmt = !ForRangeInfo.ParsedForRangeDecl(); ColonProtectionRAIIObject ColonProtection(*this, MightBeForRangeStmt); - SecondPart = - ParseCXXCondition(nullptr, ForLoc, Sema::ConditionKind::Boolean, - MightBeForRangeStmt ? &ForRangeInfo : nullptr, - /*EnterForConditionScope*/ true); + SecondPart = ParseCXXCondition( + nullptr, ForLoc, Sema::ConditionKind::Boolean, + // FIXME: recovery if we don't see another semi! + /*MissingOK=*/true, MightBeForRangeStmt ? &ForRangeInfo : nullptr, + /*EnterForConditionScope*/ true); if (ForRangeInfo.ParsedForRangeDecl()) { Diag(FirstPart.get() ? FirstPart.get()->getBeginLoc() @@ -2065,9 +2087,9 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) { if (SecondExpr.isInvalid()) SecondPart = Sema::ConditionError(); else - SecondPart = - Actions.ActOnCondition(getCurScope(), ForLoc, SecondExpr.get(), - Sema::ConditionKind::Boolean); + SecondPart = Actions.ActOnCondition( + getCurScope(), ForLoc, SecondExpr.get(), + Sema::ConditionKind::Boolean, /*MissingOK=*/true); } } } diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index a2a1f76df96d..05f926be7df9 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -19209,10 +19209,12 @@ ExprResult Sema::CheckBooleanCondition(SourceLocation Loc, Expr *E, } Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc, - Expr *SubExpr, ConditionKind CK) { - // Empty conditions are valid in for-statements. + Expr *SubExpr, ConditionKind CK, + bool MissingOK) { + // MissingOK indicates whether having no condition expression is valid + // (for loop) or invalid (e.g. while loop). if (!SubExpr) - return ConditionResult(); + return MissingOK ? ConditionResult() : ConditionError(); ExprResult Cond; switch (CK) { @@ -19230,7 +19232,7 @@ Sema::ConditionResult Sema::ActOnCondition(Scope *S, SourceLocation Loc, } if (Cond.isInvalid()) { Cond = CreateRecoveryExpr(SubExpr->getBeginLoc(), SubExpr->getEndLoc(), - {SubExpr}); + {SubExpr}, PreferredConditionType(CK)); if (!Cond.get()) return ConditionError(); } diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp index d18f89d60d78..7fe92f492b5e 100644 --- a/clang/lib/Sema/SemaStmt.cpp +++ b/clang/lib/Sema/SemaStmt.cpp @@ -869,12 +869,7 @@ StmtResult Sema::ActOnIfStmt(SourceLocation IfLoc, Stmt *thenStmt, SourceLocation ElseLoc, Stmt *elseStmt) { if (Cond.isInvalid()) - Cond = ConditionResult( - *this, nullptr, - MakeFullExpr(new (Context) OpaqueValueExpr(SourceLocation(), - Context.BoolTy, VK_PRValue), - IfLoc), - false); + return StmtError(); bool ConstevalOrNegatedConsteval = StatementKind == IfStatementKind::ConstevalNonNegated || @@ -2468,6 +2463,7 @@ StmtResult Sema::ActOnCXXForRangeStmt(Scope *S, SourceLocation ForLoc, Stmt *First, SourceLocation ColonLoc, Expr *Range, SourceLocation RParenLoc, BuildForRangeKind Kind) { + // FIXME: recover in order to allow the body to be parsed. if (!First) return StmtError(); diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h index c105a0c26ab4..c9feedbe5a89 100644 --- a/clang/lib/Sema/TreeTransform.h +++ b/clang/lib/Sema/TreeTransform.h @@ -4076,7 +4076,8 @@ Sema::ConditionResult TreeTransform::TransformCondition( if (CondExpr.isInvalid()) return Sema::ConditionError(); - return getSema().ActOnCondition(nullptr, Loc, CondExpr.get(), Kind); + return getSema().ActOnCondition(nullptr, Loc, CondExpr.get(), Kind, + /*MissingOK=*/true); } return Sema::ConditionResult(); diff --git a/clang/test/AST/ast-dump-invalid.cpp b/clang/test/AST/ast-dump-invalid.cpp index d0db13786306..0a301dba51d2 100644 --- a/clang/test/AST/ast-dump-invalid.cpp +++ b/clang/test/AST/ast-dump-invalid.cpp @@ -33,7 +33,7 @@ int g(int i) { // CHECK-NEXT: |-ParmVarDecl // CHECK-NEXT: `-CompoundStmt // CHECK-NEXT: `-IfStmt {{.*}} -// CHECK-NEXT: |-OpaqueValueExpr {{.*}} <> 'bool' +// CHECK-NEXT: |-RecoveryExpr {{.*}} 'bool' // CHECK-NEXT: |-ReturnStmt {{.*}} // CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 4 // CHECK-NEXT: `-ReturnStmt {{.*}} diff --git a/clang/test/AST/loop-recovery.cpp b/clang/test/AST/loop-recovery.cpp new file mode 100644 index 000000000000..0561846c611f --- /dev/null +++ b/clang/test/AST/loop-recovery.cpp @@ -0,0 +1,65 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s +// RUN: not %clang_cc1 -fsyntax-only -ast-dump %s -std=c++17 | FileCheck %s + +void test() { + while(!!!) // expected-error {{expected expression}} + int whileBody; + // CHECK: WhileStmt + // CHECK: RecoveryExpr {{.*}} 'bool' + // CHECK: whileBody 'int' + + for(!!!) // expected-error {{expected expression}} expected-error {{expected ';'}} + int forBody; + // CHECK: ForStmt + // FIXME: the AST should have a RecoveryExpr to distinguish from for(;;) + // CHECK-NOT: RecoveryExpr + // CHECK: forBody 'int' + + for(auto c : !!!) // expected-error {{expected expression}} + int forEachBody; + // FIXME: parse the foreach body + // CHECK-NOT: CXXForRangeStmt + // CHECK-NOT: forEachBody 'int' + + do + int doBody; + while(!!!); // expected-error {{expected expression}} + // CHECK: DoStmt + // CHECK: doBody 'int' + // CHECK: RecoveryExpr {{.*}} 'bool' + + if(!!!) // expected-error {{expected expression}} + int ifBody; + else + int elseBody; + // CHECK: IfStmt + // CHECK: RecoveryExpr {{.*}} 'bool' + // CHECK: ifBody 'int' + // CHECK: elseBody 'int' + + switch(!!!) // expected-error {{expected expression}} + int switchBody; + // CHECK: SwitchStmt + // CHECK: RecoveryExpr {{.*}} 'int' + // CHECK: switchBody 'int' + + switch (;) // expected-error {{expected expression}} + int switchBody; + // CHECK: SwitchStmt + // CHECK: NullStmt + // CHECK: RecoveryExpr {{.*}} 'int' + // CHECK: switchBody 'int' + + switch (;;) // expected-error {{expected expression}} + int switchBody; + // CHECK: SwitchStmt + // CHECK: NullStmt + // CHECK: RecoveryExpr {{.*}} 'int' + // CHECK: switchBody 'int' + + switch (!!!;) // expected-error {{expected expression}} + int switchBody; + // CHECK: SwitchStmt + // CHECK: RecoveryExpr {{.*}} 'int' + // CHECK: switchBody 'int' +} diff --git a/clang/test/Parser/cxx0x-attributes.cpp b/clang/test/Parser/cxx0x-attributes.cpp index 1b34c8cfd103..5970e99b6b74 100644 --- a/clang/test/Parser/cxx0x-attributes.cpp +++ b/clang/test/Parser/cxx0x-attributes.cpp @@ -152,6 +152,7 @@ void bad_attributes_in_do_while() { [[ab]ab] ns::i); // expected-error {{an attribute list cannot appear here}} do {} while ( // expected-note {{to match this '('}} alignas(4 ns::i; // expected-note {{to match this '('}} + // expected-error@-1 {{expected ';' after do/while}} } // expected-error 2{{expected ')'}} expected-error {{expected expression}} [[]] using T = int; // expected-error {{an attribute list cannot appear here}} diff --git a/clang/test/Sema/complex-int.c b/clang/test/Sema/complex-int.c index 4f7093256db5..44b43b7cc665 100644 --- a/clang/test/Sema/complex-int.c +++ b/clang/test/Sema/complex-int.c @@ -18,8 +18,8 @@ result = arr*brr; result = xx*yy; switch (arr) { // expected-error{{statement requires expression of integer type ('_Complex int' invalid)}} - case brr: ; - case xx: ; + case brr: ; // expected-error{{integer constant expression must have integer type}} + case xx: ; // expected-error{{integer constant expression must have integer type}} } switch (ii) { case brr: ; // expected-error{{integer constant expression must have integer type}} diff --git a/clang/test/SemaCXX/condition.cpp b/clang/test/SemaCXX/condition.cpp index 01d53dc43c82..db7694c7ee02 100644 --- a/clang/test/SemaCXX/condition.cpp +++ b/clang/test/SemaCXX/condition.cpp @@ -20,6 +20,8 @@ void test() { while (struct S {} *x=0) ; // expected-error {{'S' cannot be defined in a condition}} while (struct {} *x=0) ; // expected-error-re {{'(unnamed struct at {{.*}})' cannot be defined in a condition}} switch (enum {E} x=0) ; // expected-error-re {{'(unnamed enum at {{.*}})' cannot be defined in a condition}} + // expected-warning@-1 {{switch statement has empty body}} + // expected-note@-2 {{put the semicolon on a separate line}} if (int x=0) { // expected-note 2 {{previous definition is here}} int x; // expected-error {{redefinition of 'x'}} diff --git a/clang/test/SemaCXX/constexpr-function-recovery-crash.cpp b/clang/test/SemaCXX/constexpr-function-recovery-crash.cpp index 6f0844f1e0d0..77e22a911cb4 100644 --- a/clang/test/SemaCXX/constexpr-function-recovery-crash.cpp +++ b/clang/test/SemaCXX/constexpr-function-recovery-crash.cpp @@ -77,3 +77,25 @@ constexpr void test11() { constexpr int test12() { return "wrong"; } // expected-error {{cannot initialize return object of type 'int'}} constexpr int force12 = test12(); // expected-error {{must be initialized by a constant}} + +#define TEST_EVALUATE(Name, X) \ + constexpr int testEvaluate##Name() { \ + X return 0; \ + } \ + constexpr int forceEvaluate##Name = testEvaluate##Name() +// Check that a variety of broken loops don't crash constant evaluation. +// We're not checking specific recovery here so don't assert diagnostics. +TEST_EVALUATE(Switch, switch (!!){}); // expected-error + {{}} +TEST_EVALUATE(SwitchInit, switch (auto x = !!){}); // expected-error + {{}} +TEST_EVALUATE(For, for (!!){}); // expected-error + {{}} + // FIXME: should bail out instead of looping. + // expected-note@-2 + {{infinite loop}} + // expected-note@-3 {{in call}} +TEST_EVALUATE(ForRange, for (auto x : !!){}); // expected-error + {{}} +TEST_EVALUATE(While, while (!!){}); // expected-error + {{}} +TEST_EVALUATE(DoWhile, do {} while (!!);); // expected-error + {{}} +TEST_EVALUATE(If, if (!!){};); // expected-error + {{}} +TEST_EVALUATE(IfInit, if (auto x = !!; 1){};);// expected-error + {{}} +TEST_EVALUATE(ForInit, if (!!;;){};); // expected-error + {{}} +TEST_EVALUATE(ForCond, if (; !!;){};); // expected-error + {{}} +TEST_EVALUATE(ForInc, if (;; !!){};); // expected-error + {{}}