Closed Bug 536472 Opened 10 years ago Closed 9 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)



(Keywords: dev-doc-complete, Whiteboard: fixed-in-tracemonkey)


(5 files)

No description provided.
Duplicate of this bug: 554616
Whiteboard: [good first bug]
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...
Whiteboard: [good first bug]
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)
...following up on bug 581744.
Attachment #462148 - Flags: review?(cdleary)
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)
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)
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.

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+
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.