Skip to content

Commit 931f339

Browse files
ahoppenahoppen
committed
[Parse] Create SwitchStmt nodes for switch statements with errors
At the moment, if there is an error in the `switch` statement expression or if the `{` is missing, we return `nullptr` from `parseStmtSwitch`, but we consume tokens while trying to parse the `switch` statement. This causes the AST to not contain any nodes for the tokens that were consumed while trying to parse the `switch` statement. While this doesn’t cause any issues during compilation (compiling fails anyway so not having the `switch` statement in the AST is not a problem) this causes issues when trying to complete inside an expression that was consumed while trying to parse the `switch` statement but doesn’t have a representation in the AST. The solver-based completion approach can’t find the expression that contains the completion token (because it’s not part of the AST) and thus return empty results. To fix this, make sure we are always creating a `SwitchStmt` when consuming tokens for it. Previously, one could always assume that a `SwitchStmt` had a valid `LBraceLoc` and `RBraceLoc`. This is no longer the case because of the recovery. In order to form the `SwitchStmt`’s `SourceRange`, I needed to add a `EndLoc` property to `SwitchStmt` that keeps track of the last token in the `SwitchStmt`. Theoretically we should be able to compute this location by traversing the right brace, case stmts, subject expression, … in reverse order until we find something that’s not missing. But if the `SubjectExpr` is an `ErrorExpr`, representing a missing expression, it might have a source range that points to one after the last token in the statement (this is due to the way the `ErrorExpr` is being constructed), therefore returning an invalid range. So overall I thought it was easier and safer to add another property. Fixes rdar://76688441 [SR-14490]
1 parent 5cf19e4 commit 931f339

13 files changed

+77
-29
lines changed

‎include/swift/AST/Stmt.h

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1110,15 +1110,19 @@ class SwitchStmt final : public LabeledStmt,
11101110
friend TrailingObjects;
11111111

11121112
SourceLoc SwitchLoc, LBraceLoc, RBraceLoc;
1113+
/// The location of the last token in the 'switch' statement. For valid
1114+
/// 'switch' statements this is the same as \c RBraceLoc. If the '}' is
1115+
/// missing this points to the last token before the '}' was expected.
1116+
SourceLoc EndLoc;
11131117
Expr *SubjectExpr;
11141118

11151119
SwitchStmt(LabeledStmtInfo LabelInfo, SourceLoc SwitchLoc, Expr *SubjectExpr,
11161120
SourceLoc LBraceLoc, unsigned CaseCount, SourceLoc RBraceLoc,
1117-
Optional<bool> implicit = None)
1121+
SourceLoc EndLoc, Optional<bool> implicit = None)
11181122
: LabeledStmt(StmtKind::Switch, getDefaultImplicitFlag(implicit, SwitchLoc),
11191123
LabelInfo),
11201124
SwitchLoc(SwitchLoc), LBraceLoc(LBraceLoc), RBraceLoc(RBraceLoc),
1121-
SubjectExpr(SubjectExpr) {
1125+
EndLoc(EndLoc), SubjectExpr(SubjectExpr) {
11221126
Bits.SwitchStmt.CaseCount = CaseCount;
11231127
}
11241128

@@ -1129,6 +1133,7 @@ class SwitchStmt final : public LabeledStmt,
11291133
SourceLoc LBraceLoc,
11301134
ArrayRef<ASTNode> Cases,
11311135
SourceLoc RBraceLoc,
1136+
SourceLoc EndLoc,
11321137
ASTContext &C);
11331138

11341139
/// Get the source location of the 'switch' keyword.
@@ -1141,8 +1146,8 @@ class SwitchStmt final : public LabeledStmt,
11411146
SourceLoc getLoc() const { return SwitchLoc; }
11421147

11431148
SourceLoc getStartLoc() const { return getLabelLocOrKeywordLoc(SwitchLoc); }
1144-
SourceLoc getEndLoc() const { return RBraceLoc; }
1145-
1149+
SourceLoc getEndLoc() const { return EndLoc; }
1150+
11461151
/// Get the subject expression of the switch.
11471152
Expr *getSubjectExpr() const { return SubjectExpr; }
11481153
void setSubjectExpr(Expr *e) { SubjectExpr = e; }

‎lib/AST/Stmt.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -497,6 +497,7 @@ SwitchStmt *SwitchStmt::create(LabeledStmtInfo LabelInfo, SourceLoc SwitchLoc,
497497
SourceLoc LBraceLoc,
498498
ArrayRef<ASTNode> Cases,
499499
SourceLoc RBraceLoc,
500+
SourceLoc EndLoc,
500501
ASTContext &C) {
501502
#ifndef NDEBUG
502503
for (auto N : Cases)
@@ -509,7 +510,8 @@ SwitchStmt *SwitchStmt::create(LabeledStmtInfo LabelInfo, SourceLoc SwitchLoc,
509510
alignof(SwitchStmt));
510511
SwitchStmt *theSwitch = ::new (p) SwitchStmt(LabelInfo, SwitchLoc,
511512
SubjectExpr, LBraceLoc,
512-
Cases.size(), RBraceLoc);
513+
Cases.size(), RBraceLoc,
514+
EndLoc);
513515

