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

Fix #1990: Handle inlining where this proxies change types #1996

Merged
merged 5 commits into from Feb 21, 2017

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Feb 18, 2017

The new situation in the test was that outer of the inlined method
was A but its as-seen-from type is a subtype B. Outer path logic
needs to be adapted to that case.

The new situation in the test was that outer of the inlined method
was `A` but it's as seen from type is a subtype `B`.

We need two fixes:

 - Ignore outerSelects in TreeChecker. These are treated as having fixed symbols.
 - Adapt the outer-path logic to deal with code that's moved to another context.
This shows that the builder pattern can be expressed
with implicit function types.
def path(toCls: Symbol, start: Tree = This(ctx.owner.lexicallyEnclosingClass.asClass)): Tree = try {
def path(toCls: Symbol,
start: Tree = This(ctx.owner.lexicallyEnclosingClass.asClass),
outOfContext: Boolean = true): Tree = try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this parameter is always set to true, is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that was an oversight.

Interesting that the tests pass even if we always assume outOfContext = true.
So this raises the question why have a flag? It's just that I am not sure the
`outOfContext` behavior is correct in all cases. So I prefer to be conservative
here.
* the context owner's this node.
* node `start`, which defaults to the context owner's this node.
* @param outOfContext When true, we take the `path` in code that has been inlined
* from somewhere else. In that case, we need to stop not
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we always have outOfContext = true in transformSelect, not just in code that has been inlined.
Shouldn't the condition be something like !tpd.enclosingInlineds.isEmpty instead?

Copy link
Contributor Author

@odersky odersky Feb 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that C_<OUTER> nodes are only created for inline proxies. The code in question only applies to these.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I missed that, makes sense now.

@smarter
Copy link
Member

smarter commented Feb 18, 2017

I found the fix a bit suspicious so I played a bit with the testcase and got it to crash again:

class A { self =>
  class Foo {
    inline def inlineMeth: Unit = {
      println(self)
    }
  }
}

class C extends A {
  class B extends A
}

object Test {
  def main(args: Array[String]): Unit = {
    val c = new C
    val b = new c.B

    (new b.Foo).inlineMeth
  }
}

Stack trace: https://gist.github.com/smarter/75ee9b3da1ffe127721bb93535bd8483

They are sorted according to the nesting depth of the classes they
represent. This is no necessarily the same as the nesting level of
the symbols of the proxy classes. i1990a.scala shows an example where
the two differ.
@smarter
Copy link
Member

smarter commented Feb 19, 2017

Progress! Now I can only get it to crash at runtime, I apologize for the devious code :).

trait A { self =>
  class Foo {
    inline def inlineMeth: Unit = {
      println(self)
    }
  }
}

class A2 extends A {
  class C extends Foo with A

  val c = new C
  (new c.Foo).inlineMeth
}

object Test {
  def main(args: Array[String]): Unit = {
    new A2
  }
}
$ dotr Test
Exception in thread "main" java.lang.ClassCastException: A$Foo cannot be cast to A2$C
        at A2.<init>(i1990a.scala:13)
        at Test$.main(i1990a.scala:18)
        at Test.main(i1990a.scala)

The crash happens because the outer path stops too early:

        val Foo_this: A2.this.c.Foo = new A2.this.c.Foo()
        val A_this: A2.this.C(A2.this.c) = 
          Foo_this.asInstanceOf[A2.this.C(A2.this.c)]

If I replace class C extends Foo with A by class C extends A, the path is correct:

        val Foo_this: A2.this.c.Foo = new A2.this.c.Foo()
        val A_this: A2.this.C(A2.this.c) = 
          Foo_this.A$Foo$$$outer.asInstanceOf[A2.this.C(A2.this.c)]

@DarkDimius
Copy link
Member

DarkDimius commented Feb 19, 2017

Those bugs are very similar from ones that we had in linker.
here's another test for you to try:

trait A { self =>
  type T
  val field: T
  def bar(f: T): T
  class Foo {
    inline def inlineMeth: T = {
      bar(field)
    }
  }
}

class A2 extends A {
  type T = Int
  val field = 1
  override def bar(a: Int) = a + 1
  class C extends Foo with A

  val c = new C
  val ret = bar((new c.Foo).inlineMeth) // how many boxings? 
  val resI: Int = ret
}

object Test {
  def main(args: Array[String]): Unit = {
    new A2
  }
}

@odersky
Copy link
Contributor Author

odersky commented Feb 19, 2017

@DarkDimius That test does not typecheck. Is that intentional?

It turns out that we simply cannot do reliable outer path
computation that fills in the right hand sides of this-proxies
from the types of these proxies. As-seen-from logic can mangle
the types of proxies enough to scramble the necessary information.

What we now do instead is simply count: We record the number
of outer accesses to an outer this in inlineable code, and do the same number
of outer accesses when computing the proxy.
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@odersky odersky merged commit 9f879c1 into scala:master Feb 21, 2017
@allanrenucci allanrenucci deleted the fix-#1990 branch December 14, 2017 19:18
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.

None yet

3 participants