1175394 - Mapped arguments object should only be created when its FormalParameters is a SimpleParameterList

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[ziyunfei]

Quotes from spec 9.2.12: "mapped argument object is only provided for non-strict functions that don’t have a rest parameter, any parameter default value initializers, or any destructured parameters"

js> (function(a=1){arguments[0]=100;return a})(10)
100 // should be 10

js> (function({}={}){return typeof arguments.callee})()
"function" // should be a TypeError

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix

I said I'd fix this after bug 889158 so here we go.

This patch adds a hasMappedArgsObj flag to JSScript to indicate we need a mapped arguments object. Then we use that flag instead of strict().

Comment 2

[Jan de Mooij [:jandem]]

I'm not sure about the NormalArgumentsObject and StrictArgumentsObject classes. Should I rename them to MappedArgumentsObject and UnmappedArgumentsObject? Also StrictArgGetter -> UnmappedArgGetter etc.

I can see how normal/strict might be clearer than mapped/unmapped, although the latter is more precise and what the spec uses.

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> I'm not sure about the NormalArgumentsObject and StrictArgumentsObject
> classes. Should I rename them to MappedArgumentsObject and
> UnmappedArgumentsObject? Also StrictArgGetter -> UnmappedArgGetter etc.

Yes!

(That reminds me, ES6 redefines how this stuff works yet again, to not use getters at all, but fake data properties. Need to file that.)

> I can see how normal/strict might be clearer than mapped/unmapped, although
> the latter is more precise and what the spec uses.

Mapped/unmapped is better. We'll get used to it, and normal/strict is really misleading, given this bizarro rule.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> (In reply to Jan de Mooij [:jandem] from comment #2)
> > I'm not sure about the NormalArgumentsObject and StrictArgumentsObject
> > classes. Should I rename them to MappedArgumentsObject and
> > UnmappedArgumentsObject? Also StrictArgGetter -> UnmappedArgGetter etc.
> 
> Yes!

Thanks, will post a second patch to do this :)

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8652787 [details] [diff] [review]
Part 1 - Fix

Review of attachment 8652787 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ArgumentsObject.cpp
@@ +206,5 @@
>  template <typename CopyArgs>
>  /* static */ ArgumentsObject*
>  ArgumentsObject::create(JSContext* cx, HandleFunction callee, unsigned numActuals, CopyArgs& copy)
>  {
> +    bool strict = !callee->nonLazyScript()->hasMappedArgsObj(); // TODO: rename "strict" everywhere?

yes please

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Rename normal/strict to mapped/unmapped

This patch renames normal/strict to mapped/unmapped everywhere.

Also some minor cleanup to the ArgumentsObject class hooks.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8654889 [details] [diff] [review]
Part 2 - Rename normal/strict to mapped/unmapped

Review of attachment 8654889 [details] [diff] [review]:
-----------------------------------------------------------------

Tricky patch to review, due to the sense of a bunch of boolean parameters flipping from `strict` to `mapped`, but it looks perfect. r=me.

::: js/src/vm/ArgumentsObject.cpp
@@ +505,5 @@
>      }
>  
>      /*
>       * For simplicity we use delete/define to replace the property with a
> +     * simple data property. Note that we rely on Arguments::obj_delProperty to

ArgumentsObject::obj_delProperty

::: js/src/vm/ArgumentsObject.h
@@ +331,5 @@
>          data()->callee = MagicValue(JS_OVERWRITTEN_CALLEE);
>      }
> +
> +    static bool obj_enumerate(JSContext* cx, HandleObject obj);
> +    static bool obj_resolve(JSContext* cx, HandleObject obj, HandleId id, bool* resolvedp);

Could be private.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd0f55213a14
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6121595722

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd0f55213a14
https://hg.mozilla.org/mozilla-central/rev/8c6121595722

Comment 10

[Florian Scholz (Open Web Docs)]

Review appreciated.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments#Rest_default_and_destructured_parameters
https://developer.mozilla.org/en-US/Firefox/Releases/43#JavaScript

Comment 11

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/ecmascript-arguments-implementation-has-been-updated/