Check for duplicate literal switch cases #11257
Conversation
lib/Sema/TypeCheckSwitchStmt.cpp
Outdated
| // FIXME: Pessimistically using IEEEquad here is bad and we should | ||
| // actually figure out the bitwidth. But it's too early in Sema. | ||
| auto *FLE = cast<FloatLiteralExpr>(EL); | ||
| decltype(FloatLiteralCache)::iterator it; |
There was a problem hiding this comment.
This was left over from an experiment, could you remove it?
test/Sema/exhaustive_switch.swift
Outdated
| case 2.5: break // expected-note {{first occurence of identical literal pattern is here}} | ||
| case 2.5: break // expected-warning {{literal value is already handled by previous pattern; is this intentional?}} | ||
| case 3.5: break | ||
| default: break |
There was a problem hiding this comment.
These tests look good, but could you add some extra duplicate cases in
- Different positions e.g.
case 1.5: break
case 2.5: break //
case 2.5: break //
case 3.5: break
case 2.5: break //
default: break- Different levels of normalization:
case 1.5: break
case 1.5000: break
case 1.500: break
///
case 1
case 0b1
case 0x1
ffried
force-pushed
the
case_redundancy_check
branch
from
fa3835b to
35d7377
Compare
|
Xi should also take a look at this, but in the mean time @swift-ci please smoke test. |
| "literal value is already handled by previous pattern; " | ||
| "is this intentional?",()) | ||
| NOTE(redundant_particular_literal_case_here,none, | ||
| "first occurence of identical literal pattern is here", ()) |
There was a problem hiding this comment.
Spelling: this should be "occurrence"
| @@ -3773,6 +3773,11 @@ NOTE(missing_particular_case,none, | |||
| "add missing case: '%0'", (StringRef)) | |||
| WARNING(redundant_particular_case,none, | |||
| "case is already handled by previous patterns; consider removing it",()) | |||
| WARNING(redundant_particular_literal_case,none, | |||
| "literal value is already handled by previous pattern; " | |||
| "is this intentional?",()) | |||
There was a problem hiding this comment.
Nit: since there's no way to silence this warning without removing the redundant case, it's not actually possible for the user to answer the question "is this intentional?"--therefore, probably best to use the same wording as above (i.e. "consider removing it").
ffried
force-pushed
the
case_redundancy_check
branch
from
35d7377 to
7eb7ac7
Compare
|
@xwu done and done |
|
@swift-ci please smoke test |
|
I don't mind if this patch doesn't catch this case, but can you add a test like the following? switch (x, y) {
case (0, 0): break
case (0, 1): break // not duplicate
case (1, 0): break // not duplicate
case (0, 0): break // duplicate
default: break
}(and make sure it doesn't warn on the second and third cases) |
|
Could you also add a test for matching several cases in single case statement like: func main(_ a: Int) -> String {
switch a {
case 1, 2: return ""
case 1, 2: return ""
case 2, 3: return ""
default: return ""
}
} |
lib/Sema/TypeCheckSwitchStmt.cpp
Outdated
| SpaceEngine(TypeChecker &C, SwitchStmt *SS) : TC(C), Switch(SS) {} | ||
|
|
||
| bool checkRedundantLiteral(const Pattern *Pat, Expr *&PrevPattern) { | ||
| if (Pat->getKind() != PatternKind::Expr) return false; |
There was a problem hiding this comment.
nitpick: could we put return false into the next line?
lib/Sema/TypeCheckSwitchStmt.cpp
Outdated
| if (Pat->getKind() != PatternKind::Expr) return false; | ||
| auto *ExprPat = cast<ExprPattern>(Pat); | ||
| auto *MatchExpr = ExprPat->getSubExpr(); | ||
| if (!MatchExpr || !isa<LiteralExpr>(MatchExpr)) return false; |
This adds a check for duplicate literal switch cases. It covers string, integer and float literals. Corresponding tests were added as well.
ffried
force-pushed
the
case_redundancy_check
branch
from
7eb7ac7 to
e3fa190
Compare
|
@jrose-apple Done. Added a test with a tuple for each of the literal types. |
|
@nkcsgexi Done: Added test with multiple cases handled in a single case statement for each of the literal type. Also done: Added a line break, as well as curly brackets to the two ifs. |
|
@swift-ci please smoke test and merge |
|
With this PR, the following code prints a warning: let i: Int = 1
switch i {
case 0: print("0")
case 1, -1: print("1 or -1") // Warning: Literal value is already handled by previous pattern; consider removing it
default: print("something else")
}That doesn't look right to me...? |
|
@ianpartridge Yep, that's definitively a bug. Will open another PR fixing this asap. @CodaFi Should we revert this PR until then? |
|
@ianpartridge #11286 should fix this. |
|
I confirm it does - thanks. |
This adds a check for duplicate literal switch cases. It covers string, integer and float literals. Corresponding tests were added as well.
According to @CodaFi this should resolve rdar://28301984.