Avoid mismatched symbols in fields phase

[adriaanm]

The info of the var that stores a trait's lazy val's computed value
is expressed in terms of symbols that exist before the fields phase.
When we're implementing the lazy val in a subclass of that trait,
we now see symbols created by the fields phase, which results
in mismatches between the types of the lhs and rhs in the assignment
of lazyVar = super.lazyImpl.

So, type check the super-call to the trait's lazy accessor before our
own phase. If the lazy var's info depends on a val that is now
implemented by an accessor synthesize by our info transformer,
we'll get a mismatch when assigning rhs to lazyVarOf(getter),
unless we also run before our own phase (like when we were
creating the info for the lazy var).

This was revealed by Hanns Holger Rutz's efforts in compiling
scala-refactoring's test suite (reported on scala-internals).

Fixes scala/scala-dev#219

[kiritsuku]

Thanks, this fixes the compilation error in scala-refactoring

[retronym]

This works in 2.11 because under erased typechecking, the mismatched global symbols in the prefix aren't checked.

I used this example to understand things a bit better.

trait G { trait N }
trait T {
  val g: G = null
  def yo(p: g.type): String = "yo"
}

abstract class C extends T {
  override val g: G = null
  yo(g) // allowed
  super.g : g.type // not allowed
}

An alternative fix might be to emit a cast. Not sure which is the lesser evil (cast vs typechecking code in the previous phase)

[adriaanm]

My reasoning in favor of type checking in the previous phase was that these
symbols in the subclass are synthetic and thus it's fine (better) if we do
not seen them while type checking the accessors.

SI-9918 is hopefully another instance of this. I was thinking to push down
the afterOwnPhase of looking at the info and doing all the type checking in
the previous phase.
On Fri, Sep 9, 2016 at 01:26 Jason Zaugg notifications@github.com wrote:

This works in 2.11 because under erased typechecking, the mismatched
global symbols in the prefix aren't checked.

I used this example to understand things a bit better.

trait G { trait N }
trait T {
val g: G = null
def yo(p: g.type): String = "yo"
}

abstract class C extends T {
override val g: G = null
yo(g) // allowed
super.g : g.type // not allowed
}

An alternative fix might be to emit a cast. Not sure which is the lesser
evil (cast vs typechecking code in the previous phase)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#5388 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFjy41ej6NoZ4GV-cHYgQJg8dciza4Kks5qoJmZgaJpZM4J4MQp
.

[milessabin]

I'm seeing a huge increase in compile times for the shapeless tests (once I exclude the ones which blow up in codegen) relative to 2.12.0-M5 (about 4x) ... could the additional typechecking here be (partly) responsible for that?

[adriaanm]

It may be -- would be good to see a -verbose run on M5 and RC1 and compare time spent in the phases after uncurry.

[adriaanm]

I've pushed a commit that implements the cast-based approach. I agree this is more robust.

[adriaanm]

Looks like I was too eager with some of those casts.

[adriaanm]

Ah, no I was running afoul of the asserts in mkCast that needn't hold during the fields phase. Did a quick spot check that the casts are dropped in the backend, but nothing large scale.

[adriaanm]

Since this bug would've been caught by compiling (if not running) scala-refactoring's test suite, could you make a sweep through the community build and see if we can at least compile those test suites that we cannot currently run, @SethTisue?

[retronym]

Good description of the problem.

[SethTisue]

see if we can at least compile those test suites that we cannot currently run, @SethTisue?

scala/community-build#280

[lrytz]

this seems to introduce quite a number of casts, it looks like rather big hammer. maybe there's a potential that it hides latent issues (now or in the future). we should also check if these casts are eliminated in erasure, or if some remain.

[adriaanm]

Since this type checked without the cast for the whole community build
until we hit the glitch in the test case, the cast should get dropped by
erasure (or genbcode?). The only reason it doesn't type check before
erasure is explained in the PR. I don't feel strongly about using the
cast-based approach or not. It's probably slightly more robust than the
previous commit, but you're right we should check the byte code. I did a
spot check but not the usual full diff of a bootstrap.

On Wed, Sep 14, 2016 at 17:24 Lukas Rytz notifications@github.com wrote:

In src/compiler/scala/tools/nsc/transform/Fields.scala
#5388 (comment):

•    // This is the result of overriding a val with a def, so that no field is found in the subclass.
  

•    if (field.exists) List(Select(This(clazz), field))
  

•    else Nil
  

- }

•  def getterBody(getter: Symbol): List[Tree] = {
  

• def fieldsAndAccessors(clazz: Symbol): List[Tree] = {

•  // scala/scala-dev#219
  

•  // Cast to avoid spurious mismatch in paths containing trait vals that have
  

•  // not been rebound to accessors in the subclass we're in now.
  

•  // For example, for a lazy val mixed into a class, the lazy var's info
  

•  // will not refer to symbols created during our info transformer,
  

•  // so if its type depends on a val that is now implemented after the info transformer,
  

•  // we'll get a mismatch when assigning `rhs` to `lazyVarOf(getter)`.
  

•  // TODO: could we rebind more aggressively? consider overriding in type equality?
  

•  def cast(tree: Tree, pt: Type) = gen.mkAsInstanceOf(tree, pt)
  

this seems to introduce quite a number of casts, it looks like rather big
hammer. maybe there's a potential that it hides latent issues (now or in
the future). we should also check if these casts are eliminated in erasure,
or if some remain..

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/scala/scala/pull/5388/files/4d67c390d989e8ecf3b3916465dea1b0b70fde57..8b79ea9415ac99ebabe29577cd83ec5081d2f488#r78771687,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFjy7pGI36xDeA6XkLcovHeRQrZzn1dks5qqBHGgaJpZM4J4MQp
.

[adriaanm]

The diff of decompiled bytecode for lib/ref/cmp is zero according to https://gist.github.com/adriaanm/55100b674705f88a460f857278aad370

[adriaanm]

rebased

[retronym]

LGTM. I think emitting the cast and letting erasure decide if its needed is the easiest patch to reason about. We could revisit this later on when we've got some more time to think about the nuances of the approach in the first commit.