Bug 170844 – [JSC] use ExpressionErrorClassifier for AwaitExpression operand

• Component: New Bugs
• Status: RESOLVED FIXED

[Caitlin Potter (:caitp)]

[JSC] use ExpressionErrorClassifier for AwaitExpression operand

[Caitlin Potter (:caitp)]

Created attachment 307087 [details]
Patch

[Sam Weinig]

Comment on attachment 307087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307087&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        * parser/Parser.cpp:
> +        (JSC::Parser<LexerType>::parseAwaitExpression):

The changelog should ideally include information about why this change is being made. Just from the patch and the test, it isn't immediately clear what is being fixed.

[Caitlin Potter (:caitp)]

Created attachment 307112 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 307114 [details]
Patch

[Saam Barati]

Comment on attachment 307114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307114&action=review

> Source/JavaScriptCore/parser/Parser.cpp:3558
> +    ExpressionErrorClassifier classifier(this);

Does this change the error message that's produced?

[Caitlin Potter (:caitp)]

Comment on attachment 307114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307114&action=review

>> Source/JavaScriptCore/parser/Parser.cpp:3558
>> +    ExpressionErrorClassifier classifier(this);
> 
> Does this change the error message that's produced?

No, it still produces "Unexpected token '=>'"

[Saam Barati]

Comment on attachment 307114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307114&action=review

>>> Source/JavaScriptCore/parser/Parser.cpp:3558
>>> +    ExpressionErrorClassifier classifier(this);
>> 
>> Does this change the error message that's produced?
> 
> No, it still produces "Unexpected token '=>'"

I'm confused then what the point of this is? What does ExpressionErrorClassifier do?

[Caitlin Potter (:caitp)]

