Fix #12: Implement the behavior of hijacked classes. #13
Conversation
sjrd
force-pushed
the
upcast-downcast-hijacked-classes
branch
from
07bfaba
to
1f9b80c
Compare
sjrd
force-pushed
the
upcast-downcast-hijacked-classes
branch
from
7031e1d
to
5149c84
Compare
There was a problem hiding this comment.
@sjrd Thank you very much for this great work!
First of all, I read through the proposed design in #12 and I think preserving the Scala.js semantics and delegating the string implementation to JS is the best option for now (at least for targeting browser Wasm).
I'll leave some comments/questions on the original issue, but I believe this design looks good to me 👍
For this change itself, I left some questions and comments, but it looks great to me 👍
While it would be great to add some more explanation or link into the code comment (as I commented in the code), but I think we can merge this PR anytime soon :)
| @@ -396,15 +396,15 @@ class WasmBuilder { | |||
| )(implicit ctx: WasmContext): WasmFunction = { | |||
| val receiver = WasmLocal( | |||
| Names.WasmLocalName.receiver, | |||
| // Receiver type for non-constructor methods needs to be Object type because params are invariant | |||
| // Receiver type for non-constructor methods needs to be `(ref any)` because params are invariant | |||
| case IRTypes.CharType => | ||
| genBox(IRTypes.CharType, SpecialNames.CharBoxClass) | ||
| case IRTypes.LongType => | ||
| genBox(IRTypes.LongType, SpecialNames.LongBoxClass) |
There was a problem hiding this comment.
[note]
Representation of char and long
These are opaque to JS in the Scala.js semantics. We can implement them with real Wasm classes following the correct vtable. Upcasting will wrap a primitive into the corresponding class, and downcasting will extract the primitive from the field. This would not leave Wasm code.
#12
| * block (ref any) $done | ||
| * block anyref $notOurObject | ||
| * load receiver | ||
| * br_on_cast_fail anyref (ref $targetRealClass) $notOurObject |
There was a problem hiding this comment.
[question]
| * br_on_cast_fail anyref (ref $targetRealClass) $notOurObject | |
| * br_on_cast_fail anyref (ref $java.lang.Object) $notOurObject |
?
and, this cast gonna success only for long and char, am I right?
There was a problem hiding this comment.
It will succeed for long and char, as well as for any real class. We are in a situation where the receiver's static type is an ancestor of a hijacked class (e.g., java.lang.CharSequence, or even any). So at run-time we can have either primitives (possibly boxed for long and char) or real class instances.
loader.mjs
Outdated
| // Hash code, because it is overridden in all hijacked classes | ||
| jsValueHashCode: (x) => { | ||
| if (typeof x === 'number') | ||
| return x | 0; // TODO make this compliant for floats | ||
| if (typeof x === 'string') | ||
| return stringHashCode(x); | ||
| if (typeof x === 'boolean') | ||
| return x ? 1231 : 1237; | ||
| if (typeof x === 'undefined') | ||
| return 0; | ||
| return 42; // for any JS object | ||
| }, |
There was a problem hiding this comment.
[question]
I'm curious how they are calculated, is there a spec I can refer to?
| val charValueField = FieldName("value") | ||
| val longValueField = FieldName("value") |
There was a problem hiding this comment.
unused?
| val charValueField = FieldName("value") | |
| val longValueField = FieldName("value") |
|
Thanks for the review. I'll integrate all your comments in the code. |
This will allow us to add helper JS functions that the Wasm code can import.
…`string`. For `int`s, we use JavaScript helpers.
Using a JavaScript helper in all cases for now.
This correctly implements the interoperability semantics of strings, which are now seen by JavaScript as primitive strings. It also allows us to correctly and efficiently implement the various conversions to string. However, it makes primitive operations worse. Hopefully the `stringref` proposal will come to save us one day.
Always returning the same value is technically in-spec for objects, although that defeats its purpose, of course.
In actual classes that we automatically derive from the corresponding hijacked classes.
When the type of the receiver of an `Apply` is an ancestor of any hijacked class, it might contain a JS value instead of one of our own objects. In that case, we must perform a more elaborate dispatch that tests whether we have a true object, or whether we must dispatch to a hijacked class method. This change also surfaces that the methods of `j.l.Object` can receive an arbitrary non-null value, i.e., a `(ref any)`. This has to be reflected in vtables and itables, unfortunately.
Including with dispatch on potentially hijacked classes, and all the weird null handling corner cases.
sjrd
force-pushed
the
upcast-downcast-hijacked-classes
branch
from
5149c84
to
5acf283
Compare
|
I have now integrated all your questions and notes as additional comments in the code. |
|
Thank you very much @sjrd for adding a lot of comments, that helps me a lot 😍 |
So I got excited and carried away, and I implemented the proposed design for handling hijacked classes.
Based on #11.