Skip to content
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

Wrong pattern matching warning message 'unreachable code' #6146

Closed
scabug opened this issue Jul 26, 2012 · 27 comments
Closed

Wrong pattern matching warning message 'unreachable code' #6146

scabug opened this issue Jul 26, 2012 · 27 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Jul 26, 2012

In the following pattern matching scenario, the compiler for Scala 2.10.0-M6 emits the warning message "unreachable code". That warning is not emitted in Scala 2.9.2. Furthermore, a simple test shows the case is still reached correctly.

trait AxisCompanion {
   sealed trait Format
   object Format {
      case object Decimal extends Format
      case object Integer extends Format
      final case class Time( hours: Boolean = false, millis: Boolean = true ) extends Format
   }
}
object Axis extends AxisCompanion
class Axis {
   import Axis._
   def test( f: Format ) = f match {
      case Format.Integer => "Int"
      case Format.Time( hours, millis ) => "Time"
      case Format.Decimal => "Dec"
   }
}

The warning:

<console>:20: warning: unreachable code
             case Format.Time( hours, millis ) => "Time"                                                ^

A test:

val a = new Axis
a.test( Axis.Format.Time() ) // ok!
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 26, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6146?orig=1
Reporter: @Sciss
Affected Versions: 2.10.0-M6
See #6161

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 27, 2012

@paulp said:
Here's a somewhat terrifying minimization (at least I assume it's the same bug, haven't looked that closely at this one.) Terrifying because commenting out any of the other cases - the one before NoPrefix, or either of the two after - makes the warning go away. What, it became reachable because of the absence of later cases? Are we moving backward in pattern matcher time? Is this the matcher of adriaan grey?

trait Foo extends scala.reflect.internal.SymbolTable {
  def g(x: Any) = x match {
    case NoType           =>
    case NoPrefix         =>
    case tpe: Type        => 
    case product: Product => 
  }
}

/*
% m3scalac ./a.scala 
% m4scalac ./a.scala 
./a.scala:7: warning: unreachable code
      case NoPrefix         =>
                            ^
one warning found
*/
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 27, 2012

@paulp said:
Oh, and in case you don't like reading to the end, my transcript shows this behavior appeared between m3 and m4 - although because my test case uses the compiler, it could as easily be that the compiler changed in some relevant way between m3 and m4. It didn't minimize any further too easily.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 27, 2012

@Sciss said:
Yes, I can confirm that with my snippet - no warning in M3, warning shows in M4, M5, M6.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 27, 2012

@adriaanm said:
Thanks for minimizing further. Working on a fix right now.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 27, 2012

@adriaanm said:
the problem is in enumerateSubtypes, where we need to align the prefixes of various types

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 27, 2012

@adriaanm said (edited on Jul 27, 2012 2:24:29 PM UTC):
this is actually going to be hard to fix...
btw, it being a regression since M3 only indicates we've only had these analyses since then IIRC

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 27, 2012

@adriaanm said:
the other problem seems to be #6022 since it involves Product, should be fixed in M5

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 27, 2012

@paulp said:
Wrt hard to fix, is this purely a cosmetic error? That is, is the code generation all doing the right thing, just the analysis is off? I just want to know how hard to pray.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jul 27, 2012

@adriaanm said:
yes, the codegen is fine -- it's decoupled from where this goes wrong in the analysis (the latter can be turned off with -Yno-patmat-analysis, btw)
i have an idea of how to fix it properly (pushed to ticket-6146 on my github) , but am running out of time, and acutely out of internet connectivity
will re-emerge in a week

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 10, 2012

@adriaanm said (edited on Aug 10, 2012 9:04:07 AM UTC):
in progress here: https://github.com/adriaanm/scala/tree/ticket-6146

