Reflect API implementation conflicts with documentation for NewExpression

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: Michael Ficarra, Assigned: dherman)

Tracking

1.8 Branch
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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

5 years ago
Created attachment 638794 [details] [diff] [review]
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.
(Assignee)

Comment 2

5 years ago
Ariya, what does Esprima do?

Dave
(Reporter)

Comment 3

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

Esprima currently mirrors the SpiderMonkey behaviour.
(Assignee)

Comment 4

5 years ago
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

5 years ago
I also like constructor for NewExpression.
(Reporter)

Comment 6

5 years ago
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.
Whiteboard: [js:t]
(Assignee)

Comment 7

5 years ago
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
(Assignee)

Comment 8

5 years ago
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
Attachment #638794 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Assignee: general → dherman
Attachment #638794 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 9

5 years ago
Something to think about: assigning an own-property named `constructor` masks the `constructor` prototype property. This could be inconvenient for interop.
Good point. In light of that, documenting it as 'callee' might be the right move.
(Reporter)

Comment 11

5 years ago
`ctor` is an option if abbreviations are acceptable.
(Assignee)

Comment 12

5 years ago
(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

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

Comment 14

5 years ago
(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?
(Assignee)

Comment 15

5 years ago
I'll just close out the bug.

Dave
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.