Skip to content

[Parse] Create SwitchStmt nodes for switch statements with errors #36930

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 1 commit into from
Apr 16, 2021
Merged

[Parse] Create SwitchStmt nodes for switch statements with errors #36930

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
13 changes: 9 additions & 4 deletions include/swift/AST/Stmt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1110,15 +1110,19 @@ class SwitchStmt final : public LabeledStmt,
friend TrailingObjects;

SourceLoc SwitchLoc, LBraceLoc, RBraceLoc;
/// The location of the last token in the 'switch' statement. For valid
/// 'switch' statements this is the same as \c RBraceLoc. If the '}' is
/// missing this points to the last token before the '}' was expected.
SourceLoc EndLoc;
Comment on lines +1113 to +1116
Copy link
Member

Choose a reason for hiding this comment

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

Instead of introducing EndLoc, can we pass PreviousLoc to LBraceLoc and RBraceLoc in Parser?
This way, we can know "invalid" switch statements by checking LBraceLoc == RBraceLoc.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could… I also thought about just abusing RBraceLoc to store the end location like we do for BraceStmt. I think after the parser, no-one really cares about the LBraceLoc and RBraceLoc. But I didn’t really like that approach for BraceStmt already and I don’t want to make the situation worse.

I don’t know if there will ever be a use case for it but we wouldn’t be able to check whether there was a { but no } with your approach. And we would have LBraceLoc be after the end loc of the last case if just the } is missing, which isn’t nice either.

As an alternative, what do you think about renaming RBraceLoc to EndLoc and adding a boolean flag HasRBrace. If it is true, EndLoc represents the RBraceLoc and if it is false there was no }.

Copy link
Member

Choose a reason for hiding this comment

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

I agree abusing RBraceLoc is not nice.
It's OK. As long as you intentionally do this, I support it :)

Expr *SubjectExpr;

SwitchStmt(LabeledStmtInfo LabelInfo, SourceLoc SwitchLoc, Expr *SubjectExpr,
SourceLoc LBraceLoc, unsigned CaseCount, SourceLoc RBraceLoc,
Optional<bool> implicit = None)
SourceLoc EndLoc, Optional<bool> implicit = None)
: LabeledStmt(StmtKind::Switch, getDefaultImplicitFlag(implicit, SwitchLoc),
LabelInfo),
SwitchLoc(SwitchLoc), LBraceLoc(LBraceLoc), RBraceLoc(RBraceLoc),
SubjectExpr(SubjectExpr) {
EndLoc(EndLoc), SubjectExpr(SubjectExpr) {
Bits.SwitchStmt.CaseCount = CaseCount;
}

Expand All @@ -1129,6 +1133,7 @@ class SwitchStmt final : public LabeledStmt,
SourceLoc LBraceLoc,
ArrayRef<ASTNode> Cases,
SourceLoc RBraceLoc,
SourceLoc EndLoc,
ASTContext &C);

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

SourceLoc getStartLoc() const { return getLabelLocOrKeywordLoc(SwitchLoc); }
SourceLoc getEndLoc() const { return RBraceLoc; }
SourceLoc getEndLoc() const { return EndLoc; }

/// Get the subject expression of the switch.
Expr *getSubjectExpr() const { return SubjectExpr; }
void setSubjectExpr(Expr *e) { SubjectExpr = e; }
Expand Down
4 changes: 3 additions & 1 deletion lib/AST/Stmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,7 @@ SwitchStmt *SwitchStmt::create(LabeledStmtInfo LabelInfo, SourceLoc SwitchLoc,
SourceLoc LBraceLoc,
ArrayRef<ASTNode> Cases,
SourceLoc RBraceLoc,
SourceLoc EndLoc,
ASTContext &C) {
#ifndef NDEBUG
for (auto N : Cases)
Expand All @@ -509,7 +510,8 @@ SwitchStmt *SwitchStmt::create(LabeledStmtInfo LabelInfo, SourceLoc SwitchLoc,
alignof(SwitchStmt));
SwitchStmt *theSwitch = ::new (p) SwitchStmt(LabelInfo, SwitchLoc,
SubjectExpr, LBraceLoc,
Cases.size(), RBraceLoc);
Cases.size(), RBraceLoc,
EndLoc);

std::uninitialized_copy(Cases.begin(), Cases.end(),
theSwitch->getTrailingObjects<ASTNode>());
Expand Down
29 changes: 19 additions & 10 deletions lib/Parse/ParseStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2249,23 +2249,31 @@ ParserResult<Stmt> Parser::parseStmtSwitch(LabeledStmtInfo LabelInfo) {
SubjectExpr = makeParserErrorResult(new (Context) ErrorExpr(SubjectLoc));
} else {
SubjectExpr = parseExprBasic(diag::expected_switch_expr);
if (SubjectExpr.hasCodeCompletion()) {
return makeParserCodeCompletionResult<Stmt>();
}
if (SubjectExpr.isNull()) {
SubjectExpr = makeParserErrorResult(new (Context) ErrorExpr(SubjectLoc));
}
Status |= SubjectExpr;
}

