Possible Regression in 2.12.0-RC1 inferencing type annotation

[scabug]

This is documented in typelevel/cats#1377 and we are not sure if this a known change/improvement or bug.

Compiling file Eval.scala
Compile errror:

[error] core/src/main/scala/cats/Eval.scala:77: overriding value start in class Compute of type () => cats.Eval[this.Start];
[error]  value start has incompatible type
[error]           val start  = c.start
[error]               ^
[error] core/src/main/scala/cats/Eval.scala:78: overriding value run in class Compute of type this.Start => cats.Eval[B];
[error]  value run has incompatible type
[error]           val run = (s: c.Start) =>

Trivial fix:

@@ -74,8 +74,8 @@ sealed abstract class Eval[+A] extends Serializable { self =>
       case c: Eval.Compute[A] =>
         new Eval.Compute[B] {
           type Start = c.Start
-          val start = c.start
-          val run = (s: c.Start) =>
+          val start: () => Eval[Start] = c.start
+          val run: Start => Eval[B] = (s: c.Start) =>
             new Eval.Compute[B] {
               type Start = A
               val start = () => c.run(s)

Apologies if this is a false alarm, we do not mind just applying the fix but as Erik says in the issue, it makes sense to let you guys know just in case.

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-9931?orig=1
Reporter: Alistair Johnson (alistair.johnson-at-johnsonusm.com)
Affected Versions: 2.12.0-RC1

[scabug]

@SethTisue said:
isolated and minimized:

trait Compute[A] {
  type Start
  val start: Compute[Start]
}

object Test {
  def foo[A](c: Compute[A]): Unit =
    c match {
      case c: Compute[A] =>
        new Compute[A] {
          type Start = c.Start
          val start = c.start
        }
    }
}

Perhaps it can be minimized further, but I got stuck here. In particular, I did not succeed in getting rid of the pattern match.

This almost certainly has to do with the changes in type inference for valdefs that came in scala/scala#5141. As evidence, compiling with 2.12.0-M5 makes the error go away, as does compiling with -Xsource:2.11.

[scabug]

@SethTisue said:
Another way to make this compile is:

-      case c: Compute[A] =>
+      case d: Compute[A] =>
+        val c = d

which certainly makes it seem like it's a pattern-matcher issue.

[scabug]

@SethTisue said (edited on Sep 21, 2016 10:21:46 PM UTC):
Here's a variant minimized form (eliminating a type parameter, but introducing a type variable):

trait Compute[A] {
  type Start
  val start: Compute[Start]
}

object Test {
  def foo(c: Any): Unit =
    c match {
      case c: Compute[a] =>
        new Compute[a] {
          type Start = c.Start
          val start = c.start
        }
    }
}

It's not that interestingly different, because in both versions, the type parameter on Compute isn't actually used anywhere. Regardless, as far as I can tell, you do need to have a type parameter in order to trigger the issue. (Throwing away the type member instead makes the issue vanish, too.)

[scabug]

@SethTisue said:
Most of the commits at https://github.com/scala/scala/pull/5141/commits are red so I can't quickly bisect to a particular commit without actually building those Scala versions. But that's definitely the PR it regressed in. The code compiles in aab103e but by f1cbe8a the error has started.

[scabug]

@adriaanm said:
Thanks for minimizing. This makes a pretty strong case that it's the val signature inference changes. I'm still away from my computer until Monday, but you could try with a def instead of a val and then it should behave the same across versions. Assigning a result type so that the inherited one is overridden wil be needed from now on (as in dotty).

[scabug]

@SethTisue said:

you could try with a def instead of a val and then it should behave the same across versions

With "def" you get the error in both 2.11.8 and 2.12.0-RC1.

Dotty accepts both the "def" and "val" versions, so this isn't a case where we can say "we're like Dotty now".

[scabug]

@adriaanm said:
Fascinating! As you know, it compiles ok on 2.11.8,.... except when you add -Xprint:typer! (In the example below I added -Ydebug for a better error message, but it's otherwise not involved.)

This indicates to me it used to compile due to a latent bug in the old handling of fields. It still doesn't explain why it compiles on Dotty. Still investigating that avenue.

➜  scala git:(2.12.0) ~/scala/scala-2.11.8/bin/scalac /tmp/Eval.scala -Xprint:typer -Ydebug

[...]

/tmp/Eval.scala:7: error: overriding value start in class Compute of type scala.this.Function0[<empty>.this.Eval[$anon.this.Start]];
 value start  has incompatible type;
 found   : => scala.this.Function0[<empty>.this.Eval[x2.Start]]
 required: => scala.this.Function0[<empty>.this.Eval[Compute.this.Start]]
          val start = c.start
              ^
one error found

sealed abstract class Eval[+A] {
  def flatMap[B](f: A => Eval[B]): Eval[B] =
    this match {
      case c: Eval.Compute[A] =>
        new Eval.Compute[B] {
          type Start = c.Start
          val start = c.start
        }
    }
}

object Eval {
  sealed abstract class Compute[A] extends Eval[A] {
    type Start
    val start: () => Eval[Start]
  }
}

[scabug]

@adriaanm said:
So, that confirms your initial assessment: that x2 is a local variable that's synthesized by the pattern matcher. As you showed, when you manually introduce a local variable, the bug goes away, which points to inconsistent substitution in the pattern matcher phase. Given all this, I'm inclined to push this to 2.12.1, as it's not really a new bug in 2.12.0, and it has a workaround.

[scabug]

Alistair Johnson (alistair.johnson-at-johnsonusm.com) said:
OK that this is pushed to 2.12.1, thanks for thorough investigation.

So just doing some house cleaning: I have added a workaround in this cats PR - could someone cast a quick eye that the fix is the best for now, please? Then I can leave this for a while...

Thx - Alistair

[scabug]

@SethTisue said:
I would suggest you comment your fix, with a link to this ticket, as it isn't obvious why the type annotations would be necessary. Otherwise it seems fine to me. Except that perhaps you might prefer the "val c = d" style fix from my comment above?

[scabug]

Alistair Johnson (alistair.johnson-at-johnsonusm.com) said:
Awesome, just what I was after - I'll add on next push, when the code is reviewed ;)

[retronym]

This is a cache invalidation bug in TypeRef.normalized.

Exception in thread "main" java.lang.AssertionError: assertion failed: (Base.this.Start,c.Start,x2.Start)

diff --git a/src/reflect/scala/reflect/internal/Types.scala b/src/reflect/scala/reflect/internal/Types.scala
index 97aea13c3b..ff1fe95311 100644
--- a/src/reflect/scala/reflect/internal/Types.scala
+++ b/src/reflect/scala/reflect/internal/Types.scala
@@ -2249 +2249 @@ trait Types
-        if (normalized eq null)
+        if (normalized eq null) {
@@ -2250,0 +2251,3 @@ trait Types
+        } else {
+          assert(normalized =:= normalizeImpl, (this, normalized, normalizeImpl))
+        }
@@ -2251,0 +2255 @@ trait Types
+

We have some code that tried to invalidate caches after substitution, but this example is a step ahead. Maybe we need to rethink this, perhaps we could disable these caches in symbols and typerefs that are local, which is where all the problems arise.

[retronym]

WIP for disabling caching for types formed over local symbols: https://github.com/scala/scala/compare/2.12.x...retronym:ticket/9931?expand=1

[SethTisue]

I've moved this from the 2.12.5 milestone to Backlog, but also: bump!, in case anyone wants to resume work on this.

[som-snytt]

Minimization works in 2.13.0, test in PR but I did not investigate.

[SethTisue]

@som-snytt thanks for all the triage just lately ❤️

[som-snytt]

I'm just the right age for triage. I was looking for an old ticket and found others instead.