Skip to content

Commit 476a225

Browse files
CodaFiCodaFi
authored
Merge pull request #13438 from CodaFi/re-cover-model
[4.1] Stricter enforcement of the "large space" heuristic
2 parents bea9dc3 + ec32b57 commit 476a225

File tree

7 files changed

+122
-53
lines changed

7 files changed

+122
-53
lines changed

‎include/swift/AST/DiagnosticsSIL.def

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ WARNING(unreachable_code_after_stmt,none,
244244
"code after '%select{return|break|continue|throw}0' will never "
245245
"be executed", (unsigned))
246246
WARNING(unreachable_case,none,
247-
"%select{case|default}0 will never be executed", (bool))
247+
"case will never be executed", ())
248248
WARNING(switch_on_a_constant,none,
249249
"switch condition evaluates to a constant", ())
250250
NOTE(unreachable_code_note,none, "will never be executed", ())

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3860,6 +3860,11 @@ WARNING(redundant_particular_literal_case,none,
38603860
NOTE(redundant_particular_literal_case_here,none,
38613861
"first occurrence of identical literal pattern is here", ())
38623862

3863+
ERROR(cannot_prove_exhaustive_switch,none,
3864+
"analysis of uncovered switch statement is too complex to perform in a "
3865+
"reasonable amount of time; "
3866+
"insert a 'default' clause to cover this switch statement", ())
3867+
38633868
// HACK: Downgrades the above to warnings if any of the cases is marked
38643869
// @_downgrade_exhaustivity_check.
38653870
WARNING(non_exhaustive_switch_warn_swift3,none, "switch must be exhaustive", ())

‎lib/SILGen/SILGenPattern.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1031,7 +1031,8 @@ void PatternMatchEmission::emitDispatch(ClauseMatrix &clauses, ArgArray args,
10311031
} else {
10321032
Loc = clauses[firstRow].getCasePattern()->getStartLoc();
10331033
}
1034-
SGF.SGM.diagnose(Loc, diag::unreachable_case, isDefault);
1034+
if (!isDefault)
1035+
SGF.SGM.diagnose(Loc, diag::unreachable_case);
10351036
}
10361037
}
10371038
}

‎lib/Sema/TypeCheckSwitchStmt.cpp

Lines changed: 80 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,7 @@ namespace {
169169
}
170170
}
171171
}
172+
172173

173174
public:
174175
explicit Space(Type T, Identifier NameForPrinting)
@@ -201,6 +202,29 @@ namespace {
201202
return computeSize(TC, cache);
202203
}
203204

