Closed Bug 1339895 Opened 7 years ago Closed 7 years ago

Javascript statement var x += 2 yields misleading error

Categories

(Core :: JavaScript Engine, defect, P3)

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: captainhypertext, Assigned: sigmaths.scientia, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: triage-deferred)

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

Run the javascript statement var x += 2;


Actual results:

Misleading, possibly unhandled error:
SyntaxError: missing ; before statement


Expected results:

Google Chrome returns:
Uncaught SyntaxError: Unexpected token +=
Severity: normal → minor
Blocks: jserror
Keywords: triage-deferred
Priority: -- → P3
I,m working on this.
Thank you!
Assignee: nobody → sigmaths.scientia
Mentor: arai.unmht
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
I changed the error message
"missing ; before statement"
to
"expected ';', got (unexpected token)"
Attachment #8924416 - Flags: review?(arai.unmht)
Comment on attachment 8924416 [details] [diff] [review]
Improvement of error message for ASI

Review of attachment 8924416 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for your patch!

The change looks good.

Then, this patch will conflict with bug 1319416 patch, that is now in mozilla-inbound, and will be merged to mozilla-central within a day.
So, you'll need to rebase your patch onto it later.
(the conflict is only about surrounding lines, so the rebase won't be so hard)

fscholz, can I have your opinion about the new error message?
(the previous one is already documented in the following page. so the change will also be required on the document side)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Missing_semicolon_before_statement

::: js/src/frontend/Parser.cpp
@@ +2922,4 @@
>           *        tried to insert semicolon here
>           *
>           * Detect this situation and throw an understandable error.  Otherwise
> +         * we'd throw a confusing "expected \';\', got TokenkindToDesk(tt)" error.

please change this line to
"..., got (unexpected token)"

@@ +2932,4 @@
>              error(JSMSG_YIELD_OUTSIDE_GENERATOR);
>              return false;
>          }
> +        

please remove whitespaces in this line.
Attachment #8924416 - Flags: review?(arai.unmht) → feedback?(fscholz)
I changed
"expected \';\', got TokenkindToDesk(tt)" 
to
"expected \';\', got (unexpected token)",
and removed white space in the previous patch.
Attachment #8924428 - Flags: review?(arai.unmht)
Attachment #8924416 - Attachment is obsolete: true
Attachment #8924416 - Flags: feedback?(fscholz)
Comment on attachment 8924428 [details] [diff] [review]
Improvement of error message for ASI

Review of attachment 8924428 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again.
Looks good.

Here's try run (automation for build and test), that will take 2-3 hours.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca4ac9d70d82c1062574e7bb5db59ba89059db91


carrying forward f? for fscholz (see comment #4)
Attachment #8924428 - Flags: review?(arai.unmht)
Attachment #8924428 - Flags: review+
Attachment #8924428 - Flags: feedback?(fscholz)
now bug 1319416 get's merged to mozilla-central.
please rebase your patch onto it.
Comment on attachment 8924428 [details] [diff] [review]
Improvement of error message for ASI

As briefly discussed in IRC, for |var x += 2;| I would not add a hint that a semicolon is missing, because that's not actually the problem in this situation.

|SyntaxError: Expected ';', got identifier| is imo not much better than now, as it focuses on the semicolon still, which isn't really the problem.

|SyntaxError: Unexpected token +=| is at least clearly identifying the problem.

But I don't know in which other cases this error is thrown, so my comment might just apply to this one case.
Attachment #8924428 - Flags: feedback?(fscholz)
I changed the error message to
"unexpected token: (unexpected token)"

and rebased
Attachment #8925436 - Flags: review?(arai.unmht)
Attachment #8924428 - Attachment is obsolete: true
Comment on attachment 8925436 [details] [diff] [review]
Improve error message for unexpected token

Review of attachment 8925436 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again!

We'll need to update the following documentation later (after this patch gets merged), that is linked from errordocs.js:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Missing_semicolon_before_statement

here's try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e3c9bad997ad4c01961f0e7f495f0da8ac5d054
Attachment #8925436 - Flags: review?(arai.unmht) → review+
Correction of patch
Attachment #8925436 - Attachment is obsolete: true
Attachment #8925786 - Flags: review?(arai.unmht)
Comment on attachment 8925786 [details] [diff] [review]
Improve error message for unexpected token

Review of attachment 8925786 [details] [diff] [review]:
-----------------------------------------------------------------

thanks again!

here's try run for tests that failed before:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47176e63c40d6823f5b03782f27b46646e930149
Attachment #8925786 - Flags: review?(arai.unmht) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53ce0e7d27e7
Improve error message for unexpected token. r=arai
Keywords: checkin-needed
Blocks: 1415201
https://hg.mozilla.org/mozilla-central/rev/53ce0e7d27e7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: