Make a strict function's "name" property non-writable.

Make a strict function's "name" property non-writable.

Set [[Writable]] to false for the "name" property of strict
functions as well, mirroring what non-strict functions have
it as.

LOG=N
R=rossberg@chromium.org
TEST=mjsunit/regress/regress-270142
BUG=270142
Committed: https://code.google.com/p/v8/source/detail?r=18387

[sof, 2013-12-03 15:44:33]

At your leisure, please take a look.

(I hope I'm following expected process for this v8 CL & review. Please let me
know if not :) )

[Michael Starzinger, 2013-12-03 17:57:17]

The only kind-of-spec for the "name" property on function objects is the
proposal on the Harmony wiki (please correct me if there is a more precise
specification that I am missing). 

http://wiki.ecmascript.org/doku.php?id=harmony:function_name_property

This proposal actually specs the property to be writable (even for strict
functions). So while I agree that having different attributes for strict and
non-strict functions is weird, I am not sure this fix is going into the right
direction.

Adding Andreas as a reviewer for his opinion on the matter.

[rossberg, 2013-12-03 18:06:23]

On 2013/12/03 17:57:17, Michael Starzinger wrote:
> The only kind-of-spec for the "name" property on function objects is the
> proposal on the Harmony wiki (please correct me if there is a more precise
> specification that I am missing). 
> 
> http://wiki.ecmascript.org/doku.php?id=harmony:function_name_property
> 
> This proposal actually specs the property to be writable (even for strict
> functions). So while I agree that having different attributes for strict and
> non-strict functions is weird, I am not sure this fix is going into the right
> direction.
> 
> Adding Andreas as a reviewer for his opinion on the matter.

