Closed Bug 1310045 Opened 5 years ago Closed 5 years ago

incorrect error when accidentally using the destructuring operator


(Core :: JavaScript Engine, defect)

49 Branch
Windows 10
Not set



Tracking Status
firefox53 --- fixed


(Reporter: michiel, Unassigned, Mentored)




(1 file)

I was trying to use the destructuring operator but did not notice that the following ES7 syntax is not supported yet:

  ({a, b,} = {a:1, b:2, c:3, d:4});

However, the JS error this throws seems to be based on "what the engine is doing" rather than being based on the code the user wrote:

  SyntaxError: invalid property id

There is no "id" in the code I wrote, so the expected error is more something like "SyntaxError: unexpected token '...'" or a similarly indicative notice that lets the user figure out that Firefox is tripping up over the "..." syntax.
The error message here could perhaps be better -- but also, if we have the right developer UI, it shouldn't *need* to be, because it should include location information from SpiderMonkey.  This location information is certainly present, and it's definitely calculated correctly, because our JS shell displays that information quite correctly:

js>  ({a, b,} = {a:1, b:2, c:3, d:4});
typein:1:9 SyntaxError: invalid property id:
typein:1:9  ({a, b,} = {a:1, b:2, c:3, d:4});
typein:1:9 .........^

What developer UI are you using?  It may be worth filing a bug against something else to more clearly expose that location, in addition to crafting a better error message here.

As to this error: it looks like this is trivially fixed by modifying the |default| case in Parser.cpp's |Parser<ParseHandler>::propertyName|.  The proper change is likely something along the lines of reporting an error using JSMSG_UNEXPECTED_TOKEN, with the exact code looking something similar to how the other uses of that error number look.
Mentor: jwalden+bmo
This is just in the plain Firefox dev tools "console" tab.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Version: unspecified → 49 Branch
unfortunately, devtools console doesn't show location information for console input :/
at least it should show line/column
filed bug 1315242 for devtools console part.
See Also: → 1315242
I want to work on this bug.
Attached patch patch.txtSplinter Review
Attachment #8807935 - Flags: review?(jwalden+bmo)
Attachment #8807935 - Attachment is patch: true
Comment on attachment 8807935 [details] [diff] [review]

Review of attachment 8807935 [details] [diff] [review]:

Looks good.  Do you need someone to land this for you?
Attachment #8807935 - Flags: review?(jwalden+bmo) → review+
(IRC conversation says the answer is yes.  I've rebased the patch on tip and queued it up for landing, the next time I'm landing stuff.)
Pushed by
When a non-property-name token is observed instead of a property name in an object/class literal or destructuring pattern, indicate the unexpected token the way most unexpected tokens are indicated using JSMSG_UNEXPECTED_TOKEN.  r=jwalden
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.