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

RESOLVED FIXED in mozilla37

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gkw, Assigned: shu)

Tracking

(Blocks 1 bug, {assertion, regression, testcase})

Trunk
mozilla37
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 affected)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments)

Reporter

Description

5 years ago
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?
Flags: needinfo?(jorendorff)
Reporter

Comment 1

5 years ago
Posted file 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

5 years ago
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 :)
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.
(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
Reporter

Comment 5

5 years ago
Guptha, do you happen to know what might be the next steps here?
Flags: needinfo?(gupta.rajagopal)

Comment 6

5 years ago
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.
Reporter

Comment 7

5 years ago
:jorendorff mentioned in-person that :shu might be a good person to take this forward, so setting needinfo? as requested.
Flags: needinfo?(gupta.rajagopal) → needinfo?(shu)

Comment 8

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

Comment 9

5 years ago
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: {} })
Flags: needinfo?(shu)
Assignee

Comment 10

5 years ago
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.
Attachment #8532204 - Flags: review?(jorendorff)
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.)
Assignee

Comment 12

5 years ago
(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?
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.
Assignee

Comment 14

5 years ago
(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.
Assignee

Comment 15

5 years ago
Filed bugs 1107711 and 1107712 for the other bugs that came up while investigating this.
Assignee

Updated

5 years ago
See Also: → 1107711, 1107712
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.
Attachment #8532204 - Flags: review?(jorendorff) → review+
Reporter

Comment 17

5 years ago
Shu-yu, seems like this is now ready for landing then?
Assignee: nobody → shu
Status: NEW → ASSIGNED
Flags: needinfo?(jorendorff) → needinfo?(shu)
https://hg.mozilla.org/mozilla-central/rev/09cc04998a3d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.