Skip to content

Incorrect or missing exhaustivity warning when using type tests #9682

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

Open
smarter opened this issue Aug 29, 2020 · 4 comments
Open

Incorrect or missing exhaustivity warning when using type tests #9682

smarter opened this issue Aug 29, 2020 · 4 comments

Comments

@smarter
Copy link
Member

smarter commented Aug 29, 2020

This produces no warning, but is not exhaustive:

sealed trait Foo
final case class Bar[A](a: A) extends Foo

def test(x: Foo) = x match {
  case Bar(a: Int) =>
}

This produces both an inexhaustivity and unchecked warnng, but it is exhaustive because of erasure:

sealed trait Foo
final case class Bar[S](s: S => Unit) extends Foo

def test(x: Foo) = x match {
  case Bar(s: (Any => Unit)) =>
}
-- [E029] Pattern Match Exhaustivity Warning: try/ce.scala:19:19 ---------------
19 |def test(x: Foo) = x match {
   |                   ^
   |                   match may not be exhaustive.
   |
   |                   It would fail on pattern case: Bar(_)

longer explanation available when compiling with `-explain`
-- Warning: try/ce.scala:20:11 -------------------------------------------------
20 |  case Bar(s: (Any => Unit)) =>
   |           ^^^^^^^^^^^^^^^^
   |           the type test for Any => Unit cannot be checked at runtime

(Adding a case Bar(_) => should in fact produce an unreachability warning).

smarter added a commit to smarter/cats-effect that referenced this issue Aug 29, 2020
`Bind` has an extra type parameter `S` which does not exist in
`Resource`, so when pattern matching on `Bind`, that type parameter is
replaced by a skolem, we can get a reference to this skolem by using a
pattern-bound type variable (here called `s`), this allows us to type
the lambda parameter `r` correctly. Before this commit, `Any` was used
instead of a type variable, this is unsound since `S` appears
contravariantly in `fs` but didn't lead to any issue since it only
appeared in the expression `fs(s)` (but if someone had accidentally
written `fs(0)`, the compiler wouldn't have detected the error).

This issue was identified by Dotty (though it somewhat confusingly
reported that `Bind(_, _)` was missing from the match instead of
reporting the use of an unchecked type test, I've minimized that and
opened scala/scala3#9682), the remaining
Dotty warnings are all unchecked type tests and they all seem like
legitimate warnings to me.

It's also worth noting that if we didn't have to cross-compile with
Scala 2, we could just omit the type of the lambda parameter and Dotty
would figure it out, so no explicit pattern-bound type variable would be
needed at all :).

I haven't tried it but this change should be backportable as-is to
`series/2.x`.
smarter added a commit to smarter/cats-effect that referenced this issue Aug 29, 2020
`Bind` has an extra type parameter `S` which does not exist in
`Resource`, so when pattern matching on `Bind`, that type parameter is
replaced by a skolem (that is, a fresh abstract type whose bounds are
those of `S` in `Bind`), we can get a reference to this skolem by using
a pattern-bound type variable (here called `s`), this allows us to type
the lambda parameter `r` correctly. Before this commit, `Any` was used
instead of a type variable, this is unsound since `S` appears
contravariantly in `fs` but didn't lead to any issue since it only
appeared in the expression `fs(s)` (but if someone had accidentally
written `fs(0)`, the compiler wouldn't have detected the error).

This issue was identified by Dotty (though it somewhat confusingly
reported that `Bind(_, _)` was missing from the match instead of
reporting the use of an unchecked type test, I've minimized that and
opened scala/scala3#9682), the remaining
Dotty warnings are all unchecked type tests and they all seem like
legitimate warnings to me.

It's also worth noting that if we didn't have to cross-compile with
Scala 2, we could just omit the type of the lambda parameter and Dotty
would figure it out, so no explicit pattern-bound type variable would be
needed at all :).

I haven't tried it but this change should be backportable as-is to
`series/2.x`.
smarter added a commit to smarter/cats-effect that referenced this issue Aug 29, 2020
`Bind` has an extra type parameter `S` which does not exist in
`Resource`, so when pattern matching on `Bind`, that type parameter is
replaced by a skolem (that is, a fresh abstract type whose bounds are
those of `S` in `Bind`). This skolem doesn't have an accessible name,
so to get a reference to it we need to use a pattern-bound type variable
(here called `s`), this allows us to type the lambda parameter `r`
correctly. Before this commit, `Any` was used instead of a type
variable, this is unsound since `S` appears contravariantly in `fs` but
didn't lead to any issue since it only appeared in the expression
`fs(s)` (but if someone had accidentally written `fs(0)`, the compiler
wouldn't have detected the error).

This issue was identified by Dotty (though it somewhat confusingly
reported that `Bind(_, _)` was missing from the match instead of
reporting the use of an unchecked type test, I've minimized that and
opened scala/scala3#9682), the remaining
Dotty warnings are all unchecked type tests and they all seem like
legitimate warnings to me.

It's also worth noting that if we didn't have to cross-compile with
Scala 2, we could just omit the type of the lambda parameter and Dotty
would figure it out, so no explicit pattern-bound type variable would be
needed at all :).

I haven't tried it but this change should be backportable as-is to
`series/2.x`.
@liufengyun
Copy link
Contributor

There is at some point an intentional change to not reason in erasure semantics for exhaustivity check, and leave them to typetest check in later phases.

One motivation is to better support GADT:

sealed trait Expr[T]
case class IntExpr(x: Int) extends Expr[Int]
case class BooleanExpr(b: Boolean) extends Expr[Boolean]

def foo[T](x: Expr[T], y: Expr[T]) = (x, y) match {
    case (IntExpr(_), IntExpr(_)) =>
    case (BooleanExpr(_), BooleanExpr(_)) =>
}

In the code above, if we reason in terms of type erasure, then the scrutinee has the type (Expr[_], Expr[_]) instead of (Expr[T], Expr[T]) --- we lose the constraint that the two components are of the same type.

@Swoorup

This comment has been minimized.

@dwijnand
Copy link
Member

dwijnand commented Jun 7, 2021

What about the other case, the one about the nested a: Int going unmentioned? The exhaustivity warning not taking in account the "uncheckability" of certain type tests (i.e. the second example) seems so minor in comparison.

@liufengyun
Copy link
Contributor

What about the other case, the one about the nested a: Int going unmentioned? The exhaustivity warning not taking in account the "uncheckability" of certain type tests (i.e. the second example) seems so minor in comparison.

Good catch, I missed that part. I'll propose a fix for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants