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

Structural invocation: find method by name if by signature fails #5973

Closed
wants to merge 1 commit into from

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Jul 3, 2017

In a structural invocation, if the receiver object doesn't have
a method with the expected paramter types, check if there's a
single method matching the name. If so, invoke this method.

This fixes parts of scala/bug#10334,
namely the parts that regressed with the new lambda encoding.
But the underlying issue still remains.

In a structural invocation, if the receiver object doesn't have
a method with the expected paramter types, check if there's a
single method matching the name. If so, invoke this method.

This fixes parts of scala/bug#10334,
namely the parts that regressed with the new lambda encoding.
But the underlying issue still remains.
@sjrd
Copy link
Member

sjrd commented Jul 3, 2017

Ouch.

I'd like to stress that we won't ever be able to implement the same behavior in Scala.js. If at least there wasn't the test !x.isBridge in lookupStructuralMethod, we could make the linker to support the same fallback. But with that test, it cannot be replicated: the Scala.js linker has no idea what is or isn't a "bridge".

Reflective calls are broken, but at least at the moment they are uniformly broken across JVM and JS. We put a lot of effort into replicating the JVM behavior, even bug-compatibility. It would be very disappointing to see that compatibility go away 😟

@lrytz
Copy link
Member Author

lrytz commented Jul 3, 2017

The isBridge test is not needed for the test case, and I didn't come up with an example where it's actually needed. I added it because it seems to make sense, but we can get rid of it.

I don't have any idea for a proper fix of the issue..

@@ -139,6 +139,19 @@ object ScalaRunTime {
// More background at ticket #2318.
def ensureAccessible(m: JMethod): JMethod = scala.reflect.ensureAccessible(m)

def lookupStructuralMethod(c: Class[_], m: String, params: Array[Class[_]]): JMethod = {
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't pass MiMa. It's more than an internal issue: if we compile new 2.12 code to use this method, it won't run when an older 2.12 library is on the classpath.

@@ -108,7 +108,7 @@ abstract class CleanUp extends Statics with Transform with ast.TreeDSL {
if (method ne null)
return method
else {
method = ScalaRunTime.ensureAccessible(forReceiver.getMethod("xyz", methodCache.parameterTypes()))
method = ScalaRunTime.lookupStructuralMethod(forReceiver, "xyz", methodCache.parameterTypes())
Copy link
Member

@retronym retronym Jul 3, 2017

Choose a reason for hiding this comment

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

How about emitting:

method = try {
    ScalaRunTime.ensureAccessible(forReceiver.getMethod("xyz", methodCache.parameterTypes())) 
  } catch {
    case e: NoSuchMethodException => try { lookupStructuralMethod(...) } catch { case _: Throwable => throw e }
}

To profit from the bug fix, you'd need to the latest standard library available at runtime, but structural calls that used to work will continue to work with the old library.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could also emit the entire body of lookupStructuralMethod in CleanUp in 2.12.x, and introduce the ScalaRunTime method only in 2.13.x

@lrytz
Copy link
Member Author

lrytz commented Jul 6, 2017

Alternative in #5977

@retronym
Copy link
Member

retronym commented Jul 7, 2017

Closing this one for now. This approach might still complement the alternative fix (although it sounds there will still be cases that are impossible to unambiguously resolve), so let's pursue this further on 2.13.x

@retronym retronym closed this Jul 7, 2017
@lrytz lrytz deleted the t10334 branch July 20, 2017 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants