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

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: bbouvier)

Tracking

({testcase})

Trunk
mozilla24
x86_64
Mac OS X
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created attachment 761753 [details] [diff] [review]
proposed fix
Assignee: general → bbouvier
Status: NEW → ASSIGNED
Attachment #761753 - Flags: review?(luke)

Comment 2

4 years ago
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"
(Assignee)

Comment 3

4 years ago
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.
Attachment #761753 - Attachment is obsolete: true
Attachment #761753 - Flags: review?(luke)
Attachment #761831 - Flags: review?(luke)

Comment 4

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

Thanks!
Attachment #761831 - Flags: review?(luke) → review+
(Assignee)

Comment 5

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

Comment 6

4 years ago
https://hg.mozilla.org/mozilla-central/rev/1b40028e5891
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.