if (!Tok.is(tok::l_brace)) {
diagnose(Tok, diag::expected_lbrace_after_switch);
return nullptr;
}
SourceLoc lBraceLoc = consumeToken(tok::l_brace);
SourceLoc lBraceLoc;
SourceLoc rBraceLoc;

SmallVector<ASTNode, 8> cases;

if (Status.isErrorOrHasCompletion()) {
return makeParserResult(
Status, SwitchStmt::create(LabelInfo, SwitchLoc, SubjectExpr.get(),
lBraceLoc, cases, rBraceLoc,
/*EndLoc=*/PreviousLoc, Context));
}

if (!consumeIf(tok::l_brace, lBraceLoc)) {
diagnose(Tok, diag::expected_lbrace_after_switch);
return makeParserResult(
Status, SwitchStmt::create(LabelInfo, SwitchLoc, SubjectExpr.get(),
lBraceLoc, cases, rBraceLoc,
/*EndLoc=*/PreviousLoc, Context));
}

Status |= parseStmtCases(cases, /*IsActive=*/true);

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

return makeParserResult(
Status, SwitchStmt::create(LabelInfo, SwitchLoc, SubjectExpr.get(),
lBraceLoc, cases, rBraceLoc, Context));
lBraceLoc, cases, rBraceLoc,
/*EndLoc=*/rBraceLoc, Context));
}

ParserStatus
Expand Down
7 changes: 4 additions & 3 deletions lib/Sema/DerivedConformanceCodable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1164,7 +1164,8 @@ deriveBodyEncodable_enum_encode(AbstractFunctionDecl *encodeDecl, void *) {
/*implicit*/ true, AccessSemantics::Ordinary);

auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), enumRef,
SourceLoc(), cases, SourceLoc(), C);
SourceLoc(), cases, SourceLoc(),
SourceLoc(), C);
Comment on lines 1166 to +1168
Copy link
Member

Choose a reason for hiding this comment

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

11 instances of this in lib/Sema/Derived*... We definitely should make SwitchStmt::createImplicit().

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea!

statements.push_back(switchStmt);

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

auto switchStmt =
SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), firstExpr,
SourceLoc(), cases, SourceLoc(), C);
SourceLoc(), cases, SourceLoc(), SourceLoc(), C);

statements.push_back(switchStmt);
}
Expand Down Expand Up @@ -2092,4 +2093,4 @@ ValueDecl *DerivedConformance::deriveDecodable(ValueDecl *requirement) {
}

return deriveDecodable_init(*this);
}
}
4 changes: 2 additions & 2 deletions lib/Sema/DerivedConformanceCodingKey.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ deriveBodyCodingKey_enum_stringValue(AbstractFunctionDecl *strValDecl, void *) {
auto *selfRef = DerivedConformance::createSelfDeclRef(strValDecl);
auto *switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(),
selfRef, SourceLoc(), cases,
SourceLoc(), C);
SourceLoc(), SourceLoc(), C);
body = BraceStmt::create(C, SourceLoc(), ASTNode(switchStmt), SourceLoc());
}

Expand Down Expand Up @@ -314,7 +314,7 @@ deriveBodyCodingKey_init_stringValue(AbstractFunctionDecl *initDecl, void *) {
/*Implicit=*/true);
auto *switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(),
stringValueRef, SourceLoc(), cases,
SourceLoc(), C);
SourceLoc(), SourceLoc(), C);
auto *body = BraceStmt::create(C, SourceLoc(), ASTNode(switchStmt),
SourceLoc());
return { body, /*isTypeChecked=*/false };
Expand Down
3 changes: 2 additions & 1 deletion lib/Sema/DerivedConformanceComparable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,8 @@ deriveBodyComparable_enum_hasAssociatedValues_lt(AbstractFunctionDecl *ltDecl, v
SourceLoc(), /*HasTrailingClosure*/ false,
/*implicit*/ true);
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), abExpr,
SourceLoc(), cases, SourceLoc(), C);
SourceLoc(), cases, SourceLoc(),
SourceLoc(), C);
statements.push_back(switchStmt);

auto body = BraceStmt::create(C, SourceLoc(), statements, SourceLoc());
Expand Down
9 changes: 6 additions & 3 deletions lib/Sema/DerivedConformanceEquatableHashable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ deriveBodyEquatable_enum_uninhabited_eq(AbstractFunctionDecl *eqDecl, void *) {
/*implicit*/ true,
TupleType::get(abTupleElts, C));
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), abExpr,
SourceLoc(), cases, SourceLoc(), C);
SourceLoc(), cases, SourceLoc(),
SourceLoc(), C);
statements.push_back(switchStmt);

auto body = BraceStmt::create(C, SourceLoc(), statements, SourceLoc());
Expand Down Expand Up @@ -277,7 +278,8 @@ deriveBodyEquatable_enum_hasAssociatedValues_eq(AbstractFunctionDecl *eqDecl,
SourceLoc(), /*HasTrailingClosure*/ false,
/*implicit*/ true);
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), abExpr,
SourceLoc(), cases, SourceLoc(), C);
SourceLoc(), cases, SourceLoc(),
SourceLoc(), C);
statements.push_back(switchStmt);

