1073919 - Assertion failure: pc->lastYieldOffset != startYieldOffset, at frontend/Parser.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

function d([{
        [yield]: {}
    }
]) f

asserts js debug shell on m-c changeset 6a63bcb6e0d3 without any CLI arguments at Assertion failure: pc->lastYieldOffset != startYieldOffset, at frontend/Parser.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/7079b7552946
user:        Guptha Rajagopal
date:        Fri Aug 08 09:15:00 2014 -0400
summary:     Bug 924688 - Implement ES6 computed property names. r=jorendorff

Jason, is bug 924688 a possible regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0xc00f4, 0x000000010004306f js-dbg-opt-64-dm-nsprBuild-darwin-6a63bcb6e0d3`js::frontend::Parser<js::frontend::FullParseHandler>::functionBody(this=<unavailable>, kind=<unavailable>, type=<unavailable>) + 847 at Parser.cpp:986, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010004306f js-dbg-opt-64-dm-nsprBuild-darwin-6a63bcb6e0d3`js::frontend::Parser<js::frontend::FullParseHandler>::functionBody(this=<unavailable>, kind=<unavailable>, type=<unavailable>) + 847 at Parser.cpp:986
    frame #1: 0x0000000100043b35 js-dbg-opt-64-dm-nsprBuild-darwin-6a63bcb6e0d3`js::frontend::Parser<js::frontend::FullParseHandler>::functionArgsAndBodyGeneric(this=0x00007fff5fbfe1b0, pn=0x000000010282b820, type=<unavailable>, kind=Statement, fun=<unavailable>) + 677 at Parser.cpp:2401
    frame #2: 0x000000010003483a js-dbg-opt-64-dm-nsprBuild-darwin-6a63bcb6e0d3`js::frontend::Parser<js::frontend::FullParseHandler>::functionArgsAndBody(this=0x00007fff5fbfe1b0, pn=0x000000010282b820, type=Normal, kind=Statement, generatorKind=<unavailable>, newDirectives=<unavailable>, bodyLevelHoistedUse=<unavailable>, fun=<unavailable>, inheritedDirectives=<unavailable>) + 746 at Parser.cpp:2232
    frame #3: 0x0000000100044f63 js-dbg-opt-64-dm-nsprBuild-darwin-6a63bcb6e0d3`js::frontend::Parser<js::frontend::FullParseHandler>::functionDef(this=0x00007fff5fbfe1b0, type=Normal, kind=Statement, generatorKind=NotGenerator, funName=<unavailable>) + 867 at Parser.cpp:2049
    frame #4: 0x000000010004233d js-dbg-opt-64-dm-nsprBuild-darwin-6a63bcb6e0d3`js::frontend::Parser<js::frontend::FullParseHandler>::functionStmt(this=0x00007fff5fbfe1b0) + 589 at Parser.cpp:2476
(lldb)

Comment 2

[guptha]

Jason,

What should [yield] evaluate to? It looks like it evaluates to ToString(yield) - I'm not sure how to follow that. It is legal in this context because of the AssignmentExpression->YieldExpression->yield chain. That's tangential to the actual issue here.

Now, for the actual issue - in the Parser, computedPropertyName->assignExpr->yieldExpression, flow goes to the NotGenerator case, and then falls through to the LegacyGenerator case. This is where pc->lastYieldOffset is set. I'm not entirely sure what is supposed to happen here (no idea what lastYieldOffset is meant for). Any pointers will be appreciated :)

Comment 3

[Jeff Walden [:Waldo]]

I tried briefly following the spec terms for this and got lost a bit.  :-)  But I suspect the desired behavior is for this to be a syntax error, seeing as yield is forbidden in default expressions:

js> function f(x = yield) {}
typein:1:15 SyntaxError: yield in default expression:
typein:1:15 function f(x = yield) {}
typein:1:15 ...............^

It may be the case that, just as we have assignExprWithoutYield, we need destructuringExprWithoutYield.

That said, I'm not sure whether to be surprised or not that computed property names are allowed in argument destructuring patterns at all.

Comment 4

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> I tried briefly following the spec terms for this and got lost a bit.  :-) 
> But I suspect the desired behavior is for this to be a syntax error, seeing
> as yield is forbidden in default expressions:

`yield` needs to be parsed as an identifier in `function f(x = yield) {}` and `function f({[yield]: x}) {}`.

The parse for `yield` in `function f({[yield]: x}) {}` looks like:
FunctionDeclaration -> FormalParameters -> FormalParameterList -> FormalsList -> FormalParameter -> BindingElement -> BindingPattern -> ObjectBindingPattern -> BindingPropertyList -> BindingProperty -> PropertyName -> ComputedPropertyName -> AssignmentExpression -> ... -> PrimaryExpression -> IdentifierReference -> [~Yield] 'yield'.
Since the [Yield] production parameter is simply passed through in every production ([?Yield]) and FormalParameters did not specify [Yield], IdentifierReference can use the [~Yield] 'yield' rule.

Also: http://logs.glob.uno/?c=mozilla%23jsapi&s=18+Jul+2014&e=18+Jul+2014#c443032 :-D

Comment 5

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

Guptha, do you happen to know what might be the next steps here?

Comment 6

[guptha]

Gary, I believe that would be to parse `yield` as an identifier in these contexts. From a cursory glance at the code, the changes should be limited to frontend/Parser.cpp.

Comment 7

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

:jorendorff mentioned in-person that :shu might be a good person to take this forward, so setting needinfo? as requested.

Comment 8

[guptha]

Sorry, school's been crazy; I haven't been able to look at this at all.

