bug icon indicating copy to clipboard operation
bug copied to clipboard

Pattern matching incompatible type does not always warn

Open SethTisue opened this issue 4 years ago • 5 comments

trait UnsealedTrait
def c(c: Option[Int]) = c match { case _: UnsealedTrait =>; case _ =>  }

This should warn, as an Option cannot possibly be an UnsealedTrait.

SethTisue avatar Aug 04 '21 15:08 SethTisue

@retronym has an unfinished PR that attempts to address this: https://github.com/retronym/scala/pull/109

Jason comments:

I defer the fruitless type test check for patterns until the patmat phase and also introduce a stronger analysis that checks that if the scrutinee is sealed, at least one member of its domain could match the pattern type. It's not quite there yet as it should limit the stronger anaysis to cases to "strongly sealed" domains where non of the domain is open for further extension, ie leaves are final classes.

Jason abandoned the PR, but @jxnu-liguobin is exploring the possibility of resurrecting it (on 2.13.x instead), here: https://github.com/scala/scala/pull/9706

SethTisue avatar Aug 04 '21 15:08 SethTisue

Agree with this. If the type is final and incompatible, an error is currently being emitted. But this is also a very common case where it is possible but unlikely what the coder wants, so a warning will be very helpful in identifying the issues. For me personally, this has caused a production issue.

neontorrent avatar Aug 04 '21 19:08 neontorrent

trait UnsealedTrait
def c(c: Option[Int]) = c match { case _: UnsealedTrait =>; case _ =>  }

This should warn, as an Option cannot possibly be an UnsealedTrait.

My understand: only when the type of parameter c is the final class or the sealed class outside the domain , we can accurately judge whether it is compatible because it cannot have subclasses..

Does this meet expectations?

// scalac: -Xfatal-warnings

sealed trait SealedTrait
class SomeClass
trait UnsealedTrait

sealed abstract class O
class O1 extends O
final class O2 extends UnsealedTrait
sealed class O3
class O4 extends O3 with UnsealedTrait

class Test {

  def a(c: Option[Int]) = c match { case _: SealedTrait => ; case _ => }
  def b(c: Option[Int]) = c match { case _: SomeClass => ; case _ => }
  def c(c: Option[Int]) = c match { case _: UnsealedTrait => ; case _ => }

  //   O1 is not final , so there could be a value of type O1 with UnsealedTrait
  def nowarn1(c: O) = c match { case _: UnsealedTrait => ; case _ => }

  def nowarn2(c: O3) = c match { case _: UnsealedTrait => ; case _ => }
  def nowarn3(c: O4) = c match { case _: UnsealedTrait => ; case _ => }
}
patmat-sealed-reachable.scala:16: warning: fruitless type test: a value of type Option[Int] cannot also be a SomeClass
  def b(c: Option[Int]) = c match { case _: SomeClass => ; case _ => }
                                            ^
patmat-sealed-reachable.scala:15: warning: fruitless type test: a value of type Option[Int] cannot also be a SealedTrait
  def a(c: Option[Int]) = c match { case _: SealedTrait => ; case _ => }
                                            ^
patmat-sealed-reachable.scala:17: warning: Pattern type is not compatible with any of the sealed children of expected type
  def c(c: Option[Int]) = c match { case _: UnsealedTrait => ; case _ => }
                                            ^
error: No warnings can be incurred under -Werror.
3 warnings
1 error

jxnu-liguobin avatar Aug 28 '21 10:08 jxnu-liguobin

This is so nice!

Can we keep the same language for all cases though? Why not "fruitless type test: a value of type Option[Int] cannot also be an UnsealedTrait"? How the message got made (checked Option is sealed and that all its children don't extend UnsealedTrait) is not relevant to the user.

martijnhoekstra avatar Aug 28 '21 19:08 martijnhoekstra

Can we keep the same language for all cases though?

I think It depends on whether we need to distinguish between these(The timing of inspection is different.).

jxnu-liguobin avatar Aug 29 '21 05:08 jxnu-liguobin