Merge pull request #16383 from davidungar/swift-4.2-branch-cutoff

Merge pull request #16271 from davidungar/rdar-39805050-Switch-takes-too-long-to-compile

Author: David Ungar

include/swift/Frontend/FrontendOptions.h

@@ -90,6 +90,11 @@ class FrontendOptions {
   /// the expression type checker run before we consider an expression
   /// too complex.
   unsigned SolverExpressionTimeThreshold = 0;
+  
+  /// If non-zero, overrides the default threshold for how many times
+  /// the Space::minus function is called before we consider switch statement
+  /// exhaustiveness checking to be too complex.
+  unsigned SwitchCheckingInvocationThreshold = 0;
 
   /// The module for which we should verify all of the generic signatures.
   std::string VerifyGenericSignaturesInModule;

include/swift/Option/FrontendOptions.td

@@ -342,6 +342,9 @@ def warn_long_expression_type_checking_EQ : Joined<["-"], "warn-long-expression-
 
 def solver_expression_time_threshold_EQ : Joined<["-"], "solver-expression-time-threshold=">;
 
+def switch_checking_invocation_threshold_EQ : Joined<["-"],
+    "switch-checking-invocation-threshold=">;
+
 def enable_source_import : Flag<["-"], "enable-source-import">,
   HelpText<"Enable importing of Swift source files">;
 

include/swift/Subsystems.h

@@ -189,7 +189,8 @@ namespace swift {
                            unsigned StartElem = 0,
                            unsigned WarnLongFunctionBodies = 0,
                            unsigned WarnLongExpressionTypeChecking = 0,
-                           unsigned ExpressionTimeoutThreshold = 0);
+                           unsigned ExpressionTimeoutThreshold = 0,
+                           unsigned SwitchCheckingInvocationThreshold = 0);
 
   /// Now that we have type-checked an entire module, perform any type
   /// checking that requires the full module, e.g., Objective-C method

lib/Frontend/ArgsToFrontendOptionsConverter.cpp

@@ -78,6 +78,8 @@ bool ArgsToFrontendOptionsConverter::convert(
                              Opts.WarnLongExpressionTypeChecking);
   setUnsignedIntegerArgument(OPT_solver_expression_time_threshold_EQ, 10,
                              Opts.SolverExpressionTimeThreshold);
+  setUnsignedIntegerArgument(OPT_switch_checking_invocation_threshold_EQ, 10,
+                             Opts.SwitchCheckingInvocationThreshold);
 
   Opts.DebuggerTestingTransform = Args.hasArg(OPT_debugger_testing_transform);
 

lib/Frontend/Frontend.cpp

@@ -628,7 +628,8 @@ void CompilerInstance::parseAndCheckTypes(
                         TypeCheckOptions, /*curElem*/ 0,
                         options.WarnLongFunctionBodies,
                         options.WarnLongExpressionTypeChecking,
-                        options.SolverExpressionTimeThreshold);
+                        options.SolverExpressionTimeThreshold,
+                        options.SwitchCheckingInvocationThreshold);
   });
 
   // Even if there were no source files, we should still record known
@@ -751,7 +752,8 @@ void CompilerInstance::parseAndTypeCheckMainFile(
                           TypeCheckOptions, CurTUElem,
                           options.WarnLongFunctionBodies,
                           options.WarnLongExpressionTypeChecking,
-                          options.SolverExpressionTimeThreshold);
+                          options.SolverExpressionTimeThreshold,
+                          options.SwitchCheckingInvocationThreshold);
     }
     CurTUElem = MainFile.Decls.size();
   } while (!Done);

lib/Sema/TypeCheckSwitchStmt.cpp