auto body = BraceStmt::create(C, SourceLoc(), statements, SourceLoc());
Expand Down Expand Up @@ -751,7 +753,8 @@ deriveBodyHashable_enum_hasAssociatedValues_hashInto(
auto enumRef = new (C) DeclRefExpr(selfDecl, DeclNameLoc(),
/*implicit*/true);
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), enumRef,
SourceLoc(), cases, SourceLoc(), C);
SourceLoc(), cases, SourceLoc(),
SourceLoc(), C);

auto body = BraceStmt::create(C, SourceLoc(), {ASTNode(switchStmt)},
SourceLoc());
Expand Down
6 changes: 4 additions & 2 deletions lib/Sema/DerivedConformanceRawRepresentable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@ deriveBodyRawRepresentable_raw(AbstractFunctionDecl *toRawDecl, void *) {

auto selfRef = DerivedConformance::createSelfDeclRef(toRawDecl);
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), selfRef,
SourceLoc(), cases, SourceLoc(), C);
SourceLoc(), cases, SourceLoc(),
SourceLoc(), C);
auto body = BraceStmt::create(C, SourceLoc(),
ASTNode(switchStmt),
SourceLoc());
Expand Down Expand Up @@ -386,7 +387,8 @@ deriveBodyRawRepresentable_init(AbstractFunctionDecl *initDecl, void *) {
switchArg = CallExpr;
}
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), switchArg,
SourceLoc(), cases, SourceLoc(), C);
SourceLoc(), cases, SourceLoc(),
SourceLoc(), C);
auto body = BraceStmt::create(C, SourceLoc(),
ASTNode(switchStmt),
SourceLoc());
Expand Down
3 changes: 2 additions & 1 deletion lib/Sema/DerivedConformances.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,7 +666,8 @@ DeclRefExpr *DerivedConformance::convertEnumToIndex(SmallVectorImpl<ASTNode> &st
AccessSemantics::Ordinary,
enumVarDecl->getType());
auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), enumRef,
SourceLoc(), cases, SourceLoc(), C);
SourceLoc(), cases, SourceLoc(),
SourceLoc(), C);

stmts.push_back(indexBind);
stmts.push_back(switchStmt);
Expand Down
6 changes: 6 additions & 0 deletions lib/Sema/TypeCheckSwitchStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,6 +1058,12 @@ namespace {

void diagnoseMissingCases(RequiresDefault defaultReason, Space uncovered,
const CaseStmt *unknownCase = nullptr) {
if (!Switch->getLBraceLoc().isValid()) {
// There is no '{' in the switch statement, which we already diagnosed
// in the parser. So there's no real body to speak of and it doesn't
// make sense to emit diagnostics about missing cases.
return;
}
auto &DE = Context.Diags;
SourceLoc startLoc = Switch->getStartLoc();
SourceLoc insertLoc;
Expand Down
18 changes: 18 additions & 0 deletions test/IDE/complete_sr14454.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// RUN: %target-swift-ide-test -batch-code-completion -source-filename %s -filecheck %raw-FileCheck -completion-output-dir %t

struct Foo {
let bar: Bool
}

func foo(_: (Foo) -> Void) {}

foo { x in
switch x.#^COMPLETE_WITHOUT_BRACE?check=CHECK^#
}
foo { x in
switch x.#^COMPLETE_WITH_BRACES?check=CHECK^# {}
}

// CHECK: Begin completions
// CHECK: Decl[InstanceVar]/CurrNominal: bar[#Bool#];
// CHECK: End completions
2 changes: 1 addition & 1 deletion test/Parse/recovery.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ func missingControllingExprInForEach() {
}

func missingControllingExprInSwitch() {
switch // expected-error {{expected expression in 'switch' statement}} expected-error {{expected '{' after 'switch' subject expression}}
switch // expected-error {{expected expression in 'switch' statement}}

switch { // expected-error {{expected expression in 'switch' statement}} expected-error {{'switch' statement body must have at least one 'case' or 'default' block}}
}
Expand Down
2 changes: 1 addition & 1 deletion test/Parse/switch.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ func ~= (x: (Int,Int), y: (Int,Int)) -> Bool {
}

func parseError1(x: Int) {
switch func {} // expected-error {{expected expression in 'switch' statement}} expected-error {{expected '{' after 'switch' subject expression}} expected-error {{expected identifier in function declaration}} expected-error {{closure expression is unused}} expected-note{{did you mean to use a 'do' statement?}} {{15-15=do }}
switch func {} // expected-error {{expected expression in 'switch' statement}} expected-error {{expected identifier in function declaration}} expected-error {{closure expression is unused}} expected-note{{did you mean to use a 'do' statement?}} {{15-15=do }}
}

func parseError2(x: Int) {
Expand Down