WebAssembly / spec

WebAssembly specification, reference interpreter, and test suite.
https://webassembly.github.io/spec/
Other
3.16k stars 452 forks source link

Inconsistent error messages in test suite #1846

Closed mohanson closed 1 day ago

mohanson commented 1 day ago

I'm the author of pywasm. I'm trying to introduce more friendly error messages in my project, but I found some inconsistencies.

From spec/interpreter, the "uninitialized element" error should include the index value, but I found that the index value is both present and absent in spec/test.

https://github.com/WebAssembly/spec/blob/bd2aa85d5f6c6129760a3705f4de8acdb45b8e67/interpreter/exec/eval.ml#L101

https://github.com/WebAssembly/spec/blob/bd2aa85d5f6c6129760a3705f4de8acdb45b8e67/test/core/bulk.wast#L222

https://github.com/WebAssembly/spec/blob/bd2aa85d5f6c6129760a3705f4de8acdb45b8e67/test/core/bulk.wast#L227

If the index value is forgotten in spec/test, I can submit a PR to fix spec/test.

keithw commented 1 day ago

Please see https://github.com/WebAssembly/spec/issues/1841 (https://github.com/WebAssembly/spec/pull/1076 for the history).

mohanson commented 1 day ago

Interesting history. I submitted a PR to make the behavior consistent. https://github.com/WebAssembly/spec/pull/1847

keithw commented 1 day ago

I think some of the reasonable options for your test runner include:

I don't think you'll be happy trying to match the error text exactly -- even the spec interpreter test runner doesn't do that. But if you really want to, you could submit a #1076/#1089-style PR that goes and removes the indices everywhere they have leaked back in to the spec tests. My guess is that you'll be lonely trying to maintain that property; whatever forces motivated #1076/#1089 at the time don't seem to exist anymore, or somebody else would have complained earlier when an index was reintroduced.

mohanson commented 1 day ago

I understand your concerns, and after evaluation I think these changes are not necessary, so I will close it, thank you.