Skip to content

Commit 9acac07

Browse files
CodaFiCodaFi
authored
Merge pull request #9997 from CodaFi/measure-twice-cut-once
Add a size heuristic to the Space Engine
2 parents 77fc122 + bffa22a commit 9acac07

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)