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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla43
People
(Reporter: 446240525, Assigned: jandem)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])
Attachments
(2 files)
23.56 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
41.99 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•9 years ago
|
||
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 | ||
Comment 2•9 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)
Comment 3•9 years ago
|
||
(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•9 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 5•9 years ago
|
||
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•9 years ago
|
||
This patch renames normal/strict to mapped/unmapped everywhere. Also some minor cleanup to the ArgumentsObject class hooks.
Attachment #8654889 -
Flags: review?(jorendorff)
Comment 7•9 years ago
|
||
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/integration/mozilla-inbound/rev/cd0f55213a14 https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6121595722
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cd0f55213a14 https://hg.mozilla.org/mozilla-central/rev/8c6121595722
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment 10•9 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
Comment 11•9 years ago
|
||
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.
Description
•