514516
std::uninitialized_copy(Cases.begin(), Cases.end(),
515517
theSwitch->getTrailingObjects<ASTNode>());

‎lib/Parse/ParseStmt.cpp

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2249,23 +2249,31 @@ ParserResult<Stmt> Parser::parseStmtSwitch(LabeledStmtInfo LabelInfo) {
22492249
SubjectExpr = makeParserErrorResult(new (Context) ErrorExpr(SubjectLoc));
22502250
} else {
22512251
SubjectExpr = parseExprBasic(diag::expected_switch_expr);
2252-
if (SubjectExpr.hasCodeCompletion()) {
2253-
return makeParserCodeCompletionResult<Stmt>();
2254-
}
22552252
if (SubjectExpr.isNull()) {
22562253
SubjectExpr = makeParserErrorResult(new (Context) ErrorExpr(SubjectLoc));
22572254
}
22582255
Status |= SubjectExpr;
22592256
}
22602257

2261-
if (!Tok.is(tok::l_brace)) {
2262-
diagnose(Tok, diag::expected_lbrace_after_switch);
2263-
return nullptr;
2264-
}
2265-
SourceLoc lBraceLoc = consumeToken(tok::l_brace);
2258+
SourceLoc lBraceLoc;
22662259
SourceLoc rBraceLoc;
2267-
22682260
SmallVector<ASTNode, 8> cases;
2261+
2262+
if (Status.isErrorOrHasCompletion()) {
2263+
return makeParserResult(
2264+
Status, SwitchStmt::create(LabelInfo, SwitchLoc, SubjectExpr.get(),
2265+
lBraceLoc, cases, rBraceLoc,
2266+
/*EndLoc=*/PreviousLoc, Context));
2267+
}
2268+
2269+
if (!consumeIf(tok::l_brace, lBraceLoc)) {
2270+
diagnose(Tok, diag::expected_lbrace_after_switch);
2271+
return makeParserResult(
2272+
Status, SwitchStmt::create(LabelInfo, SwitchLoc, SubjectExpr.get(),
2273+
lBraceLoc, cases, rBraceLoc,
2274+
/*EndLoc=*/PreviousLoc, Context));
2275+
}
2276+
22692277
Status |= parseStmtCases(cases, /*IsActive=*/true);
22702278

22712279
// We cannot have additional cases after a default clause. Complain on
@@ -2288,7 +2296,8 @@ ParserResult<Stmt> Parser::parseStmtSwitch(LabeledStmtInfo LabelInfo) {
22882296

22892297
return makeParserResult(
22902298
Status, SwitchStmt::create(LabelInfo, SwitchLoc, SubjectExpr.get(),
2291-
lBraceLoc, cases, rBraceLoc, Context));
2299+
lBraceLoc, cases, rBraceLoc,
2300+
/*EndLoc=*/rBraceLoc, Context));
22922301
}
22932302

22942303
ParserStatus

‎lib/Sema/DerivedConformanceCodable.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1164,7 +1164,8 @@ deriveBodyEncodable_enum_encode(AbstractFunctionDecl *encodeDecl, void *) {
11641164
/*implicit*/ true, AccessSemantics::Ordinary);
11651165

11661166
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), enumRef,
1167-
SourceLoc(), cases, SourceLoc(), C);
1167+
SourceLoc(), cases, SourceLoc(),
1168+
SourceLoc(), C);
11681169
statements.push_back(switchStmt);
11691170

11701171
auto *body = BraceStmt::create(C, SourceLoc(), statements, SourceLoc(),
@@ -1801,7 +1802,7 @@ deriveBodyDecodable_enum_init(AbstractFunctionDecl *initDecl, void *) {
18011802

18021803
auto switchStmt =
18031804
SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), firstExpr,
1804-
SourceLoc(), cases, SourceLoc(), C);
1805+
SourceLoc(), cases, SourceLoc(), SourceLoc(), C);
18051806

18061807
statements.push_back(switchStmt);
18071808
}
@@ -2092,4 +2093,4 @@ ValueDecl *DerivedConformance::deriveDecodable(ValueDecl *requirement) {
20922093
}
20932094

20942095
return deriveDecodable_init(*this);
2095-
}
2096+
}

‎lib/Sema/DerivedConformanceCodingKey.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ deriveBodyCodingKey_enum_stringValue(AbstractFunctionDecl *strValDecl, void *) {
234234
auto *selfRef = DerivedConformance::createSelfDeclRef(strValDecl);
235235
auto *switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(),
236236
selfRef, SourceLoc(), cases,
237-
SourceLoc(), C);
237+
SourceLoc(), SourceLoc(), C);
238238
body = BraceStmt::create(C, SourceLoc(), ASTNode(switchStmt), SourceLoc());
239239
}
240240

