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"
Status: RESOLVED FIXED
: 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]
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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"
    else
      ... "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 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] 2013-06-12 18:59:01 PDT
Comment on attachment 761831 [details] [diff] [review]
Luke's solution

Thanks!
Comment 5 Benjamin Bouvier [:bbouvier] 2013-06-13 11:55:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b40028e5891
Comment 6 Ed Morley [:emorley] 2013-06-14 04:17:45 PDT
https://hg.mozilla.org/mozilla-central/rev/1b40028e5891

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