Closed Bug 1175394 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- affected
firefox43 --- fixed

People

(Reporter: 446240525, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(2 files)

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
See Also: → 1175823
Attached patch Part 1 - FixSplinter Review
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)
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)
(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+
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
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: