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

Nested methods capture outer instance when lifted #9390

Closed
scabug opened this issue Jul 10, 2015 · 11 comments

Comments

Projects
None yet
2 participants
@scabug
Copy link

commented Jul 10, 2015

In this unit test, methodLift fails, while asFunction passes.

@Test
def methodLift {
    def isPrime(c: Int) = BigInt(c).isProbablePrime(1)
  assertNoOuter(isPrime)
}
@Test
def asFunction {
  val isPrime = (c: Int) => BigInt(c).isProbablePrime(1)
  assertNoOuter(isPrime)
}
private def assertNoOuter(f: Int => Boolean) {
  assertFalse(f.getClass.getDeclaredFields.exists(_.getType == this.getClass))
}
@scabug

This comment has been minimized.

Copy link
Author

commented Jul 10, 2015

Imported From: https://issues.scala-lang.org/browse/SI-9390?orig=1
Reporter: @nilskp
Affected Versions: 2.11.7
See #4581, #4440, #9408, #1419

@scabug

This comment has been minimized.

Copy link
Author

commented Jul 15, 2015

@nilskp said:
I haven't checked if this is also present in 2.12.

@scabug

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

@lrytz said (edited on Jul 24, 2015 8:14:42 AM UTC):
This is actually not a bug. The reason is that the lambda body of the eta-expansion of isPrime invokes the isPrime method, which is an instance method, so the lambda needs to capture the C instance.

The class

class C {
  def methodLift = {
    def isPrime(c: Int) = BigInt(c).isProbablePrime(1)
    val f: Int => Boolean = isPrime
    f(0)
  }
}

compiles to (with delambdafy:method, indylambda)

class C {
  def anonfun$1(i: Int) = this.isPrime$1(i)
  def isPrime$1(i: Int) = BigInt(c).isProbablePrime(1)
  def methodLift = {
    // lambda captures `this`, in order to be able to invoke the instance method `anonfun$1`
    val f = indyLambda(this, MethodHandle(anonfun$1))
    f.apply(0)
  }
}
@scabug

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

@lrytz said:
Maybe the optimizer could fix this in some cases: if we inline isPrime$1 into anonfun$1 then we can get rid of the captured value. Logged here: https://github.com/scala-opt/scala/issues/34

@scabug

This comment has been minimized.

Copy link
Author

commented Jul 24, 2015

@lrytz said:
In fact, the lifted isPrime$1 method could be made static, or moved to the companion object, that would also fix the issue. Somewhat related to #4581.

@scabug

This comment has been minimized.

Copy link
Author

commented Jul 25, 2015

@nilskp said:
I just tried to @inline the def, but unfortunately that didn't work either. Not sure if it should or not.

@scabug

This comment has been minimized.

Copy link
Author

commented Jul 25, 2015

@lrytz said (edited on Jul 25, 2015 5:45:38 PM UTC):
at the optimizer phase, where inlining is performed, the outer reference is already there. so even if the method is inlined, we need an additional optimization to eliminate unused outer references, which is not implemented, neither in the 2.11 optimizer nor in the new one for 2.12.

@scabug

This comment has been minimized.

Copy link
Author

commented Apr 13, 2016

@retronym said:
This might be something we need to fix to be serialization-compatible in 2.12, see https://gist.github.com/JoshRosen/8aacdee0162da430868e7f73247d45d8

@scabug

This comment has been minimized.

Copy link
Author

commented Apr 13, 2016

@retronym said:
The Delambdafy phase already implements a similar virtual -> static translation for lambda implementation methods, perhaps we can generalize that to do the same thing for lambda-lifted local defs.

@scabug

This comment has been minimized.

Copy link
Author

commented Apr 14, 2016

@retronym said:
Here's an attempt to generalize delambdafy to STATIC-ify lifted methods: scala/scala#5099

@scabug

This comment has been minimized.

Copy link
Author

commented Jul 1, 2016

@Blaisorblade said:
Fixed by scala/scala#5099 as mentioned in the release notes for 2.12.0-M5: https://github.com/scala/scala/releases/tag/v2.12.0-M5. I've reopened this just to change from "Not a bug" to "Fixed".

@scabug scabug closed this Jul 1, 2016

@scabug scabug added the optimizer label Apr 7, 2017

@scabug scabug added this to the 2.12.0-M5 milestone Apr 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.