Update lit tests to parse with the new parser #6290
Conversation
Removing support for the legacy syntax will allow us to avoid implementing support for it in the new text parser.
Get as many of the lit tests as possible to parse with the new parser, mostly by moving declared module items to be after imports. Also fix a bug in the new parser's pop validation to allow supertypes of the expected type. The two big issues that still prevent some lit tests from working correctly under the new parser are missing support for symbolic field names and missing support for source map annotations.
| Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
| } | ||
| auto expectedType = scope.exprStack[0]->type; | ||
| if (type != expectedType) { | ||
| if (!Type::isSubType(expectedType, type)) { |
There was a problem hiding this comment.
Shouldn't this be flipped? That is, the seen type should be a subtype of the expected type?
There was a problem hiding this comment.
No, in this case if I catch e.g. eqref, it would be valid and safe to overapproximate and have a pop anyref, but it would not be safe or valid to have pop i31ref, since the eqref I'm catching may not be an i31ref.
| | ||
| ;; CHECK: (table $0 2 2 funcref) | ||
| ;; CHECK: (memory $m 1 2) | ||
| (memory $m 1 2) |
There was a problem hiding this comment.
Why add a name here rather than just move it around?
There was a problem hiding this comment.
Just to improve the output ordering. Adding the name doesn't affect whether the new parser can parse it.
| ;; As above, but the side effects now are a br. Again, the br must happen | ||
| ;; before the trap (in fact, the br will skip the trap here). | ||
| (block | ||
| (block $block |
There was a problem hiding this comment.
Weird, how did this parse and validate before..?
There was a problem hiding this comment.
Because the parser magically assigned the default name "$block" and it happened to match 😆
| ;; CHECK: (import "wasm:js-string" "fromCodePoint" (func $fromCodePoint_5 (type $5) (param i32) (result (ref extern)))) | ||
| | ||
| ;; CHECK: (memory $m 1) | ||
| (memory $m 1) |
There was a problem hiding this comment.
Why does this test need a memory? I don't seem to see any memory-using instructions.
There was a problem hiding this comment.
Hmm, you're right. Will remove.
Get as many of the lit tests as possible to parse with the new parser, mostly by moving declared module items to be after imports. Also fix a bug in the new parser's pop validation to allow supertypes of the expected type. The two big issues that still prevent some lit tests from working correctly under the new parser are missing support for symbolic field names and missing support for source map annotations.
2 participants
Get as many of the lit tests as possible to parse with the new parser, mostly by
moving declared module items to be after imports. Also fix a bug in the new
parser's pop validation to allow supertypes of the expected type.
The two big issues that still prevent some lit tests from working correctly
under the new parser are missing support for symbolic field names and missing
support for source map annotations.