Comment-only input files no longer rejected by 1.0.13
lars-t-hansen opened this issue · 7 comments
After upgrading to 1.0.13, some tests in the SpiderMonkey wasm test suites that were supposed to fail stopped failing, this is from wasm/comments.js:
assertErrorMessage(() => wasmEvalText(';; only comment'), SyntaxError, /wasm text error/);
assertErrorMessage(() => wasmEvalText(';; only comment\n'), SyntaxError, /wasm text error/);
assertErrorMessage(() => wasmEvalText('(; only comment ;)'), SyntaxError, /wasm text error/);
assertErrorMessage(() => wasmEvalText(';; only comment\n'), SyntaxError, /wasm text error/);
These test that a comment by itself does not make a source file; the source file has to define a module, as suggested by https://webassembly.github.io/spec/core/text/modules.html#text-module. Clearly a text processor can do whatever it wants here but at least there's a discussion to be had.
I'm not dogmatic on this but I'd love to know if this change was deliberate or not.
I'm guessing that this was changed in this commit.
@alexcrichton was this change intentional? The message still says that it's an error to not have a module field, but I'm guessing that a comment is being treated as a token which allows this check to succeed.
Oh dear sorry for the breakage!
So this was semi-intentional, but it's mostly just me trying to make sense of the spec tests. The annotations proposal has tests now that this is a valid wasm file:
(@a )which broke the original check that empty modules were rejected. I believe that there's also tests that the (module ...) wrapper is actually optional too?
In any case I'm down to implement whatever behavior here, I'm mostly just trying to get something that passes all the spec tests. I could change the check for "is there any token" to "is there any non-comment or non-whitespace token?" and that'd fix everything, but I'm curious if y'all have thoughts on this?
I think the stakes are pretty low here for us. If changing the check to "is there any non-comment or non-whitespace token" is easy, then let's do that. That would let us keep these tests, and not disrupt the spec-testing here.
Whatever works for you guys. After all this is a very geeky corner case that has no practical importance, from what I can tell, apart from error-compatibility with other consumers.
I personally want to keep "mostly empty things" as parse errors (like the empty string) because that seems like it'll help weed out accidental errors sooner. I do think it's a bug in this implementation now that a single space ( ) parses as an empty module. So I think let's go back to what it was before, but we can allow empty modules with at least one annotation, but we still disallow modules that are have no tokens or only whitespace/comment tokens.
Ok I went a head and made a small tweak to require at least one non-whitespace/comment token. Don't mean to steal work from @eqrion but no need to pile it on you either!