Skip to content

Commit bffa22a

Browse files
author
Robert Widmann
committed
Add a size heuristic to the Space Engine
<rdar://32480026> This is a particularly tricky tradeoff to have to make here. On the one hand, we want diagnostics about incomplete patterns to provide as much information as possible. On the other, pattern matrices grow quasi-factorially in the size of the argument. That is, an enum with 10 cases that is switched on as a 2-tuple/argument list creates a total subspace covering of size 100. For sufficiently large inputs, this can DOS the Swift compiler. It is simply not useful to present more than about 100 notes to the user, nor is it useful to waste an enormous amount of time trying to compute these subspaces for the limited information the diagnostic will provide. Instead, short circuit if the size of the enum is above some arbitrary threshold (currently 128) and just offer to add a 'default'. Notably, this change is not *technically* source compatible in that it ignores the new '@_downgrade_exhaustivity_check'. However to hit up against this heuristic would require the user to be switching over four DispatchTimeIntervals in a quadruple or using an equivalently-sized enormous enum case. At which point we're hitting on the very reason this heuristic exists. There are definitely other, more informative, paths that we can take here. GHC, for example, seems to run a highly limited form of space subtraction when the input space is too large, and simply reports the top 3 missing cases along with some ellipses. For now, I just want to cut off this hang in the compiler.
1 parent 85c8340 commit bffa22a

File tree

2 files changed

+240
-2
lines changed

2 files changed

+240
-2
lines changed

‎lib/Sema/TypeCheckSwitchStmt.cpp

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,14 +71,68 @@ namespace {
7171
};
7272

