Closed Bug 1155900 Opened 9 years ago Closed 9 years ago

TypeError not thrown for invalid ObjectAssignmentPattern

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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)

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.
Pretty easily fixt, patch shortly.
Assignee: nobody → jwalden+bmo
Blocks: es6
OS: Linux → All
Hardware: x86_64 → All
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.
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.  :-)
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)
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 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 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+
Hmm, yeah, this is a lot simpler.
Attachment #8596986 - Flags: review?(shu)
Attachment #8595100 - Attachment is obsolete: true
Attachment #8596987 - Flags: review?(shu)
Attachment #8595102 - Attachment is obsolete: true
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 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+
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;
---
(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;
---
Attached patch bug1155900.diff (obsolete) — Splinter Review
Shouldn't it be possible to use CheckObjectCoercible plus some extra logic for empty destructuring assignment and binding destructuring?
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.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18)
> There's no real rush to fix this.

Yes, agreed.
Attached patch bug1155900.diffSplinter Review
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
Attached patch Final patchSplinter Review
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 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 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";|
https://hg.mozilla.org/mozilla-central/rev/241420daaa13
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Awesome--thanks!
You need to log in before you can comment on or make changes to this bug.