205+
// Walk one level deep into the space to return whether it is
206+
// composed entirely of irrefutable patterns - these are quick to check
207+
// regardless of the size of the total type space.
208+
bool isAllIrrefutable() const {
209+
switch (getKind()) {
210+
case SpaceKind::Empty:
211+
case SpaceKind::Type:
212+
return true;
213+
case SpaceKind::BooleanConstant:
214+
return false;
215+
case SpaceKind::Constructor:
216+
return llvm::all_of(getSpaces(), [](const Space &sp) {
217+
return sp.getKind() == SpaceKind::Type
218+
|| sp.getKind() == SpaceKind::Empty;
219+
});
220+
case SpaceKind::Disjunct: {
221+
return llvm::all_of(getSpaces(), [](const Space &sp) {
222+
return sp.isAllIrrefutable();
223+
});
224+
}
225+
}
226+
}
227+
204228
static size_t getMaximumSize() {
205229
return MAX_SPACE_SIZE;
206230
}
@@ -1011,14 +1035,16 @@ namespace {
10111035
if (subjectType && subjectType->isStructurallyUninhabited()) {
10121036
return;
10131037
}
1014-
1038+
1039+
// Reject switch statements with empty blocks.
1040+
if (limitedChecking && Switch->getCases().empty()) {
1041+
SpaceEngine::diagnoseMissingCases(TC, Switch,
1042+
RequiresDefault::EmptySwitchBody,
1043+
SpaceEngine::Space());
1044+
}
1045+
1046+
// If the switch body fails to typecheck, end analysis here.
10151047
if (limitedChecking) {
1016-
// Reject switch statements with empty blocks.
1017-
if (Switch->getCases().empty()) {
1018-
SpaceEngine::diagnoseMissingCases(TC, Switch,
1019-
/*justNeedsDefault*/true,
1020-
SpaceEngine::Space());
1021-
}
10221048
return;
10231049
}
10241050

@@ -1063,23 +1089,18 @@ namespace {
10631089

10641090
Space totalSpace(subjectType, Identifier());
10651091
Space coveredSpace(spaces);
1066-
size_t totalSpaceSize = totalSpace.getSize(TC);
1067-
if (totalSpaceSize > Space::getMaximumSize()) {
1068-
// Because the space is large, we have to extend the size
1069-
// heuristic to compensate for actually exhaustively pattern matching
1070-
// over enormous spaces. In this case, if the covered space covers
1071-
// as much as the total space, and there were no duplicates, then we
1072-
// can assume the user did the right thing and that they don't need
1073-
// a 'default' to be inserted.
1074-
if (!sawRedundantPattern
1075-
&& coveredSpace.getSize(TC) >= totalSpaceSize) {
1076-
return;
1077-
}
10781092

1079-
diagnoseMissingCases(TC, Switch, /*justNeedsDefault*/true, Space());
1093+
size_t totalSpaceSize = totalSpace.getSize(TC);
1094+
if (totalSpaceSize > Space::getMaximumSize() && !coveredSpace.isAllIrrefutable()) {
1095+
// Because the space is large, fall back to requiring 'default'.
1096+
//
1097+
// FIXME: Explore ways of reducing runtime of this analysis or doing
1098+
// partial analysis to recover this case.
1099+
diagnoseMissingCases(TC, Switch,
1100+
RequiresDefault::SpaceTooLarge, Space());
10801101
return;
10811102
}
1082-
1103+
10831104
auto uncovered = totalSpace.minus(coveredSpace, TC).simplify(TC);
10841105
if (uncovered.isEmpty()) {
10851106
return;
@@ -1092,11 +1113,12 @@ namespace {
10921113
if (Space::canDecompose(uncovered.getType())) {
10931114
SmallVector<Space, 4> spaces;
10941115
Space::decompose(TC, uncovered.getType(), spaces);
1095-
diagnoseMissingCases(TC, Switch,
1096-
/*justNeedsDefault*/ false, Space(spaces));
1116+
diagnoseMissingCases(TC, Switch, RequiresDefault::No, Space(spaces));
10971117
} else {
1098-
diagnoseMissingCases(TC, Switch,
1099-
/*justNeedsDefault*/ true, Space());
1118+
diagnoseMissingCases(TC, Switch, Switch->getCases().empty()
1119+
? RequiresDefault::EmptySwitchBody
1120+
: RequiresDefault::UncoveredSwitch,
1121+
Space());
11001122
}
11011123
return;
11021124
}
@@ -1107,7 +1129,7 @@ namespace {
11071129
uncovered = Space(spaces);
11081130
}
11091131

1110-
diagnoseMissingCases(TC, Switch, /*justNeedsDefault*/ false, uncovered,
1132+
diagnoseMissingCases(TC, Switch, RequiresDefault::No, uncovered,
11111133
sawDowngradablePattern);
11121134
}
11131135

@@ -1139,8 +1161,15 @@ namespace {
11391161
}
11401162
}
11411163

1164+
enum class RequiresDefault {
1165+
No,
1166+
EmptySwitchBody,
1167+
UncoveredSwitch,
1168+
SpaceTooLarge,
1169+
};
1170+
11421171
static void diagnoseMissingCases(TypeChecker &TC, const SwitchStmt *SS,
1143-
bool justNeedsDefault,
1172+
RequiresDefault defaultReason,
11441173
Space uncovered,
11451174
bool sawDowngradablePattern = false) {
11461175
SourceLoc startLoc = SS->getStartLoc();
@@ -1149,20 +1178,30 @@ namespace {
11491178
llvm::SmallString<128> buffer;
11501179
llvm::raw_svector_ostream OS(buffer);
11511180

1152-
bool InEditor = TC.Context.LangOpts.DiagnosticsEditorMode;
1153-
1154-
if (justNeedsDefault) {
1181+
switch (defaultReason) {
1182+
case RequiresDefault::EmptySwitchBody: {
11551183
OS << tok::kw_default << ":\n" << placeholder << "\n";
1156-
if (SS->getCases().empty()) {
1157-
TC.Context.Diags.diagnose(startLoc, diag::empty_switch_stmt)
1158-
.fixItInsert(endLoc, buffer.str());
1159-
} else {
1160-
TC.Context.Diags.diagnose(startLoc, diag::non_exhaustive_switch);
1161-
TC.Context.Diags.diagnose(startLoc, diag::missing_several_cases,
1162-
uncovered.isEmpty()).fixItInsert(endLoc,
1163-
buffer.str());
1164-
}
1184+
TC.diagnose(startLoc, diag::empty_switch_stmt)
1185+
.fixItInsert(endLoc, buffer.str());
1186+
}
1187+
return;
1188+
case RequiresDefault::UncoveredSwitch: {
1189+
OS << tok::kw_default << ":\n" << placeholder << "\n";
1190+
TC.diagnose(startLoc, diag::non_exhaustive_switch);
1191+
TC.diagnose(startLoc, diag::missing_several_cases, uncovered.isEmpty())
1192+
.fixItInsert(endLoc, buffer.str());
1193+
}
11651194
return;
1195+
case RequiresDefault::SpaceTooLarge: {
1196+
OS << tok::kw_default << ":\n" << "<#fatalError()#>" << "\n";
1197+
TC.diagnose(startLoc, diag::cannot_prove_exhaustive_switch);
1198+
TC.diagnose(startLoc, diag::missing_several_cases, uncovered.isEmpty())
1199+
.fixItInsert(endLoc, buffer.str());
1200+
}
1201+
return;
1202+
case RequiresDefault::No:
1203+
// Break out to diagnose below.
1204+
break;
11661205
}
11671206

11681207
// If there's nothing else to diagnose, bail.
@@ -1189,7 +1228,7 @@ namespace {
11891228
//
11901229
// missing case '(.none, .some(_))'
11911230
// missing case '(.some(_), .none)'
1192-
if (InEditor) {
1231+
if (TC.Context.LangOpts.DiagnosticsEditorMode) {
11931232
buffer.clear();
11941233
SmallVector<Space, 8> emittedSpaces;
11951234
for (auto &uncoveredSpace : uncovered.getSpaces()) {
@@ -1212,7 +1251,7 @@ namespace {
12121251
TC.diagnose(startLoc, diag::missing_several_cases, false)
12131252
.fixItInsert(endLoc, buffer.str());
12141253
} else {
1215-
TC.Context.Diags.diagnose(startLoc, mainDiagType);
1254+
TC.diagnose(startLoc, mainDiagType);
12161255

12171256
SmallVector<Space, 8> emittedSpaces;
12181257
for (auto &uncoveredSpace : uncovered.getSpaces()) {

‎test/SILGen/unreachable_code.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ func testUnreachableCase5(a : Tree) {
106106
switch a {
107107
case _:
108108
break
109-
default: // expected-warning {{default will never be executed}}
109+
default:
110110
return
111111
}
112112
}

‎test/SILOptimizer/unreachable_code.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ class r20097963MyClass {
208208
str = "A"
209209
case .B:
210210
str = "B"
211-
default: // expected-warning {{default will never be executed}}
211+
default:
212212
str = "unknown" // Should not be rejected.
213213
}
214214
return str

‎test/Sema/exhaustive_switch.swift

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -444,7 +444,7 @@ enum ContainsOverlyLargeEnum {
444444
}
445445

446446
func quiteBigEnough() -> Bool {
447-
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { // expected-error {{switch must be exhaustive}}
447+
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { // expected-error {{analysis of uncovered switch statement is too complex to perform in a reasonable amount of time}}
448448
// expected-note@-1 {{do you want to add a default clause?}}
449449
case (.case0, .case0): return true
450450
case (.case1, .case1): return true
@@ -460,8 +460,7 @@ func quiteBigEnough() -> Bool {
460460
case (.case11, .case11): return true
461461
}
462462

463-
// No diagnostic
464-
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { // expected-error {{switch must be exhaustive}}
463+
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { // expected-error {{analysis of uncovered switch statement is too complex to perform in a reasonable amount of time}}
465464
// expected-note@-1 {{do you want to add a default clause?}}
466465
case (.case0, _): return true
467466
case (.case1, _): return true
@@ -477,8 +476,9 @@ func quiteBigEnough() -> Bool {
477476
}
478477

479478

480-
// No diagnostic
479+
// expected-error@+1 {{analysis of uncovered switch statement is too complex to perform in a reasonable amount of time}}
481480
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) {
481+
// expected-note@-1 {{do you want to add a default clause?}}
482482
case (.case0, _): return true
483483
case (.case1, _): return true
484484
case (.case2, _): return true
@@ -493,8 +493,9 @@ func quiteBigEnough() -> Bool {
493493
case (.case11, _): return true
494494
}
495495

496-
// No diagnostic
496+
// expected-error@+1 {{analysis of uncovered switch statement is too complex to perform in a reasonable amount of time}}
497497
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) {
498+
// expected-note@-1 {{do you want to add a default clause?}}
498499
case (_, .case0): return true
499500
case (_, .case1): return true
500501
case (_, .case2): return true
@@ -509,21 +510,22 @@ func quiteBigEnough() -> Bool {
509510
case (_, .case11): return true
510511
}
511512

512-
// No diagnostic
513+
// No diagnostic
513514
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) {
514515
case (_, _): return true
515516
}
516517

517-
// No diagnostic
518+
// expected-error@+1 {{analysis of uncovered switch statement is too complex to perform in a reasonable amount of time}}
518519
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) {
520+
// expected-note@-1 {{do you want to add a default clause?}}
519521
case (.case0, .case0): return true
520522
case (.case1, .case1): return true
521523
case (.case2, .case2): return true
522524
case (.case3, .case3): return true
523525
case _: return true
524526
}
525527

526-
// No diagnostic
528+
// No diagnostic
527529
switch ContainsOverlyLargeEnum.one(.case0) {
528530
case .one: return true
529531
case .two: return true
@@ -559,6 +561,28 @@ func infinitelySized() -> Bool {
559561
}
560562
}
561563

564+
// SR-6316: Size heuristic is insufficient to catch space covering when the
565+
// covered space size is greater than or equal to the master space size.
566+
func largeSpaceMatch(_ x: Bool) {
567+
switch (x, (x, x, x, x), (x, x, x, x)) { // expected-error {{analysis of uncovered switch statement is too complex to perform in a reasonable amount of time}}
568+
// expected-note@-1 {{do you want to add a default clause?}}
569+
case (true, (_, _, _, _), (_, true, true, _)):
570+
break
571+
case (true, (_, _, _, _), (_, _, false, _)):
572+
break
573+
case (_, (_, true, true, _), (_, _, false, _)):
574+
break
575+
case (_, (_, _, false, _), (_, true, true, _)):
576+
break
577+
case (_, (_, true, true, _), (_, true, true, _)):
578+
break
579+
case (_, (_, _, false, _), (_, _, false, _)):
580+
break
581+
case (_, (false, false, false, false), (_, _, _, _)):
582+
break
583+
}
584+
}
585+
562586
func diagnoseDuplicateLiterals() {
563587
let str = "def"
564588
let int = 2

0 commit comments

Comments
 (0)