Last Comment Bug 878501 - Confusing error "asm.js module must end with a return export statement"
: Confusing error "asm.js module must end with a return export statement"
: testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- minor (vote)
: mozilla24
Assigned To: Benjamin Bouvier [:bbouvier]
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2013-06-01 15:20 PDT by Jesse Ruderman
Modified: 2013-06-15 09:38 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

proposed fix (2.65 KB, patch)
2013-06-12 15:43 PDT, Benjamin Bouvier [:bbouvier]
no flags Details | Diff | Splinter Review
Luke's solution (2.84 KB, patch)
2013-06-12 18:54 PDT, Benjamin Bouvier [:bbouvier]
luke: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2013-06-01 15:20:28 PDT
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] 2013-06-12 15:43:05 PDT
Created attachment 761753 [details] [diff] [review]
proposed fix
Comment 2 Luke Wagner [:luke] 2013-06-12 18:15:28 PDT
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"
      ... "asm.js module must end with a return export statement"
Comment 3 Benjamin Bouvier [:bbouvier] 2013-06-12 18:54:27 PDT
Created attachment 761831 [details] [diff] [review]
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, "invalid asm.js statement");
else if (!returnNode || (!returnNode->isKind(PNK_RETURN) && !NextNode(returnNode)))
   return, "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] 2013-06-12 18:59:01 PDT
Comment on attachment 761831 [details] [diff] [review]
Luke's solution

Comment 5 Benjamin Bouvier [:bbouvier] 2013-06-13 11:55:27 PDT
Comment 6 Ed Morley [:emorley] 2013-06-14 04:17:45 PDT

Note You need to log in before you can comment on or make changes to this bug.