Skip to content

Commit 3acc0d6

Browse files
tshortlitshortli
committed
SILGen/Sema: Avoid diagnosing @unknown default switch cases as unreachable.
Suppose you have an exhaustive switch statement which matches all the cases of a Swift enum defined in a different module named `External`: ``` import External var e: External.SomeEnum = //... switch e { case .a: break } ``` If `External` is compiled with library evolution and `SomeEnum` is not frozen, then the compiler will warn: ``` warning: switch covers known cases, but 'SomeEnum' may have additional unknown values ``` You add an `@unknown default` to the switch to resolve this warning. Now suppose in another build configuration, `External` is built _without_ library evolution. The compiler will complain about the unreachability of the default case: ``` warning: Default will never be executed ``` These contradictory compiler diagnostics encourage the developer to change the code in a way that will cause a diagnostic in the other configuration. Developers should have the tools to address all warning diagnostics in a reasonable fashion and this is a case where the compiler makes that especially difficult. Given that writing `@unknown default` instead of `default` is a very intentional action that would be the result of addressing the library evolution configuration, it seems reasonable to suppress the `Default will never be executed` diagnostic.
1 parent ba6cd77 commit 3acc0d6

6 files changed

+174
-20
lines changed

‎lib/SILGen/SILGenPattern.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -1136,7 +1136,7 @@ void PatternMatchEmission::emitDispatch(ClauseMatrix &clauses, ArgArray args,
11361136
return item.getPattern()->getKind() == PatternKind::Expr;
11371137
});
11381138
isParentDoCatch = CS->getParentKind() == CaseParentKind::DoCatch;
1139-
isDefault = CS->isDefault();
1139+
isDefault = CS->isDefault() && !CS->hasUnknownAttr();
11401140
}
11411141
} else {
11421142
Loc = clauses[firstRow].getCasePattern()->getStartLoc();

‎lib/Sema/TypeCheckSwitchStmt.cpp

+7-5
Original file line numberDiff line numberDiff line change
@@ -1058,16 +1058,18 @@ namespace {
10581058
unknownCase);
10591059
return;
10601060
}
1061-
1061+
10621062
auto uncovered = diff.value();
1063-
if (unknownCase && uncovered.isEmpty()) {
1064-
DE.diagnose(unknownCase->getLoc(), diag::redundant_particular_case)
1065-
.highlight(unknownCase->getSourceRange());
1066-
}
10671063

10681064
// Account for unknown cases. If the developer wrote `unknown`, they're
10691065
// all handled; otherwise, we ignore the ones that were added for enums
10701066
// that are implicitly frozen.
1067+
//
1068+
// Note that we do not diagnose an unknown case as redundant, even if the
1069+
// uncovered space is empty because we trust that if the developer went to
1070+
// the trouble of writing @unknown that it was for a good reason, like
1071+
// addressing diagnostics in another build configuration where there are
1072+
// potentially unknown cases.
10711073
uncovered = *uncovered.minus(Space::forUnknown(unknownCase == nullptr),
10721074
DC, /*&minusCount*/ nullptr);
10731075

‎test/Compatibility/exhaustive_switch.swift

