Skip to content

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented May 31, 2017

• Explanation: Type checking switch statements looking for exhaustive patterns is a potentially factorial process. Introduce a reasonable size limit to fall back to a linear-time heuristic instead of the full check.
• Scope of Issue: A large enough switch over a large enough enum case can cause the compiler to hang for hours computing a fixit.
• Origination: Noticed by Harlan Haskins while TableGen’ing a very very large enum and a similarly enormous switch statement .
• Risk: Low.
• Reviewed By: Xi Ge, Joe Groff
• Testing: New regression tests added, confirmed locally that the patch resolves Harlan’s original issue.
• Directions for QE: (Optional) Verify with the test case in the patch.
• Radar URL: rdar://32480026

@CodaFi CodaFi requested a review from jckarter May 31, 2017 22:50
@CodaFi
Copy link
Contributor Author

CodaFi commented May 31, 2017

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 69dd65161aea859f6fb1cf9c6f5450dfd52d1ab1
Test requested by - @CodaFi

Robert Widmann added 2 commits May 31, 2017 16:13
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.
<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.
@CodaFi CodaFi force-pushed the code-covfeferage branch from 69dd651 to b5a69c7 Compare May 31, 2017 23:13
@CodaFi
Copy link
Contributor Author

CodaFi commented May 31, 2017

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 69dd65161aea859f6fb1cf9c6f5450dfd52d1ab1
Test requested by - @CodaFi

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 69dd65161aea859f6fb1cf9c6f5450dfd52d1ab1
Test requested by - @CodaFi

@CodaFi
Copy link
Contributor Author

CodaFi commented Jun 1, 2017

@swift-ci please test source compatibility

@tkremenek tkremenek merged commit 52fcadf into swiftlang:swift-4.0-branch Jun 1, 2017
@CodaFi CodaFi deleted the code-covfeferage branch June 1, 2017 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants