This repository was archived by the owner on Jul 12, 2024. It is now read-only.
Fix reflective proxy for Hijacked classes #108
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sjrd
reviewed
Apr 17, 2024
| // } | ||
| if (methods.exists( m => | ||
| m.name.simpleName == methodName.nameString && | ||
| !m.isAbstract |
tanishiking
force-pushed
the
reflective-object-methods
branch
3 times, most recently
from
cc381eb to
6747768
Compare
tanishiking
changed the title
tanishiking
commented
Apr 18, 2024
| // The synthetic `This` may contains boxed primitive, instead of primitive types. | ||
| // However, the checker should ensure that the `This` is never boxed in Scala.js linker frontend. | ||
| // As a workaround, we unbox the `This` here. | ||
| val fixedTpe = t.tpe match { |
There was a problem hiding this comment.
Any better explanation?
There was a problem hiding this comment.
Perhaps as follows:
Workaround wrong
t.tpeforThisnodes inside reflective proxies.
In a hijacked class,Thisnodes are supposed to be typed as the corresponding primitive type.
However, the Scala.js linker frontend synthesizes reflective proxies that containThisnodes typed as the hijacked class'ClassTypeinstead.
This is bad for us because it meansgenAdaptfails to box the primitives when required.
We work around this issue here by re-computing the correct type ofThisnodes.
This was
linked to
issues
Apr 18, 2024
sjrd
reviewed
Apr 18, 2024
This commit fix 2 things (1) Reflective proxy for methods in `j.l.Object` should call the methods in `j.l.Object` Resolves: #97 Related: #98 When a reflective call is made on the `clone` method, reflective proxies are generated for the `clone` method on the Hijacked classes, including `java.lang.Void` and `java.lang.Boolean`. The issue arises because the generated reflective proxy contains a function call to a non-existent function, such as `$f#java.lang.Void#clone_Ljava.lang.Object` in this example. ``` (func $f#java.lang.Void#clone_R (type $f.21) (param $___<this> (ref any)) (result anyref) local.get $___<this> call $f#java.lang.Void#clone_Ljava.lang.Object) ``` For Hijacked classes, `genApply` is transformed into `genApplyStatically`, which looks up the method in the Hijacked class and calls it directly, instead of using table dispatching. However, `resolvePublicMethod` incorrectly resolves the "clone" method from the Hijacked classes, such as `java.lang.Boolean`, even though the method doesn't exist in the Hijacked class itself (it is inherited from java.lang.Object). In this context, `resolvePublicMethod` should not resolve methods that are not defined in the specific Hijacked class. --- (2) Workaround wrong t.tpe for `This` nodes inside reflective proxies. In a hijacked class, This nodes are supposed to be typed as the corresponding primitive type. However, the Scala.js linker frontend synthesizes reflective proxies that contain This nodes typed as the hijacked class' ClassType instead. This is bad for us because it means genAdapt fails to box the primitives when required. We work around this issue here by re-computing the correct type of This nodes. #100 Synthetic `This` should have a primitive type instead of BoxedType Fix #100
tanishiking
force-pushed
the
reflective-object-methods
branch
from
6747768 to
8d8a6c7
Compare
|
Thanks, edited the comment, and squashed 👍 |
tanishiking
changed the title
sjrd
approved these changes
Apr 18, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fix 2 things
(1) Reflective proxy for methods in
j.l.Objectshould call the methods inj.l.ObjectResolves: #97
Related: #98
When a reflective call is made on the
clonemethod, reflective proxies are generatedfor the
clonemethod on the Hijacked classes, includingjava.lang.Voidandjava.lang.Boolean.The issue arises because the generated reflective proxy contains a function call
to a non-existent function, such as
$f#java.lang.Void#clone_Ljava.lang.Objectin this example.For Hijacked classes,
genApplyis transformed intogenApplyStatically, which looks upthe method in the Hijacked class and calls it directly, instead of using table dispatching.
However,
resolvePublicMethodincorrectly resolves the "clone" method from the Hijacked classes,such as
java.lang.Boolean, even though the method doesn't exist in the Hijacked class itself(it is inherited from java.lang.Object).
In this context,
resolvePublicMethodshould not resolve methods that are not defined in thespecific Hijacked class.
(2) Workaround wrong t.tpe for
Thisnodes inside reflective proxies.In a hijacked class, This nodes are supposed to be typed as the corresponding primitive type.
However, the Scala.js linker frontend synthesizes reflective proxies that contain This nodes typed as the hijacked class' ClassType instead.
This is bad for us because it means genAdapt fails to box the primitives when required.
We work around this issue here by re-computing the correct type of This nodes.
#100
Synthetic
Thisshould have a primitive type instead of BoxedTypeFix #100