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

lambdalift crashes when Block in constructor-argument position requires outer() access #6666

Closed
scabug opened this issue Nov 14, 2012 · 23 comments
Closed
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Nov 14, 2012

As discussed in https://groups.google.com/d/topic/scala-internals/Ief8xGFd6fU/discussion , currently lambdalift crashes on:

class Foo(a: Any);

class IP extends Foo({
   def poorMansMH1 = ""
   object X {
     poorMansMH1	
   }
 }
)


Uncaught exception during compilation: scala.reflect.internal.FatalError
error: scala.reflect.internal.FatalError: 
     while compiling: sandbox/test.scala
        during phase: global=lambdalift, atPhase=constructors



no-symbol does not have an owner
at scala.reflect.internal.SymbolTable.abort(SymbolTable.scala:49)
at scala.tools.nsc.Global.abort(Global.scala:253)
at scala.reflect.internal.Symbols$NoSymbol.owner(Symbols.scala:3195)
at scala.tools.nsc.transform.ExplicitOuter$OuterPathTransformer.outerSelect(ExplicitOuter.scala:229)
at scala.tools.nsc.transform.ExplicitOuter$OuterPathTransformer.outerValue(ExplicitOuter.scala:215)
at scala.tools.nsc.transform.LambdaLift$LambdaLifter.memberRef(LambdaLift.scala:327)
at scala.tools.nsc.transform.LambdaLift$LambdaLifter.postTransform(LambdaLift.scala:472)

