fix: Backport heroic fixes made in #1559 #1568
Conversation
| /** Compiles the `visit_members` function. */ | ||
| export function compileVisitMembers(compiler: Compiler): void { | ||
| /** Ensures that the visitor function of the specified class is compiled. */ | ||
| function ensureVisitMembersOf(compiler: Compiler, instance: Class): void { |
There was a problem hiding this comment.
A new runtime visitor implementation that generates per-class functions instead of a huge unwieldy switch. Increases binary size somewhat, but is much easier to reason about.
| for (let i = 0, k = parameterTypes.length; i < k; ++i) { | ||
| if (!this.checkTypeSupported(parameterTypes[i], reportNode.parameters[i])) { | ||
| let parameterReportNode: Node; | ||
| if (parameterNodes.length > i) parameterReportNode = parameterNodes[i]; |
There was a problem hiding this comment.
Here, the function may come from a function expression with an omitted argument being compiled in context of a function type with fewer argument expressions, implicitly dropping excess arguments due to being irrelevant. Doesn't error when compiling to JS due to undefined, but produced a fine runtime index-out-of-bounds in Wasm.
| @@ -612,7 +612,7 @@ export class Flow { | |||
| /** Tests if the specified `this` field has the specified flag or flags. */ | |||
| isThisFieldFlag(field: Field, flag: FieldFlags): bool { | |||
| var fieldFlags = this.thisFieldFlags; | |||
| if (fieldFlags) { | |||
| if (fieldFlags != null && fieldFlags.has(field)) { | |||
There was a problem hiding this comment.
This is old code apparently, written before the requirement for a Map#has before a Map#get was conceived. Produced a wholesome key-does-not-exist error in Wasm.
| @@ -23,7 +23,7 @@ function i64_is<T>(value: T): bool { | |||
| // @ts-ignore: decorator | |||
| @global @inline | |||
| function i64_new(lo: i32, hi: i32 = 0): i64 { | |||
| return lo | (hi << 32); | |||
| return <i64><u32>lo | (<i64>hi << 32); | |||
There was a problem hiding this comment.
A spectacular piece of code that was completely wrong before, and looks absolutely bogus now. Apart from the missing cast to i64 to ensure proper bit size, the <i64><u32> is necessary so the compiler doesn't attempt to sign-extend due to the cast value being signed. Somewhat strange, and made me wonder whether we can do better, but without any deeper changes that's the awesome fix fixing things.
| if (overloads) { | ||
| let overload = overloads.get(kind); | ||
| if (overload) return overload; | ||
| if (overloads != null && overloads.has(kind)) { |
There was a problem hiding this comment.
Another fix within the Map#has before Map#get wasnt-even-conceived-by-someone category. No error in JS, entirely justified fireworks in Wasm.
| @@ -563,7 +563,7 @@ export class Array<T> { | |||
|
|
|||
| // RT integration | |||
|
|
|||
| @unsafe private __visit_impl(cookie: u32): void { | |||
| @unsafe private __visit(cookie: u32): void { | |||
There was a problem hiding this comment.
Perfection is achieved not when there is nothing more to add, but when there is nothing left to take away.
|
Btw, in case you are disappointed, a more impressive fix was in #1567 already. Finding that without source maps support running Wasm in node was quite the achievement. Thanks to Max for helping me to figure this one out :) |
This PR backports the series of herculean fixes made in the tracing GC branch to master, in the hope that these also benefit ARC.