the problem is it requires dealing more precisely with prefixes to be more conservative about unreachability in this example,
but the added precision causes spurious warnings in other places -- this is fixable, but will take more time (see commit messages in that branch or https://raw.github.com/gist/2235522/9823db3f7a.buildlog)

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 10, 2012

@adriaanm said:
I'm going to have to demote this to major due to lack of time, unfortunately.
It should definitely be fixed in 2.10.1, but I think we can live with the spurious warning for now.
The spurious warning only arises for nested sealed classes that show up in type tests that use a different prefix than the one that follows from the definition of the nested type.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 14, 2012

@adriaanm said:
actually, I think it's probably going to be easier to fix after all -- need to construct the correct prefix in enumerateSubtypes

here's what i've been experimenting with using :power in the repl:

scala> trait Companion {
     |    sealed trait Super
     |    object Subs {
     |       case object Decimal extends Super
     |       case object Integer extends Super
     |       case class Time(t: String) extends Super
     |    }
     | }
defined trait Companion

scala> 

scala> object Outer extends Companion
defined module Outer

scala> 

scala> 

scala> val tp = ?[Outer.Super].tpe
tp: $r.power.intp.global.Type = Outer.Super

scala> val subSyms = tp.typeSymbol.sealedDescendants
subSyms: Set[$r.power.intp.global.Symbol] = Set(object Decimal, object Integer, class Time, trait Super)

scala> val subSym = subSyms.head
subSym: $r.power.intp.global.Symbol = object Decimal

scala> 

scala> tp.prefix.memberType(subSym.owner).memberType(subSyms.head)
res1: $r.power.intp.global.Type = Outer.Subs.type#Decimal.type

Outer.Subst.Decimal is closer to the type we should for the sealed descendant of Outer.Super -- currently we use Companion.Subst.Decimal...

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 30, 2013

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 30, 2013

@adriaanm said:
awesome! i was dreading this one...

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 31, 2013

@retronym said:
I wonder whether AsSeenFrom should be made to figure this out. It seems this might be the same as #6161.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Jan 31, 2013

@paulp said:
Oh, oh, you just made me realize #7051 is related. Will comment in #6161.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Feb 2, 2013

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Feb 3, 2013

@retronym said:
Some discussion to fill in gaps of my TypeRef knowledge: https://groups.google.com/d/topic/scala-internals/mORghOw2dtI/discussion

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Feb 4, 2013

@retronym said:
I sort of hit a wall with this one, bouncing it back.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Feb 5, 2013

@retronym said:
This isn't quite quite right, but it's getting somewhere.

:power
val u = rootMirror.universe; import u._, language._, intp.deconstruct.show
def show(t: Type) {println(t); println(show(t))}
class C[T] {
  sealed trait F[A <: T]
  object X {
    object S1 extends F[T]
  }
  class S2 extends F[T]
}
object O extends C[Int] {
  def foo(f: F[Int]) = f match { case X.S1 => }

  class S3 extends F[Int]
}

def subType(base: Symbol, sub: Symbol, baseType: Type) = {
  val subTpe = sub.tpeHK
  val widenNarrow = subTpe.map(_.widen.narrow)
  val clazz = subTpe.baseType(base).typeSymbol.owner
  val pre = baseType.prefix
  val seen = widenNarrow asSeenFrom (baseType.prefix, clazz)
  val result = seen.widen

  def tpToString(t: Type) = t.toString.replaceAllLiterally("$iw.", "")
  def p(label: String)(t: Type) = println(s"$label: ${tpToString(t)}")
  println(s"subtype(base = $base, sub = $sub, baseType = ${tpToString(baseType)}")
  println(s"(${tpToString(widenNarrow)}).asSeenFrom(${tpToString(pre)}, $clazz) = ${tpToString(seen)}")
  p("result")(result)

  result
}

val S1 = typeOf[c.X.S1.type forSome { val c: C[_] }].typeSymbol.tpeHK
val S2 = typeOf[O.S2].typeSymbol.tpeHK
val S3 = typeOf[O.S3].typeSymbol.tpeHK
val F = typeOf[c.F[_] forSome { val c: C[_] }].typeSymbol.tpeHK
val fTpe = typeOf[O.type].decl(newTermName("foo")).paramss.head.head.tpe

subType(F.typeSymbol, S1.typeSymbol, fTpe)

subType(F.typeSymbol, S2.typeSymbol, fTpe)

subType(F.typeSymbol, S3.typeSymbol, fTpe)
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Feb 6, 2013

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Feb 7, 2013

@scabug scabug closed this Feb 8, 2013
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 24, 2013

@paulp said:
After a boatload of difficult pattern matcher, THIS is now my one test failure? Ay caramba. Back into the fray...

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 24, 2013

@retronym said:
Let me know if you want me to take a look at how its failing.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 24, 2013

@paulp said:
It wasn't so bad, I got it.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Aug 24, 2013

@paulp said:
Thanks though.

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.