Uh oh!
There was an error while loading. Please reload this page.
Fuzzer: Add another stringview subtyping error message to ignore - #6575
Conversation
tlively
commented
May 9, 2024
But this error is probably because a non-nullable reference is never a supertype of a nullable reference, right? If you change |
tlively
commented
May 9, 2024
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.
kripken
commented
May 9, 2024
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? |
tlively
commented
May 9, 2024
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. |
kripken
commented
May 9, 2024
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? |
kripken
commented
May 9, 2024
Here is a full testcase showing the issue: (module
(type$0 (func))
(rec
(type$1 (sub (func (param (ref null $13)) (result (refstruct)))))
(type$2 (func (param (ref null $12) i64)))
(type$3 (sub (func (param nullref f64) (result (ref null $8)))))
(type$4 (sub (array (mutf32))))
(type$5 (func (parami64 (ref null $9) (ref$4) arrayref) (resultv128)))
(type$6 (sub (array (mutf64))))
(type$7 (func (parami64) (result (ref$3))))
(type$8 (sub (arrayi8)))
(type$9 (sub (arrayf64)))
(type$10 (array (mutv128)))
(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 (mutf32)) (field (mut (ref$2))) (field (mutf32)) (field (ref$7)) (field (muti32)) (fieldeqref)))
(type$14 (sub (array (ref$10))))
)
(func$0
(local$0f32)
(local$1 (ref$10))
(local$2 (ref$10))
)
) |
kripken
commented
May 9, 2024
IIUC you are saying that it's ok to consider string views supertypes of none, and we should work around that in parsing? |
tlively
commented
May 9, 2024
Are we sure that stringviews are no longer supertypes of |
kripken
commented
May 9, 2024
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? |
tlively
commented
May 9, 2024
No, the shorthands exist in both the binary and text format. Even with nullable references disallowed, we still have |
kripken
commented
May 9, 2024
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. |
tlively
commented
May 9, 2024
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.