Skip to content

Commit 5d185d1

Browse files
CodaFiCodaFi
authored
Merge pull request #9121 from CodaFi/lassen-fleigen
Lift case redundancy checks into Sema
2 parents ca15119 + 4e2f36c commit 5d185d1

File tree

10 files changed

+123
-78
lines changed

10 files changed

+123
-78
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3610,6 +3610,8 @@ ERROR(non_exhaustive_switch,none,
36103610
"%select{:|?}0 %2", (bool, bool, StringRef))
36113611
NOTE(missing_particular_case,none,
36123612
"missing case: '%0'", (StringRef))
3613+
WARNING(redundant_particular_case,none,
3614+
"case is already handled by previous patterns; consider removing it",())
36133615

36143616
#ifndef DIAG_NO_UNDEF
36153617
# if defined(DIAG)

‎lib/Sema/TypeCheckSwitchStmt.cpp

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,37 @@ namespace {
128128
return TypeAndVal.getInt();
129129
}
130130

131+
// Defines a "usefulness" metric that returns whether the given space
132+
// contributes meaningfully to the exhaustiveness of a pattern.
133+
bool isUseful() const {
134+
auto subspacesUseful = [](const Space &space) {
135+
for (auto &subspace : space.getSpaces()) {
136+
if (!subspace.isUseful()) {
137+
return false;
138+
}
139+
}
140+
return true;
141+
};
142+
143+
switch (getKind()) {
144+
case SpaceKind::Empty:
145+
return false;
146+
case SpaceKind::Type:
147+
case SpaceKind::BooleanConstant:
148+
return true;
149+
case SpaceKind::Disjunct:
150+
if (getSpaces().empty()) {
151+
return false;
152+
}
153+
return subspacesUseful(*this);
154+
case SpaceKind::Constructor:
155+
if (getSpaces().empty()) {
156+
return true;
157+
}
158+
return subspacesUseful(*this);
159+
}
160+
}
161+
131162
// An optimization that computes if the difference of this space and
132163
// another space is empty.
133164
bool isSubspace(const Space &other) const {
@@ -235,7 +266,7 @@ namespace {
235266
}
236267
return true;
237268
}
238-
PAIRCASE(SpaceKind::Constructor, SpaceKind::Disjunct):
269+
PAIRCASE (SpaceKind::Constructor, SpaceKind::Disjunct):
239270
PAIRCASE (SpaceKind::BooleanConstant, SpaceKind::Disjunct): {
240271
// S <= (S1 | ... | Sn) <= S iff (S <= S1) || ... || (S <= Sn)
241272
for (auto &param : other.getSpaces()) {
@@ -402,7 +433,6 @@ namespace {
402433
return Space();
403434

404435
PAIRCASE (SpaceKind::Type, SpaceKind::BooleanConstant): {
405-
// if (isSubType(tp2, tp1)) b
406436
if (canDecompose(this->getType())) {
407437
SmallVector<Space, 4> spaces;
408438
decompose(this->getType(), spaces);
@@ -613,7 +643,7 @@ namespace {
613643
}
614644
break;
615645
case SpaceKind::BooleanConstant:
616-
buffer << ((getBoolValue()) ? "true" : "false");
646+
buffer << (getBoolValue() ? "true" : "false");
617647
break;
618648
case SpaceKind::Constructor: {
619649
if (!Head.empty()) {
@@ -806,11 +836,17 @@ namespace {
806836
if (caseItem.getGuardExpr())
807837
continue;
808838

809-
auto projection = projectPattern(Ctx, caseItem.getPattern());
810-
811839
// Space is trivially covered with a default clause.
812840
if (caseItem.isDefault())
813841
return;
842+
843+
auto projection = projectPattern(Ctx, caseItem.getPattern());
844+
if (projection.isUseful() && projection.isSubspace(Space(spaces))) {
845+
Ctx.Diags
846+
.diagnose(caseItem.getStartLoc(),
847+
diag::redundant_particular_case)
848+
.highlight(caseItem.getSourceRange());
849+
}
814850
spaces.push_back(projection);
815851
}
816852
}
@@ -895,17 +931,17 @@ namespace {
895931
if (flats.empty()) {
896932
flats.append({ uncoveredSpace });
897933
}
898-
for (size_t i = 0; i < flats.size(); ++i) {
899-
auto &FlatSpace = flats[i];
934+
for (auto &flat : flats) {
900935
OS << tok::kw_case << " ";
901-
FlatSpace.show(OS);
936+
flat.show(OS);
902937
OS << ": " << Placeholder << "\n";
903938
}
904939
}
905940

906-
auto Diag = Ctx.Diags.diagnose(StartLoc, diag::non_exhaustive_switch,
907-
InEditor, false, Buffer.str());
908-
Diag.fixItInsert(EndLoc, Buffer.str());
941+
Ctx.Diags
942+
.diagnose(StartLoc, diag::non_exhaustive_switch, InEditor, false,
943+
Buffer.str())
944+
.fixItInsert(EndLoc, Buffer.str());
909945
} else {
910946
Ctx.Diags.diagnose(StartLoc, diag::non_exhaustive_switch,
911947
InEditor, false, Buffer.str());
@@ -916,10 +952,9 @@ namespace {
916952
if (flats.empty()) {
917953
flats.append({ uncoveredSpace });
918954
}
919-
for (size_t i = 0; i < flats.size(); ++i) {
955+
for (auto &flat : flats) {
920956
Buffer.clear();
921-
auto &FlatSpace = flats[i];
922-
FlatSpace.show(OS);
957+
flat.show(OS);
923958
Ctx.Diags.diagnose(StartLoc, diag::missing_particular_case,
924959
Buffer.str());
925960
}

‎test/Constraints/patterns.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,8 +158,8 @@ case let x???: print(x, terminator: "")
158158
case let x??: print(x as Any, terminator: "")
159159
case let x?: print(x as Any, terminator: "")
160160
case 4???: break
161-
case nil??: break
162-
case nil?: break
161+
case nil??: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
162+
case nil?: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
163163
default: break
164164
}
165165

‎test/Parse/matching_patterns.swift

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,20 @@ case square(9):
2828
// 'var' and 'let' patterns.
2929
case var a:
3030
a = 1
31-
case let a:
31+
case let a: // expected-warning {{case is already handled by previous patterns; consider removing it}}
3232
a = 1 // expected-error {{cannot assign}}
3333
case var var a: // expected-error {{'var' cannot appear nested inside another 'var' or 'let' pattern}}
34+
// expected-warning@-1 {{case is already handled by previous patterns; consider removing it}}
3435
a += 1
3536
case var let a: // expected-error {{'let' cannot appear nested inside another 'var' or 'let' pattern}}
37+
// expected-warning@-1 {{case is already handled by previous patterns; consider removing it}}
3638
print(a, terminator: "")
3739
case var (var b): // expected-error {{'var' cannot appear nested inside another 'var'}}
40+
// expected-warning@-1 {{case is already handled by previous patterns; consider removing it}}
3841
b += 1
3942

4043
// 'Any' pattern.
41-
case _:
44+
case _: // expected-warning {{case is already handled by previous patterns; consider removing it}}
4245
()
4346

4447
// patterns are resolved in expression-only positions are errors.
@@ -49,7 +52,7 @@ case 1 + (_): // expected-error{{'_' can only appear in a pattern or on the left
4952
switch (x,x) {
5053
case (var a, var a): // expected-error {{definition conflicts with previous value}} expected-note {{previous definition of 'a' is here}} expected-warning {{variable 'a' was never used; consider replacing with '_' or removing it}} expected-warning {{variable 'a' was never used; consider replacing with '_' or removing it}}
5154
fallthrough
52-
case _:
55+
case _: // expected-warning {{case is already handled by previous patterns; consider removing it}}
5356
()
5457
}
5558

@@ -153,10 +156,10 @@ struct ContainsEnum {
153156
// expected-note@-1 {{missing case: '.Mere(_)'}}
154157
// expected-note@-2 {{missing case: '.Twain(_, _)'}}
155158
case ContainsEnum.Possible<Int>.Naught,
156-
ContainsEnum.Possible.Naught,
157-
Possible<Int>.Naught,
158-
Possible.Naught,
159-
.Naught:
159+
ContainsEnum.Possible.Naught, // expected-warning {{case is already handled by previous patterns; consider removing it}}
160+
Possible<Int>.Naught, // expected-warning {{case is already handled by previous patterns; consider removing it}}
161+
Possible.Naught, // expected-warning {{case is already handled by previous patterns; consider removing it}}
162+
.Naught: // expected-warning {{case is already handled by previous patterns; consider removing it}}
160163
()
161164
}
162165
}
@@ -167,7 +170,7 @@ func nonmemberAccessesMemberType(_ n: ContainsEnum.Possible<Int>) {
167170
// expected-note@-1 {{missing case: '.Mere(_)'}}
168171
// expected-note@-2 {{missing case: '.Twain(_, _)'}}
169172
case ContainsEnum.Possible<Int>.Naught,
170-
.Naught:
173+
.Naught: // expected-warning {{case is already handled by previous patterns; consider removing it}}
171174
()
172175
}
173176
}
@@ -176,15 +179,15 @@ var m : ImportedEnum = .Simple
176179

177180
switch m {
178181
case imported_enums.ImportedEnum.Simple,
179-
ImportedEnum.Simple,
180-
.Simple:
182+
ImportedEnum.Simple, // expected-warning {{case is already handled by previous patterns; consider removing it}}
183+
.Simple: // expected-warning {{case is already handled by previous patterns; consider removing it}}
181184
()
182185
case imported_enums.ImportedEnum.Compound,
183-
imported_enums.ImportedEnum.Compound(_),
184-
ImportedEnum.Compound,
185-
ImportedEnum.Compound(_),
186-
.Compound,
187-
.Compound(_):
186+
imported_enums.ImportedEnum.Compound(_), // expected-warning {{case is already handled by previous patterns; consider removing it}}
187+
ImportedEnum.Compound, // expected-warning {{case is already handled by previous patterns; consider removing it}}
188+
ImportedEnum.Compound(_), // expected-warning {{case is already handled by previous patterns; consider removing it}}
189+
.Compound, // expected-warning {{case is already handled by previous patterns; consider removing it}}
190+
.Compound(_): // expected-warning {{case is already handled by previous patterns; consider removing it}}
188191
()
189192
}
190193

@@ -206,22 +209,22 @@ case .Payload(name: 0):
206209
case let .Payload(x):
207210
acceptInt(x)
208211
acceptString("\(x)")
209-
case let .Payload(name: x):
212+
case let .Payload(name: x): // expected-warning {{case is already handled by previous patterns; consider removing it}}
210213
acceptInt(x)
211214
acceptString("\(x)")
212-
case let .Payload((name: x)):
215+
case let .Payload((name: x)): // expected-warning {{case is already handled by previous patterns; consider removing it}}
213216
acceptInt(x)
214217
acceptString("\(x)")
215-
case .Payload(let (name: x)):
218+
case .Payload(let (name: x)): // expected-warning {{case is already handled by previous patterns; consider removing it}}
216219
acceptInt(x)
217220
acceptString("\(x)")
218-
case .Payload(let (name: x)):
221+
case .Payload(let (name: x)): // expected-warning {{case is already handled by previous patterns; consider removing it}}
219222
acceptInt(x)
220223
acceptString("\(x)")
221-
case .Payload(let x):
224+
case .Payload(let x): // expected-warning {{case is already handled by previous patterns; consider removing it}}
222225
acceptInt(x)
223226
acceptString("\(x)")
224-
case .Payload((let x)):
227+
case .Payload((let x)): // expected-warning {{case is already handled by previous patterns; consider removing it}}
225228
acceptInt(x)
226229
acceptString("\(x)")
227230
}
@@ -302,7 +305,7 @@ switch op2 {
302305
case nil: break
303306
case _?: break
304307
case (1?)?: break
305-
case (_?)?: break
308+
case (_?)?: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
306309
}
307310

308311

‎test/Parse/switch.swift

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,17 +221,21 @@ case (1, var b): // expected-warning {{variable 'b' was never used; consider rep
221221
case (1, let b): // let bindings expected-warning {{immutable value 'b' was never used; consider replacing with '_' or removing it}}
222222
()
223223

224-
case (_, 2), (let a, _): // expected-error {{'a' must be bound in every pattern}}
224+
case (_, 2), (let a, _): // expected-error {{'a' must be bound in every pattern}} expected-warning {{case is already handled by previous patterns; consider removing it}}
225225
()
226226

227227
// OK
228228
case (_, 2), (1, _):
229229
()
230230

231231
case (_, var a), (_, var a): // expected-warning {{variable 'a' was never used; consider replacing with '_' or removing it}}
232+
// expected-warning@-1 {{case is already handled by previous patterns; consider removing it}}
233+
// expected-warning@-2 {{case is already handled by previous patterns; consider removing it}}
232234
()
233235

234236
case (var a, var b), (var b, var a): // expected-warning {{variable 'a' was never used; consider replacing with '_' or removing it}} expected-warning {{variable 'b' was never used; consider replacing with '_' or removing it}}
237+
// expected-warning@-1 {{case is already handled by previous patterns; consider removing it}}
238+
// expected-warning@-2 {{case is already handled by previous patterns; consider removing it}}
235239
()
236240

237241
case (_, 2): // expected-error {{'case' label in a 'switch' should have at least one executable statement}} {{13-13= break}}

‎test/SILGen/unreachable_code.swift

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ func testUnreachableCase1(a : Tree) {
6868
_ = Leaf
6969
return
7070
case .Branch(_): // expected-warning {{case will never be executed}}
71+
// expected-warning@-1 {{case is already handled by previous patterns; consider removing it}}
7172
return
7273
}
7374
}
@@ -77,7 +78,7 @@ func testUnreachableCase2(a : Tree) {
7778
case let Leaf:
7879
_ = Leaf
7980
fallthrough
80-
case .Branch(_):
81+
case .Branch(_): // expected-warning {{case is already handled by previous patterns; consider removing it}}
8182
return
8283
}
8384
}
@@ -87,6 +88,7 @@ func testUnreachableCase3(a : Tree) {
8788
case _:
8889
break
8990
case .Branch(_): // expected-warning {{case will never be executed}}
91+
// expected-warning@-1 {{case is already handled by previous patterns; consider removing it}}
9092
return
9193
}
9294
}

‎test/Sema/exhaustive_switch.swift

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -232,29 +232,27 @@ func f(a: X, b: X) {
232232
}
233233
}
234234

235-
// FIXME: Duplicate case warning is not caught by dataflow diagnostics.
236-
//func f2(a: X, b: X) {
237-
// switch (a, b) {
238-
//
239-
// case (.A, .A): ()
240-
// case (.B, .B): ()
241-
//
242-
// case (.A, .B): ()
243-
// case (.B, .A): ()
244-
//
245-
// case (_, .Empty): ()
246-
// case (.Empty, _): ()
247-
//
248-
// // duplicate cases, but no warning? (comment out the line above and warnings appear!?)
249-
// case (.A, .A): ()
250-
// case (.B, .B): ()
251-
//
252-
// case (.A, .B): ()
253-
// case (.B, .A): ()
254-
//
255-
// default: ()
256-
// }
257-
//}
235+
func f2(a: X, b: X) {
236+
switch (a, b) {
237+
238+
case (.A, .A): ()
239+
case (.B, .B): ()
240+
241+
case (.A, .B): ()
242+
case (.B, .A): ()
243+
244+
case (_, .Empty): ()
245+
case (.Empty, _): ()
246+
247+
case (.A, .A): () // expected-warning {{case is already handled by previous patterns; consider removing it}}
248+
case (.B, .B): () // expected-warning {{case is already handled by previous patterns; consider removing it}}
249+
250+
case (.A, .B): () // expected-warning {{case is already handled by previous patterns; consider removing it}}
251+
case (.B, .A): () // expected-warning {{case is already handled by previous patterns; consider removing it}}
252+
253+
default: ()
254+
}
255+
}
258256

259257
enum XX : Int {
260258
case A

0 commit comments

Comments
 (0)