Actually, in ES6, .name will be non-writable but configurable (that is already
spec'ed in the draft). Could you modify the CL to do that?

[sof, 2013-12-03 18:11:19]

On 2013/12/03 18:06:23, rossberg wrote:
> On 2013/12/03 17:57:17, Michael Starzinger wrote:
> > The only kind-of-spec for the "name" property on function objects is the
> > proposal on the Harmony wiki (please correct me if there is a more precise
> > specification that I am missing). 
> > 
> > http://wiki.ecmascript.org/doku.php?id=harmony:function_name_property
> > 
> > This proposal actually specs the property to be writable (even for strict
> > functions). So while I agree that having different attributes for strict and
> > non-strict functions is weird, I am not sure this fix is going into the
right
> > direction.
> > 
> > Adding Andreas as a reviewer for his opinion on the matter.
> 
> Actually, in ES6, .name will be non-writable but configurable (that is already
> spec'ed in the draft). Could you modify the CL to do that?

The behaviour added here for this old compat property aligns with other engines,
sorry for not mentioning that.

Do you want to make it non-writable for non-strict functions also?

[rossberg, 2013-12-03 18:27:27]

On 2013/12/03 18:11:19, sof wrote:
> On 2013/12/03 18:06:23, rossberg wrote:
> > On 2013/12/03 17:57:17, Michael Starzinger wrote:
> > > The only kind-of-spec for the "name" property on function objects is the
> > > proposal on the Harmony wiki (please correct me if there is a more precise
> > > specification that I am missing). 
> > > 
> > > http://wiki.ecmascript.org/doku.php?id=harmony:function_name_property
> > > 
> > > This proposal actually specs the property to be writable (even for strict
> > > functions). So while I agree that having different attributes for strict
and
> > > non-strict functions is weird, I am not sure this fix is going into the
> right
> > > direction.
> > > 
> > > Adding Andreas as a reviewer for his opinion on the matter.
> > 
> > Actually, in ES6, .name will be non-writable but configurable (that is
already
> > spec'ed in the draft). Could you modify the CL to do that?
> 
> The behaviour added here for this old compat property aligns with other
engines,
> sorry for not mentioning that.
> 
> Do you want to make it non-writable for non-strict functions also?

Yeah, I'm a bit scared this might break the web. I don't think we want to try
this without a flag.

OTOH, I also do not see much point in only changing it for strict mode
functions. So to be honest, I would rather hold off this change for the time
being.

[sof, 2013-12-03 18:45:02]

On 2013/12/03 18:27:27, rossberg wrote:
> On 2013/12/03 18:11:19, sof wrote:
> > On 2013/12/03 18:06:23, rossberg wrote:
> > > On 2013/12/03 17:57:17, Michael Starzinger wrote:
> > > > The only kind-of-spec for the "name" property on function objects is the
> > > > proposal on the Harmony wiki (please correct me if there is a more
precise
> > > > specification that I am missing). 
> > > > 
> > > > http://wiki.ecmascript.org/doku.php?id=harmony:function_name_property
> > > > 
> > > > This proposal actually specs the property to be writable (even for
strict
> > > > functions). So while I agree that having different attributes for strict
> and
> > > > non-strict functions is weird, I am not sure this fix is going into the
> > right
> > > > direction.
> > > > 
> > > > Adding Andreas as a reviewer for his opinion on the matter.
> > > 
> > > Actually, in ES6, .name will be non-writable but configurable (that is
> already
> > > spec'ed in the draft). Could you modify the CL to do that?
> > 
> > The behaviour added here for this old compat property aligns with other
> engines,
> > sorry for not mentioning that.
> > 
> > Do you want to make it non-writable for non-strict functions also?
> 
> Yeah, I'm a bit scared this might break the web. I don't think we want to try
> this without a flag.
> 
> OTOH, I also do not see much point in only changing it for strict mode
> functions. So to be honest, I would rather hold off this change for the time
> being.

That was my concern also, but I confused [[Configurable]] with [[Writable]] in
what you were suggesting for "name" when asking that question.

So, the change requested is to make [[Configurable]] for "name" be true for both
strict and non-strict functions. The handling of [[Writable]] stays as it is
here.

fwiw, i think that's fine (without a flag)... agree?

[sof, 2013-12-05 15:53:35]

On 2013/12/03 18:06:23, rossberg wrote:
> On 2013/12/03 17:57:17, Michael Starzinger wrote:
> > The only kind-of-spec for the "name" property on function objects is the
> > proposal on the Harmony wiki (please correct me if there is a more precise
> > specification that I am missing). 
> > 
> > http://wiki.ecmascript.org/doku.php?id=harmony:function_name_property
> > 
> > This proposal actually specs the property to be writable (even for strict
> > functions). So while I agree that having different attributes for strict and
> > non-strict functions is weird, I am not sure this fix is going into the
right
> > direction.
> > 
> > Adding Andreas as a reviewer for his opinion on the matter.
> 
> Actually, in ES6, .name will be non-writable but configurable (that is already
> spec'ed in the draft). Could you modify the CL to do that?

Done. (Apologies for not following up right away, other issues demanded my
attention.)

As you can see, this exposes a couple of issues:

 - the interaction with a function also having "name" on its proto chain and
implemented
   by a callback accessor, makes for quite interesting behavior if you delete
the property
   and then attempt to assign or redefine it. Spent quite some time making
object-observe.js
   also handle such a property, but it became too ad-hoc, that I ended up
blacklisting
   it. Not ideal.
 - redefining "name" runs into unusual interactions upon re-definition, see
regress-1530.js 
   A bug uncovered?

[rossberg, 2013-12-06 10:34:30]

On 2013/12/05 15:53:35, sof wrote:
> On 2013/12/03 18:06:23, rossberg wrote:
> > On 2013/12/03 17:57:17, Michael Starzinger wrote:
> > > The only kind-of-spec for the "name" property on function objects is the
> > > proposal on the Harmony wiki (please correct me if there is a more precise
> > > specification that I am missing). 
> > > 
> > > http://wiki.ecmascript.org/doku.php?id=harmony:function_name_property
> > > 
> > > This proposal actually specs the property to be writable (even for strict
> > > functions). So while I agree that having different attributes for strict
and
> > > non-strict functions is weird, I am not sure this fix is going into the
> right
> > > direction.
> > > 
> > > Adding Andreas as a reviewer for his opinion on the matter.
> > 
> > Actually, in ES6, .name will be non-writable but configurable (that is
already
> > spec'ed in the draft). Could you modify the CL to do that?
> 
> Done. (Apologies for not following up right away, other issues demanded my
> attention.)
> 
> As you can see, this exposes a couple of issues:
> 
>  - the interaction with a function also having "name" on its proto chain and
> implemented
>    by a callback accessor, makes for quite interesting behavior if you delete
> the property
>    and then attempt to assign or redefine it. Spent quite some time making
> object-observe.js
>    also handle such a property, but it became too ad-hoc, that I ended up
> blacklisting
>    it. Not ideal.
>  - redefining "name" runs into unusual interactions upon re-definition, see
> regress-1530.js 
>    A bug uncovered?

Yeah, these sounds like bugs. Can you reproduce both issues with ordinary JS
code? If so, can you please file them as bug reports?

These issues make me somewhat reluctant to land this change for now. We should
probably understand and fix the issues first.

Another thought: ES6 also makes .length configurable. And for consistency, we
should probably apply the same change to the (non-official) .caller and
.arguments properties. It would be best to do that all in the same CL. (Sorry
for not pointing this out earlier.)

[rossberg, 2013-12-06 10:35:43]

https://codereview.chromium.org/99203006/diff/20001/test/mjsunit/harmony/obje...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/99203006/diff/20001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:1136: // A read-only, configurable
property with another along the prototype chain.
Nit: line length

[sof, 2013-12-06 11:06:04]

On 2013/12/06 10:34:30, rossberg wrote:
> On 2013/12/05 15:53:35, sof wrote:
> > On 2013/12/03 18:06:23, rossberg wrote:
> > > On 2013/12/03 17:57:17, Michael Starzinger wrote:
> > > > The only kind-of-spec for the "name" property on function objects is the
> > > > proposal on the Harmony wiki (please correct me if there is a more
precise
> > > > specification that I am missing). 
> > > > 
> > > > http://wiki.ecmascript.org/doku.php?id=harmony:function_name_property
> > > > 
> > > > This proposal actually specs the property to be writable (even for
strict
> > > > functions). So while I agree that having different attributes for strict
> and
> > > > non-strict functions is weird, I am not sure this fix is going into the
> > right
> > > > direction.
> > > > 
> > > > Adding Andreas as a reviewer for his opinion on the matter.
> > > 
> > > Actually, in ES6, .name will be non-writable but configurable (that is
> already
> > > spec'ed in the draft). Could you modify the CL to do that?
> > 
> > Done. (Apologies for not following up right away, other issues demanded my
> > attention.)
> > 
> > As you can see, this exposes a couple of issues:
> > 
> >  - the interaction with a function also having "name" on its proto chain and
> > implemented
> >    by a callback accessor, makes for quite interesting behavior if you
delete
> > the property
> >    and then attempt to assign or redefine it. Spent quite some time making
> > object-observe.js
> >    also handle such a property, but it became too ad-hoc, that I ended up
> > blacklisting
> >    it. Not ideal.
> >  - redefining "name" runs into unusual interactions upon re-definition, see
> > regress-1530.js 
> >    A bug uncovered?
> 
> Yeah, these sounds like bugs. Can you reproduce both issues with ordinary JS
> code? If so, can you please file them as bug reports?
> 

Not readily (RO configurable properties implemented as foreign callback
accessors aren't easy to find), but will try some more.

> These issues make me somewhat reluctant to land this change for now. We should
> probably understand and fix the issues first.
> 

Agreed; however, the original CL patchset had a straight bugfix (having
Function.name's [[Writable]] as true for stricts doesn't make much sense if
non-stricts has it as false.)

> Another thought: ES6 also makes .length configurable. And for consistency, we
> should probably apply the same change to the (non-official) .caller and
> .arguments properties. It would be best to do that all in the same CL. (Sorry
> for not pointing this out earlier.)

thanks, so it (length) does. Makes good sense; should I revert the
[[Configurable]] change here & file one or two bugs? I may have a go at the
[[Configurable]] change.

[rossberg, 2013-12-06 11:57:48]

On 2013/12/06 11:06:04, sof wrote:
> thanks, so it (length) does. Makes good sense; should I revert the
> [[Configurable]] change here & file one or two bugs? I may have a go at the
> [[Configurable]] change.

Yes, sounds good. Sorry for having you take the detour.

[sof, 2013-12-06 12:22:16]

On 2013/12/06 11:57:48, rossberg wrote:
> On 2013/12/06 11:06:04, sof wrote:
> > thanks, so it (length) does. Makes good sense; should I revert the
> > [[Configurable]] change here & file one or two bugs? I may have a go at the
> > [[Configurable]] change.
> 
> Yes, sounds good. Sorry for having you take the detour.

It is/was fun :) Done - I'll follow up with bug reporting.

[rossberg, 2013-12-06 12:26:17]

LGTM, will land it

[Dmitry Lomov (no reviews), 2013-12-06 12:27:49]

On 2013/12/06 12:26:17, rossberg wrote:
> LGTM, will land it

sheriff's note: please do not land before the branch point. Thanks!

[sof, 2013-12-07 09:00:44]

Filed https://code.google.com/p/v8/issues/detail?id=3045

[rossberg, 2013-12-20 12:06:18]

Committed patchset #3 manually as r18387 (presubmit successful).