ES5: { get x(v) { } } and { set x(v, v2) { } } should be syntax errors

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(5 attachments)

Duplicate of this bug: 554616

Updated

7 years ago
Whiteboard: [good first bug]
Keywords: dev-doc-needed
Depends on: 581744
Ugh, function parsing is a disaster right now, no way this should have been [good first bug].  Patches coming up to do this...
Status: NEW → ASSIGNED
Whiteboard: [good first bug]
Created attachment 462146 [details] [diff] [review]
1: Move variable definitions in Parser::functionDef as close to uses as possible

It's hard to understand possible syntactic transformations when you have to worry about effects on half a dozen apparently-non-extraneous variables.
Attachment #462146 - Flags: review?(cdleary)
Created attachment 462148 [details] [diff] [review]
2: Don't pointlessly set JSPROP_{G,S}ETTER in fun->flags

...following up on bug 581744.
Attachment #462148 - Flags: review?(cdleary)
Created attachment 462149 [details] [diff] [review]
3: Move function name parsing to callers

Making one Swiss army knife function parse names doesn't make sense when different callers have different naming requirements -- better for them to compute the name themselves and pass it in.
Attachment #462149 - Flags: review?(cdleary)
Created attachment 462150 [details] [diff] [review]
4: Move arguments parsing into a separate function

Parser::functionDef is monstrously huge, 400+ lines before any of these patches.  Splitting it up a bit (here by moving argument parsing out of the main function), done with even a slight amount of discernment, can only help readability.
Attachment #462150 - Flags: review?(cdleary)
Created attachment 462180 [details] [diff] [review]
5: Actually check the number of arguments to parse

I was hoping to make more cleanups here, and to eventually move argument parsing into the callers, but the number of enclosing-scope variables required to parse arguments is pretty large, and boxing up a struct of them is just ugly.  There must be a better way to do this that doesn't involve everything intermingled like here, but I'm not sure it's an incremental step away, as the previous patches have been.  I lack the energy and time to do anything more than an incremental step, so this is where the cleanup stops and the special-casing begins -- on to other bugs with this done, now.

I haven't run through tests other than js tests to see whether any others need updates for this change -- some probably do, but those changes aren't the interesting part of this patch, from the JS engine point of view.
Attachment #462180 - Flags: review?(cdleary)
Nice work, Waldo!

Could maybe use the split-out functionArguments parser from jsfun.cpp:Function, but then we'd accept destructuring formals. An allowed extension? Language lawyer needed, ES5 Chapter 16, also Chapter 15 intro, has revisions to tighten rules.

/be
Attachment #462146 - Flags: review?(cdleary) → review+
Attachment #462148 - Flags: review?(cdleary) → review+
Comment on attachment 462149 [details] [diff] [review]
3: Move function name parsing to callers

Yup, pushing name parsing into the callers makes more sense.
Attachment #462149 - Flags: review?(cdleary) → review+
Attachment #462150 - Flags: review?(cdleary) → review+
Comment on attachment 462180 [details] [diff] [review]
5: Actually check the number of arguments to parse

Please add tests for functions with duplicate formals. Other than that, looks great.
Attachment #462180 - Flags: review?(cdleary) → review+
http://hg.mozilla.org/tracemonkey/rev/e46a5e0d30b5
http://hg.mozilla.org/tracemonkey/rev/9ff3514bd267
http://hg.mozilla.org/tracemonkey/rev/474ceb938fb1
http://hg.mozilla.org/tracemonkey/rev/2dbf74b2525e
http://hg.mozilla.org/tracemonkey/rev/4571f2680302
Whiteboard: fixed-in-tracemonkey

Comment 12

7 years ago
http://hg.mozilla.org/mozilla-central/rev/e46a5e0d30b5
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Appropriate notes have been added:

https://developer.mozilla.org/en/JavaScript/Reference/Operators/Special_Operators/set_Operator
https://developer.mozilla.org/en/JavaScript/Reference/Operators/Special_Operators/get_Operator
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.