Add cutoff to limit time taking to check exhaustiveness of switch statement. #16271

davidungar · 2018-05-01T01:09:00Z

A quick-fix to rdar://39805050: Stop checking for exhaustion in a switch statement if it goes on for too long. Uses the same logic as if the space is too large to avoid issuing a diagnostic.
Without this fix, the following takes several minutes to type-check:

enum A {a1, ... a5}
enum B {b1, ... b4}
var a: A; var b: B
(initialize a and b)
switch (a, b) {
 case: (a1, b1), ... (a5, b4): break // all possibilities
}

With this fix, the check is abandoned after about a second.

davidungar · 2018-05-01T01:09:26Z

@swift-ci please test

swift-ci · 2018-05-01T01:32:21Z

Build failed
Swift Test Linux Platform
Git Sha - 3d41eda9a459aeb9bd9828852368ff0ec7a89ef6

swift-ci · 2018-05-01T01:56:27Z

Build failed
Swift Test OS X Platform
Git Sha - 3d41eda9a459aeb9bd9828852368ff0ec7a89ef6

davidungar · 2018-05-01T07:23:14Z

@swift-ci please smoke test

davidungar · 2018-05-01T15:56:20Z

@swift-ci please test source compatibility

davidungar · 2018-05-01T16:12:25Z

@swift-ci Please Test Source Compatibility

xedin

LGTM! I think it makes sense to make threshold configurable as a frontend optional.

xedin · 2018-05-02T01:20:12Z

lib/Sema/TypeCheckSwitchStmt.cpp

-      auto uncovered = totalSpace.minus(coveredSpace, TC, DC).simplify(TC, DC);
+      // Why this number? Times out in about a second on my machine.
+      // (It's arbitrary, but will keep the compiler from taking too much time.)
+      const int maxMinusCount = 200000;


I think we need to make this a frontend flag so it's configurable to a different value if needed...

Agreed. Also, instead of "my machine", can you put some basic specs? (Model and clock speed, maybe.)

@AnnaZaks I think you voiced an objection to a flag for this. Would you be OK with me putting one in?

CodaFi · 2018-05-02T15:32:07Z

This seems a much more reasonable heuristic than the space size one. Once this is merged, I'm going to look into killing it (again).

jrose-apple

I don't like this because "number of minus operations" seems completely arbitrary, but maybe we don't have anything better.

jrose-apple · 2018-05-02T18:04:46Z

lib/Sema/TypeCheckSwitchStmt.cpp

-      auto uncovered = totalSpace.minus(coveredSpace, TC, DC).simplify(TC, DC);
+      // Why this number? Times out in about a second on my machine.
+      // (It's arbitrary, but will keep the compiler from taking too much time.)
+      const int maxMinusCount = 200000;


Agreed. Also, instead of "my machine", can you put some basic specs? (Model and clock speed, maybe.)

jrose-apple · 2018-05-02T18:05:21Z

lib/Sema/TypeCheckSwitchStmt.cpp

+      // remaining minus operations allowed before abandoning the computation.
+      // Returns None if the computation "timed out".
+      Optional<Space> minus(const Space &other, TypeChecker &TC,
+                  const DeclContext *DC, int *minusCount) const {


Nitpick: indentation got messed up.

Specs added.

davidungar · 2018-05-03T04:41:00Z

@swift-ci please smoke test

davidungar · 2018-05-03T04:54:14Z

@swift-ci please smoke test

jrose-apple · 2018-05-03T16:12:20Z

include/swift/Option/FrontendOptions.td

@@ -345,6 +345,8 @@ 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 : Joined<["-"], "switch-checking-invocation-threshold=">;


Nitpick: this ought to have _EQ on the end to match the spelling of the option.

jrose-apple · 2018-05-03T16:13:11Z

lib/Sema/TypeChecker.h

@@ -997,10 +1005,25 @@ 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.


Is it worth building this into SwitchCheckingInvocationThreshold too?

Yes, it is, but the comment was incomplete. Fixed.

…tement.

davidungar · 2018-05-03T17:30:51Z

@swift-ci please smoke test

davidungar · 2018-05-03T17:41:44Z

@jrose-apple Thanks for reviewing this. I think I've addressed your concerns and hope to land and cherry pick it by end of tomorrow, after which it apparently gets harder.

jrose-apple

@xedin already approved this but it looks good to me too.

…ch-takes-too-long-to-compile Add cutoff to limit time taking to check exhaustiveness of switch statement.

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

…ch-takes-too-long-to-compile Add cutoff to limit time taking to check exhaustiveness of switch statement.

davidungar requested review from jrose-apple and xedin May 1, 2018 01:10

davidungar force-pushed the rdar-39805050-Switch-takes-too-long-to-compile branch from 3d41eda to df1f4e6 Compare May 1, 2018 07:22

xedin approved these changes May 2, 2018

View reviewed changes

jrose-apple reviewed May 2, 2018

View reviewed changes

davidungar force-pushed the rdar-39805050-Switch-takes-too-long-to-compile branch 2 times, most recently from 044f717 to bd901bb Compare May 3, 2018 04:40

davidungar force-pushed the rdar-39805050-Switch-takes-too-long-to-compile branch from bd901bb to 0061356 Compare May 3, 2018 04:52

jrose-apple reviewed May 3, 2018

View reviewed changes

Add cutoff to limit time taking to check exhaustiveness of switch sta…

037a2c8

…tement.

davidungar force-pushed the rdar-39805050-Switch-takes-too-long-to-compile branch from 0061356 to 037a2c8 Compare May 3, 2018 17:30

jrose-apple approved these changes May 3, 2018

View reviewed changes

davidungar merged commit 7b6ec6c into swiftlang:master May 3, 2018

davidungar pushed a commit to davidungar/swift that referenced this pull request May 4, 2018

Merge pull request swiftlang#16271 from davidungar/rdar-39805050-Swit…

438c478

…ch-takes-too-long-to-compile Add cutoff to limit time taking to check exhaustiveness of switch statement.

davidungar pushed a commit that referenced this pull request May 4, 2018

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

0d7de8c

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

davidungar pushed a commit to davidungar/swift that referenced this pull request May 4, 2018

Merge pull request swiftlang#16271 from davidungar/rdar-39805050-Swit…

b621733

…ch-takes-too-long-to-compile Add cutoff to limit time taking to check exhaustiveness of switch statement.

davidungar deleted the rdar-39805050-Switch-takes-too-long-to-compile branch June 20, 2019 21:27

		@@ -345,6 +345,8 @@ 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 : Joined<["-"], "switch-checking-invocation-threshold=">;

Add cutoff to limit time taking to check exhaustiveness of switch statement. #16271

Add cutoff to limit time taking to check exhaustiveness of switch statement. #16271

Uh oh!

Conversation

davidungar commented May 1, 2018

Uh oh!

davidungar commented May 1, 2018

Uh oh!

swift-ci commented May 1, 2018

Uh oh!

swift-ci commented May 1, 2018

Uh oh!

davidungar commented May 1, 2018

Uh oh!

davidungar commented May 1, 2018

Uh oh!

davidungar commented May 1, 2018

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented May 2, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidungar commented May 3, 2018

Uh oh!

davidungar commented May 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidungar commented May 3, 2018

Uh oh!

davidungar commented May 3, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!