Fix #2944: propagate information from GADT bounds to normal info. #2958

DarkDimius · 2017-08-04T11:43:10Z

This allows all other parts of pipeline to be less aware of existence
of the GADT bounds.

In particular, it makes us pickle them as normal type bounds.

DarkDimius · 2017-08-04T11:58:02Z

All tests passed!
@nicolasstucki, could you please check if it fixes the original test-case?
@odersky please review the code.

DarkDimius · 2017-08-04T12:00:14Z

@nicolasstucki, i've added a test file for #2944 but it just tests compilation, which produced wrong tasty before this fix.

nicolasstucki · 2017-08-04T12:08:00Z

tests/pos/i2944.scala

+  def get(k: K): K = k
+  def foo: K = {
+    this match {
+      case that: Map2[b] => that.get(3.asInstanceOf[b])


This test case passes my the check with my tests 👍

Do we have a real test for this now?

nicolasstucki · 2017-08-04T12:08:47Z

tests/pos/i2944.scala

+  def foo: K = {
+    this match {
+      case that: Map2[b] => that.get(3.asInstanceOf[b])
+      //case that: Map2[c] => that.get(4.asInstanceOf[K])


This one still crashes while trying to type that.get(4.asInstanceOf[K].asInstanceOf[b @ b]).

smarter · 2017-08-04T12:41:52Z

compiler/src/dotty/tools/dotc/transform/PostTyper.scala


  import tpd._

+  def transformSym(sym: SymDenotation)(implicit ctx: Context): SymDenotation = {
+    if (sym.is(BindDefinedType) && ctx.gadt.bounds.contains(sym.symbol)) {
+      sym.copySymDenotation(info = ctx.gadt.bounds.apply(sym.symbol) & sym.info)


you could probably remove sym.symbol from the ctx.gadt.bounds map at that point too.

I still think this is possible but ¯\_(ツ)_/¯

smarter · 2017-08-04T12:42:23Z

compiler/src/dotty/tools/dotc/transform/PostTyper.scala


  import tpd._

+  def transformSym(sym: SymDenotation)(implicit ctx: Context): SymDenotation = {


Nitpick: sym is usually used for symbols, for a denotation I would use ref or d.

DarkDimius · 2017-08-04T12:45:22Z

You can't. You'll need to remove it from top context

…

On 4 August 2017 2:41:58 pm Guillaume Martres ***@***.***> wrote: smarter commented on this pull request. > import tpd._ + def transformSym(sym: SymDenotation)(implicit ctx: Context): SymDenotation = { + if (sym.is(BindDefinedType) && ctx.gadt.bounds.contains(sym.symbol)) { + sym.copySymDenotation(info = ctx.gadt.bounds.apply(sym.symbol) & sym.info) you could probably remove `sym.symbol` from the `ctx.gadt.bounds` map at that point too. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #2958 (review)

DarkDimius · 2017-08-04T12:46:22Z

Name sym is used consistently in all SymTransforms. See original trait.

…

On 4 August 2017 2:42:30 pm Guillaume Martres ***@***.***> wrote: smarter commented on this pull request. > import tpd._ + def transformSym(sym: SymDenotation)(implicit ctx: Context): SymDenotation = { Nitpick: `sym` is usually used for symbols, for a denotation I would use `ref` or `d`. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #2958 (review)

smarter · 2017-08-04T12:49:19Z

Name sym is used consistently in all SymTransforms. See original trait.

% git grep "def transformSym" compiler
compiler/src/dotty/tools/dotc/core/DenotTransformers.scala:    def transformSym(sym: SymDenotation)(implicit ctx: Context): SymDenotation
compiler/src/dotty/tools/dotc/transform/Flatten.scala:  def transformSym(ref: SymDenotation)(implicit ctx: Context) = {
compiler/src/dotty/tools/dotc/transform/Getters.scala:  override def transformSym(d: SymDenotation)(implicit ctx: Context): SymDenotation = {
compiler/src/dotty/tools/dotc/transform/Mixin.scala:  override def transformSym(sym: SymDenotation)(implicit ctx: Context): SymDenotation =
compiler/src/dotty/tools/dotc/transform/MoveStatics.scala:  def transformSym(sym: SymDenotation)(implicit ctx: Context): SymDenotation = {
compiler/src/dotty/tools/dotc/transform/NormalizeFlags.scala:  def transformSym(ref: SymDenotation)(implicit ctx: Context) = {
compiler/src/dotty/tools/dotc/transform/PrivateToStatic.scala.disabled:  override def transformSym(sd: SymDenotation)(implicit ctx: Context): SymDenotation =
compiler/src/dotty/tools/dotc/transform/TreeChecker.scala:  def transformSym(symd: SymDenotation)(implicit ctx: Context): SymDenotation = {

I think everyone is confused about what name to use :).

DarkDimius · 2017-08-04T12:51:05Z

This one still crashes while trying to type that.get(4.asInstanceOf[K].asInstanceOf[b @ b]).

Nice) will look into it! Curious why the bind node is inside of cast...

smarter · 2017-08-04T12:52:02Z

You can't. You'll need to remove it from top context

Why? When we create a fresh context we keep a reference to the same GADTMap in _gadt.

DarkDimius · 2017-08-04T14:12:37Z

This one still crashes while trying to type that.get(4.asInstanceOf[K].asInstanceOf[b @ b]).

@nicolasstucki , though I authored this testcase, I forgot how to make this bind appear.
The tasty looks correct. The error happens on reading. How did you force reading it?

DarkDimius · 2017-08-04T14:13:39Z

Ok, found a bug by reading code. TreeUnpickeler.readTpt is obviously wrong.

nicolasstucki · 2017-08-04T14:33:26Z

@DarkDimius you can reproduce it easily for this branch https://github.com/dotty-staging/dotty/tree/link-from-tasty-show-bug. Just run the reproducefailure.sh script.

nicolasstucki · 2017-08-04T14:35:15Z

That branch contains the current fix.

DarkDimius · 2017-08-04T14:42:17Z

@nicolasstucki, could you please validate that the last commit fixes the issue?

nicolasstucki · 2017-08-04T14:52:46Z

It did not fix it, but that is clearly the source of the issue. Now I get:

exception while typing c of class class dotty.tools.dotc.ast.Trees$Ident # 401946
exception while typing 4.asInstanceOf[K].asInstanceOf[c] of class class dotty.tools.dotc.ast.Trees$TypeApply # 401947
exception while typing that.get(4.asInstanceOf[K].asInstanceOf[c]) of class class dotty.tools.dotc.ast.Trees$Apply # 402179

Note that you did replace the binding by the Ident but it still can't pas Ycheck.

DarkDimius · 2017-08-04T14:56:32Z

@nicolasstucki, sorry. I did a stupid mistake :-) Fix incomming.

DarkDimius · 2017-08-04T14:59:43Z

Oh... it is actually much worse: on unpickling, c becomes a term.

DarkDimius · 2017-08-04T15:03:20Z

aaaand we create symbol for it twice on unpickling. And those two symbols are obviously incompatible between each other.

DarkDimius · 2017-08-04T15:18:42Z

I propose we postpone fixing the remaining bug till next dotty meeting. My proposed fix is to revert 79e2228 and ad6b578. Those commits made bind be handled as-if it was TypeTree in Tasty. This broke a lot of stuff because unpickler assumes that it can read TypeTrees repeatedly, which is not true for bind, which is a DefTree. We assume in multiple locations that DefTree should be only read once, as you'll get a fresh symbol every time you read it.

DarkDimius · 2017-08-04T15:19:00Z

@nicolasstucki, is the remaining bug also a blocker?

nicolasstucki · 2017-08-04T15:23:37Z

@DarkDimius ~~~the fix in the first commit unblocked it~~~. The second could be left for later, lets open a separate issue for it. Thanks.

nicolasstucki · 2017-08-04T16:18:00Z

Unfortunately the the first fix did not work in the original code from strawman.collection.Map.equals.

I minimised the issue again to

trait Map2[K] {
  def get(k: K): K = k
  def foo = {
    val o = new Object 
    o match { // now match on Object and not this
      case that: Map2[b] => that.get(3.asInstanceOf[b])
      case _ => get(5.asInstanceOf[K])
    }
  }
}

DarkDimius · 2017-08-07T08:38:35Z

@nicolasstucki and I have figured that this is another manifestation of #2958 (comment)

This allows all other parts of pipeline to be less aware of existence of the GADT bounds. In particular, it makes us pickle them as normal type bounds.

binds are "shared" as type trees would be shared, see scala#2510 But they are not type trees, as you can't have them inside TypeApply. See i2944a.scala.

This reverts commit 30ec34f.

nicolasstucki · 2017-09-21T07:56:22Z

Rebased.

@smarter could you re-review this quickly before merging.

liufengyun · 2017-09-25T12:39:05Z

test performance please

This PR fails the compilation of https://github.com/liufengyun/scalaPB on the bench server.

dottybot · 2017-09-25T12:39:29Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2017-09-25T13:42:53Z

performance test failed:

[check /data/workspace/bench/logs/pull-2958-09-25-14.39.out for more information]

DarkDimius requested review from odersky and nicolasstucki August 4, 2017 11:58

nicolasstucki reviewed Aug 4, 2017

View reviewed changes

smarter reviewed Aug 4, 2017

View reviewed changes

nicolasstucki mentioned this pull request Aug 4, 2017

Link from tasty #2910

Merged

nicolasstucki added the stat:needs review label Aug 10, 2017

Fix scala#2944: propagate information from GADT bounds to normal info.

099b91b

This allows all other parts of pipeline to be less aware of existence of the GADT bounds. In particular, it makes us pickle them as normal type bounds.

DarkDimius and others added 4 commits September 21, 2017 09:54

Add testfile for scala#2944.

bfa3d69

Fix Binds being read from tasty as if they were type trees.

c45a3bc

binds are "shared" as type trees would be shared, see scala#2510 But they are not type trees, as you can't have them inside TypeApply. See i2944a.scala.

Revert "Fix Binds being read from tasty as if they were type trees."

0139673

This reverts commit 30ec34f.

Change argument name

1c0f63f

nicolasstucki force-pushed the fix-2944 branch from 0a5e873 to 1c0f63f Compare September 21, 2017 07:54

nicolasstucki approved these changes Sep 21, 2017

View reviewed changes

smarter approved these changes Sep 21, 2017

View reviewed changes

nicolasstucki merged commit 0802abd into scala:master Sep 21, 2017

smarter mentioned this pull request Nov 17, 2017

Type binding cannot be read from TASTY #3500

Closed

allanrenucci deleted the fix-2944 branch December 14, 2017 19:19


		import tpd._

		def transformSym(sym: SymDenotation)(implicit ctx: Context): SymDenotation = {

Fix #2944: propagate information from GADT bounds to normal info. #2958

Fix #2944: propagate information from GADT bounds to normal info. #2958

Uh oh!

Conversation

DarkDimius commented Aug 4, 2017

Uh oh!

DarkDimius commented Aug 4, 2017

Uh oh!

DarkDimius commented Aug 4, 2017

Uh oh!

nicolasstucki Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

nicolasstucki Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarter Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

DarkDimius commented Aug 4, 2017 via email

Uh oh!

DarkDimius commented Aug 4, 2017 via email

Uh oh!

smarter commented Aug 4, 2017

Uh oh!

DarkDimius commented Aug 4, 2017

Uh oh!

smarter commented Aug 4, 2017

Uh oh!

DarkDimius commented Aug 4, 2017

Uh oh!

DarkDimius commented Aug 4, 2017

Uh oh!

nicolasstucki commented Aug 4, 2017

Uh oh!

nicolasstucki commented Aug 4, 2017

Uh oh!

DarkDimius commented Aug 4, 2017

Uh oh!

nicolasstucki commented Aug 4, 2017

Uh oh!

DarkDimius commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DarkDimius commented Aug 4, 2017

Uh oh!

DarkDimius commented Aug 4, 2017

Uh oh!

DarkDimius commented Aug 4, 2017

Uh oh!

DarkDimius commented Aug 4, 2017

Uh oh!

nicolasstucki commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasstucki commented Aug 4, 2017

Uh oh!

DarkDimius commented Aug 7, 2017

Uh oh!

nicolasstucki commented Sep 21, 2017

Uh oh!

liufengyun commented Sep 25, 2017

Uh oh!

dottybot commented Sep 25, 2017

Uh oh!

dottybot commented Sep 25, 2017

Uh oh!

Uh oh!

nicolasstucki Aug 4, 2017 •

edited

Loading

DarkDimius commented Aug 4, 2017 •

edited

Loading

nicolasstucki commented Aug 4, 2017 •

edited

Loading