520696 - Implement support for |{ get "string literal"() { /* ... */ }, get 5.4() { /* ... */ }, 6.72: 3 }|

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jeff Walden [:Waldo]]

ES5's rules for object literals:

ObjectLiteral :
      { }
      { PropertyNameAndValueList }
      { PropertyNameAndValueList , }
PropertyNameAndValueList :
      PropertyAssignment
      PropertyNameAndValueList , PropertyAssignment
PropertyAssignment :
      PropertyName : AssignmentExpression
      get PropertyName ( ) { FunctionBody }
      set PropertyName ( PropertySetParameterList ) { FunctionBody }
PropertyName :
      IdentifierName
      StringLiteral
      NumericLiteral
PropertySetParameterList :
      Identifier

We support |PropertyName: AssignmentExpression|, and we support |[gs]et IdentifierName|, but we don't support |[gs]et (String|Numeric)Literal|.  Needs to happen to be able to get rid of |x getter:| syntax.

Comment 1

[Jeff Walden [:Waldo]]

Note that NumericLiteral is any number literal, possibly fractional, possibly exponential (even with positive or negative exponent), not just DecimalIntegerLiteral.

Comment 2

[Jeff Walden [:Waldo]]

...which means we also have to handle { 3.141592654: "π" }.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch

Hm, so we already supported non-DecimalInteger numeric literals as property names (if not as getter/setter names).  That makes things a little simpler.

Comment 4

[Jim Blandy :jimb]

Curious --- why the initialization of funAtom in FunctionDef?  That's definitely set by the first 'if'; was some compiler too dumb to see that?

Comment 5

[Jim Blandy :jimb]

(In reply to comment #3)
> Created an attachment (id=417717) [details]
> Patch
> 
> Hm, so we already supported non-DecimalInteger numeric literals as property
> names (if not as getter/setter names).  That makes things a little simpler.

Yeah; that's tested in js/src/tests/ecma_5/strict/11.1.5.js.

Comment 6

[Jim Blandy :jimb]

Shouldn't those tests go in js/src/tests/ecma_5/Expressions/11.1.5.js?  We're just implementing what the standard specifies.

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #4)
> why the initialization of funAtom in FunctionDef?

I think that's a slight bit of patch-cest, anticipating a change much later in my mq where I make |{ set foo foo() { } }| (you don't really want to know) a syntax error.

Comment 8

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> Shouldn't those tests go in js/src/tests/ecma_5/Expressions/11.1.5.js?

I don't see an ecma_5/Expressions directory in TM or m-c.

(And to be clear, I can remove the patch-cest if desired, don't think it hurts as is tho.)

Comment 9

[Brendan Eich [:brendan]]

What's a patch-cest? If it's what it sounds like, the taboo against it stands :-/.

/be

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Updated

That hunk more properly fit in the patch atop this one in my mq.  I've moved it out of here and, incidentally, moved the declaration nearer to point of first use; it's probably better moving anyway.

Comment 11

[Brendan Eich [:brendan]]

Yes, please do move declarations to nearest dominating position and initialize 'em -- the old C heritage with a thicket of uninitialized vars makes it easy to miss an initializer (not to mention a var).

/be

Comment 12

[Jim Blandy :jimb]

(In reply to comment #9)
> What's a patch-cest? If it's what it sounds like, the taboo against it stands
> :-/.

Little bits like whitespace cleanup I've decided not to complain

Comment 13

[Jim Blandy :jimb]

[koff koff] ... about because otherwise they'd never get done; no reason to douse someone's enthusiasm by making them do a bunch of dotting and crossing.

But stuff with semantic significance... no cest is best.

Comment 14

[Jim Blandy :jimb]

(In reply to comment #8)
> (In reply to comment #6)
> > Shouldn't those tests go in js/src/tests/ecma_5/Expressions/11.1.5.js?
> 
> I don't see an ecma_5/Expressions directory in TM or m-c.

Yeah, we'll need to make one.

Comment 15

[Jeff Walden [:Waldo]]

Frankly, I don't think much of sub-organizing tests where distinctions don't map to source directories (as with SpiderMonkey where ~everything's in js/src/*); it just results in angst-fests about where they should go.  Even the ecma_5 and friends (especially js1_*) distinction is lost on me -- JS is usually just JS, and subdividing by version is just needless complexity (especially when we stick new tests in the most recent directory regardless whether they're relevant to that version's changes).

Comment 16

[Jim Blandy :jimb]

Comment on attachment 424883 [details] [diff] [review]
Updated

Code changes look good to me; the tests need to go in the right place.

Comment 17

[Jeff Walden [:Waldo]]

Attachment: .

Comment 18

[Jim Blandy :jimb]

Comment on attachment 425129 [details] [diff] [review]
.

Looks good, but (mea culpa) you'll need to fix up the jstest.list changes.  There is now a js/src/tests/ecma_5/extensions directory, so the patches to jstests.list and shell.js will need to be changed.  And doesn't js/src/tests/ecma_5/Expressions need to be added to js/src/tests/ecma_5/jstests.list?

Comment 19

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/63efa8786575

Comment 20

[d]

Is this something worth mentioning at https://developer.mozilla.org/en/Upcoming_Firefox_features_for_developers? I'm not experienced enough with JavaScript to tell, if it should be, myself.

Comment 21

[Jeff Walden [:Waldo]]

Yes, probably, should have thought to add the keyword.

Comment 22

[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

Comment 23

[Eric Shepherd [:sheppy]]

Oops, wrong bug. This hasn't been done yet, I think.

Comment 24

[Eric Shepherd [:sheppy]]

Actually, the same note that fixes the other bug fixes this one too, so this really is doc-complete.