7373
#define PAIRCASE(XS, YS) case PairSwitch(XS, YS)
74-
74+
7575
class Space final {
7676
private:
7777
SpaceKind Kind;
7878
llvm::PointerIntPair<Type, 1, bool> TypeAndVal;
7979
Identifier Head;
8080
std::forward_list<Space> Spaces;
8181

82+
// NB: This constant is arbitrary. Anecdotally, the Space Engine is
83+
// capable of efficiently handling Spaces of around size 200, but it would
84+
// potentially push an enormous fixit on the user.
85+
static const size_t MAX_SPACE_SIZE = 128;
86+
87+
size_t computeSize(TypeChecker &TC,
88+
SmallPtrSetImpl<TypeBase *> &cache) const {
89+
switch (getKind()) {
90+
case SpaceKind::Empty:
91+
return 0;
92+
case SpaceKind::BooleanConstant:
93+
return 1;
94+
case SpaceKind::Type: {
95+
if (!canDecompose(getType())) {
96+
return 1;
97+
}
98+
cache.insert(getType().getPointer());
99+
100+
SmallVector<Space, 4> spaces;
101+
decompose(TC, getType(), spaces);
102+
size_t acc = 0;
103+
for (auto &sp : spaces) {
104+
// Decomposed pattern spaces grow with the sum of the subspaces.
105+
acc += sp.computeSize(TC, cache);
106+
}
107+
108+
cache.erase(getType().getPointer());
109+
return acc;
110+
}
111+
case SpaceKind::Constructor: {
112+
size_t acc = 1;
113+
for (auto &sp : getSpaces()) {
114+
// Break self-recursive references among enum arguments.
115+
if (sp.getKind() == SpaceKind::Type
116+
&& cache.count(sp.getType().getPointer())) {
117+
continue;
118+
}
119+
120+
// Constructor spaces grow with the product of their arguments.
121+
acc *= sp.computeSize(TC, cache);
122+
}
123+
return acc;
124+
}
125+
case SpaceKind::Disjunct: {
126+
size_t acc = 0;
127+
for (auto &sp : getSpaces()) {
128+
// Disjoint grow with the sum of the subspaces.
129+
acc += sp.computeSize(TC, cache);
130+
}
131+
return acc;
132+
}
133+
}
134+
}
135+
82136
public:
83137
explicit Space(Type T)
84138
: Kind(SpaceKind::Type), TypeAndVal(T, false), Head(Identifier()),
@@ -100,6 +154,15 @@ namespace {
100154

101155
void dump() const LLVM_ATTRIBUTE_USED;
102156

157+
size_t getSize(TypeChecker &TC) const {
158+
SmallPtrSet<TypeBase *, 4> cache;
159+
return computeSize(TC, cache);
160+
}
161+
162+
static size_t getMaximumSize() {
163+
return MAX_SPACE_SIZE;
164+
}
165+
103166
bool isEmpty() const { return getKind() == SpaceKind::Empty; }
104167

105168
bool canDowngrade() const {
@@ -863,6 +926,7 @@ namespace {
863926
}
864927

865928
bool sawDowngradablePattern = false;
929+
bool sawRedundantPattern = false;
866930
SmallVector<Space, 4> spaces;
867931
for (unsigned i = 0, e = Switch->getCases().size(); i < e; ++i) {
868932
auto *caseBlock = Switch->getCases()[i];
@@ -880,6 +944,8 @@ namespace {
880944
sawDowngradablePattern);
881945
if (projection.isUseful()
882946
&& projection.isSubspace(Space(spaces), TC)) {
947+
sawRedundantPattern |= true;
948+
883949
TC.diagnose(caseItem.getStartLoc(),
884950
diag::redundant_particular_case)
885951
.highlight(caseItem.getSourceRange());
@@ -890,6 +956,23 @@ namespace {
890956

891957
Space totalSpace(Switch->getSubjectExpr()->getType());
892958
Space coveredSpace(spaces);
959+
size_t totalSpaceSize = totalSpace.getSize(TC);
960+
if (totalSpaceSize > Space::getMaximumSize()) {
961+
// Because the space is large, we have to extend the size
962+
// heuristic to compensate for actually exhaustively pattern matching
963+
// over enormous spaces. In this case, if the covered space covers
964+
// as much as the total space, and there were no duplicates, then we
965+
// can assume the user did the right thing and that they don't need
966+
// a 'default' to be inserted.
967+
if (!sawRedundantPattern
968+
&& coveredSpace.getSize(TC) >= totalSpaceSize) {
969+
return;
970+
}
971+
972+
diagnoseMissingCases(TC, Switch, /*justNeedsDefault*/true, Space());
973+
return;
974+
}
975+
893976
auto uncovered = totalSpace.minus(coveredSpace, TC).simplify(TC);
894977
if (uncovered.isEmpty()) {
895978
return;

‎test/Sema/exhaustive_switch.swift

Lines changed: 156 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -329,7 +329,7 @@ func checkDiagnosticMinimality(x: Runcible?) {
329329
case (.hat, .spoon):
330330
break
331331
}
332-
332+
333333
switch (x!, x!) { // expected-error {{switch must be exhaustive}}
334334
// expected-note@-1 {{add missing case: '(.fork, _)'}}
335335
// expected-note@-2 {{add missing case: '(.hat, .spoon)'}}
@@ -343,3 +343,158 @@ func checkDiagnosticMinimality(x: Runcible?) {
343343
break
344344
}
345345
}
346+
347+
enum LargeSpaceEnum {
348+
case case0
349+
case case1
350+
case case2
351+
case case3
352+
case case4
353+
case case5
354+
case case6
355+
case case7
356+
case case8
357+
case case9
358+
case case10
359+
}
360+
361+
func notQuiteBigEnough() -> Bool {
362+
switch (LargeSpaceEnum.case1, LargeSpaceEnum.case2) { // expected-error {{switch must be exhaustive}}
363+
// expected-note@-1 110 {{add missing case:}}
364+
case (.case0, .case0): return true
365+
case (.case1, .case1): return true
366+
case (.case2, .case2): return true
367+
case (.case3, .case3): return true
368+
case (.case4, .case4): return true
369+
case (.case5, .case5): return true
370+
case (.case6, .case6): return true
371+
case (.case7, .case7): return true
372+
case (.case8, .case8): return true
373+
case (.case9, .case9): return true
374+
case (.case10, .case10): return true
375+
}
376+
}
377+
378+
enum OverlyLargeSpaceEnum {
379+
case case0
380+
case case1
381+
case case2
382+
case case3
383+
case case4
384+
case case5
385+
case case6
386+
case case7
387+
case case8
388+
case case9
389+
case case10
390+
case case11
391+
}
392+
393+
func quiteBigEnough() -> Bool {
394+
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { // expected-error {{switch must be exhaustive}}
395+
// expected-note@-1 {{do you want to add a default clause?}}
396+
case (.case0, .case0): return true
397+
case (.case1, .case1): return true
398+
case (.case2, .case2): return true
399+
case (.case3, .case3): return true
400+
case (.case4, .case4): return true
401+
case (.case5, .case5): return true
402+
case (.case6, .case6): return true
403+
case (.case7, .case7): return true
404+
case (.case8, .case8): return true
405+
case (.case9, .case9): return true
406+
case (.case10, .case10): return true
407+
case (.case11, .case11): return true
408+
}
409+
410+
// No diagnostic
411+
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { // expected-error {{switch must be exhaustive}}
412+
// expected-note@-1 {{do you want to add a default clause?}}
413+
case (.case0, _): return true
414+
case (.case1, _): return true
415+
case (.case2, _): return true
416+
case (.case3, _): return true
417+
case (.case4, _): return true
418+
case (.case5, _): return true
419+
case (.case6, _): return true
420+
case (.case7, _): return true
421+
case (.case8, _): return true
422+
case (.case9, _): return true
423+
case (.case10, _): return true
424+
}
425+
426+
427+
// No diagnostic
428+
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) {
429+
case (.case0, _): return true
430+
case (.case1, _): return true
431+
case (.case2, _): return true
432+
case (.case3, _): return true
433+
case (.case4, _): return true
434+
case (.case5, _): return true
435+
case (.case6, _): return true
436+
case (.case7, _): return true
437+
case (.case8, _): return true
438+
case (.case9, _): return true
439+
case (.case10, _): return true
440+
case (.case11, _): return true
441+
}
442+
443+
// No diagnostic
444+
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) {
445+
case (_, .case0): return true
446+
case (_, .case1): return true
447+
case (_, .case2): return true
448+
case (_, .case3): return true
449+
case (_, .case4): return true
450+
case (_, .case5): return true
451+
case (_, .case6): return true
452+
case (_, .case7): return true
453+
case (_, .case8): return true
454+
case (_, .case9): return true
455+
case (_, .case10): return true
456+
case (_, .case11): return true
457+
}
458+
459+
// No diagnostic
460+
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) {
461+
case (_, _): return true
462+
}
463+
464+
// No diagnostic
465+
switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) {
466+
case (.case0, .case0): return true
467+
case (.case1, .case1): return true
468+
case (.case2, .case2): return true
469+
case (.case3, .case3): return true
470+
case _: return true
471+
}
472+
}
473+
474+
indirect enum InfinitelySized {
475+
case one
476+
case two
477+
case recur(InfinitelySized)
478+
case mutualRecur(MutuallyRecursive, InfinitelySized)
479+
}
480+
481+
indirect enum MutuallyRecursive {
482+
case one
483+
case two
484+
case recur(MutuallyRecursive)
485+
case mutualRecur(InfinitelySized, MutuallyRecursive)
486+
}
487+
488+
func infinitelySized() -> Bool {
489+
switch (InfinitelySized.one, InfinitelySized.one) { // expected-error {{switch must be exhaustive}}
490+
// expected-note@-1 10 {{add missing case:}}
491+
case (.one, .one): return true
492+
case (.two, .two): return true
493+
}
494+
495+
switch (MutuallyRecursive.one, MutuallyRecursive.one) { // expected-error {{switch must be exhaustive}}
496+
// expected-note@-1 10 {{add missing case:}}
497+
case (.one, .one): return true
498+
case (.two, .two): return true
499+
}
500+
}

0 commit comments

Comments
 (0)