@@ -686,9 +686,19 @@ namespace {
       // Returns the result of subtracting the other space from this space.  The
       // result is empty if the other space completely covers this space, or
       // non-empty if there were any uncovered cases.  The difference of spaces
-      // is the smallest uncovered set of cases.
-      Space minus(const Space &other, TypeChecker &TC,
-                  const DeclContext *DC) const {
+      // is the smallest uncovered set of cases. The result is absent if the
+      // computation had to be abandoned.
+      //
+      // \p minusCount is an optional pointer counting the number of
+      // times minus has run.
+      // Returns None if the computation "timed out".
+      Optional<Space> minus(const Space &other, TypeChecker &TC,
+                            const DeclContext *DC, unsigned *minusCount) const {
+
+        if (minusCount && TC.getSwitchCheckingInvocationThreshold() &&
+            (*minusCount)++ >= TC.getSwitchCheckingInvocationThreshold())
+          return None;
+        
         if (this->isEmpty()) {
           return Space();
         }
@@ -719,7 +729,7 @@ namespace {
             SmallVector<Space, 4> spaces;
             this->decompose(TC, DC, this->getType(), spaces);
             auto decomp = Space::forDisjunct(spaces);
-            return decomp.minus(other, TC, DC);
+            return decomp.minus(other, TC, DC, minusCount);
           } else {
             return *this;
           }
@@ -738,12 +748,14 @@ namespace {
         PAIRCASE (SpaceKind::Disjunct, SpaceKind::Disjunct):
         PAIRCASE (SpaceKind::BooleanConstant, SpaceKind::Disjunct):
         PAIRCASE (SpaceKind::UnknownCase, SpaceKind::Disjunct): {
-          return std::accumulate(other.getSpaces().begin(),
-                                 other.getSpaces().end(),
-                                 *this,
-                                 [&](const Space &left, const Space &right){
-            return left.minus(right, TC, DC);
-          });
+          Space tot = *this;
+          for (auto s : other.getSpaces()) {
+            if (auto diff = tot.minus(s, TC, DC, minusCount))
+              tot = *diff;
+            else
+              return None;
+          }
+          return tot;
         }
 
         PAIRCASE (SpaceKind::Disjunct, SpaceKind::Empty):
@@ -752,19 +764,23 @@ namespace {
         PAIRCASE (SpaceKind::Disjunct, SpaceKind::BooleanConstant):
         PAIRCASE (SpaceKind::Disjunct, SpaceKind::UnknownCase): {
           SmallVector<Space, 4> smallSpaces;
-          std::transform(this->getSpaces().begin(), this->getSpaces().end(),
-                         std::back_inserter(smallSpaces),
-                         [&](const Space &first){
-            return first.minus(other, TC, DC);
-          });
+          for (auto s : this->getSpaces()) {
+            if (auto diff = s.minus(other, TC, DC, minusCount))
+              smallSpaces.push_back(*diff);
+            else
+              return None;
+          }
           return Space::forDisjunct(smallSpaces);
         }
         PAIRCASE (SpaceKind::Constructor, SpaceKind::Type):
           return Space();
         PAIRCASE (SpaceKind::Constructor, SpaceKind::UnknownCase): {
           SmallVector<Space, 4> newSubSpaces;
           for (auto subSpace : this->getSpaces()) {
-            Space nextSpace = subSpace.minus(other, TC, DC).simplify(TC, DC);
+            auto diff = subSpace.minus(other, TC, DC, minusCount);
+            if (!diff)
+              return None;
+            auto nextSpace = diff->simplify(TC, DC);
             if (nextSpace.isEmpty())
               return Space();
             newSubSpaces.push_back(nextSpace);
@@ -813,7 +829,11 @@ namespace {
             SmallVector<Space, 4> copyParams(this->getSpaces().begin(),
                                              this->getSpaces().end());
 
-            auto reducedSpace = s1.minus(s2, TC, DC);
+            auto reducedSpaceOrNone = s1.minus(s2, TC, DC, minusCount);
+            if (!reducedSpaceOrNone)
+              return None;
+            auto reducedSpace = *reducedSpaceOrNone;
+            
             // If one of the constructor parameters is empty it means
             // the whole constructor space is empty as well, so we can
             // safely skip it.
@@ -857,7 +877,7 @@ namespace {
             SmallVector<Space, 4> spaces;
             this->decompose(TC, DC, other.getType(), spaces);
             auto disjunctSp = Space::forDisjunct(spaces);
-            return this->minus(disjunctSp, TC, DC);
+            return this->minus(disjunctSp, TC, DC, minusCount);
           }
           return *this;
         }
@@ -871,7 +891,7 @@ namespace {
             SmallVector<Space, 4> spaces;
             this->decompose(TC, DC, this->getType(), spaces);
             auto orSpace = Space::forDisjunct(spaces);
-            return orSpace.minus(other, TC, DC);
+            return orSpace.minus(other, TC, DC, minusCount);
           } else {
             return *this;
           }
@@ -1286,26 +1306,21 @@ namespace {
       Space totalSpace = Space::forType(subjectType, Identifier());
       Space coveredSpace = Space::forDisjunct(spaces);
 
-      size_t totalSpaceSize = totalSpace.getSize(TC, DC);
+      const size_t totalSpaceSize = totalSpace.getSize(TC, DC);
       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.
-        // FIXME: Do something sensible for non-frozen enums.
-        if (!sawRedundantPattern
-            && coveredSpace.getSize(TC, DC) >= totalSpaceSize) {
-          return;
-        }
-
-        diagnoseMissingCases(RequiresDefault::SpaceTooLarge, Space(),
-                             unknownCase);
+        diagnoseCannotCheck(sawRedundantPattern, totalSpaceSize, coveredSpace,
+                            unknownCase);
         return;
       }
-
-      auto uncovered = totalSpace.minus(coveredSpace, TC, DC).simplify(TC, DC);
+      unsigned minusCount = 0;
+      auto diff = totalSpace.minus(coveredSpace, TC, DC, &minusCount);
+      if (!diff) {
+        diagnoseCannotCheck(sawRedundantPattern, totalSpaceSize, coveredSpace,
+                            unknownCase);
+        return;
+      }
+      
+      auto uncovered = diff->simplify(TC, DC);
       if (unknownCase && uncovered.isEmpty()) {
         TC.diagnose(unknownCase->getLoc(), diag::redundant_particular_case)
           .highlight(unknownCase->getSourceRange());
@@ -1314,8 +1329,8 @@ namespace {
       // Account for unknown cases. If the developer wrote `unknown`, they're
       // all handled; otherwise, we ignore the ones that were added for enums
       // that are implicitly frozen.
-      uncovered = uncovered.minus(Space::forUnknown(unknownCase == nullptr),
-                                  TC, DC);
+      uncovered = *uncovered.minus(Space::forUnknown(unknownCase == nullptr),
+                                   TC, DC, /*&minusCount*/ nullptr);
       uncovered = uncovered.simplify(TC, DC);
 
       if (uncovered.isEmpty())
@@ -1350,9 +1365,27 @@ namespace {
       UncoveredSwitch,
       SpaceTooLarge,
     };
-
-    void diagnoseMissingCases(const RequiresDefault defaultReason,
-                              Space uncovered,
+    
+    void diagnoseCannotCheck(const bool sawRedundantPattern,
+                             const size_t totalSpaceSize,
+                             const Space &coveredSpace,
+                             const CaseStmt *unknownCase) {
+      // Because the space is large or the check is too slow,
+      // 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.
+      // FIXME: Do something sensible for non-frozen enums.
+      if (!sawRedundantPattern &&
+          coveredSpace.getSize(TC, DC) >= totalSpaceSize)
+        return;
+      diagnoseMissingCases(RequiresDefault::SpaceTooLarge, Space(),
+                           unknownCase);
+    }
+    
+    void diagnoseMissingCases(RequiresDefault defaultReason, Space uncovered,
                               const CaseStmt *unknownCase = nullptr,
                               bool sawDowngradablePattern = false) {
       SourceLoc startLoc = Switch->getStartLoc();

lib/Sema/TypeChecker.cpp

@@ -576,7 +576,8 @@ void swift::performTypeChecking(SourceFile &SF, TopLevelContext &TLC,
                                 unsigned StartElem,
                                 unsigned WarnLongFunctionBodies,
                                 unsigned WarnLongExpressionTypeChecking,
-                                unsigned ExpressionTimeoutThreshold) {
+                                unsigned ExpressionTimeoutThreshold,
+                                unsigned SwitchCheckingInvocationThreshold) {
   if (SF.ASTStage == SourceFile::TypeChecked)
     return;
 
@@ -608,6 +609,10 @@ void swift::performTypeChecking(SourceFile &SF, TopLevelContext &TLC,
       if (ExpressionTimeoutThreshold != 0)
         MyTC->setExpressionTimeoutThreshold(ExpressionTimeoutThreshold);
 
+      if (SwitchCheckingInvocationThreshold != 0)
+        MyTC->setSwitchCheckingInvocationThreshold(
+            SwitchCheckingInvocationThreshold);
+
       if (Options.contains(TypeCheckingFlags::DebugTimeFunctionBodies))
         MyTC->enableDebugTimeFunctionBodies();
 

lib/Sema/TypeChecker.h

@@ -922,6 +922,14 @@ class TypeChecker final : public LazyResolver {
   /// than this many seconds.
   unsigned ExpressionTimeoutThreshold = 600;
 
+  /// If non-zero, abort the switch statement exhaustiveness checker if
+  /// the Space::minus function is called more than this many times.
+  ///
+  /// Why this number? Times out in about a second on a 2017 iMac, Retina 5K,
+  // 4.2 GHz Intel Core i7.
+  // (It's arbitrary, but will keep the compiler from taking too much time.)
+  unsigned SwitchCheckingInvocationThreshold = 200000;
+
   /// If true, the time it takes to type-check each function will be dumped
   /// to llvm::errs().
   bool DebugTimeFunctionBodies = false;
@@ -997,10 +1005,26 @@ class TypeChecker final : public LazyResolver {
   /// the upper bound for the number of seconds we'll let the
   /// expression type checker run before considering an expression
   /// "too complex".
+  /// If zero, do not limit the checking.
   unsigned getExpressionTimeoutThresholdInSeconds() {
     return ExpressionTimeoutThreshold;
   }
 
+  /// Get the threshold that determines the upper bound for the number
+  /// of times we'll let the Space::minus routine run before
+  /// considering a switch statement "too complex".
+  /// If zero, do not limit the checking.
+  unsigned getSwitchCheckingInvocationThreshold() const {
+    return SwitchCheckingInvocationThreshold;
+  }
+
+  /// Set the threshold that determines the upper bound for the number
+  /// of times we'll let the Space::minus routine run before
+  /// considering a switch statement "too complex".
+  void setSwitchCheckingInvocationThreshold(unsigned invocationCount) {
+    SwitchCheckingInvocationThreshold = invocationCount;
+  }
+
   bool getInImmediateMode() {
     return InImmediateMode;
   }