770567 - Reflect API implementation conflicts with documentation for NewExpression

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Michael Ficarra]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_3) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.56 Safari/536.5

Steps to reproduce:

Use the Reflect API to parse a NewExpression.


Actual results:

It generates an object with a `callee` property and no `constructor` property.


Expected results:

According to the documentation (https://developer.mozilla.org/en/SpiderMonkey/Parser_API#Expressions), it should generate an object with a `constructor` property and no `callee` property.

Unfortunately, http://hg.mozilla.org/mozilla-central/file/f38d6df93cad/js/src/jsreflect.cpp#l1077 causes it to produce an object that conflicts with the documentation. 

See related esprima bug: http://code.google.com/p/esprima/issues/detail?id=290

I, personally, prefer the `constructor` name from the documentation. It is more descriptive.

Comment 1

[Yusuke Suzuki]

Attachment: v1, rename 'callee' to 'constructor' of NewExpression

Attached patch, renaming 'callee' to 'constructor' of NewExpression in Reflect API and jstests.
Personally, I also prefer the `constructor` name from.

Comment 2

[Dave Herman [:dherman]]

Ariya, what does Esprima do?

Dave

Comment 3

[Michael Ficarra]

dherman: See the esprima bug I linked above: http://code.google.com/p/esprima/issues/detail?id=290

Esprima currently mirrors the SpiderMonkey behaviour.

Comment 4

[Dave Herman [:dherman]]

I can't address this till I get back from vacation mid-month, but we do have a question of how many people will be affected by API changes. There are now a couple of public changes I'd like to make, and Ariya has said he's OK with it, but I don't know how many people would be affected.

Let me just write a quick blog post asking if people would be upset about a few minor changes like this.

Dave

Comment 5

[Ariya Hidayat]

I also like constructor for NewExpression.

Comment 6

[Michael Ficarra]

It appears that Program is also inconsistent. The interface description in the documentation claims that Program has an `elements` member that contains a list of Statement, but the implementation provides a `body` member. See http://hg.mozilla.org/mozilla-central/file/f38d6df93cad/js/src/jsreflect.cpp#l714

I favour `body` for consistency with BlockStatement.

Comment 7

[Dave Herman [:dherman]]

Re: comment 6, I've corrected the docs so that Program contains a `body` property rather than `elements`.

Meanwhile, Yusuke's patch looks good. I'll rebase it soon and get a review; hopefully we can land ASAP. This and bug 742612 are the only API changes I need to land, and I want to make sure they both land in the same release, so consumers of Reflect.parse can change all at once, and so Esprima can keep in sync with us.

Thanks,
Dave

Comment 8

[Dave Herman [:dherman]]

Comment on attachment 638794 [details] [diff] [review]
v1, rename 'callee' to 'constructor' of NewExpression

I could post a new patch but it'd look identical (modulo line numbers), so just requesting review on this one.

Thanks,
Dave

Comment 9

[Michael Ficarra]

Something to think about: assigning an own-property named `constructor` masks the `constructor` prototype property. This could be inconvenient for interop.

Comment 10

[Jason Orendorff [:jorendorff]]

Good point. In light of that, documenting it as 'callee' might be the right move.

Comment 11

[Michael Ficarra]

`ctor` is an option if abbreviations are acceptable.

Comment 12

[Dave Herman [:dherman]]

(In reply to Michael Ficarra [groupon acct.] from comment #9)
> Something to think about: assigning an own-property named `constructor`
> masks the `constructor` prototype property. This could be inconvenient for
> interop.

Arg. I'm thinking this particular change is too cosmetic to be worth it. It also makes it less convenient to have logic that doesn't care whether the call was made with `new` or not.

I think I'm just going to drop this change and update the docs to say `callee`. Ariya, what do you think?

Dave

Comment 13

[Ariya Hidayat]

I agree, let's just stick with `callee`.

Comment 14

[Yusuke Suzuki]

(In reply to Dave Herman [:dherman] from comment #12)
> (In reply to Michael Ficarra [groupon acct.] from comment #9)
> > Something to think about: assigning an own-property named `constructor`
> > masks the `constructor` prototype property. This could be inconvenient for
> > interop.
> 
> Arg. I'm thinking this particular change is too cosmetic to be worth it. It
> also makes it less convenient to have logic that doesn't care whether the
> call was made with `new` or not.
> 
> I think I'm just going to drop this change and update the docs to say
> `callee`. Ariya, what do you think?
> 
> Dave

I agree too. So I'll make my patch obsolete, OK?

Comment 15

[Dave Herman [:dherman]]

I'll just close out the bug.

Dave