@@ -314,7 +314,7 @@ deriveBodyCodingKey_init_stringValue(AbstractFunctionDecl *initDecl, void *) {
314314
/*Implicit=*/true);
315315
auto *switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(),
316316
stringValueRef, SourceLoc(), cases,
317-
SourceLoc(), C);
317+
SourceLoc(), SourceLoc(), C);
318318
auto *body = BraceStmt::create(C, SourceLoc(), ASTNode(switchStmt),
319319
SourceLoc());
320320
return { body, /*isTypeChecked=*/false };

‎lib/Sema/DerivedConformanceComparable.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,8 @@ deriveBodyComparable_enum_hasAssociatedValues_lt(AbstractFunctionDecl *ltDecl, v
211211
SourceLoc(), /*HasTrailingClosure*/ false,
212212
/*implicit*/ true);
213213
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), abExpr,
214-
SourceLoc(), cases, SourceLoc(), C);
214+
SourceLoc(), cases, SourceLoc(),
215+
SourceLoc(), C);
215216
statements.push_back(switchStmt);
216217

217218
auto body = BraceStmt::create(C, SourceLoc(), statements, SourceLoc());

‎lib/Sema/DerivedConformanceEquatableHashable.cpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,8 @@ deriveBodyEquatable_enum_uninhabited_eq(AbstractFunctionDecl *eqDecl, void *) {
8080
/*implicit*/ true,
8181
TupleType::get(abTupleElts, C));
8282
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), abExpr,
83-
SourceLoc(), cases, SourceLoc(), C);
83+
SourceLoc(), cases, SourceLoc(),
84+
SourceLoc(), C);
8485
statements.push_back(switchStmt);
8586

8687
auto body = BraceStmt::create(C, SourceLoc(), statements, SourceLoc());
@@ -277,7 +278,8 @@ deriveBodyEquatable_enum_hasAssociatedValues_eq(AbstractFunctionDecl *eqDecl,
277278
SourceLoc(), /*HasTrailingClosure*/ false,
278279
/*implicit*/ true);
279280
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), abExpr,
280-
SourceLoc(), cases, SourceLoc(), C);
281+
SourceLoc(), cases, SourceLoc(),
282+
SourceLoc(), C);
281283
statements.push_back(switchStmt);
282284

283285
auto body = BraceStmt::create(C, SourceLoc(), statements, SourceLoc());
@@ -751,7 +753,8 @@ deriveBodyHashable_enum_hasAssociatedValues_hashInto(
751753
auto enumRef = new (C) DeclRefExpr(selfDecl, DeclNameLoc(),
752754
/*implicit*/true);
753755
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), enumRef,
754-
SourceLoc(), cases, SourceLoc(), C);
756+
SourceLoc(), cases, SourceLoc(),
757+
SourceLoc(), C);
755758

756759
auto body = BraceStmt::create(C, SourceLoc(), {ASTNode(switchStmt)},
757760
SourceLoc());

‎lib/Sema/DerivedConformanceRawRepresentable.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,8 @@ deriveBodyRawRepresentable_raw(AbstractFunctionDecl *toRawDecl, void *) {
129129

130130
auto selfRef = DerivedConformance::createSelfDeclRef(toRawDecl);
131131
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), selfRef,
132-
SourceLoc(), cases, SourceLoc(), C);
132+
SourceLoc(), cases, SourceLoc(),
133+
SourceLoc(), C);
133134
auto body = BraceStmt::create(C, SourceLoc(),
134135
ASTNode(switchStmt),
135136
SourceLoc());
@@ -386,7 +387,8 @@ deriveBodyRawRepresentable_init(AbstractFunctionDecl *initDecl, void *) {
386387
switchArg = CallExpr;
387388
}
388389
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), switchArg,
389-
SourceLoc(), cases, SourceLoc(), C);
390+
SourceLoc(), cases, SourceLoc(),
391+
SourceLoc(), C);
390392
auto body = BraceStmt::create(C, SourceLoc(),
391393
ASTNode(switchStmt),
392394
SourceLoc());

‎lib/Sema/DerivedConformances.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -666,7 +666,8 @@ DeclRefExpr *DerivedConformance::convertEnumToIndex(SmallVectorImpl<ASTNode> &st
666666
AccessSemantics::Ordinary,
667667
enumVarDecl->getType());
668668
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), enumRef,
669-
SourceLoc(), cases, SourceLoc(), C);
669+
SourceLoc(), cases, SourceLoc(),
670+
SourceLoc(), C);
670671

671672
stmts.push_back(indexBind);
672673
stmts.push_back(switchStmt);

‎lib/Sema/TypeCheckSwitchStmt.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1058,6 +1058,12 @@ namespace {
10581058

10591059
void diagnoseMissingCases(RequiresDefault defaultReason, Space uncovered,
10601060
const CaseStmt *unknownCase = nullptr) {
1061+
if (!Switch->getLBraceLoc().isValid()) {
1062+
// There is no '{' in the switch statement, which we already diagnosed
1063+
// in the parser. So there's no real body to speak of and it doesn't
1064+
// make sense to emit diagnostics about missing cases.
1065+
return;
1066+
}
10611067
auto &DE = Context.Diags;
10621068
SourceLoc startLoc = Switch->getStartLoc();
10631069
SourceLoc insertLoc;

0 commit comments

Comments
 (0)