Skip to content

Stabilizing definitions fixes#6312

Merged
lrytz merged 1 commit intoscala:2.13.xfrom
milessabin:topic/t10714
Mar 7, 2018
Merged

Stabilizing definitions fixes#6312
lrytz merged 1 commit intoscala:2.13.xfrom
milessabin:topic/t10714

Conversation

@milessabin
Copy link
Contributor

@milessabin milessabin commented Feb 6, 2018

Fixes scala/bug#10714. Also fixes scala/bug#10736.

#5999 introduces a synthetic val for the unstable LHS of an implicit method application where the method type depends on the LHS. Unfortunately this only handles the case where an expression requires a single stabilizing val: if the expression involves chained method applications, each requiring a stabilizing val, then only the last of these is emitted resulting in a failure during bytecode generation,

object Test {
  class Bar {
    class Foo {
      type Out
    }
    object Foo {
      implicit def foo: Foo { type Out = Bar } = ???
    }

    def foo(implicit foo: Foo): foo.Out = ???
  }

  (new Bar).foo.foo
}

results in a NoSuchElementException,

java.util.NoSuchElementException: value stabilizer$1
        at scala.collection.mutable.AnyRefMap$ExceptionDefault.apply(AnyRefMap.scala:425)
        at scala.collection.mutable.AnyRefMap$ExceptionDefault.apply(AnyRefMap.scala:424)
        at scala.collection.mutable.AnyRefMap.apply(AnyRefMap.scala:180)
        at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder$locals$.load(BCodeSkelBuilder.scala:392)
...

Prior to this PR only a single stabilizer is recorded on the expression tree which is overwritten by later ones. This is fixed here by allowing multiple stabilizers to be recorded and emitted.

In addition to that, previously spurious dependencies were picked up due to insufficient dealiasing. The following,

object Test {
  class Bar {
    class Foo {
      type Out
    }
    object Foo {
      implicit def foo: Foo { type Out = Bar } = ???
    }

    def foo(implicit foo: Foo): foo.Out = ???
  }

  (new Bar).foo.foo
}

triggered the preceding error despite not having a true dependency which would require a stabilizer in the first place. The test for a dependent argument now dealiases to eliminate these spurious dependencies.

Finally we avoid retyping stabilizing definitions when they are emitted, fixing scala/bug#10736.

@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Feb 6, 2018
@milessabin milessabin changed the title Record and emit multiple stabilizing definitions if needed [WIP] Record and emit multiple stabilizing definitions if needed Feb 7, 2018
@milessabin milessabin added the WIP label Feb 7, 2018
@milessabin
Copy link
Contributor Author

Demoted to WIP ... it's along the right lines, but needs more work.

@milessabin milessabin changed the title [WIP] Record and emit multiple stabilizing definitions if needed Record and emit multiple stabilizing definitions if needed Feb 14, 2018
@milessabin milessabin removed the WIP label Feb 14, 2018
@milessabin
Copy link
Contributor Author

Although the multiple stabilizer issue was real, in the actual scenario, shapeless's Zipper breaking, the argument type wasn't genuinely dependent ... the type member referenced was an alias of an independent type. Dealiasing appropriately in the test for a dependent argument fixes that.

This is ready for review now ...

@milessabin milessabin requested a review from adriaanm February 14, 2018 12:57
@milessabin
Copy link
Contributor Author

/synch

@milessabin
Copy link
Contributor Author

/rebuild

@milessabin
Copy link
Contributor Author

Rebased in the hope of unwedging Jenkins.

@adriaanm
Copy link
Contributor

/synch

1 similar comment
@milessabin
Copy link
Contributor Author

/synch

@milessabin
Copy link
Contributor Author

More synch did it!

@milessabin
Copy link
Contributor Author

Rebased.

scala#5999 introduces a synthetic val for
the unstable LHS of an implicit method application where the method type
depends on the LHS. Unfortunately this only handles the case where an
expression requires a _single_ stabilizing val: if the expression
involves chained method applications, each requiring a stabilizing val,
then only the last of these is emitted resulting in a failure during
bytecode generation,

object Test {
  class Bar {
    class Foo {
      type Out
    }
    object Foo {
      implicit def foo: Foo { type Out = Bar } = ???
    }

    def foo(implicit foo: Foo): foo.Out = ???
  }

  (new Bar).foo.foo
}

results in a NoSuchElementException,

java.util.NoSuchElementException: value stabilizer$1
        at scala.collection.mutable.AnyRefMap$ExceptionDefault.apply(AnyRefMap.scala:425)
        at scala.collection.mutable.AnyRefMap$ExceptionDefault.apply(AnyRefMap.scala:424)
        at scala.collection.mutable.AnyRefMap.apply(AnyRefMap.scala:180)
        at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder$locals$.load(BCodeSkelBuilder.scala:392)
...

Prior to this commit only a single stabilizer is recorded on the
expression tree which is overwritten by later ones. This is fixed here
by allowing multiple stabilizers to be recorded and emitted.

In addition to that, previously spurious dependencies were picked up due
to insufficient dealiasing. The following,

object Test {
  class Bar {
    class Foo {
      type Out
    }
    object Foo {
      implicit def foo: Foo { type Out = Bar } = ???
    }

    def foo(implicit foo: Foo): foo.Out = ???
  }

  (new Bar).foo.foo
}

triggered the preceding error despite not having a true dependency which
would require a stabilizer in the first place. The test for a dependent
argument now dealiases to eliminate these spurious dependencies.

Finally we avoid retyping stabilizing definitions when they are emitted,
fixing scala/bug#10736.
@milessabin milessabin changed the title Record and emit multiple stabilizing definitions if needed Stabilizing definitions fixes Mar 7, 2018
@soronpo
Copy link

soronpo commented Feb 10, 2019

I think this PR caused the following regression scala/bug#11397

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression in implicit resolution between 2.12 and 2.13 Only one stabilizing definition is emitted even when more than one required

5 participants