(In reply to Saam Barati from comment #7)
> Comment on attachment 307114 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307114&action=review
> 
> >>> Source/JavaScriptCore/parser/Parser.cpp:3558
> >>> +    ExpressionErrorClassifier classifier(this);
> >> 
> >> Does this change the error message that's produced?
> > 
> > No, it still produces "Unexpected token '=>'"
> 
> I'm confused then what the point of this is? What does
> ExpressionErrorClassifier do?

The classifier records hints that indicate which cover grammar productions to try. It allows the parser to skip cover grammars in some cases.

[Saam Barati]

Comment on attachment 307114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307114&action=review

>>>>> Source/JavaScriptCore/parser/Parser.cpp:3558
>>>>> +    ExpressionErrorClassifier classifier(this);
>>>> 
>>>> Does this change the error message that's produced?
>>> 
>>> No, it still produces "Unexpected token '=>'"
>> 
>> I'm confused then what the point of this is? What does ExpressionErrorClassifier do?
> 
> The classifier records hints that indicate which cover grammar productions to try. It allows the parser to skip cover grammars in some cases.

Ok, I just read the code to see what this does. I'm still a bit confused why this is needed for correctness. Can you explain that a bit more? What's the issue you're fixing?

[Caitlin Potter (:caitp)]

(In reply to Saam Barati from comment #9)
> Comment on attachment 307114 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307114&action=review
> 
> >>>>> Source/JavaScriptCore/parser/Parser.cpp:3558
> >>>>> +    ExpressionErrorClassifier classifier(this);
> >>>> 
> >>>> Does this change the error message that's produced?
> >>> 
> >>> No, it still produces "Unexpected token '=>'"
> >> 
> >> I'm confused then what the point of this is? What does ExpressionErrorClassifier do?
> > 
> > The classifier records hints that indicate which cover grammar productions to try. It allows the parser to skip cover grammars in some cases.
> 
> Ok, I just read the code to see what this does. I'm still a bit confused why
> this is needed for correctness. Can you explain that a bit more? What's the
> issue you're fixing?

Inside parseUnaryExpression, we eventually reach parseMemberExpression(), and see `async()` followed by a `=>` token.

In an AssignmentExpression context rather than a UnaryExpression context, this would mean "parse the async arrow function cover grammar", so it records a hint to do that. But the AssignmentExpression started with `await` rather than `async`, so restoring the save point to parse the cover grammar fails this assertion and crashes.

The goal is to prevent the branch that leads to that assertion from being parsed at all. This is done by adding another hint recording scope, which allows the hint to be discarded properly.

parseAssignmentExpression uses the hint to determine which arrow function cover grammar to parse, rather than trying both.

[Saam Barati]

(In reply to Caitlin Potter (:caitp) from comment #10)
> (In reply to Saam Barati from comment #9)
> > Comment on attachment 307114 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=307114&action=review
> > 
> > >>>>> Source/JavaScriptCore/parser/Parser.cpp:3558
> > >>>>> +    ExpressionErrorClassifier classifier(this);
> > >>>> 
> > >>>> Does this change the error message that's produced?
> > >>> 
> > >>> No, it still produces "Unexpected token '=>'"
> > >> 
> > >> I'm confused then what the point of this is? What does ExpressionErrorClassifier do?
> > > 
> > > The classifier records hints that indicate which cover grammar productions to try. It allows the parser to skip cover grammars in some cases.
> > 
> > Ok, I just read the code to see what this does. I'm still a bit confused why
> > this is needed for correctness. Can you explain that a bit more? What's the
> > issue you're fixing?
> 
> Inside parseUnaryExpression, we eventually reach parseMemberExpression(),
> and see `async()` followed by a `=>` token.
> 
> In an AssignmentExpression context rather than a UnaryExpression context,
> this would mean "parse the async arrow function cover grammar", so it
> records a hint to do that. But the AssignmentExpression started with `await`
> rather than `async`, so restoring the save point to parse the cover grammar
> fails this assertion and crashes.
> 
> The goal is to prevent the branch that leads to that assertion from being
> parsed at all. This is done by adding another hint recording scope, which
> allows the hint to be discarded properly.
> 
> parseAssignmentExpression uses the hint to determine which arrow function
> cover grammar to parse, rather than trying both.

Gotcha. Can you add this explanation to the changelog?

[Saam Barati]

Comment on attachment 307114 [details]
Patch

r=me

[Caitlin Potter (:caitp)]

(In reply to Saam Barati from comment #11)
> (In reply to Caitlin Potter (:caitp) from comment #10)
> > (In reply to Saam Barati from comment #9)
> > > Comment on attachment 307114 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=307114&action=review
> > > 
> > > >>>>> Source/JavaScriptCore/parser/Parser.cpp:3558
> > > >>>>> +    ExpressionErrorClassifier classifier(this);
> > > >>>> 
> > > >>>> Does this change the error message that's produced?
> > > >>> 
> > > >>> No, it still produces "Unexpected token '=>'"
> > > >> 
> > > >> I'm confused then what the point of this is? What does ExpressionErrorClassifier do?
> > > > 
> > > > The classifier records hints that indicate which cover grammar productions to try. It allows the parser to skip cover grammars in some cases.
> > > 
> > > Ok, I just read the code to see what this does. I'm still a bit confused why
> > > this is needed for correctness. Can you explain that a bit more? What's the
> > > issue you're fixing?
> > 
> > Inside parseUnaryExpression, we eventually reach parseMemberExpression(),
> > and see `async()` followed by a `=>` token.
> > 
> > In an AssignmentExpression context rather than a UnaryExpression context,
> > this would mean "parse the async arrow function cover grammar", so it
> > records a hint to do that. But the AssignmentExpression started with `await`
> > rather than `async`, so restoring the save point to parse the cover grammar
> > fails this assertion and crashes.
> > 
> > The goal is to prevent the branch that leads to that assertion from being
> > parsed at all. This is done by adding another hint recording scope, which
> > allows the hint to be discarded properly.
> > 
> > parseAssignmentExpression uses the hint to determine which arrow function
> > cover grammar to parse, rather than trying both.
> 
> Gotcha. Can you add this explanation to the changelog?

Sure.

Hows this, does this seem clear/understandable?

```
        In parseAssignmentExpression(), several cover grammars are handled, and
        use ExpressionErrorClassifier to record hints about which grammars to
        try.

        In parseAwaitExpression(), the hints recorded during parsing of the
        operand need to be discarded, because if they propagate to the outer
        parseAssignmentExpression(), the hints will lead the parser down invalid
        branches that should be skipped, but are not.

        This change adds an additional ExpressionErrorClassifier to
        parseAwaitExpression(), in order to discard hints recorded by the
        operand.
```

[Caitlin Potter (:caitp)]

Created attachment 307129 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 307130 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 307130 [details]
Patch for landing

Rejecting attachment 307130 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 307130, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3535149

[Caitlin Potter (:caitp)]

Created attachment 307131 [details]
Patch for landing

[Caitlin Potter (:caitp)]

(In reply to WebKit Commit Bot from comment #16)
> Comment on attachment 307130 [details]
> Patch for landing
> 
> Rejecting attachment 307130 [details] from commit-queue.
> 
> Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch',
> '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03',
> 'validate-changelog', '--check-oops', '--non-interactive', 307130,
> '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
> 
> ChangeLog entry in JSTests/ChangeLog contains OOPS!.
> 
> Full output: http://webkit-queues.webkit.org/results/3535149

webkit-patch workflow remains very confusing T_T

[WebKit Commit Bot]

Comment on attachment 307131 [details]
Patch for landing

Clearing flags on attachment: 307131

Committed r215370: <http://trac.webkit.org/changeset/215370>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.