1155900 - TypeError not thrown for invalid ObjectAssignmentPattern

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Pennisi [:jugglinmike]]

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.

Comment 1

[Jeff Walden [:Waldo]]

Pretty easily fixt, patch shortly.

Comment 2

[Jeff Walden [:Waldo]]

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.

Comment 3

[Jeff Walden [:Waldo]]

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.  :-)

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Pass destructuring right-hand-side expressions through ToObject before properties are destructured out of them

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.  :-(

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Equivalently make GetIterator perform ToObject before doing GetMethod(..., @@iterator), and add tests

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.

Comment 6

[Shu-yu Guo [: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.

Comment 7

[Shu-yu Guo [: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.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Use the ToObject intrinsic instead

Hmm, yeah, this is a lot simpler.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Use ToObject for GetIterator, too

Comment 10

[Jeff Walden [:Waldo]]

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.

Comment 11

[Shu-yu Guo [:shu]]

Comment on attachment 8596986 [details] [diff] [review]
Use the ToObject intrinsic instead

Review of attachment 8596986 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d7b3b8cce5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/195a3736c877

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3bb908f6fc

Comment 14

[André Bargull [:anba]]

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

[Ryan VanderMeulen [:RyanVM]]

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

[André Bargull [:anba]]

(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

[André Bargull [:anba]]

Attachment: bug1155900.diff

Shouldn't it be possible to use CheckObjectCoercible plus some extra logic for empty destructuring assignment and binding destructuring?

Comment 18

[Jeff Walden [:Waldo]]

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

[André Bargull [:anba]]

(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

[André Bargull [:anba]]

Attachment: bug1155900.diff

Updated CheckObjectCoercible patch (https://bugs.ecmascript.org/show_bug.cgi?id=4351 added RequireObjectCoercible for non-empty destructuring assignment).

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Final patch

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.

Comment 22

[Shu-yu Guo [: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?

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/241420daaa13

Comment 24

[André Bargull [:anba]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/241420daaa13

Comment 26

[Mike Pennisi [:jugglinmike]]

Awesome--thanks!