Closed
Bug 1155900
Opened 9 years ago
Closed 9 years ago
TypeError not thrown for invalid ObjectAssignmentPattern
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: jugglinmike, Assigned: Waldo)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 3 obsolete files)
9.32 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
9.95 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
2.75 KB,
patch
|
Details | Diff | Splinter Review | |
24.31 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
SpiderMonkey build: 07bb578e6a93d11a3e1365e2b0d9296a755543ee (Fri Apr 17 13:52:50 2015 -0700) Architecture: x64 (Ubuntu Linux) The following forms currently do not generate an error: ({} = undefined); ({} = null); ...but the ES6 specification dictates a SyntaxError in both cases: > 12.14.5.2 Runtime Semantics: DestructuringAssignmentEvaluation > > with parameter value > > ObjectAssignmentPattern : { } > > 1. Let valid be RequireObjectCoercible(value). > 2. ReturnIfAbrupt(valid). > 3. Return NormalCompletion(empty). ...and: > 7.2.1 RequireObjectCoercible ( argument ) > > The abstract operation RequireObjectCoercible throws > an error if argument is a value that cannot be converted > to an Object using ToObject. It is defined by Table 14: > > Table 14 — RequireObjectCoercible Results > > Argument Type | Result > ------------------|------------------------------------ > Completion Record | If argument is an abrupt completion, > | return argument. Otherwise return > | RequireObjectCoercible(argument.[[value]]). > Undefined | Throw a TypeError exception. > Null | Throw a TypeError exception. > Boolean | Return argument. > Number | Return argument. > String | Return argument. > Symbol | Return argument. > Object | Return argument.
Assignee | ||
Comment 1•9 years ago
|
||
Pretty easily fixt, patch shortly.
Assignee | ||
Comment 2•9 years ago
|
||
Well, mostly easily fixt. Then I had to push through the JIT bits, which weren't that bad but presented slight roadbumps to someone not regularly deep in JITs. Oh, and I found another bug in the process: Boolean.prototype[Symbol.iterator] = function() { "use strict"; assertEq(typeof this, "object"); return { next() { return { done: true }; } }; }; for (var i of false) assertEq(true, false, "not reached"); ...the result of which is: typein:3:64 Error: Assertion failed: got "boolean", expected "object" The cause of that bug is that the GetIterator-alike we use when ascribing semantics to various iteration constructs in the frontend doesn't do ToObject, either. So we do val[@@iterator]() when we should be doing ToObject(val)[@@iterator]. I'm going to fix that here as well, albeit in a separate patch.
Assignee | ||
Comment 3•9 years ago
|
||
Actually, no, comment 2's test is wrong. The @@iterator function is invoked with the original value, so should be "boolean" above. It's the *getting* of the function that occurs on a boxed primitive. Which implicates bug 603201, so I guess for that aspect of things I'll make the change and land the tests, with the caveat that the tests pass right now because two wrongs make a right. :-)
Assignee | ||
Comment 4•9 years ago
|
||
This seems to be visible only for null-destructurings, e.g. ({} = null), and for the |this| inside the iterator function retrieved for a boxed primitive. We always box up |this|, so only the former is visible. That devtools test change ate up far more of my time than I care to admit. :-(
Attachment #8595100 -
Flags: review?(shu)
Assignee | ||
Comment 5•9 years ago
|
||
We previous just did the lookup without boxing, which would throw for null/undefined, so that aspect isn't visible. That the |this| passed to the @@iterator function is boxed is similarly not visible, because of bug 603201. Add some tests to verify this, for the near future when bug 603201 is fixed.
Attachment #8595102 -
Flags: review?(shu)
Comment 6•9 years ago
|
||
Comment on attachment 8595100 [details] [diff] [review] Pass destructuring right-hand-side expressions through ToObject before properties are destructured out of them Review of attachment 8595100 [details] [diff] [review]: ----------------------------------------------------------------- As discussed on IRC, I feel like the extra opcode is unnecessary. If we can emit a call to the intrinsic ToObject function, Ion already has an inline for that, and we can avoid the extra opcode and all the complexity it entails.
Attachment #8595100 -
Flags: review?(shu)
Comment 7•9 years ago
|
||
Comment on attachment 8595102 [details] [diff] [review] Equivalently make GetIterator perform ToObject before doing GetMethod(..., @@iterator), and add tests Review of attachment 8595102 [details] [diff] [review]: ----------------------------------------------------------------- LGTM up to rebasing to calling the ToObject intrinsic approach.
Attachment #8595102 -
Flags: review?(shu) → review+
Assignee | ||
Comment 8•9 years ago
|
||
Hmm, yeah, this is a lot simpler.
Attachment #8596986 -
Flags: review?(shu)
Assignee | ||
Updated•9 years ago
|
Attachment #8595100 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8596987 -
Flags: review?(shu)
Assignee | ||
Updated•9 years ago
|
Attachment #8595102 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8596987 [details] [diff] [review] Use ToObject for GetIterator, too Oops, missed the r+. The one-liner change is obvious and trivial atop the other patch, no need for a second look.
Attachment #8596987 -
Flags: review?(shu) → review+
Comment 11•9 years ago
|
||
Comment on attachment 8596986 [details] [diff] [review] Use the ToObject intrinsic instead Review of attachment 8596986 [details] [diff] [review]: ----------------------------------------------------------------- Beautiful.
Attachment #8596986 -
Flags: review?(shu) → review+
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d7b3b8cce5a https://hg.mozilla.org/integration/mozilla-inbound/rev/195a3736c877
Comment 14•9 years ago
|
||
This test case will fail with the current patch (it used to work correctly): --- var objects = []; Object.defineProperty(String.prototype, "quux", { get() { objects.push(this); } }); var {quux, quux} = ""; assertEq(objects.length, 2); assertEq(objects[0] === objects[1], false); --- The spec requires the following steps: --- var {quux, quux} = value; => let $temp = RequireObjectCoercible(value); var quux = ToObject($temp).quux; var quux = ToObject($temp).quux; ---
Comment 15•9 years ago
|
||
Backed out for frequent browser_net_cached-status.js failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/c119270176bb https://treeherder.mozilla.org/logviewer.html#?job_id=9486391&repo=mozilla-inbound
Comment 16•9 years ago
|
||
(In reply to André Bargull from comment #14) > The spec requires the following steps: > --- > var {quux, quux} = value; > > => > > let $temp = RequireObjectCoercible(value); > var quux = ToObject($temp).quux; > var quux = ToObject($temp).quux; > --- Wow, that was so wrong. I was mislead by the ToObject call in ES6, 7.3.2 GetV. Actually only this is required: --- let $temp = RequireObjectCoercible(value); var quux = $temp.quux; var quux = $temp.quux; ---
Comment 17•9 years ago
|
||
Shouldn't it be possible to use CheckObjectCoercible plus some extra logic for empty destructuring assignment and binding destructuring?
Assignee | ||
Comment 18•9 years ago
|
||
Yeah, it should be. Given the slight confusion in all this wording, or at least in my understanding of it, at this point I want a clear answer to the questions in https://bugs.ecmascript.org/show_bug.cgi?id=4351 before I spend the time to finish this up. :-) There's no real rush to fix this.
Comment 19•9 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18) > There's no real rush to fix this. Yes, agreed.
Comment 20•9 years ago
|
||
Updated CheckObjectCoercible patch (https://bugs.ecmascript.org/show_bug.cgi?id=4351 added RequireObjectCoercible for non-empty destructuring assignment).
Attachment #8600501 -
Attachment is obsolete: true
Assignee | ||
Comment 21•9 years ago
|
||
Sigh, this bug has been one big mess. :-( This patch makes object pattern destructuring be prefaced with a call to RequireObjectCoercible, per spec. It renames that method from CheckObjectCoercible, its name in ES5. If we were to optimize it, we *could not* use existing ToObject code: we'd need a new op and new JIT handling for it. Perhaps something to do eventually, as noted in patch comments, but let's get this *right* and then leave JIT handling to some future time, if desired. The modification to browser_net_cached-status.js is necessary to deal with the intermittent orange mentioned in comment 15. I don't believe there's any way this bug directly triggered that one. The failure mode is identical to that of browser_net_charts-01.js, whose potential error message I changed in earlier patching here. The failure tracks back to a |let { a, b } = c;| in devtools code, where |c| is wrongly |undefined| because something in service worker code causes promises to be resolved with 100% wrong semantics, based on what other service worker promises are in flight at the time. I have no idea why the current code isn't erroring in identical way to that test, but there's no reason why it couldn't. So I copied over the this-could-fail code from that test, into this other one, to deal with the orange in comment 15. It happens, on closer inspection, that there's nothing wrong with the array-destructuring code *at all*. I misread the GetIterator spec code. :-( GetIterator(v) does GetMethod(v, @@iterator). GetMethod does GetV(v, @@iterator). And while that does ToObject(v), then does .[[Get]] for @@iterator on that, it passes v as the receiver for the [[Get]]. So while officially an object is created, it's never *exposed*. I missed the receiver bit of this. :-( So, no GetIterator adjustments are needed. This *should* be the last thing that has to be done for this bug. And no ToObject additions, reuses, code-sharing, etc. are needed.
Attachment #8603616 -
Flags: review?(shu)
Comment 22•9 years ago
|
||
Comment on attachment 8603616 [details] [diff] [review] Final patch Review of attachment 8603616 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +4962,5 @@ > + // bytecode versus 1 byte for a dedicated opcode. As more places need this > + // behavior, we may want to reconsider this tradeoff. > + > +#ifdef DEBUG > + auto depth = this->stackDepth; Maybe a DebugOnly? (Out of curiosity, why is this an auto?) @@ +4968,5 @@ > + MOZ_ASSERT(depth > 0); // VAL > + if (!emit1(JSOP_DUP)) // VAL VAL > + return false; > + > + // Note that "intrinsic" is a misnomer: we're calling a *self-hosted* lol ::: js/src/jit-test/tests/basic/destructuring-requireobjectcoercible.js @@ +98,5 @@ > + > + assertThrowsInstanceOf(() => g(null), TypeError); > + assertThrowsInstanceOf(() => g(undefined), TypeError); > +} > +if (primitiveThisSupported()) What is this?
Attachment #8603616 -
Flags: review?(shu) → review+
Comment 24•9 years ago
|
||
Comment on attachment 8603616 [details] [diff] [review] Final patch Review of attachment 8603616 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/basic/destructuring-requireobjectcoercible.js @@ +57,5 @@ > + { get() { return typeof this; }, > + enumerable: true, > + configurable: true }); > + > + return 3.14.custom === "number"; Typo? "custom" -> "v" |return 3.14.v === "number";|
Comment 25•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/241420daaa13
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Reporter | ||
Comment 26•9 years ago
|
||
Awesome--thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•