The motivation for this ticket is an alternative lowering for closures that is amenable to MethodHandles (both the real thing and so-called "Poor Man's Method Handles" , https://groups.google.com/d/topic/scala-internals/f7TektSb26s/discussion , ie the general rule is:

    /**  Transform a function node (x_1,...,x_n) => body of type FunctionN[T_1, .., T_N, R] to
     *
     *  {
     *    def hoistedMethodDef(x_1: T_1, ..., x_N: T_n): R = body
     *
     *    class $anon() extends AbstractFunctionN[T_1, .., T_N, R] with Serializable {
     *      def apply(x_1: T_1, ..., x_N: T_n): R = hoistedMethodDef(x_1, ..., x_N)
     *    }
     *
     *    new $anon()
     *  }
     *
     *  By 'hoisting' the closure-body out of the anon-closure-class, lambdalift and explicitouter
     *  are prompted to add formal-params to convey values captured from the lexical environment.
     *  This amounts to 'scalar replacement of aggregates' that cuts down on heap-hops over outer() methods.
     *  TODO In case the closure-body captures nothing, it need not be hoisted out of the closure.
     *
     */

Thus the following (longer) example is representative of a use case around "Poor Man's Method Handles"

// PoorMansMHs.scala
class Foo(val bar : () => String);

class IP extends {
  val baz = "bar";
} with Foo( {
  def poorMansMH1(): String = baz;

  @SerialVersionUID(0) final class anonfun extends scala.runtime.AbstractFunction0[String] with Serializable {
     def apply(): String = poorMansMH1()
  };

  new anonfun()
}
  );

object Test extends App{
  (new IP).bar();
}
@scabug
Copy link
Author

@scabug scabug commented Nov 14, 2012

@scabug
Copy link
Author

@scabug scabug commented Nov 14, 2012

@magarciaEPFL said (edited on Nov 14, 2012 3:34:17 PM UTC):
BTW, the alternative closure lowering (during uncurry) described above is needed:

  • both for JDK 8's MethodHandle , as well as
  • the "poor man's" version that doesn't require JDK 8.

Sidenote: plans for MethodHandles at http://lamp.epfl.ch/~magarcia/ScalaCompilerCornerReloaded/2012Q4/2012-11-04-FasterClosures.pdf

@scabug
Copy link
Author

@scabug scabug commented Nov 14, 2012

@paulp said:
This appears for all the world to be a duplicate of #2487.

@scabug
Copy link
Author

@scabug scabug commented Nov 14, 2012

@magarciaEPFL said:
A workaround: turn the Block-that-was-in-constructor-argument-position (say, "Block-B") into "{ val dummy = Block-B; dummy }" and compilation goes fine:

class Foo(a: Any);

class IP extends Foo( {
    val dummy = {
       def poorMansMH1 = ""
       object X {
         poorMansMH1
       }
     }

    dummy
  }
)

@scabug
Copy link
Author

@scabug scabug commented Nov 14, 2012

@paulp said:
I closed #2487 as the duplicate.

@scabug
Copy link
Author

@scabug scabug commented Dec 8, 2012

@retronym said:
The workaround fools lambdalift into generating invalid bytecode:

scala> class IP extends Foo( {
     |     val dummy = {
     |        def poorMansMH1 = println("!")
     |        object X {
     |          poorMansMH1
     |        }
     |        X
     |      }
     | 
     |     dummy
     |   }
     | )
defined class IP

scala> new IP
java.lang.VerifyError: (class: IP, method: <init> signature: ()V) Expecting to find object/array on stack

@scabug
Copy link
Author

@scabug scabug commented Dec 9, 2012

@retronym said (edited on Dec 9, 2012 9:26:01 PM UTC):
Maybe someone can shed a little light on the small patch of the problem I'm looking at.

The difference in behaviour between the original case and the case in which the object is nested in val dummy hinges on the INCONSTRUCTOR flag, which is calculated for templates as:

    def inConstructorFlag: Long =
      if (owner.isConstructor && !context.inConstructorSuffix || owner.isEarlyInitialized) INCONSTRUCTOR
      else 0l
class ClassSymbol {
 override def isClassLocalToConstructor = this hasFlag INCONSTRUCTOR
}

Perhaps not relevant to this problem, but for completeness, I'll note that for functions translated into anonymous classes, this flag is set in Uncurry

              if (dd.symbol.isClassConstructor) {
                atOwner(sym) {
                  val rhs1 = (rhs: @unchecked) match {
                    case Block(stats, expr) =>
                      def transformInConstructor(stat: Tree) =
                        withInConstructorFlag(INCONSTRUCTOR) { transform(stat) }
                      val presupers = treeInfo.preSuperFields(stats) map transformInConstructor
                      val rest = stats drop presupers.length
                      val supercalls = rest take 1 map transformInConstructor
                      val others = rest drop 1 map transform
                      treeCopy.Block(rhs, presupers ::: supercalls ::: others, transform(expr))
                  }
                  treeCopy.DefDef(
                    dd, mods, name, transformTypeDefs(tparams),
                    transformValDefss(vparamssNoRhs), transform(tpt), rhs1)
                }
              } else {
                super.transform(treeCopy.DefDef(dd, mods, name, tparams, vparamssNoRhs, tpt, rhs))
              }
...
          case Template(_, _, _) =>
            withInConstructorFlag(0) { super.transform(tree) }
...

val anonClass = fun.symbol.owner newAnonymousFunctionClass(fun.pos, inConstructorFlag) addAnnotation serialVersionUIDAnnotation

The accessor for this flag:

    // class C extends D( { class E { ... } ... } ). Here, E is a class local to a constructor
    def isClassLocalToConstructor = false

is only used in Symbol#outerClass:

    /** The class that is logically an outer class of given `clazz`.
     *  This is the enclosing class, except for classes defined locally to constructors,
     *  where it is the outer class of the enclosing class.
     */
    final def outerClass: Symbol =
      if (owner.isClass) owner
      else if (isClassLocalToConstructor) owner.enclClass.outerClass
      else owner.outerClass

Which is in turn only used in ExplicitOuter.

So, after all that, a question: should isClassLocalToConstructor also be true for class E in the following? (I'd say yes.)

class C extends D( { val x = { class E } })

If so, what about? (I'd say no.)

class C extends D( { object O { class E } })

And a trickier one:

class C extends D( { () => { class E } })
class C extends D( assert(true, /*by-name*/{ class E } )

@scabug
Copy link
Author

@scabug scabug commented Dec 9, 2012

@paulp said:
I won't try to answer, but presumably a complete definition of "defined locally to a constructor" would answer all those questions and many more. While we're at it, maybe we could motivate all this complexity a little better. Here's a thought:

implementation restriction: you can't declare classes in the midst of a supercall
scalac editorializes: come on, don't tell me this is so important you want us
picking through debris for INCONSTRUCTOR flag clues.

@scabug
Copy link
Author

@scabug scabug commented Dec 9, 2012

@retronym said (edited on Dec 9, 2012 10:27:36 PM UTC):
Yeah, I'd definitely like to solve these with restrictions. We have to watch out for classes that the compiler creates in there as well as ones the user does.

class C(a: Any)
object F {
  def byname(a: => Any) = println(a)
  def hof(a: () => Any) = println(a())
}

// java.lang.NullPointerException
//   at O1$$anonfun$$init$$1.apply(<console>:11)
//
// The thunk's apply method accesses the MODULE$
// field before it is set.
//
//   0: getstatic #23; //Field O1$.MODULE$:LO1$;
//   3: invokevirtual #26; //Method O1$.O1$$x$1:()Ljava/lang/String;
object O1 extends C({
  def x = "".toString
  F.byname(x)
})

// java.lang.NullPointerException
//   at O2$$anonfun$$init$$1.apply(<console>:11)
object O2 extends C({
  lazy val x = "".toString
  F.byname(x)
})

// java.lang.NullPointerException
//   at O3$$anonfun$$init$$1.apply(<console>:11)
object O3 extends C({
  def x = "".toString
  F.hof(() => x)
})

// java.lang.VerifyError: (class: O5$, method: <init> signature: ()V) Expecting to find object/array on stack
object O4 extends C({
  object Nested
  Nested
})


// Okay, the nested classes don't get an outer pointer passed,
// just an extra param for `x: String`.
object O6 extends C({
  val x = "".toString
  F.byname(x); F.hof(() => x); (new { val xx = x }.xx)
})


// no-symbol does not have an owner
//   ...
//   at scala.tools.nsc.transform.ExplicitOuter$OuterPathTransformer.outerSelect(ExplicitOuter.scala:229)
class C1 extends C({
  def x = "".toString
  F.byname(x)
})
class C2 extends C({
  lazy val x = "".toString
  F.byname(x)
})
class C3 extends C({
  def x = "".toString
  F.hof(() => x)
})
class C4 extends C({
  def x = "".toString
  object Nested { def xx = x}
  Nested.xx
})

// java.lang.VerifyError: (class: C5, method: <init> signature: ()V) Expecting to find object/array on stack
class C5 extends C({
  def x = "".toString
  val y = {
    object Nested { def xx = x}
    Nested.xx
  }
})

// okay, for same reason as O6
class C6 extends C({
  val x = "".toString
  F.byname(x); F.hof(() => x); (new { val xx = x }.xx)
})

@scabug
Copy link
Author

@scabug scabug commented Dec 10, 2012

@paulp said:
Whoa. Were there any doubt about whether this would be a worthwhile rathole to occupy, that should be pretty well extinguished.

@scabug
Copy link
Author

@scabug scabug commented Dec 10, 2012

@adriaanm said:
I agree this is worthwhile spot to draw the line. No more!
if we get a flood of complaints about this, we can look into it then
until that day, let's error nicely instead of crashing
we could even spec it (or have some kind of implementation limitations spec...)

Jason, as you're on top of this, assigning to you.
Miguel, is this essential to you in some way, or did you just run across it?

@scabug
Copy link
Author

@scabug scabug commented Dec 10, 2012

@retronym said (edited on Dec 10, 2012 7:13:15 AM UTC):
From what I understand, Miguel only needed this to rewrite closures in a more efficient manner. He could simply disable the optimization under this context. Or maybe our prohibition will mean he won't find anything that needs rewriting.

I might need a little help to find the best place to put the check. It has to be post-uncurry (to see expanded by-name thunks). explicitouter? lambdalift?

@scabug
Copy link
Author

@scabug scabug commented Dec 10, 2012

@magarciaEPFL said:
The proposed implementation restriction is completely reasonable.

The experimental optimizer can live without this bug being fixed, in fact it currently sidesteps the problematic case via:

  if((inConstructorFlag != 0)) {
    // checking inConstructorFlag prevents hitting SI-6666
    closureConversionTraditional(fun)
  }
  else {
    closureConversionMethodHandle(fun)
  }

@scabug
Copy link
Author

@scabug scabug commented Dec 15, 2012

@paulp said:
See also #6819.

@scabug
Copy link
Author

@scabug scabug commented Jan 10, 2013

@JamesIry said:
Declaring classes in super calls #6957

@scabug
Copy link
Author

@scabug scabug commented Jan 10, 2013

@retronym said:
Additional test case for #6957:

class Foo 
class Parent(f:Foo)
class Child extends Parent({val x=new Foo{}; x})

new Child

@scabug
Copy link
Author

@scabug scabug commented Jan 15, 2013

@retronym said:
Another one:

import scala.collection.immutable.TreeMap
import scala.math.Ordering

class Test[K](param:TreeMap[K,Int]){
    def this() = this({
      implicit object TreeOrd extends Ordering[K](){
        def compare(a:K, b:K) = {
          -1
        }
      }
      new TreeMap[K, Int]()
    })
}

@scabug
Copy link
Author

@scabug scabug commented Jan 20, 2013

@scabug
Copy link
Author

@scabug scabug commented Jan 23, 2013

@retronym said:
I've managed to coral a few more VerifyErrrors-in-waiting.

retronym/scala@scala:2.10.x...retronym:ticket/6666-wip

Along the way I've had to reopen #6259 (which was flagged, correctly, by the new checks.)

#6997 is not yet prevented.

@scabug
Copy link
Author

@scabug scabug commented Feb 8, 2013

@scabug
Copy link
Author

@scabug scabug commented May 20, 2013

@JamesIry said:
2.10.2 is about to be cut. Kicking down the road and un-assigning to foster work stealing.

1 similar comment
@scabug
Copy link
Author

@scabug scabug commented May 20, 2013

@JamesIry said:
2.10.2 is about to be cut. Kicking down the road and un-assigning to foster work stealing.

@scabug scabug closed this Sep 15, 2013
@scabug
Copy link
Author

@scabug scabug commented Sep 15, 2013

@retronym said:
Closing after collateral damage from this ticket was cleaned up in scala/scala#2884

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

Successfully merging a pull request may close this issue.

None yet
2 participants