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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 2

[Jeff Walden [:Waldo]]

Ugh, function parsing is a disaster right now, no way this should have been [good first bug].  Patches coming up to do this...

Comment 3

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: 2: Don't pointlessly set JSPROP_{G,S}ETTER in fun->flags

...following up on bug 581744.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 8

[Brendan Eich [:brendan]]

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

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 462149 [details] [diff] [review]
3: Move function name parsing to callers

Yup, pushing name parsing into the callers makes more sense.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

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.

Comment 11

[Jeff Walden [:Waldo]]

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

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e46a5e0d30b5

Comment 13

[Eric Shepherd [:sheppy]]

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