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

Possible Regression in 2.12.0-RC1 inferencing type annotation #9931

Closed
scabug opened this issue Sep 19, 2016 · 18 comments · Fixed by scala/scala#10170
Closed

Possible Regression in 2.12.0-RC1 inferencing type annotation #9931

scabug opened this issue Sep 19, 2016 · 18 comments · Fixed by scala/scala#10170
Labels
Milestone

Comments

@scabug
Copy link

scabug commented Sep 19, 2016

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
Copy link
Author

scabug commented Sep 19, 2016

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
Copy link
Author

scabug commented Sep 21, 2016

@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
Copy link
Author

scabug commented Sep 21, 2016

@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
Copy link
Author

scabug commented Sep 21, 2016

@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
Copy link
Author

scabug commented Sep 21, 2016

@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
Copy link
Author

scabug commented Sep 22, 2016

@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
Copy link
Author

scabug commented Sep 22, 2016

@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
Copy link
Author

scabug commented Sep 26, 2016

@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
Copy link
Author

scabug commented Sep 26, 2016

@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
Copy link
Author

scabug commented Sep 29, 2016

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
Copy link
Author

scabug commented Sep 29, 2016

@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
Copy link
Author

scabug commented Sep 29, 2016

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 ;)

@scabug scabug added this to the 2.12.3 milestone Apr 7, 2017
@retronym retronym modified the milestones: 2.12.3, 2.12.4 Jul 8, 2017
@retronym
Copy link
Member

retronym commented Jul 8, 2017

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 retronym self-assigned this Jul 8, 2017
@retronym
Copy link
Member

retronym commented Jul 9, 2017

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

@adriaanm adriaanm modified the milestones: 2.12.4, 2.12.5 Oct 10, 2017
@SethTisue SethTisue modified the milestones: 2.12.5, Backlog Mar 1, 2018
@SethTisue
Copy link
Member

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

@adriaanm adriaanm removed their assignment Sep 28, 2018
@som-snytt
Copy link

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

@SethTisue SethTisue modified the milestones: Backlog, 2.13.0 Sep 29, 2022
@SethTisue
Copy link
Member

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

@som-snytt
Copy link

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

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

Successfully merging a pull request may close this issue.

5 participants