-
Notifications
You must be signed in to change notification settings - Fork 831
Fuzzer: Add another stringview subtyping error message to ignore #6575
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
|
But this error is probably because a non-nullable reference is never a supertype of a nullable reference, right? If you change |
|
I guess we haven't updated our parsers or binary writer, so we're probably still interpreting the stringview types as nullable. |
This reverts commit 7acb1a4.
|
Hmm, that snippet by itself does not error now, so it must have been related to the full testcase, which unfortunately I've not kept. I reverted that part and added a separate fix here, which I'm fuzzing with now to try to reproduce the original issue with. Can you elaborate on your last comment though? What parser/writer changes are we missing? |
|
To match V8's new behavior, we would need to fix this code to use We would need to make analogous fixes in the binary parser, the binary writer, and the printer. Without that, it's not surprising that round-tripping leads to disagreements with V8. |
|
Ah yes, I see about the text format parsing. But that is just a text format issue? It can't explain the problem I saw. And what do you mean by changes to the binary parsers? Are you saying to ignore a nullable annotation and treat it as non-nullable? |
|
Here is a full testcase showing the issue: (module
(type $0 (func))
(rec
(type $1 (sub (func (param (ref null $13)) (result (ref struct)))))
(type $2 (func (param (ref null $12) i64)))
(type $3 (sub (func (param nullref f64) (result (ref null $8)))))
(type $4 (sub (array (mut f32))))
(type $5 (func (param i64 (ref null $9) (ref $4) arrayref) (result v128)))
(type $6 (sub (array (mut f64))))
(type $7 (func (param i64) (result (ref $3))))
(type $8 (sub (array i8)))
(type $9 (sub (array f64)))
(type $10 (array (mut v128)))
(type $11 (sub (array (mut (ref $2)))))
(type $12 (sub final $3 (func (param stringview_wtf16 f64) (result (ref null $8)))))
(type $13 (struct (field (mut f32)) (field (mut (ref $2))) (field (mut f32)) (field (ref $7)) (field (mut i32)) (field eqref)))
(type $14 (sub (array (ref $10))))
)
(func $0
(local $0 f32)
(local $1 (ref $10))
(local $2 (ref $10))
)
) |
|
IIUC you are saying that it's ok to consider string views supertypes of none, and we should work around that in parsing? |
|
Are we sure that stringviews are no longer supertypes of |
|
Wait, are the shorthands not a feature of the text format only? That might be what I am missing in your point.
It seems quite odd for them to be supertypes of none when they are not allowed to be nullable? |
|
No, the shorthands exist in both the binary and text format. Even with nullable references disallowed, we still have |
|
I see. I'm not totally familiar with that part of the spec... I'll land this trivial change (with proper PR title) as a followup to the previous one. Next week I can look into these shorthands, unless you want to do it. |
|
I can fix the shorthands real fast. |
Original text:
V8 errors on this, e.g. this was created:
V8 says that subtyping is invalid.
Later PR focuses on ignoring a related message.