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
Closed

Nested methods capture outer instance when lifted #9390

scabug opened this issue Jul 10, 2015 · 11 comments
Assignees
Milestone

Comments

@scabug
Copy link

scabug 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
Copy link
Author

scabug 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
Copy link
Author

scabug commented Jul 15, 2015

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

@scabug
Copy link
Author

scabug 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
Copy link
Author

scabug 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
Copy link
Author

scabug 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
Copy link
Author

scabug 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
Copy link
Author

scabug 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
Copy link
Author

scabug 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
Copy link
Author

scabug 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
Copy link
Author

scabug commented Apr 14, 2016

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

@scabug
Copy link
Author

scabug 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 as completed Jul 1, 2016
@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
Projects
None yet
Development

No branches or pull requests

2 participants