From 03527d9eec65324434011d23219e2bf7525642c6 Mon Sep 17 00:00:00 2001 From: Robert Widmann Date: Thu, 25 May 2017 09:13:08 -0700 Subject: [PATCH 1/2] Implement @_downgrade_exhaustivity_check Dispatch requests the ability to add a new case, but to treat missing instances of that case in patterns as warnings instead of errors. It is still an error to make reference to the annotated case in at least one pattern then not cover the rest of the space, but it is not an error to omit the space of patterns referencing the case entirely. This attribute is private and uglified to intentionally discourage its use outside just this one use case. --- include/swift/AST/Attr.def | 5 + include/swift/AST/DiagnosticsSema.def | 4 + lib/AST/ASTDumper.cpp | 2 + lib/AST/Attr.cpp | 4 + lib/Sema/TypeCheckAttr.cpp | 2 + lib/Sema/TypeCheckDecl.cpp | 1 + lib/Sema/TypeCheckProtocol.cpp | 2 +- lib/Sema/TypeCheckSwitchStmt.cpp | 194 ++++++++++++------ .../downgrade_exhaustivity_swift3.swift | 48 +++++ test/attr/attr_downgrade_exhaustivity.swift | 150 ++++++++++++++ .../lib/SwiftLang/SwiftLangSupport.cpp | 1 + 11 files changed, 352 insertions(+), 61 deletions(-) create mode 100644 test/SILGen/downgrade_exhaustivity_swift3.swift create mode 100644 test/attr/attr_downgrade_exhaustivity.swift diff --git a/include/swift/AST/Attr.def b/include/swift/AST/Attr.def index d96613b5eb754..22b0dbb2a9706 100644 --- a/include/swift/AST/Attr.def +++ b/include/swift/AST/Attr.def @@ -286,6 +286,11 @@ SIMPLE_DECL_ATTR(NSKeyedArchiverEncodeNonGenericSubclassesOnly, OnClass | NotSerialized | LongAttribute, /*Not serialized */ 70) +// HACK: Attribute needed to preserve source compatibility by downgrading errors +// due to an SDK change in Dispatch +SIMPLE_DECL_ATTR(_downgrade_exhaustivity_check, DowngradeExhaustivityCheck, + OnEnumElement | LongAttribute | UserInaccessible, 71) + #undef TYPE_ATTR #undef DECL_ATTR_ALIAS #undef SIMPLE_DECL_ATTR diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index dd0c6a158406c..37a0add1610ca 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -3701,6 +3701,10 @@ NOTE(missing_particular_case,none, WARNING(redundant_particular_case,none, "case is already handled by previous patterns; consider removing it",()) +// HACK: Downgrades the above to warnings if any of the cases is marked +// @_downgrade_exhaustivity_check. +WARNING(non_exhaustive_switch_warn_swift3,none, "switch must be exhaustive", ()) + #ifndef DIAG_NO_UNDEF # if defined(DIAG) # undef DIAG diff --git a/lib/AST/ASTDumper.cpp b/lib/AST/ASTDumper.cpp index 66a9dde06adfc..71e7d997396a1 100644 --- a/lib/AST/ASTDumper.cpp +++ b/lib/AST/ASTDumper.cpp @@ -840,6 +840,8 @@ namespace { void visitEnumElementDecl(EnumElementDecl *EED) { printCommon(EED, "enum_element_decl"); + if (EED->getAttrs().hasAttribute()) + OS << "@_downgrade_exhaustivity_check"; PrintWithColorRAII(OS, ParenthesisColor) << ')'; } diff --git a/lib/AST/Attr.cpp b/lib/AST/Attr.cpp index e7824692c02d9..bb065bba7fa12 100644 --- a/lib/AST/Attr.cpp +++ b/lib/AST/Attr.cpp @@ -506,6 +506,10 @@ bool DeclAttribute::printImpl(ASTPrinter &Printer, const PrintOptions &Options, Printer.printAttrName("@_staticInitializeObjCMetadata"); break; + case DAK_DowngradeExhaustivityCheck: + Printer.printAttrName("@_downgrade_exhaustivity_check"); + break; + case DAK_Count: llvm_unreachable("exceed declaration attribute kinds"); diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp index 6c657b335969e..a43c95e68d34a 100644 --- a/lib/Sema/TypeCheckAttr.cpp +++ b/lib/Sema/TypeCheckAttr.cpp @@ -107,6 +107,7 @@ class AttributeEarlyChecker : public AttributeVisitor { IGNORED_ATTR(NSKeyedArchiverClassName) IGNORED_ATTR(StaticInitializeObjCMetadata) IGNORED_ATTR(NSKeyedArchiverEncodeNonGenericSubclassesOnly) + IGNORED_ATTR(DowngradeExhaustivityCheck) #undef IGNORED_ATTR // @noreturn has been replaced with a 'Never' return type. @@ -784,6 +785,7 @@ class AttributeChecker : public AttributeVisitor { IGNORED_ATTR(ObjCMembers) IGNORED_ATTR(StaticInitializeObjCMetadata) IGNORED_ATTR(NSKeyedArchiverEncodeNonGenericSubclassesOnly) + IGNORED_ATTR(DowngradeExhaustivityCheck) #undef IGNORED_ATTR void visitAvailableAttr(AvailableAttr *attr); diff --git a/lib/Sema/TypeCheckDecl.cpp b/lib/Sema/TypeCheckDecl.cpp index 57892c3c4e58d..40f2da3ee7b54 100644 --- a/lib/Sema/TypeCheckDecl.cpp +++ b/lib/Sema/TypeCheckDecl.cpp @@ -6103,6 +6103,7 @@ class DeclChecker : public DeclVisitor { UNINTERESTING_ATTR(NSKeyedArchiverClassName) UNINTERESTING_ATTR(StaticInitializeObjCMetadata) UNINTERESTING_ATTR(NSKeyedArchiverEncodeNonGenericSubclassesOnly) + UNINTERESTING_ATTR(DowngradeExhaustivityCheck) #undef UNINTERESTING_ATTR void visitAvailableAttr(AvailableAttr *attr) { diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp index 46cc777daa297..f840484058861 100644 --- a/lib/Sema/TypeCheckProtocol.cpp +++ b/lib/Sema/TypeCheckProtocol.cpp @@ -5915,7 +5915,7 @@ static bool hasGenericAncestry(ClassDecl *classDecl) { /// Infer the attribute tostatic-initialize the Objective-C metadata for the /// given class, if needed. static void inferStaticInitializeObjCMetadata(ClassDecl *classDecl, - bool requiresNSCodingAttr) { + bool requiresNSCodingAttr) { // If we already have the attribute, there's nothing to do. if (classDecl->getAttrs().hasAttribute()) return; diff --git a/lib/Sema/TypeCheckSwitchStmt.cpp b/lib/Sema/TypeCheckSwitchStmt.cpp index 78b67b242452f..e47f04c6b60d0 100644 --- a/lib/Sema/TypeCheckSwitchStmt.cpp +++ b/lib/Sema/TypeCheckSwitchStmt.cpp @@ -83,8 +83,8 @@ namespace { explicit Space(Type T) : Kind(SpaceKind::Type), TypeAndVal(T, false), Head(Identifier()), Spaces({}){} - explicit Space(Type T, Identifier H, SmallVectorImpl &SP) - : Kind(SpaceKind::Constructor), TypeAndVal(T, false), Head(H), + explicit Space(Type T, Identifier H, bool downgrade, SmallVectorImpl &SP) + : Kind(SpaceKind::Constructor), TypeAndVal(T, downgrade), Head(H), Spaces(SP.begin(), SP.end()) {} explicit Space(SmallVectorImpl &SP) : Kind(SpaceKind::Disjunct), TypeAndVal(Type(), false), @@ -101,6 +101,12 @@ namespace { void dump() const LLVM_ATTRIBUTE_USED; bool isEmpty() const { return getKind() == SpaceKind::Empty; } + + bool canDowngrade() const { + assert(getKind() == SpaceKind::Constructor + && "Wrong kind of space tried to access downgrade"); + return TypeAndVal.getInt(); + } Type getType() const { assert((getKind() == SpaceKind::Type @@ -585,7 +591,8 @@ namespace { SmallVector copyParams(this->getSpaces().begin(), this->getSpaces().end()); copyParams[idx] = s1.minus(s2, TC); - Space CS(this->getType(), this->Head, copyParams); + Space CS(this->getType(), this->getHead(), this->canDowngrade(), + copyParams); constrSpaces.push_back(CS); } @@ -721,7 +728,7 @@ namespace { return Space(); } } - return Space(getType(), Head, simplifiedSpaces); + return Space(getType(), Head, canDowngrade(), simplifiedSpaces); } case SpaceKind::Type: { // If the decomposition of a space is empty, the space is empty. @@ -813,7 +820,10 @@ namespace { constElemSpaces.push_back(Space(TTy->getUnderlyingType())); } } - return Space(tp, eed->getName(), constElemSpaces); + return Space(tp, eed->getName(), + eed->getAttrs() + .getAttribute(), + constElemSpaces); }); } else if (auto *TTy = tp->castTo()) { // Decompose each of the elements into its component type space. @@ -824,7 +834,8 @@ namespace { return Space(ty.getType()); }); // Create an empty constructor head for the tuple space. - arr.push_back(Space(tp, Identifier(), constElemSpaces)); + arr.push_back(Space(tp, Identifier(), /*canDowngrade*/false, + constElemSpaces)); } else { llvm_unreachable("Can't decompose type?"); } @@ -851,6 +862,7 @@ namespace { return; } + bool sawDowngradablePattern = false; SmallVector spaces; for (unsigned i = 0, e = Switch->getCases().size(); i < e; ++i) { auto *caseBlock = Switch->getCases()[i]; @@ -864,7 +876,8 @@ namespace { if (caseItem.isDefault()) return; - auto projection = projectPattern(TC, caseItem.getPattern()); + auto projection = projectPattern(TC, caseItem.getPattern(), + sawDowngradablePattern); if (projection.isUseful() && projection.isSubspace(Space(spaces), TC)) { TC.diagnose(caseItem.getStartLoc(), @@ -874,7 +887,7 @@ namespace { spaces.push_back(projection); } } - + Space totalSpace(Switch->getSubjectExpr()->getType()); Space coveredSpace(spaces); auto uncovered = totalSpace.minus(coveredSpace, TC).simplify(TC); @@ -904,43 +917,81 @@ namespace { uncovered = Space(spaces); } - diagnoseMissingCases(TC, Switch, /*justNeedsDefault*/ false, uncovered); + diagnoseMissingCases(TC, Switch, /*justNeedsDefault*/ false, uncovered, + sawDowngradablePattern); + } + + // HACK: Search the space for any remaining cases that were labelled + // @_downgrade_exhaustivity_check. + static bool shouldDowngradeToWarning(const Space &masterSpace) { + switch (masterSpace.getKind()) { + case SpaceKind::Type: + case SpaceKind::BooleanConstant: + case SpaceKind::Empty: + return false; + // Traverse the constructor and its subspaces. + case SpaceKind::Constructor: + return masterSpace.canDowngrade() + || std::accumulate(masterSpace.getSpaces().begin(), + masterSpace.getSpaces().end(), + false, + [](bool acc, const Space &space) { + return acc || shouldDowngradeToWarning(space); + }); + case SpaceKind::Disjunct: + // Traverse the disjunct's subspaces. + return std::accumulate(masterSpace.getSpaces().begin(), + masterSpace.getSpaces().end(), + false, + [](bool acc, const Space &space) { + return acc || shouldDowngradeToWarning(space); + }); + } } - static void diagnoseMissingCases(TypeChecker &TC, const SwitchStmt *Switch, - bool JustNeedsDefault, - SpaceEngine::Space uncovered) { - bool Empty = Switch->getCases().empty(); - SourceLoc StartLoc = Switch->getStartLoc(); - SourceLoc EndLoc = Switch->getEndLoc(); - StringRef Placeholder = getCodePlaceholder(); - llvm::SmallString<128> Buffer; - llvm::raw_svector_ostream OS(Buffer); + static void diagnoseMissingCases(TypeChecker &TC, const SwitchStmt *SS, + bool justNeedsDefault, + Space uncovered, + bool sawDowngradablePattern = false) { + SourceLoc startLoc = SS->getStartLoc(); + SourceLoc endLoc = SS->getEndLoc(); + StringRef placeholder = getCodePlaceholder(); + llvm::SmallString<128> buffer; + llvm::raw_svector_ostream OS(buffer); bool InEditor = TC.Context.LangOpts.DiagnosticsEditorMode; - if (JustNeedsDefault) { - OS << tok::kw_default << ":\n" << Placeholder << "\n"; - if (Empty) { - TC.Context.Diags.diagnose(StartLoc, diag::empty_switch_stmt) - .fixItInsert(EndLoc, Buffer.str()); + if (justNeedsDefault) { + OS << tok::kw_default << ":\n" << placeholder << "\n"; + if (SS->getCases().empty()) { + TC.Context.Diags.diagnose(startLoc, diag::empty_switch_stmt) + .fixItInsert(endLoc, buffer.str()); } else { - TC.Context.Diags.diagnose(StartLoc, diag::non_exhaustive_switch); - TC.Context.Diags.diagnose(StartLoc, diag::missing_several_cases, - uncovered.isEmpty()).fixItInsert(EndLoc, - Buffer.str()); + TC.Context.Diags.diagnose(startLoc, diag::non_exhaustive_switch); + TC.Context.Diags.diagnose(startLoc, diag::missing_several_cases, + uncovered.isEmpty()).fixItInsert(endLoc, + buffer.str()); } return; } // If there's nothing else to diagnose, bail. if (uncovered.isEmpty()) return; - + + auto mainDiagType = diag::non_exhaustive_switch; + if (TC.Context.isSwiftVersion3()) { + if (!sawDowngradablePattern && shouldDowngradeToWarning(uncovered)) { + mainDiagType = diag::non_exhaustive_switch_warn_swift3; + } + } + // If editing is enabled, emit a formatted error of the form: // // switch must be exhaustive, do you want to add missing cases? - // case (.none, .some(_)): <#code#> - // case (.some(_), .none): <#code#> + // case (.none, .some(_)): + // <#code#> + // case (.some(_), .none): + // <#code#> // // else: // @@ -949,7 +1000,7 @@ namespace { // missing case '(.none, .some(_))' // missing case '(.some(_), .none)' if (InEditor) { - Buffer.clear(); + buffer.clear(); SmallVector emittedSpaces; for (auto &uncoveredSpace : uncovered.getSpaces()) { SmallVector flats; @@ -961,17 +1012,18 @@ namespace { OS << tok::kw_case << " "; flat.show(OS); - OS << ":\n" << Placeholder << "\n"; + OS << ":\n" << placeholder << "\n"; emittedSpaces.push_back(flat); } } - TC.diagnose(StartLoc, diag::non_exhaustive_switch); - TC.diagnose(StartLoc, diag::missing_several_cases, false) - .fixItInsert(EndLoc, Buffer.str()); + TC.diagnose(startLoc, diag::non_exhaustive_switch); + TC.diagnose(startLoc, diag::missing_several_cases, false) + .fixItInsert(endLoc, buffer.str()); + } else { - TC.Context.Diags.diagnose(StartLoc, diag::non_exhaustive_switch); + TC.Context.Diags.diagnose(startLoc, mainDiagType); SmallVector emittedSpaces; for (auto &uncoveredSpace : uncovered.getSpaces()) { @@ -982,9 +1034,9 @@ namespace { continue; } - Buffer.clear(); + buffer.clear(); flat.show(OS); - TC.diagnose(StartLoc, diag::missing_particular_case, Buffer.str()); + TC.diagnose(startLoc, diag::missing_particular_case, buffer.str()); emittedSpaces.push_back(flat); } @@ -1006,7 +1058,7 @@ namespace { flats.push_back(space); return; } - + // To recursively recover a pattern matrix from a bunch of disjuncts: // 1) Unpack the arguments to the constructor under scrutiny. // 2) Traverse each argument in turn. @@ -1022,17 +1074,13 @@ namespace { SmallVector columnVect; flatten(subspace, columnVect); - // Pattern matrices grow quasi-factorially in the size of the - // input space. - multiplier *= columnVect.size(); - size_t startSize = matrix.size(); if (!matrix.empty() && columnVect.size() > 1) { size_t oldCount = matrix.size(); - matrix.reserve(multiplier * oldCount); + matrix.reserve(oldCount * columnVect.size()); // Indexing starts at 1, we already have 'startSize'-many elements // in the matrix; multiplies by 1 are no-ops. - for (size_t i = 1; i < multiplier; ++i) { + for (size_t i = 1; i < columnVect.size(); ++i) { std::copy_n(matrix.begin(), oldCount, std::back_inserter(matrix)); } } @@ -1067,11 +1115,16 @@ namespace { matrix[rowIdx].push_back(columnVect[colIdx]); } } + + // Pattern matrices grow quasi-factorially in the size of the + // input space. + multiplier *= columnVect.size(); } // Wrap the matrix rows into this constructor. for (auto &row : matrix) { - flats.push_back(Space(space.getType(), space.getHead(), row)); + flats.push_back(Space(space.getType(), space.getHead(), + space.canDowngrade(), row)); } } break; @@ -1090,7 +1143,8 @@ namespace { } // Recursively project a pattern into a Space. - static Space projectPattern(TypeChecker &TC, const Pattern *item) { + static Space projectPattern(TypeChecker &TC, const Pattern *item, + bool &sawDowngradablePattern) { switch (item->getKind()) { case PatternKind::Any: case PatternKind::Named: @@ -1122,28 +1176,39 @@ namespace { return Space(); case PatternKind::Var: { auto *VP = cast(item); - return projectPattern(TC, VP->getSubPattern()); + return projectPattern(TC, VP->getSubPattern(), sawDowngradablePattern); } case PatternKind::Paren: { auto *PP = cast(item); - return projectPattern(TC, PP->getSubPattern()); + return projectPattern(TC, PP->getSubPattern(), sawDowngradablePattern); } case PatternKind::OptionalSome: { auto *OSP = cast(item); SmallVector payload = { - projectPattern(TC, OSP->getSubPattern()) + projectPattern(TC, OSP->getSubPattern(), sawDowngradablePattern) }; - return Space(item->getType(), TC.Context.getIdentifier("some"), payload); + return Space(item->getType(), TC.Context.getIdentifier("some"), /*canDowngrade*/false, + payload); } case PatternKind::EnumElement: { auto *VP = cast(item); TC.validateDecl(item->getType()->getEnumOrBoundGenericEnum()); + + bool canDowngrade = false; + if (auto *eed = VP->getElementDecl()) { + if (eed->getAttrs().getAttribute()) { + canDowngrade |= true; + sawDowngradablePattern |= true; + } + } + SmallVector conArgSpace; auto *SP = VP->getSubPattern(); if (!SP) { // If there's no sub-pattern then there's no further recursive // structure here. Yield the constructor space. - return Space(item->getType(), VP->getName(), conArgSpace); + return Space(item->getType(), VP->getName(), canDowngrade, + conArgSpace); } switch (SP->getKind()) { @@ -1152,9 +1217,11 @@ namespace { std::transform(TP->getElements().begin(), TP->getElements().end(), std::back_inserter(conArgSpace), [&](TuplePatternElt pate) { - return projectPattern(TC, pate.getPattern()); + return projectPattern(TC, pate.getPattern(), + sawDowngradablePattern); }); - return Space(item->getType(), VP->getName(), conArgSpace); + return Space(item->getType(), VP->getName(), /*canDowngrade*/false, + conArgSpace); } case PatternKind::Paren: { auto *PP = dyn_cast(SP); @@ -1171,15 +1238,21 @@ namespace { conArgSpace.push_back(Space(ty.getType())); } } else { - conArgSpace.push_back(projectPattern(TC, SP)); + conArgSpace.push_back(projectPattern(TC, SP, + sawDowngradablePattern)); } } else { - conArgSpace.push_back(projectPattern(TC, SP)); + conArgSpace.push_back(projectPattern(TC, SP, + sawDowngradablePattern)); } - return Space(item->getType(), VP->getName(), conArgSpace); + // FIXME: This isn't *technically* correct, but we only use the + // downgradability of the master space, not the projected space, + // when reconstructing the missing pattern matrix. + return Space(item->getType(), VP->getName(), /*canDowngrade*/false, + conArgSpace); } default: - return projectPattern(TC, SP); + return projectPattern(TC, SP, sawDowngradablePattern); } } case PatternKind::Tuple: { @@ -1188,9 +1261,10 @@ namespace { std::transform(TP->getElements().begin(), TP->getElements().end(), std::back_inserter(conArgSpace), [&](TuplePatternElt pate) { - return projectPattern(TC, pate.getPattern()); + return projectPattern(TC, pate.getPattern(), sawDowngradablePattern); }); - return Space(item->getType(), Identifier(), conArgSpace); + return Space(item->getType(), Identifier(), /*canDowngrade*/false, + conArgSpace); } } } diff --git a/test/SILGen/downgrade_exhaustivity_swift3.swift b/test/SILGen/downgrade_exhaustivity_swift3.swift new file mode 100644 index 0000000000000..f0be6c1ab617a --- /dev/null +++ b/test/SILGen/downgrade_exhaustivity_swift3.swift @@ -0,0 +1,48 @@ +// RUN: %target-swift-frontend -swift-version 3 -emit-silgen %s | %FileCheck %s + +enum Downgradable { + case spoon + case hat + @_downgrade_exhaustivity_check + case fork +} + +// CHECK-LABEL: sil hidden @_T029downgrade_exhaustivity_swift343testDowngradableOmittedPatternIsUnreachableyAA0E0OSg3pat_tF +func testDowngradableOmittedPatternIsUnreachable(pat : Downgradable?) { + // CHECK: switch_enum {{%.*}} : $Downgradable, case #Downgradable.spoon!enumelt: [[CASE1:bb[0-9]+]], case #Downgradable.hat!enumelt: [[CASE2:bb[0-9]+]], default [[DEFAULT_CASE:bb[0-9]+]] + switch pat! { + // CHECK: [[CASE1]]: + case .spoon: + break + // CHECK: [[CASE2]]: + case .hat: + break + // CHECK: [[DEFAULT_CASE]]: + // CHECK-NEXT: unreachable + } + + // CHECK: switch_enum {{%[0-9]+}} : $Downgradable, case #Downgradable.spoon!enumelt: {{bb[0-9]+}}, case #Downgradable.hat!enumelt: {{bb[0-9]+}}, default [[TUPLE_DEFAULT_CASE_1:bb[0-9]+]] + // CHECK: switch_enum [[Y:%[0-9]+]] : $Downgradable, case #Downgradable.spoon!enumelt: {{bb[0-9]+}}, case #Downgradable.hat!enumelt: {{bb[0-9]+}}, default [[TUPLE_DEFAULT_CASE_2:bb[0-9]+]] + switch (pat!, pat!) { + case (.spoon, .spoon): + break + case (.spoon, .hat): + break + case (.hat, .spoon): + break + case (.hat, .hat): + break + // CHECK: [[TUPLE_DEFAULT_CASE_2]]: + // CHECK-NEXT: unreachable + + // CHECK: switch_enum [[Y]] : $Downgradable, case #Downgradable.spoon!enumelt: {{bb[0-9]+}}, case #Downgradable.hat!enumelt: {{bb[0-9]+}}, default [[TUPLE_DEFAULT_CASE_3:bb[0-9]+]] + + // CHECK: [[TUPLE_DEFAULT_CASE_3]]: + // CHECK-NEXT: unreachable + + // CHECK: [[TUPLE_DEFAULT_CASE_1]]: + // CHECK-NEXT: unreachable + } + +} + diff --git a/test/attr/attr_downgrade_exhaustivity.swift b/test/attr/attr_downgrade_exhaustivity.swift new file mode 100644 index 0000000000000..c3572099bd96a --- /dev/null +++ b/test/attr/attr_downgrade_exhaustivity.swift @@ -0,0 +1,150 @@ +// RUN: %target-typecheck-verify-swift -swift-version 3 %s + +enum Runcible { + case spoon + case hat + @_downgrade_exhaustivity_check + case fork +} + +enum Trioptional { + case some(Runcible) + case just(Runcible) + case none +} + +enum Fungible { + case cash + case giftCard +} + +func missingCases(x: Runcible?, y: Runcible?, z: Trioptional) { + // Should warn in S3 mode that `fork` isn't used + switch x! { // expected-warning {{switch must be exhaustive}} + // expected-note@-1 {{add missing case: '.fork'}} + case .spoon: + break + case .hat: + break + } + + // Should warn in S3 mode that `fork` isn't used + switch (x!, y!) { // expected-warning {{switch must be exhaustive}} + // expected-note@-1 3 {{add missing case:}} + case (.spoon, .spoon): + break + case (.spoon, .hat): + break + case (.hat, .spoon): + break + case (.hat, .hat): + break + } + + // Should error, since `fork` is used but not totally covered + switch (x!, y!) { // expected-error {{switch must be exhaustive}} + // expected-note@-1 {{add missing case: '(.fork, .hat)'}} + // expected-note@-2 {{add missing case: '(.fork, .fork)'}} + // expected-note@-3 {{add missing case: '(.hat, .fork)'}} + // expected-note@-4 {{add missing case: '(_, .fork)'}} + case (.spoon, .spoon): + break + case (.spoon, .hat): + break + case (.hat, .spoon): + break + case (.hat, .hat): + break + case (.fork, .spoon): + break + } + + // Should error, since `fork` is used but not totally covered + switch (x!, y!) { // expected-error {{switch must be exhaustive}} + // expected-note@-1 {{add missing case: '(.fork, _)'}} + // expected-note@-2 {{add missing case: '(.spoon, .fork)'}} + case (.spoon, .spoon): + break + case (.spoon, .hat): + break + case (.hat, .spoon): + break + case (.hat, .hat): + break + case (.hat, .fork): + break + } + + // Should warn in S3 mode that `fork` isn't used + switch x { // expected-warning {{switch must be exhaustive}} + // expected-note@-1 {{add missing case: '.some(.fork)'}} + case .some(.spoon): + break + case .some(.hat): + break + case .none: + break + } + + // Should warn in S3 mode that `fork` isn't used + switch (x, y!) { // expected-warning {{switch must be exhaustive}} + // expected-note@-1 {{add missing case: '(.some(.fork), _)'}} + // expected-note@-2 {{add missing case: '(.none, .fork)'}} + // expected-note@-3 {{add missing case: '(.some(.hat), .fork)'}} + // expected-note@-4 {{add missing case: '(_, .fork)'}} + case (.some(.spoon), .spoon): + break + case (.some(.spoon), .hat): + break + case (.some(.hat), .spoon): + break + case (.some(.hat), .hat): + break + case (.none, .spoon): + break + case (.none, .hat): + break + } + + // Should warn in S3 mode that `fork` isn't used + switch (x, y) { // expected-warning {{switch must be exhaustive}} + // expected-note@-1 {{add missing case: '(.some(.fork), _)'}} + // expected-note@-2 {{add missing case: '(_, .some(.fork))'}} + // expected-note@-3 {{add missing case: '(.some(.hat), .some(.fork))'}} + // expected-note@-4 {{add missing case: '(.none, _)'}} + case (.some(.spoon), .some(.spoon)): + break + case (.some(.spoon), .some(.hat)): + break + case (.some(.spoon), .none): + break + case (.some(.hat), .some(.spoon)): + break + case (.some(.hat), .some(.hat)): + break + case (.some(.hat), .none): + break + case (.some(.hat), .some(.spoon)): // expected-warning {{case is already handled by previous patterns; consider removing it}} + break + case (.some(.hat), .some(.hat)): // expected-warning {{case is already handled by previous patterns; consider removing it}} + break + case (.some(.hat), .none): // expected-warning {{case is already handled by previous patterns; consider removing it}} + break + } + + // Should warn in S3 mode that `fork` isn't used + switch z { // expected-warning {{switch must be exhaustive}} + // expected-note@-1 {{add missing case: '.some(.fork)'}} + // expected-note@-2 {{add missing case: '.just(.fork)'}} + case .some(.spoon): + break + case .some(.hat): + break + case .just(.spoon): + break + case .just(.hat): + break + case .none: + break + } +} diff --git a/tools/SourceKit/lib/SwiftLang/SwiftLangSupport.cpp b/tools/SourceKit/lib/SwiftLang/SwiftLangSupport.cpp index f7b1d8a13d612..d2712daba4ed8 100644 --- a/tools/SourceKit/lib/SwiftLang/SwiftLangSupport.cpp +++ b/tools/SourceKit/lib/SwiftLang/SwiftLangSupport.cpp @@ -729,6 +729,7 @@ std::vector SwiftLangSupport::UIDsFromDeclAttributes(const DeclAttribute // Ignore these. case DAK_ShowInInterface: case DAK_RawDocComment: + case DAK_DowngradeExhaustivityCheck: continue; default: break; From b5a69c7205c67951292d57d44bb00b05c0d376e0 Mon Sep 17 00:00:00 2001 From: Robert Widmann Date: Wed, 31 May 2017 01:09:55 -0700 Subject: [PATCH 2/2] Add a size heuristic to the Space Engine 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. --- lib/Sema/TypeCheckSwitchStmt.cpp | 85 +++++++++++++++- test/Sema/exhaustive_switch.swift | 157 +++++++++++++++++++++++++++++- 2 files changed, 240 insertions(+), 2 deletions(-) diff --git a/lib/Sema/TypeCheckSwitchStmt.cpp b/lib/Sema/TypeCheckSwitchStmt.cpp index e47f04c6b60d0..bfb590ea38988 100644 --- a/lib/Sema/TypeCheckSwitchStmt.cpp +++ b/lib/Sema/TypeCheckSwitchStmt.cpp @@ -71,7 +71,7 @@ namespace { }; #define PAIRCASE(XS, YS) case PairSwitch(XS, YS) - + class Space final { private: SpaceKind Kind; @@ -79,6 +79,60 @@ namespace { Identifier Head; std::forward_list Spaces; + // NB: This constant is arbitrary. Anecdotally, the Space Engine is + // capable of efficiently handling Spaces of around size 200, but it would + // potentially push an enormous fixit on the user. + static const size_t MAX_SPACE_SIZE = 128; + + size_t computeSize(TypeChecker &TC, + SmallPtrSetImpl &cache) const { + switch (getKind()) { + case SpaceKind::Empty: + return 0; + case SpaceKind::BooleanConstant: + return 1; + case SpaceKind::Type: { + if (!canDecompose(getType())) { + return 1; + } + cache.insert(getType().getPointer()); + + SmallVector spaces; + decompose(TC, getType(), spaces); + size_t acc = 0; + for (auto &sp : spaces) { + // Decomposed pattern spaces grow with the sum of the subspaces. + acc += sp.computeSize(TC, cache); + } + + cache.erase(getType().getPointer()); + return acc; + } + case SpaceKind::Constructor: { + size_t acc = 1; + for (auto &sp : getSpaces()) { + // Break self-recursive references among enum arguments. + if (sp.getKind() == SpaceKind::Type + && cache.count(sp.getType().getPointer())) { + continue; + } + + // Constructor spaces grow with the product of their arguments. + acc *= sp.computeSize(TC, cache); + } + return acc; + } + case SpaceKind::Disjunct: { + size_t acc = 0; + for (auto &sp : getSpaces()) { + // Disjoint grow with the sum of the subspaces. + acc += sp.computeSize(TC, cache); + } + return acc; + } + } + } + public: explicit Space(Type T) : Kind(SpaceKind::Type), TypeAndVal(T, false), Head(Identifier()), @@ -100,6 +154,15 @@ namespace { void dump() const LLVM_ATTRIBUTE_USED; + size_t getSize(TypeChecker &TC) const { + SmallPtrSet cache; + return computeSize(TC, cache); + } + + static size_t getMaximumSize() { + return MAX_SPACE_SIZE; + } + bool isEmpty() const { return getKind() == SpaceKind::Empty; } bool canDowngrade() const { @@ -863,6 +926,7 @@ namespace { } bool sawDowngradablePattern = false; + bool sawRedundantPattern = false; SmallVector spaces; for (unsigned i = 0, e = Switch->getCases().size(); i < e; ++i) { auto *caseBlock = Switch->getCases()[i]; @@ -880,6 +944,8 @@ namespace { sawDowngradablePattern); if (projection.isUseful() && projection.isSubspace(Space(spaces), TC)) { + sawRedundantPattern |= true; + TC.diagnose(caseItem.getStartLoc(), diag::redundant_particular_case) .highlight(caseItem.getSourceRange()); @@ -890,6 +956,23 @@ namespace { Space totalSpace(Switch->getSubjectExpr()->getType()); Space coveredSpace(spaces); + size_t totalSpaceSize = totalSpace.getSize(TC); + if (totalSpaceSize > Space::getMaximumSize()) { + // Because the space is large, we have to extend the size + // heuristic to compensate for actually exhaustively pattern matching + // over enormous spaces. In this case, if the covered space covers + // as much as the total space, and there were no duplicates, then we + // can assume the user did the right thing and that they don't need + // a 'default' to be inserted. + if (!sawRedundantPattern + && coveredSpace.getSize(TC) >= totalSpaceSize) { + return; + } + + diagnoseMissingCases(TC, Switch, /*justNeedsDefault*/true, Space()); + return; + } + auto uncovered = totalSpace.minus(coveredSpace, TC).simplify(TC); if (uncovered.isEmpty()) { return; diff --git a/test/Sema/exhaustive_switch.swift b/test/Sema/exhaustive_switch.swift index 0ea73f2dcf5ae..24c0200ba67f2 100644 --- a/test/Sema/exhaustive_switch.swift +++ b/test/Sema/exhaustive_switch.swift @@ -329,7 +329,7 @@ func checkDiagnosticMinimality(x: Runcible?) { case (.hat, .spoon): break } - + switch (x!, x!) { // expected-error {{switch must be exhaustive}} // expected-note@-1 {{add missing case: '(.fork, _)'}} // expected-note@-2 {{add missing case: '(.hat, .spoon)'}} @@ -343,3 +343,158 @@ func checkDiagnosticMinimality(x: Runcible?) { break } } + +enum LargeSpaceEnum { + case case0 + case case1 + case case2 + case case3 + case case4 + case case5 + case case6 + case case7 + case case8 + case case9 + case case10 +} + +func notQuiteBigEnough() -> Bool { + switch (LargeSpaceEnum.case1, LargeSpaceEnum.case2) { // expected-error {{switch must be exhaustive}} + // expected-note@-1 110 {{add missing case:}} + case (.case0, .case0): return true + case (.case1, .case1): return true + case (.case2, .case2): return true + case (.case3, .case3): return true + case (.case4, .case4): return true + case (.case5, .case5): return true + case (.case6, .case6): return true + case (.case7, .case7): return true + case (.case8, .case8): return true + case (.case9, .case9): return true + case (.case10, .case10): return true + } +} + +enum OverlyLargeSpaceEnum { + case case0 + case case1 + case case2 + case case3 + case case4 + case case5 + case case6 + case case7 + case case8 + case case9 + case case10 + case case11 +} + +func quiteBigEnough() -> Bool { + switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { // expected-error {{switch must be exhaustive}} + // expected-note@-1 {{do you want to add a default clause?}} + case (.case0, .case0): return true + case (.case1, .case1): return true + case (.case2, .case2): return true + case (.case3, .case3): return true + case (.case4, .case4): return true + case (.case5, .case5): return true + case (.case6, .case6): return true + case (.case7, .case7): return true + case (.case8, .case8): return true + case (.case9, .case9): return true + case (.case10, .case10): return true + case (.case11, .case11): return true + } + + // No diagnostic + switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { // expected-error {{switch must be exhaustive}} + // expected-note@-1 {{do you want to add a default clause?}} + case (.case0, _): return true + case (.case1, _): return true + case (.case2, _): return true + case (.case3, _): return true + case (.case4, _): return true + case (.case5, _): return true + case (.case6, _): return true + case (.case7, _): return true + case (.case8, _): return true + case (.case9, _): return true + case (.case10, _): return true + } + + + // No diagnostic + switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { + case (.case0, _): return true + case (.case1, _): return true + case (.case2, _): return true + case (.case3, _): return true + case (.case4, _): return true + case (.case5, _): return true + case (.case6, _): return true + case (.case7, _): return true + case (.case8, _): return true + case (.case9, _): return true + case (.case10, _): return true + case (.case11, _): return true + } + + // No diagnostic + switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { + case (_, .case0): return true + case (_, .case1): return true + case (_, .case2): return true + case (_, .case3): return true + case (_, .case4): return true + case (_, .case5): return true + case (_, .case6): return true + case (_, .case7): return true + case (_, .case8): return true + case (_, .case9): return true + case (_, .case10): return true + case (_, .case11): return true + } + + // No diagnostic + switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { + case (_, _): return true + } + + // No diagnostic + switch (OverlyLargeSpaceEnum.case1, OverlyLargeSpaceEnum.case2) { + case (.case0, .case0): return true + case (.case1, .case1): return true + case (.case2, .case2): return true + case (.case3, .case3): return true + case _: return true + } +} + +indirect enum InfinitelySized { + case one + case two + case recur(InfinitelySized) + case mutualRecur(MutuallyRecursive, InfinitelySized) +} + +indirect enum MutuallyRecursive { + case one + case two + case recur(MutuallyRecursive) + case mutualRecur(InfinitelySized, MutuallyRecursive) +} + +func infinitelySized() -> Bool { + switch (InfinitelySized.one, InfinitelySized.one) { // expected-error {{switch must be exhaustive}} + // expected-note@-1 10 {{add missing case:}} + case (.one, .one): return true + case (.two, .two): return true + } + + switch (MutuallyRecursive.one, MutuallyRecursive.one) { // expected-error {{switch must be exhaustive}} + // expected-note@-1 10 {{add missing case:}} + case (.one, .one): return true + case (.two, .two): return true + } +}