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

Error in code execution order, possibly related to stabilization issues #11397

Closed
soronpo opened this issue Feb 10, 2019 · 14 comments · Fixed by scala/scala#8202
Closed

Error in code execution order, possibly related to stabilization issues #11397

soronpo opened this issue Feb 10, 2019 · 14 comments · Fixed by scala/scala#8202
Assignees
Milestone

Comments

@soronpo
Copy link

soronpo commented Feb 10, 2019

Possibly related to #10714 which was fixed in scala/scala#6312

The following code demonstrates an error in code execution order.
An assertion placed show exactly where the error occurs.

trait MyAny {
  type TSomething
  type This <: MyAny
}

class MyBool extends MyAny {
  type TSomething = DummyImplicit
  type This = MyBool
}

object Test extends App {
  val i = new MyBool
  var call = 0

  final class MyMatch[MV <: MyAny](val matchVal : MV)  {
    def myCase[MC](pattern : Boolean)(block : => Unit)(
      implicit patternBld : matchVal.TSomething
    ) : MyMatch[MV] = {
      call -= 1
      block
      this
    }
  }

  def myMatch[MV <: MyAny](matchValue : MV) : MyMatch[matchValue.This] = {
    assert(call == 0)
    call += 1
    new MyMatch[matchValue.This](matchValue.asInstanceOf[matchValue.This])
  }

  myMatch(i)
    .myCase(true) {
      myMatch(i)
        .myCase(true) {

        }
    }
}

Affected Scalac version: from 2.13.0-M4
2.13.0-M2 is OK. 2.13.0-M3 causes a crash as demonstrated in #10714
The reason I'm thinking it's related to the stabilization bug is that if we change the code to the following, there is no error:

  myMatch(i)
    .myCase(true) {
      val noErrorNow = myMatch(i)
        .myCase(true) {

        }
    }
@adriaanm
Copy link
Contributor

@milessabin could you investigate whether we need to fix this in RC1?

@milessabin
Copy link

I'll take a look.

@milessabin
Copy link

@adriaanm I'm afraid I'm not going to have time to dig in to this for a week or two.

@adriaanm adriaanm modified the milestones: 2.13.0-RC1, 2.13.1 Feb 13, 2019
@dwijnand dwijnand changed the title 2.13 Regression. Possibly related to stabilization issues Error in code execution order, possibly related to stabilization issues Jul 2, 2019
@som-snytt
Copy link

I'm taking a quick look today (Tuesday) before my commute. I'll delete this post when I am positively stymied. I thought I had a half-line fix, so now I kind of want to figure it out.

There was a huge backup on the 85N this morning, which I saw from the 85S while chauffeuring to morning school. So my next commute is really "second commute" as we say "second breakfast".

@SethTisue
Copy link
Member

tentatively labeling as "blocker", pending investigation

@retronym
Copy link
Member

retronym commented Jul 3, 2019

The bug lies in:

https://github.com/scala/scala/blob/def82ba2f956a30d55fa428d4fb567b39d9e822b/src/compiler/scala/tools/nsc/typechecker/Typers.scala#L5191-L5201

.. which chooses to put val stabilizer2 in an enclosing context which changes execution order.

    {
      <synthetic> <stable> <artifact> val stabilizer$1: Test.MyMatch[Test.i.This] @scala.reflect.internal.annotations.uncheckedBounds = Test.this.myMatch[MyBool](Test.this.i);
      <synthetic> <stable> <artifact> val stabilizer$2: Test.MyMatch[Test.i.This] @scala.reflect.internal.annotations.uncheckedBounds = Test.this.myMatch[MyBool](Test.this.i);
      stabilizer$1.myCase[Nothing](true)({
        stabilizer$2.myCase[Nothing](true)(())(scala.this.DummyImplicit.dummyImplicit);
        ()
      })(scala.this.DummyImplicit.dummyImplicit)
    }

Synthetic stabilizing vals are only added when the called method is a dependent method type. Still a blocker, but limited somewhat in it scope.

@som-snytt
Copy link

som-snytt commented Jul 3, 2019

Yes, my one-liner was in the code finding the "insertion context", it should look for the Block. I'll take another look before I'm down for the night. But maybe this has to be fixed right now Australia time because it's a contract issue.

It's quite ominous that there is a link to "The Future of Typelevel Scala". (dum, da-dum)

@milessabin
Copy link

I'm looking at this now ...

@milessabin
Copy link

@retronym is spot on ... stabilizing vals shouldn't be pulled out over by name argument positions.

@som-snytt
Copy link

@milessabin Me too. If you have better things to do. I'm just back from chauffeuring the child...

@milessabin
Copy link

@som-snytt please take it if you want it :-)

@som-snytt
Copy link

I don't want it, but maybe I need it.

Maybe I got it wrong, using half the brain, the way dolphins sleep without drowning.

scala/scala#8202

@som-snytt
Copy link

Worth adding that the workaround is to turn the arg expression into a block.

  myMatch(i)
    .myCase(true) {
      val x = 42
      myMatch(j)
        .myCase(true) {

        }
    }

@soronpo
Copy link
Author

soronpo commented Oct 2, 2019

It turns out the general problem was not completely fixed. Opened #11756

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

Successfully merging a pull request may close this issue.

7 participants