+6-6
Original file line numberDiff line numberDiff line change
@@ -872,7 +872,7 @@ public func testNonExhaustive(_ value: NonExhaustive, _ payload: NonExhaustivePa
872872

873873
switch value {
874874
case _: break
875-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
875+
@unknown case _: break
876876
}
877877

878878
// Test being part of other spaces.
@@ -973,25 +973,25 @@ public func testNonExhaustive(_ value: NonExhaustive, _ payload: NonExhaustivePa
973973
switch interval {
974974
case .seconds, .milliseconds, .microseconds, .nanoseconds: break
975975
case .never: break
976-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
976+
@unknown case _: break
977977
}
978978

979979
switch flag {
980980
case true: break
981981
case false: break
982-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
982+
@unknown case _: break
983983
}
984984

985985
switch flag as Optional {
986986
case _?: break
987987
case nil: break
988-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
988+
@unknown case _: break
989989
}
990990

991991
switch (flag, value) {
992992
case (true, _): break
993993
case (false, _): break
994-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
994+
@unknown case _: break
995995
}
996996
}
997997

@@ -1028,7 +1028,7 @@ public func testNonExhaustiveWithinModule(_ value: NonExhaustive, _ payload: Non
10281028

10291029
switch value {
10301030
case _: break
1031-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
1031+
@unknown case _: break
10321032
}
10331033

10341034
// Test being part of other spaces.

‎test/SILOptimizer/diagnose_unreachable_enum_resilience.swift

+152
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
// RUN: %target-swift-frontend -emit-sil -swift-version 5 -enable-library-evolution -primary-file %s -o /dev/null -verify
2+
3+
// Re-run with optimizations to check if -O does not make any difference
4+
// RUN: %target-swift-frontend -O -emit-sil -swift-version 5 -enable-library-evolution -primary-file %s -o /dev/null -verify
5+
6+
public enum NonExhaustive {
7+
case a
8+
}
9+
10+
@frozen public enum Exhaustive {
11+
case a
12+
}
13+
14+
public var optional: Void? = nil
15+
16+
public func testNonExhaustive(_ e: NonExhaustive) {
17+
switch e {
18+
case .a: break
19+
}
20+
21+
switch e {
22+
case .a: break
23+
default: break
24+
// expected-warning@-1 {{default will never be executed}}
25+
}
26+
27+
switch e {
28+
case .a: break
29+
@unknown default: break
30+
// Ok, @unknown suppresses "default will never be executed"
31+
}
32+
33+
switch (e, optional) {
34+
case (.a, _): break
35+
}
36+
37+
switch (e, optional) {
38+
case (.a, _): break
39+
default: break
40+
// expected-warning@-1 {{default will never be executed}}
41+
}
42+
43+
switch (e, optional) {
44+
case (.a, _): break
45+
@unknown default: break
46+
// Ok, @unknown suppresses "default will never be executed"
47+
}
48+
}
49+
50+
@inlinable
51+
public func testNonExhaustiveInlinable(_ e: NonExhaustive) {
52+
switch e {
53+
// expected-warning@-1 {{switch covers known cases, but 'NonExhaustive' may have additional unknown values}}
54+
// expected-note@-2 {{handle unknown values using "@unknown default"}}
55+
case .a: break
56+
}
57+
58+
switch e {
59+
case .a: break
60+
default: break
61+
}
62+
63+
switch e {
64+
case .a: break
65+
@unknown default: break
66+
}
67+
68+
switch (e, optional) {
69+
// expected-warning@-1 {{switch covers known cases, but '(NonExhaustive, Void?)' may have additional unknown values}}
70+
// expected-note@-2 {{add missing case: '(_, _)'}}
71+
case (.a, _): break
72+
}
73+
74+
switch (e, optional) {
75+
case (.a, _): break
76+
default: break
77+
}
78+
79+
switch (e, optional) {
80+
case (.a, _): break
81+
@unknown default: break
82+
}
83+
}
84+
85+
public func testExhaustive(_ e: Exhaustive) {
86+
switch e {
87+
case .a: break
88+
}
89+
90+
switch e {
91+
case .a: break
92+
default: break
93+
// expected-warning@-1 {{default will never be executed}}
94+
}
95+
96+
switch e {
97+
case .a: break
98+
@unknown default: break
99+
// Ok, @unknown suppresses "default will never be executed"
100+
}
101+
102+
switch (e, optional) {
103+
case (.a, _): break
104+
}
105+
106+
switch (e, optional) {
107+
case (.a, _): break
108+
default: break
109+
// expected-warning@-1 {{default will never be executed}}
110+
}
111+
112+
switch (e, optional) {
113+
case (.a, _): break
114+
@unknown default: break
115+
// Ok, @unknown suppresses "default will never be executed"
116+
}
117+
}
118+
119+
@inlinable
120+
public func testExhaustiveInlinable(_ e: Exhaustive) {
121+
switch e {
122+
case .a: break
123+
}
124+
125+
switch e {
126+
case .a: break
127+
default: break
128+
// expected-warning@-1 {{default will never be executed}}
129+
}
130+
131+
switch e {
132+
case .a: break
133+
@unknown default: break
134+
// Ok, @unknown suppresses "default will never be executed"
135+
}
136+
137+
switch (e, optional) {
138+
case (.a, _): break
139+
}
140+
141+
switch (e, optional) {
142+
case (.a, _): break
143+
default: break
144+
// expected-warning@-1 {{default will never be executed}}
145+
}
146+
147+
switch (e, optional) {
148+
case (.a, _): break
149+
@unknown default: break
150+
// Ok, @unknown suppresses "default will never be executed"
151+
}
152+
}

‎test/Sema/exhaustive_switch.swift

+6-6
Original file line numberDiff line numberDiff line change
@@ -838,7 +838,7 @@ public func testNonExhaustive(_ value: NonExhaustive, _ payload: NonExhaustivePa
838838

839839
switch value {
840840
case _: break
841-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
841+
@unknown case _: break
842842
}
843843

844844
// Test being part of other spaces.
@@ -939,25 +939,25 @@ public func testNonExhaustive(_ value: NonExhaustive, _ payload: NonExhaustivePa
939939
switch interval {
940940
case .seconds, .milliseconds, .microseconds, .nanoseconds: break
941941
case .never: break
942-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
942+
@unknown case _: break
943943
}
944944

945945
switch flag {
946946
case true: break
947947
case false: break
948-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
948+
@unknown case _: break
949949
}
950950

951951
switch flag as Optional {
952952
case _?: break
953953
case nil: break
954-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
954+
@unknown case _: break
955955
}
956956

957957
switch (flag, value) {
958958
case (true, _): break
959959
case (false, _): break
960-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
960+
@unknown case _: break
961961
}
962962
}
963963

@@ -994,7 +994,7 @@ public func testNonExhaustiveWithinModule(_ value: NonExhaustive, _ payload: Non
994994

995995
switch value {
996996
case _: break
997-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
997+
@unknown case _: break
998998
}
999999

10001000
// Test being part of other spaces.

‎test/Sema/exhaustive_switch_debugger.swift

+2-2
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ public func testNonExhaustive(_ value: NonExhaustive, _ payload: NonExhaustivePa
4545

4646
switch value {
4747
case _: break
48-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
48+
@unknown case _: break
4949
}
5050

5151
// Test being part of other spaces.
@@ -144,7 +144,7 @@ public func testNonExhaustiveWithinModule(_ value: NonExhaustive, _ payload: Non
144144

145145
switch value {
146146
case _: break
147-
@unknown case _: break // expected-warning {{case is already handled by previous patterns; consider removing it}}
147+
@unknown case _: break
148148
}
149149

150150
// Test being part of other spaces.

0 commit comments

Comments
 (0)