Comment 9

[Shu-yu Guo [:shu]]

The bug is that all destructuring computed property names in the formals list is evaluated in the scope of the body function, and not the outer scope. Consider:

> var foo = "foo";
> function f({ [foo]: { x } }) { var foo = "blah"; }
> f({ foo: {} })
TypeError
> f({ undefined: {} })

Comment 10

[Shu-yu Guo [:shu]]

Attachment: "Fix" yield use in destructuring exprs in formals.

We can't easily bring our behavior up to ES6-compliance due to legacy
generators, since seeing a 'yield' in versions >= 1.7 coerces any function into
a generator, while ES6 says any occurrences of 'yield' inside a
not-explicitly-marked-as-generators functions. We don't version ES6, so... I
just... okay.

Comment 11

[Jeff Walden [:Waldo]]

So I read/stared harder.  All destructuring stuff happens in the scope of the body function.  It's a mess to find.  But what you're looking for, for assigning all the names in an arguments sequence containing destructuring, is IteratorBindingInitialization.  That in turn calls BindingInitialization to perform destructuring assignments.  And *that*, if you look deep enough, you find that the way BindingProperty is processed, is:

BindingProperty : PropertyName : BindingElement
1. Let P be the result of evaluating PropertyName
2. ReturnIfAbrupt(P).
3. Return the result of performing KeyedBindingInitialization for BindingElement using value, environment, and P as arguments.

So we evaluate the computed property name every single time, within the scope of the body function.  And comment 9, which I would have sworn should have been the case in ES6, apparently is not.  Meh.  I am not sure if this is a good idea or not.

(...I did not actually bother to double-check the "within the scope of the function body" spec language, to be exactly sure it's within the function body's scope, and not in a nested scope, or some other crazy scope.  The point remains: computed property names in destructuring patterns in arguments to a function, are evaluated each and every single time the function is called.)

Comment 12

[Shu-yu Guo [:shu]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> So I read/stared harder.  All destructuring stuff happens in the scope of
> the body function.  It's a mess to find.  But what you're looking for, for
> assigning all the names in an arguments sequence containing destructuring,
> is IteratorBindingInitialization.  That in turn calls BindingInitialization
> to perform destructuring assignments.  And *that*, if you look deep enough,
> you find that the way BindingProperty is processed, is:
> 
> BindingProperty : PropertyName : BindingElement
> 1. Let P be the result of evaluating PropertyName
> 2. ReturnIfAbrupt(P).
> 3. Return the result of performing KeyedBindingInitialization for
> BindingElement using value, environment, and P as arguments.
> 
> So we evaluate the computed property name every single time, within the
> scope of the body function.  And comment 9, which I would have sworn should
> have been the case in ES6, apparently is not.  Meh.  I am not sure if this
> is a good idea or not.
> 
> (...I did not actually bother to double-check the "within the scope of the
> function body" spec language, to be exactly sure it's within the function
> body's scope, and not in a nested scope, or some other crazy scope.  The
> point remains: computed property names in destructuring patterns in
> arguments to a function, are evaluated each and every single time the
> function is called.)

Okay, and BindingInitialization is 9.2.13.23, and the creation of the extra locals scope *just in the case of the formals list having expressions* is 9.2.13.26, so the computed property names in destructuring patterns are being evaluated in the params-only scope every call.

So ES6 *does* allow yield expressions in formals expressions, right?

Comment 13

[Jeff Walden [:Waldo]]

Uh...I am not entirely sure.  I think in some cases, the answer is yes.  Maybe.  I've stared at this far longer than I really wanted to now, or probably should continue to do.

Comment 14

[Shu-yu Guo [:shu]]

(In reply to André Bargull from comment #4)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> > I tried briefly following the spec terms for this and got lost a bit.  :-) 
> > But I suspect the desired behavior is for this to be a syntax error, seeing
> > as yield is forbidden in default expressions:
> 
> `yield` needs to be parsed as an identifier in `function f(x = yield) {}`
> and `function f({[yield]: x}) {}`.
> 
> The parse for `yield` in `function f({[yield]: x}) {}` looks like:
> FunctionDeclaration -> FormalParameters -> FormalParameterList ->
> FormalsList -> FormalParameter -> BindingElement -> BindingPattern ->
> ObjectBindingPattern -> BindingPropertyList -> BindingProperty ->
> PropertyName -> ComputedPropertyName -> AssignmentExpression -> ... ->
> PrimaryExpression -> IdentifierReference -> [~Yield] 'yield'.
> Since the [Yield] production parameter is simply passed through in every
> production ([?Yield]) and FormalParameters did not specify [Yield],
> IdentifierReference can use the [~Yield] 'yield' rule.
> 
> Also:
> http://logs.glob.uno/?c=mozilla%23jsapi&s=18+Jul+2014&e=18+Jul+2014#c443032
> :-D

The shell sets the default version to be 185 or something, which is *not* ES6, which is why this wasn't parsed as an identifier.

Versionless JS already parses not-inside-generator 'yield' occurrences as identifiers, so I take comment 10 back.

Comment 15

[Shu-yu Guo [:shu]]

Filed bugs 1107711 and 1107712 for the other bugs that came up while investigating this.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 8532204 [details] [diff] [review]
"Fix" yield use in destructuring exprs in formals.

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

shu and I talked this over in Portland... we want to get rid of the assertion right away. Fixing all the ES6 compliance issues and other weirdness going on here at once is not an option.

Comment 17

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

Shu-yu, seems like this is now ready for landing then?

Comment 18

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09cc04998a3d

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09cc04998a3d