Skip to content

Fix #2578 Part 1: Tighten type checking of pattern bindings #6389

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 12 commits into from
May 7, 2019

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Apr 28, 2019

Check that patterns in val/var defs are irrefutable, unless the pattern is followed by : @unchecked.

For the moment, this requires -strict mode. We plan to turn this on by default once the bootstrap compiler supports : @unchecked in patterns (i.e. once this PR is part of the bootstrap compiler).

Edit: In fact, we probably can do this only in 3.1, for cross compilation reasons with Scala 2.

@odersky odersky force-pushed the add-checked-patdef branch 3 times, most recently from 6857f88 to 02a5657 Compare April 29, 2019 22:16
@odersky odersky changed the title Fix #2578: Tighten type checking of pattern bindings Fix #2578 Part 1: Tighten type checking of pattern bindings May 2, 2019
@odersky odersky requested a review from abgruszecki May 2, 2019 21:01
odersky added 8 commits May 4, 2019 10:50
Allow ascriptions in match scrutinees without requiring parentheses.
Allow `val a, b, c = ...` only if a, b, c are identifiers.
It looks weird if they are general patterns, and it seems this gives
unnecessary power.
... unless followed by `: @unchecked`.

For the moment, this requires -strict mode. We plan to turn this on
by default once bootstrap compiler supports `: @unchecked` in patterns
(i.e. once this PR is part of the bootstrap compiler).
 - move all strict neg tests to a special directory
 - disable -strict for library bootstrap.
   tasty.reflect sources fail under -strict, but this can be fixed
   only when the original bootstrap compiler accepts : @unchecked in pattern definitions.
@smarter
Copy link
Member

smarter commented May 4, 2019

Edit: In fact, we probably can do this only in 3.1, for cross compilation reasons with Scala 2.

We can do it earlier if we get some of the parser changes into Scala 2, right ? What's the minimum set of changes needed ?

Copy link
Contributor

@abgruszecki abgruszecki left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

check(pat, pt) && {
val argPts = unapplyArgs(fn.tpe.finalResultType, fn, pats, pat.sourcePos)
pats.corresponds(argPts)(checkIrrefutable)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For UnApply, we should also check if the extractor itself is irrefutable, i.e. if it returns Some, or true, or a product. The logic to do that is in SpaceEngine#irrefutable.

Additionally, for some reason unapplies are simply rejected as val definitions:

scala> {
     |   object Positive { def unapply(i: Int): Option[Int] = Some(i).filter(_ > 0) }
     |   val Positive(p) = 5
     |   5 match { case Positive(p) => p }
     | }
3 |  val Positive(p) = 5
  |      ^^^^^^^^^^^
  | ((i: Int): Option[Int])(Positive.unapply) is not a valid result type of an unapply method of an extractor.

@abgruszecki abgruszecki assigned odersky and unassigned abgruszecki May 6, 2019
odersky added 3 commits May 7, 2019 12:34
This warning should trigger only for explicitly written type tests,
never for type tests coming from a match.

So far we did not notice this, since it triggered only for types that are
known to be null, and there are very few of those so far (basically,
just the primitive value types) and those types did not tend to be
tested irrefutably in patterns.
I believe scala#4674 had the wrong resolution. The test leads to spurious warnings for compiler-generated pattern matches.
I believe it's better to revert the decision and check only explicit isInstanceOf, never patterns.

Note the scalac does not check patterns either.
@odersky odersky merged commit f85877d into scala:master May 7, 2019
@allanrenucci allanrenucci deleted the add-checked-patdef branch May 7, 2019 16:14
@anatoliykmetyuk anatoliykmetyuk added this to the 0.15 Tech Preview milestone May 21, 2019
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.

4 participants