1066414 - Assertion failure: opn->isArity(PN_NAME), at frontend/ParseNode.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

for (var [...y] in Object) {}

asserts js debug shell on m-c changeset bc7deafdac4b without any CLI flags at Assertion failure: opn->isArity(PN_NAME), at frontend/ParseNode.cpp.

Debug configure flags:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5c2363e6e9ad
user:        Arpad Borsos
date:        Fri Aug 22 10:16:58 2014 +0200
summary:     Bug 933276 - Implement [...x] in assignment target (spread operator); r=jorendorff

Jason, is bug 933276 a possible regressor?

Comment 1

[Arpad Borsos [:Swatinem]]

That might as well be the cause. I will take a look.
A call stack would be helpful though.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

Oops, forgot the stack, so here it is for m-c rev 2db5b64f6d49.

Comment 3

[Arpad Borsos [:Swatinem]]

Interesting. The stack does not include any of the functions I touched.

Is it that cloneLeftHandSide simply can’t deal with the new rest syntax (yet?).

I’ll take a look.

Comment 4

[Arpad Borsos [:Swatinem]]

Attachment: part1: fix assertion

This fixes the assert at least. Still need to figure out what the 
correct behavior here is. for (var [...x] in [1,2,3]) yields "2" for x. 
Need to read up in the spec on this.

Comment 5

[Arpad Borsos [:Swatinem]]

meh, the code is indeed correct. A string is iterable (since when? I haven’t seen that before), so it yields an array with each char.
If you want, I can add tests for that.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8488515 [details] [diff] [review]
part1: fix assertion

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

Nicely done, thank you.

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to Arpad Borsos (Swatinem) from comment #5)
> meh, the code is indeed correct. A string is iterable (since when? I haven’t
> seen that before), so it yields an array with each char.

Right. This changed in rev 26:
  https://bugs.ecmascript.org/show_bug.cgi?id=2884
but I think we implemented it that way before it was technically correct to do so...

> If you want, I can add tests for that.

Sounds good! Passing tests are always welcome. :)

Comment 8

[Arpad Borsos [:Swatinem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4735c318a46

This includes some additional tests.

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b4735c318a46