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

RESOLVED FIXED in Firefox 43

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ziyunfei, Assigned: jandem)

Tracking

(Blocks: 1 bug, {dev-doc-complete, site-compat})

Trunk
mozilla43
dev-doc-complete, site-compat
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 affected, firefox43 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
See Also: → bug 1175823
(Assignee)

Comment 1

3 years ago
Created attachment 8652787 [details] [diff] [review]
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().
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8652787 - Flags: review?(jorendorff)
(Assignee)

Comment 2

3 years ago
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.
Flags: needinfo?(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.
Flags: needinfo?(jorendorff)
(Assignee)

Comment 4

3 years ago
(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 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
Attachment #8652787 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8654889 [details] [diff] [review]
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.
Attachment #8654889 - Flags: review?(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.
Attachment #8654889 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/cd0f55213a14
https://hg.mozilla.org/mozilla-central/rev/8c6121595722
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/ecmascript-arguments-implementation-has-been-updated/
Keywords: site-compat
You need to log in before you can comment on or make changes to this bug.