Skip to content

[Sema] Check for let vs var bindings of the same variable name in multiple case patterns #15488

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.

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
Mar 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -2930,6 +2930,9 @@ ERROR(fallthrough_into_case_with_var_binding,none,
ERROR(unnecessary_cast_over_optionset,none,
"unnecessary cast over raw value of %0", (Type))

ERROR(mutability_mismatch_multiple_pattern_list,none,
"'%select{var|let}0' pattern binding must match previous "
"'%select{var|let}1' pattern binding", (bool, bool))
ERROR(type_mismatch_multiple_pattern_list,none,
"pattern variable bound to type %0, expected type %1", (Type, Type))
ERROR(type_mismatch_fallthrough_pattern_list,none,
Expand Down
50 changes: 33 additions & 17 deletions lib/Sema/TypeCheckStmt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -871,25 +871,41 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
// For each variable in the pattern, make sure its type is identical to what it
// was in the first label item's pattern.
auto firstPattern = caseBlock->getCaseLabelItems()[0].getPattern();
if (pattern != firstPattern) {
SmallVector<VarDecl *, 4> vars;
firstPattern->collectVariables(vars);
pattern->forEachVariable([&](VarDecl *VD) {
if (!VD->hasName())
return;
for (auto *expected : vars) {
if (expected->hasName() && expected->getName() == VD->getName()) {
if (!VD->getType()->isEqual(expected->getType())) {
TC.diagnose(VD->getLoc(), diag::type_mismatch_multiple_pattern_list,
VD->getType(), expected->getType());
VD->markInvalid();
expected->markInvalid();
}
return;
SmallVector<VarDecl *, 4> vars;
firstPattern->collectVariables(vars);
pattern->forEachVariable([&](VarDecl *VD) {
if (!VD->hasName())
return;
for (auto *expected : vars) {
if (expected->hasName() && expected->getName() == VD->getName()) {
if (VD->hasType() && expected->hasType() && !expected->isInvalid() &&
!VD->getType()->isEqual(expected->getType())) {
TC.diagnose(VD->getLoc(), diag::type_mismatch_multiple_pattern_list,
VD->getType(), expected->getType());
VD->markInvalid();
expected->markInvalid();
}
if (expected->isLet() != VD->isLet()) {
auto diag = TC.diagnose(VD->getLoc(),
diag::mutability_mismatch_multiple_pattern_list,
VD->isLet(), expected->isLet());

VarPattern *foundVP = nullptr;
VD->getParentPattern()->forEachNode([&](Pattern *P) {
if (auto *VP = dyn_cast<VarPattern>(P))
if (VP->getSingleVar() == VD)
foundVP = VP;
});
if (foundVP)
diag.fixItReplace(foundVP->getLoc(),
expected->isLet() ? "let" : "var");
VD->markInvalid();
expected->markInvalid();
}
return;
}
});
}
}
});
}
// Check the guard expression, if present.
if (auto *guard = labelItem.getGuardExpr()) {
Expand Down
22 changes: 22 additions & 0 deletions test/Parse/switch.swift
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,28 @@ func patternVarDiffType(x: Int, y: Double) {
}
}

func patternVarDiffMutability(x: Int, y: Double) {
switch x {
case let a where a < 5, var a where a > 10: // expected-error {{'var' pattern binding must match previous 'let' pattern binding}}{{27-30=let}}
break
default:
break
}
switch (x, y) {
// Would be nice to have a fixit in the following line if we detect that all bindings in the same pattern have the same problem.
case let (a, b) where a < 5, var (a, b) where a > 10: // expected-error 2{{'var' pattern binding must match previous 'let' pattern binding}}{{none}}
break
case (let a, var b) where a < 5, (let a, let b) where a > 10: // expected-error {{'let' pattern binding must match previous 'var' pattern binding}}{{44-47=var}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test case with three patterns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jrose-apple Ah, good call, with three patterns and two wrong, invalidating the first pattern variable when comparing with the 2nd caused a spurious diagnostic about the type of the 3rd not matching error-type. Fixed now!

break
case (let a, let b) where a < 5, (var a, let b) where a > 10, (let a, var b) where a == 8:
// expected-error@-1 {{'var' pattern binding must match previous 'let' pattern binding}}{{37-40=let}}
// expected-error@-2 {{'var' pattern binding must match previous 'let' pattern binding}}{{73-76=let}}
break
default:
break
}
}

func test_label(x : Int) {
Gronk: // expected-error {{switch must be exhaustive}} expected-note{{do you want to add a default clause?}}
switch x {
Expand Down