[JSC] use ExpressionErrorClassifier for AwaitExpression operand
Created attachment 307087 [details] Patch
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.
Created attachment 307112 [details] Patch
Created attachment 307114 [details] Patch
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?
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 '=>'"
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?
(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.
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?
(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.
(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?
Comment on attachment 307114 [details] Patch r=me
(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. ```
Created attachment 307129 [details] Patch
Created attachment 307130 [details] Patch for landing
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
Created attachment 307131 [details] Patch for landing
(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
Comment on attachment 307131 [details] Patch for landing Clearing flags on attachment: 307131 Committed r215370: <http://trac.webkit.org/changeset/215370>
All reviewed patches have been landed. Closing bug.