Skip to content

Commit 04c88aa

Browse files
CodaFiCodaFi
committed
Drop the coverage size heuristic
In light of the invocation limits placed on space subtraction, this grossly incorrect check is being dropped. Resolves a source of miscompiles in mostly machine-generated code including SR-6652 and SR-6316.
1 parent 518f26a commit 04c88aa

File tree

3 files changed

+159
-166
lines changed

3 files changed

+159
-166
lines changed

‎lib/Sema/TypeCheckSwitchStmt.cpp

Lines changed: 3 additions & 138 deletions
Original file line numberDiff line numberDiff line change
@@ -115,11 +115,6 @@ namespace {
115115
Identifier Head;
116116
std::forward_list<Space> Spaces;
117117

118-
// NB: This constant is arbitrary. Anecdotally, the Space Engine is
119-
// capable of efficiently handling Spaces of around size 200, but it would
120-
// potentially push an enormous fixit on the user.
121-
static const size_t MAX_SPACE_SIZE = 128;
122-
123118
size_t computeSize(TypeChecker &TC, const DeclContext *DC,
124119
SmallPtrSetImpl<TypeBase *> &cache) const {
125120
switch (getKind()) {
@@ -250,10 +245,6 @@ namespace {
250245
return computeSize(TC, DC, cache);
251246
}
252247

253-
static size_t getMaximumSize() {
254-
return MAX_SPACE_SIZE;
255-
}
256-
257248
bool isEmpty() const { return getKind() == SpaceKind::Empty; }
258249

259250
bool isAllowedButNotRequired() const {
@@ -895,103 +886,6 @@ namespace {
895886
}
896887
}
897888

898-
/// Estimate how big is the search space that exhaustivity
899-
/// checker needs to cover, based on the total space and information
900-
/// from the `switch` statement itself. Some of the easy situations
901-
/// like `case .foo(let bar)` don't really contribute to the complexity
902-
/// of the search so their sub-space sizes could be excluded from
903-
/// consideration.
904-
///
905-
/// \param total The total space to check.
906-
/// \param covered The space covered by the `case` statements in the switch.
907-
///
908-
/// \returns The size of the search space exhastivity checker has to check.
909-
size_t estimateSearchSpaceSize(const Space &total, const Space &covered) {
910-
switch (PairSwitch(total.getKind(), covered.getKind())) {
911-
PAIRCASE(SpaceKind::Type, SpaceKind::Type): {
912-
return total.getType()->isEqual(covered.getType())
913-
? 0
914-
: total.getSize(TC, DC);
915-
}
916-
PAIRCASE(SpaceKind::Type, SpaceKind::Disjunct):
917-
PAIRCASE(SpaceKind::Type, SpaceKind::Constructor): {
918-
if (!Space::canDecompose(total.getType(), DC))
919-
break;
920-
921-
auto decomposition = Space::decompose(TC, DC, total.getType());
922-
return estimateSearchSpaceSize(decomposition, covered);
923-
}
924-
925-
PAIRCASE(SpaceKind::Disjunct, SpaceKind::Disjunct):
926-
PAIRCASE(SpaceKind::Disjunct, SpaceKind::Constructor): {
927-
auto &spaces = total.getSpaces();
928-
return std::accumulate(spaces.begin(), spaces.end(), 0,
929-
[&](size_t totalSize, const Space &space) {
930-
return totalSize + estimateSearchSpaceSize(
931-
space, covered);
932-
});
933-
}
934-
935-
// Search space size computation is not commutative, because it
936-
// tries to check if space on right-hand side is covering any
937-
// portion of the "total" space on the left.
938-
PAIRCASE(SpaceKind::Constructor, SpaceKind::Disjunct): {
939-
for (const auto &space : covered.getSpaces()) {
940-
// enum E { case foo }
941-
// func bar(_ lhs: E, _ rhs: E) {
942-
// switch (lhs, rhs) {
943-
// case (_, _): break
944-
// }
945-
if (total == space)
946-
return 0;
947-
948-
if (!space.isSubspace(total, TC, DC))
949-
continue;
950-
951-
if (estimateSearchSpaceSize(total, space) == 0)
952-
return 0;
953-
}
954-
break;
955-
}
956-
957-
PAIRCASE(SpaceKind::Constructor, SpaceKind::Constructor): {
958-
if (total.getHead() != covered.getHead())
959-
break;
960-
961-
auto &lhs = total.getSpaces();
962-
auto &rhs = covered.getSpaces();
963-
964-
if (std::distance(lhs.begin(), lhs.end()) !=
965-
std::distance(rhs.begin(), rhs.end()))
966-
return total.getSize(TC, DC);
967-
968-
auto i = lhs.begin();
969-
auto j = rhs.begin();
970-
971-
size_t totalSize = 0;
972-
for (; i != lhs.end() && j != rhs.end(); ++i, ++j) {
973-
// The only light-weight checking we can do
974-
// is when sub-spaces on both sides are types
975-
// otherwise we'd have to decompose, which
976-
// is too heavy, so let's just return total
977-
// space size if such situation is detected.
978-
if (i->getKind() != SpaceKind::Type ||
979-
j->getKind() != SpaceKind::Type)
980-
return total.getSize(TC, DC);
981-
982-
totalSize += estimateSearchSpaceSize(*i, *j);
983-
}
984-
985-
return totalSize;
986-
}
987-
988-
default:
989-
break;
990-
}
991-
992-
return total.getSize(TC, DC);
993-
}
994-
995889
void checkExhaustiveness(bool limitedChecking) {
996890
// If the type of the scrutinee is uninhabited, we're already dead.
997891
// Allow any well-typed patterns through.
@@ -1008,7 +902,6 @@ namespace {
1008902
return;
1009903
}
1010904

1011-
bool sawRedundantPattern = false;
1012905
const CaseStmt *unknownCase = nullptr;
1013906
SmallVector<Space, 4> spaces;
1014907
for (auto *caseBlock : Switch->getCases()) {
@@ -1032,8 +925,6 @@ namespace {
1032925

1033926
if (!projection.isEmpty() &&
1034927
projection.isSubspace(Space::forDisjunct(spaces), TC, DC)) {
1035-
sawRedundantPattern |= true;
1036-
1037928
TC.diagnose(caseItem.getStartLoc(),
1038929
diag::redundant_particular_case)
1039930
.highlight(caseItem.getSourceRange());
@@ -1058,18 +949,11 @@ namespace {
1058949
Space totalSpace = Space::forType(subjectType, Identifier());
1059950
Space coveredSpace = Space::forDisjunct(spaces);
1060951

1061-
const size_t searchSpaceSizeEstimate =
1062-
estimateSearchSpaceSize(totalSpace, coveredSpace);
1063-
if (searchSpaceSizeEstimate > Space::getMaximumSize()) {
1064-
diagnoseCannotCheck(sawRedundantPattern, totalSpace, coveredSpace,
1065-
unknownCase);
1066-
return;
1067-
}
1068952
unsigned minusCount = 0;
1069953
auto diff = totalSpace.minus(coveredSpace, TC, DC, &minusCount);
1070954
if (!diff) {
1071-
diagnoseCannotCheck(sawRedundantPattern, totalSpace, coveredSpace,
1072-
unknownCase);
955+
diagnoseMissingCases(RequiresDefault::SpaceTooLarge, Space(),
956+
unknownCase);
1073957
return;
1074958
}
1075959

@@ -1115,26 +999,7 @@ namespace {
1115999
UncoveredSwitch,
11161000
SpaceTooLarge,
11171001
};
1118-
1119-
void diagnoseCannotCheck(const bool sawRedundantPattern,
1120-
const Space &totalSpace,
1121-
const Space &coveredSpace,
1122-
const CaseStmt *unknownCase) {
1123-
// Because the space is large or the check is too slow,
1124-
// we have to extend the size
1125-
// heuristic to compensate for actually exhaustively pattern matching
1126-
// over enormous spaces. In this case, if the covered space covers
1127-
// as much as the total space, and there were no duplicates, then we
1128-
// can assume the user did the right thing and that they don't need
1129-
// a 'default' to be inserted.
1130-
// FIXME: Do something sensible for non-frozen enums.
1131-
if (!sawRedundantPattern &&
1132-
coveredSpace.getSize(TC, DC) >= totalSpace.getSize(TC, DC))
1133-
return;
1134-
diagnoseMissingCases(RequiresDefault::SpaceTooLarge, Space(),
1135-
unknownCase);
1136-
}
1137-
1002+
11381003
void diagnoseMissingCases(RequiresDefault defaultReason, Space uncovered,
11391004
const CaseStmt *unknownCase = nullptr) {
11401005
SourceLoc startLoc = Switch->getStartLoc();

‎test/Compatibility/exhaustive_switch.swift

Lines changed: 39 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,10 @@ enum OverlyLargeSpaceEnum {
432432
case case9
433433
case case10
434434
case case11
435+
case case12
436+
case case13
437+
case case14
438+
case case15
435439
}
436440

437441
enum ContainsOverlyLargeEnum {
@@ -455,10 +459,13 @@ func quiteBigEnough() -> Bool {
455459
case (.case9, .case9): return true
456460
case (.case10, .case10): return true
457461
case (.case11, .case11): return true
462+
case (.case12, .case12): return true
463+
case (.case13, .case13): return true
464+
case (.case14, .case14): return true
465+
case (.case15, .case15): return true
458466
}
459467

460-
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { // expected-error {{the compiler is unable to check that this switch is exhaustive in reasonable time}}
461-
// expected-note@-1 {{do you want to add a default clause?}}
468+
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) {
462469
case (.case0, _): return true
463470
case (.case1, _): return true
464471
case (.case2, _): return true
@@ -470,20 +477,30 @@ func quiteBigEnough() -> Bool {
470477
case (.case8, _): return true
471478
case (.case9, _): return true
472479
case (.case10, _): return true
480+
case (.case11, _): return true
481+
case (.case12, _): return true
482+
case (.case13, _): return true
483+
case (.case14, _): return true
484+
case (.case15, _): return true
473485
}
474486

475487
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { // expected-error {{the compiler is unable to check that this switch is exhaustive in reasonable time}}
476-
case (.case0, _): return true
477-
case (.case1, _): return true
478-
case (.case2, _): return true
479-
case (.case3, _): return true
480-
case (.case4, _): return true
481-
case (.case5, _): return true
482-
case (.case6, _): return true
483-
case (.case7, _): return true
484-
case (.case8, _): return true
485-
case (.case9, _): return true
486-
case (.case10, _): return true
488+
case (.case0, .case0): return true
489+
case (.case1, .case1): return true
490+
case (.case2, .case2): return true
491+
case (.case3, .case3): return true
492+
case (.case4, .case4): return true
493+
case (.case5, .case5): return true
494+
case (.case6, .case6): return true
495+
case (.case7, .case7): return true
496+
case (.case8, .case8): return true
497+
case (.case9, .case9): return true
498+
case (.case10, .case10): return true
499+
case (.case11, .case11): return true
500+
case (.case12, .case12): return true
501+
case (.case13, .case13): return true
502+
case (.case14, .case14): return true
503+
case (.case15, .case15): return true
487504
@unknown default: return false // expected-note {{remove '@unknown' to handle remaining values}} {{3-12=}}
488505
}
489506

@@ -502,6 +519,10 @@ func quiteBigEnough() -> Bool {
502519
case (.case9, _): return true
503520
case (.case10, _): return true
504521
case (.case11, _): return true
522+
case (.case12, _): return true
523+
case (.case13, _): return true
524+
case (.case14, _): return true
525+
case (.case15, _): return true
505526
}
506527

507528
// No diagnostic
@@ -518,6 +539,10 @@ func quiteBigEnough() -> Bool {
518539
case (_, .case9): return true
519540
case (_, .case10): return true
520541
case (_, .case11): return true
542+
case (_, .case12): return true
543+
case (_, .case13): return true
544+
case (_, .case14): return true
545+
case (_, .case15): return true
521546
}
522547

523548
// No diagnostic
@@ -542,7 +567,7 @@ func quiteBigEnough() -> Bool {
542567
}
543568

544569
// Make sure we haven't just stopped emitting diagnostics.
545-
switch OverlyLargeSpaceEnum.case1 { // expected-error {{switch must be exhaustive}} expected-note 12 {{add missing case}}
570+
switch OverlyLargeSpaceEnum.case1 { // expected-error {{switch must be exhaustive}} expected-note 16 {{add missing case}}
546571
}
547572
}
548573

0 commit comments

Comments
 (0)