878501 - Confusing error "asm.js module must end with a return export statement"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

function oopsIForgotTheVarKeyword(stdlib)
{
  "use asm"
  abs = stdlib.Math.abs;
  return {};
}

Unhelpful error message with unhelpful line number:
  e.js:1:0 warning: asm.js type error: asm.js module must end with a return export statement

Expected something like:
  e.js:4:0 warning: invalid asm.js statement

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: proposed fix

Comment 2

[Luke Wagner [:luke]]

Instead of this fix, which narrowly matches assignment expressions (also, watch out, UnaryKid(pn) can be NULL), what about improving the error reporting in CheckModuleExports; something like:

  if (!returnNode || !returnNode->isKind(PNK_RETURN))
    if (returnNode && NextNode(returnNode) != NULL)
      ... "invalid asm.js statement"
    else
      ... "asm.js module must end with a return export statement"

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Luke's solution

Not sure how we could make this condition more readable (the !returnNode in the first condition and the returnNode in the second one seems confusing to me).
This is logically equivalent to these two:

if (returnNode && !returnNode->isKind(PNK_RETURN) && NextNode(returnNode) != NULL)
   return m.fail(returnNode, "invalid asm.js statement");
else if (!returnNode || (!returnNode->isKind(PNK_RETURN) && !NextNode(returnNode)))
   return m.fail(fn, "asm.js module must end with a return export statement");

but I am not sure these are more understandable, so I kept your way.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 761831 [details] [diff] [review]
Luke's solution

Thanks!

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b40028e5891

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1b40028e5891