Stricter enforcement of the "large space" heuristic #12818
Conversation
|
@swift-ci please smoke test |
|
(Tagging Joe because he was the last one I talked to about this) |
|
@swift-ci please smoke test compiler performance |
|
SGTM. @graydon might be able to help you interpret the compiler performance counters. |
|
@swift-ci Please smoke test |
|
The other thing we could do is err on the side of requiring the default. We could even phrase the diagnostic apologetically, i.e. "the compiler was unable to prove that all cases are handled". |
|
Trouble with that is SILOptimizer likes to pop unreachable diagnostics on default clauses. |
|
Yeah…maybe we need a new kind of "unreachable" that says "yes, I know, but Sema can't do better". |
|
@jrose-apple That seems reasonable to me too. @CodaFi is there a reason we don't do that already? |
|
Worth noting that Robert and I casually switched to talking about a different problem. This bug is about an uncovered switch that Sema thought was covered, and assuming "you need a default" would have fixed it. However, there are also covered switches that Sema might think are uncovered, but SIL diagnostics later prove to be covered. In this case we'd be back in a "damned if you do, damned if you don't" scenario. |
|
The SIL diagnostic behavior pre-dates the space engine. I'd be fine with delegating the problem of exhaustiveness analysis to Sema and suppressing the diagnostic in SIL. |
|
Compiler perf benchmarks look like noise, which is good. Gonna run some more tests locally to see if I can get it to regress on less common idioms. |
|
Also relevant is SR-3278. I will push another commit here that shows what removing the SIL diagnostic would look like. |
|
@CodaFi Is this ready to go in? |
|
@swift-ci Please test source compatibility |
|
I want to test it on one more set of inputs. I’ll merge by tomorrow and PR to 4.1 then. |
|
@CodaFi Great, thanks! |
| @@ -3857,6 +3857,10 @@ WARNING(redundant_particular_literal_case,none, | |||
| NOTE(redundant_particular_literal_case_here,none, | |||
| "first occurrence of identical literal pattern is here", ()) | |||
|
|
|||
| ERROR(cannot_prove_exhaustive_switch,none, | |||
There was a problem hiding this comment.
@jrose-apple Here's the new diagnostic
There was a problem hiding this comment.
I think the diagnostic should also include a mention of the consequence of the "too large" condition so the user knows what action to take—they just have to provide a default, right?
There was a problem hiding this comment.
This sounds good, along with what Joe said. We want the user to know immediately what to do next, even if there's some grumbling involved.
There was a problem hiding this comment.
If it doesn't require too much SourceLoc gymnastics, can we have a fixit insert default: <#fatalError()#> or something like that?
| @@ -1031,7 +1031,8 @@ void PatternMatchEmission::emitDispatch(ClauseMatrix &clauses, ArgArray args, | |||
| } else { | |||
| Loc = clauses[firstRow].getCasePattern()->getStartLoc(); | |||
| } | |||
| SGF.SGM.diagnose(Loc, diag::unreachable_case, isDefault); | |||
| if (!isDefault) | |||
| SGF.SGM.diagnose(Loc, diag::unreachable_case); | |||
There was a problem hiding this comment.
@jckarter Is this reasonable?
There was a problem hiding this comment.
I don't recall the history here—why are we diagnosing this in SILGen?
There was a problem hiding this comment.
Just checking, since I've been out of it: we do diagnose unreachable defaults in Sema when we can, right?
There was a problem hiding this comment.
As of now: no (and I don’t believe we should because of things like SR-3278). default early-exits the current exhaustiveness algorithm.
There was a problem hiding this comment.
Ah, right. I guess that's on me to put in once we switch to resilient builds with the exhaustive/non-exhaustive distinction on.
The Space Engine includes a heuristic that attempted a combinatorics- based check to see if a pattern covered an insufficient amount of cases. In straight-line switches this avoided computing a subspace match that would have been quite expensive computationally. However, when expressive patterns (like tuple patterns) are used, it can fool the check because the covered space is much larger than the actual size of the space due to pattern overlap that counting like this simply can't detect. Instead, just do the right thing and perform a space subtraction after the existing preconditions for the heuristic are satisfied. Resolves SR-6316
This presents a regression in diagnostic quality that is definitely worth it not to lie to SILGen about whether a switch is covered or not. At the same time, disable SIL’s unreachable diagnostic for ‘default’ clauses which would previously cause a warning to be emitted if the default was proven to be unreachable. This analysis is incomplete anyways and can be done by Sema in the future if we desire.
|
@swift-ci please smoke test |
|
@swift-ci please test compiler performance |
|
Finally, @swift-ci please test source compatibility |
|
⛵️ |
Build comment file:Summary for master fullUnexpected test results, stats may be off for Kronos, CoreStore, vapor, Dollar, RxDataSources No regressions above thresholds Debugdebug briefRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
debug detailedRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
Debug-optdebug-opt briefRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
debug-opt detailedRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
Wmo-ononewmo-onone briefRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
wmo-onone detailedRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
Releaserelease briefRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
release detailedRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
|
|
@CodaFi Were you still planning on opening a 4.1 PR? |
|
Thanks! |
The Space Engine includes a heuristic that attempted a combinatorics-
based check to see if a pattern covered an insufficient amount of cases.
In straight-line switches this avoided computing a subspace match that
would have been quite expensive computationally. However, when
expressive patterns (like tuple patterns) are used, it can fool the
check because the covered space is much larger than the actual size of
the space due to pattern overlap that counting like this simply can't detect.
Instead weaken the heuristic to diagnose large spaces as "uncheckable" and suggest a
defaultclause. This requires disabling SILGen's warning for unreachabledefaultclauses which is itself incomplete.Resolves SR-6316.
I'm concerned about the performance impact of this change, which is what drove us to use the heuristic in the first place.