Skip to content

[4.1] Stricter enforcement of the "large space" heuristic #13438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 19, 2017

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Dec 14, 2017

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 default clause. This requires disabling SILGen's warning for unreachable default clauses which is itself incomplete.

  • Scope: Affects users that write complex switch statements with (large) tuple types.
  • Issue: (Needs radar post-nomination: see rdar://problem/35389001 and SR-6316)
  • Reviewed by: @jckarter
  • Risk: Low. Improves on a broken heuristic and does some diagnostics QoI work too.
  • Testing: Added compiler regression tests.

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.
@CodaFi
Copy link
Contributor Author

CodaFi commented Dec 14, 2017

@swift-ci please test

Copy link
Contributor

@jckarter jckarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@rudkx
Copy link
Contributor

rudkx commented Dec 18, 2017

Okay to merge or is there something more you wanted to do here?

@CodaFi
Copy link
Contributor Author

CodaFi commented Dec 18, 2017

Does this need a nomination?

@rudkx
Copy link
Contributor

rudkx commented Dec 19, 2017

@swift-ci Please test source compatibility

@CodaFi
Copy link
Contributor Author

CodaFi commented Dec 19, 2017

⛵️

I’d appreciate if someone could close out the linked radar.

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