Last Comment Bug 770567 - Reflect API implementation conflicts with documentation for NewExpression
: Reflect API implementation conflicts with documentation for NewExpression
Status: RESOLVED WONTFIX
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.8 Branch
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Dave Herman [:dherman]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-03 09:28 PDT by Michael Ficarra
Modified: 2012-08-22 22:16 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1, rename 'callee' to 'constructor' of NewExpression (4.88 KB, patch)
2012-07-03 10:36 PDT, Yusuke Suzuki
jorendorff: review+
Details | Diff | Review

Description Michael Ficarra 2012-07-03 09:28:41 PDT
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 2012-07-03 10:36:50 PDT
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.
Comment 2 Dave Herman [:dherman] 2012-07-03 11:30:15 PDT
Ariya, what does Esprima do?

Dave
Comment 3 Michael Ficarra 2012-07-03 11:32:08 PDT
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] 2012-07-03 11:52:26 PDT
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 2012-07-04 17:35:52 PDT
I also like constructor for NewExpression.
Comment 6 Michael Ficarra 2012-07-12 14:42:17 PDT
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] 2012-08-19 21:11:06 PDT
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] 2012-08-20 10:17:09 PDT
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 2012-08-22 13:23:07 PDT
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] 2012-08-22 13:32:59 PDT
Good point. In light of that, documenting it as 'callee' might be the right move.
Comment 11 Michael Ficarra 2012-08-22 13:38:02 PDT
`ctor` is an option if abbreviations are acceptable.
Comment 12 Dave Herman [:dherman] 2012-08-22 16:36:01 PDT
(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 2012-08-22 17:02:40 PDT
I agree, let's just stick with `callee`.
Comment 14 Yusuke Suzuki 2012-08-22 22:00:15 PDT
(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] 2012-08-22 22:16:40 PDT
I'll just close out the bug.

Dave

Note You need to log in before you can comment on or make changes to this bug.