-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Refine parameter adaptation logic for arrays #23591
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
Conversation
This seems misguided to me. Why AnyRef but not any of its subtypes? Also, this is not supported by spec text, whether from the Scala 2 or Scala 3 spec.
Changes to erasure are extremely risky, because they basically break tasty and binary compatibility every time. They need a very strong justification. Typically, only fixing unsoundness is a good enough reason to do this.
(削除) Scala 2 certainly erases this array type to What should be the general rule?Object[]. (削除ここまで)
Edit: Hmm it does not, at least judging by -Xprint:erasure. However in the resulting bytecode, the two relevant arguments to altMetaFactory are consistently array types, whereas in Scala 3 the second one ends up being Object.
BootstrapMethods:
0: #38 REF_invokeStatic java/lang/invoke/LambdaMetafactory.altMetafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;
Method arguments:
#26 ([Ljava/lang/Object;)Ljava/lang/Object;
#30 REF_invokeStatic Test$.$anonfun$new1ドル$adapted:([Ljava/lang/Object;)Ljava/lang/Object;
#26 ([Ljava/lang/Object;)Ljava/lang/Object;
#31 0
If we do not want to mess with erasure, could we just uniformly use the erased SAM methodtype for the implementation method type as well in the emitted bytecode? Would this always be safe for a type-correct source program?
Probably, but we would need to do that at the backend level. Basically consider it a bridge method, or an $adapted something something. The extracted private def likely cannot be altered to take a different parameter type.
Fixes scala#23179 Since JDK 9, argument checks for LambdaMetaFactory have become stricter. Which is to say that its JDK 8 version was too lax. We apply adaptation now in case the samType is an array, but the implType is not.
10bc131 to
8acd585
Compare
Array[? >: AnyRef] erases to Object[] (削除ここまで)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new approach seems reasonable.
It might need similar changes in ExpandSAMs for Scala SAM types that don't translate to target SAM types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be the reverse order as well ((_, defn.ArrayOf(_)))? Like if the SAM takes an Array[T] where T <: AnyRef but the lambda is specialized to T = String, for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unable to construct such a reversed example that will be accepted by the compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need something similar for the result type (resultAdaptationNeeded)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I also fail here to construct an example that would cause a run time crash (which is of course weak evidence).
The new approach seems reasonable.
It might need similar changes in
ExpandSAMsfor Scala SAM types that don't translate to target SAM types.
I tried constructing each of the five cases in the comment at the top of ExpandSAMs, but those were either rejected or did not crash at run time.
It might be worth including the other combinations in the test case as regression tests.
This solution doesn't appear to work for ScalaJS, I'll need to dig deeper.
Uh oh!
There was an error while loading. Please reload this page.
Fixes #23179
Since JDK 9, argument checks for LambdaMetaFactory have become stricter. Which is to say that its JDK 8 version was too lax.
We apply adaptation now in case the samType is an array, but the implType is not.