Closed Bug 878501 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect, minor)

x86_64
macOS
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jruderman, Assigned: bbouvier)

Details

(Keywords: testcase)

Attachments

(1 file, 1 obsolete file)

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
Attached patch proposed fix (obsolete) — Splinter Review
Assignee: general → bbouvier
Status: NEW → ASSIGNED
Attachment #761753 - Flags: review?(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"
Attached patch Luke's solutionSplinter Review
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.
Attachment #761753 - Attachment is obsolete: true
Attachment #761753 - Flags: review?(luke)
Attachment #761831 - Flags: review?(luke)
Comment on attachment 761831 [details] [diff] [review]
Luke's solution

Thanks!
Attachment #761831 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/1b40028e5891
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.