Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1061983004:
Allow eval/arguments in arrow functions (Closed)
DescriptionAllow eval/arguments in arrow functions
R=arv@chromium.org, adamk@chromium.org, marja@chromium.org
BUG=v8:4020
LOG=N
Committed: https://crrev.com/71d3213a3f9da3f2ade37fe22ad02d8a658172c2
Cr-Commit-Position: refs/heads/master@{#27824}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Differentiate between eval/arguments and strict reserved arrow formal parameters; fix nits #
Total comments: 4
Patch Set 3 : Rename "result" parameter #
Messages
Total messages: 24 (4 generated)
This is a bit of a WIP. It works but I get errors like:
=== cctest/test-parsing/ArrowFunctionsSloppyParameterNames ===
Expected parser and preparser to produce the same error on:
"use strong"; eval => {};
However, found the following error messages
parser: Unexpected eval or arguments in strict mode
preparser: Unexpected strict mode reserved word
This is because the preparser currently doesn't distinguish between
eval/arguments in a comma expression and strict-mode reserved words in a comma
expression. Actually in general making the preparser produce the same errors is
a huge drag for arrow functions, but oh wells. Fixes 4020. WDYT about the
approach?
On 2015/04/13 17:15:52, wingo wrote:
> This is a bit of a WIP. It works but I get errors like:
>
> === cctest/test-parsing/ArrowFunctionsSloppyParameterNames ===
> Expected parser and preparser to produce the same error on:
> "use strong"; eval => {};
> However, found the following error messages
> parser: Unexpected eval or arguments in strict mode
> preparser: Unexpected strict mode reserved word
>
> This is because the preparser currently doesn't distinguish between
> eval/arguments in a comma expression and strict-mode reserved words in a comma
> expression. Actually in general making the preparser produce the same errors
is
> a huge drag for arrow functions, but oh wells. Fixes 4020. WDYT about the
> approach?
I like the approach. This CL is a great cleanup.
What do you think about reparsing in case we get an error so that we can report
the same errors?
https://codereview.chromium.org/1061983004/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1061983004/diff/1/src/parser.cc#newcode1138 src/parser.cc:1138: params = ParseFormalParameterList(&error_locs, &has_rest, &ok); Nice. https://codereview.chromium.org/1061983004/diff/1/src/parser.cc#newcode1143 src/parser.cc:1143: DuplicateFinder duplicate_finder(scanner()->unicode_cache()); Is it worth refactoring to not create a duplicate_finder here? https://codereview.chromium.org/1061983004/diff/1/src/parser.cc#newcode1157 src/parser.cc:1157: // only for arrow functions with single statement bodies, since there single statement bodies -> single expression bodies https://codereview.chromium.org/1061983004/diff/1/src/parser.cc#newcode3723 src/parser.cc:3723: if (IsEvalOrArguments(raw_name) && !error_locs->eval_or_arguments_.IsValid()) Maybe check IsValid first? https://codereview.chromium.org/1061983004/diff/1/src/parser.cc#newcode3767 src/parser.cc:3767: if (params != nullptr && params->is_multi_parenthesized()) { Change params != nullptr to DCHECK_NOT_NULL. https://codereview.chromium.org/1061983004/diff/1/src/parser.cc#newcode3781 src/parser.cc:3781: // Sadly, for the various malformed_arrow_function_parameter_list errors, we An idea to fixing the preparser would be to: - Parse as we do today - If there was a parse error, - go back and parse again (with a flag or parse as StrictFormalParamers) and report the first error with a location That way only the error case gets slower. (if that seems feasible we can do that in a follow up CL) https://codereview.chromium.org/1061983004/diff/1/src/parser.h File src/parser.h (right): https://codereview.chromium.org/1061983004/diff/1/src/parser.h#newcode757 src/parser.h:757: ZoneList<const v8::internal::AstRawString*>* params, VariableProxy* proxy, Why can't this be? ZoneList<const AstRawString*>* params I see that we use the fully qualified name in other places but not everywhere. https://codereview.chromium.org/1061983004/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1061983004/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:3509: "eval => { \"use strict\"; 0 }", \" -> ' for readability
On 2015/04/13 18:26:30, arv wrote:
> On 2015/04/13 17:15:52, wingo wrote:
> > This is a bit of a WIP. It works but I get errors like:
> >
> > === cctest/test-parsing/ArrowFunctionsSloppyParameterNames ===
>
> > Expected parser and preparser to produce the same error on:
> > "use strong"; eval => {};
> > However, found the following error messages
> > parser: Unexpected eval or arguments in strict mode
> > preparser: Unexpected strict mode reserved word
> >
> > This is because the preparser currently doesn't distinguish between
> > eval/arguments in a comma expression and strict-mode reserved words in a
comma
> > expression. Actually in general making the preparser produce the same
errors
> is
> > a huge drag for arrow functions, but oh wells. Fixes 4020. WDYT about the
> > approach?
>
> I like the approach. This CL is a great cleanup.
>
> What do you think about reparsing in case we get an error so that we can
report
> the same errors?
Interesting idea. This would provide good messages for all kinds of structural
errors in arrow function parameter lists, which would be swell. It would not
solve duplicate parameter identification in the pre-parser though. Still, seems
a sensible approach. Andreas what do you think? Since we would only backtrack
on an error continuation I don't think we have the exponential penalties of
backtracking in general.
rossberg@chromium.org changed reviewers: + marja@chromium.org
rossberg@chromium.org changed required reviewers: + marja@chromium.org
On 2015/04/14 06:16:03, wingo wrote:
> On 2015/04/13 18:26:30, arv wrote:
> > On 2015/04/13 17:15:52, wingo wrote:
> > > This is a bit of a WIP. It works but I get errors like:
> > >
> > > === cctest/test-parsing/ArrowFunctionsSloppyParameterNames ===
>
> >
> > > Expected parser and preparser to produce the same error on:
> > > "use strong"; eval => {};
> > > However, found the following error messages
> > > parser: Unexpected eval or arguments in strict mode
> > > preparser: Unexpected strict mode reserved word
> > >
> > > This is because the preparser currently doesn't distinguish between
> > > eval/arguments in a comma expression and strict-mode reserved words in a
> comma
> > > expression. Actually in general making the preparser produce the same
> errors
> > is
> > > a huge drag for arrow functions, but oh wells. Fixes 4020. WDYT about
the
> > > approach?
> >
> > I like the approach. This CL is a great cleanup.
> >
> > What do you think about reparsing in case we get an error so that we can
> report
> > the same errors?
>
> Interesting idea. This would provide good messages for all kinds of
structural
> errors in arrow function parameter lists, which would be swell. It would not
> solve duplicate parameter identification in the pre-parser though. Still,
seems
> a sensible approach. Andreas what do you think? Since we would only
backtrack
> on an error continuation I don't think we have the exponential penalties of
> backtracking in general.
I would be rather hesitant opening this particular can of worms. Adding Marja as
Reviewer, who is the expert on pre/parser issues.
Reparsing on error conditions only is less scary than always reparsing... but the scanner character streams are not currently rewindable, generally, (though they might soon be as a side product of vogelheim@'s work).
On 2015/04/14 07:26:38, marja wrote: > Reparsing on error conditions only is less scary than always reparsing... but > the scanner character streams are not currently rewindable, generally, (though > they might soon be as a side product of vogelheim@'s work). Cool. In this patch I think I'll just steal another bit for the ValidArrowParam to distinguish eval/arguments from future reserved strict words, and leave any rewinding followup to the future if that sounds all right.
(Not many comments aside what arv@ already said...) https://codereview.chromium.org/1061983004/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1061983004/diff/1/src/parser.cc#newcode1143 src/parser.cc:1143: DuplicateFinder duplicate_finder(scanner()->unicode_cache()); On 2015/04/13 18:26:38, arv wrote: > Is it worth refactoring to not create a duplicate_finder here? +1, this is unnecessary... https://codereview.chromium.org/1061983004/diff/1/src/parser.cc#newcode3834 src/parser.cc:3834: bool* ok) { The bool ok seems unnecessary now as this cannot fail. Does it make sense to remove it, or is it ever imaginable this stage could fail?
Yep, def makes sense to do the possible reparsing changes in a separate CL.
Thanks for the feedback; updated patch addresses nits, has a couple more minor cleanups, but mainly introduces another ValidArrowParam flag to distinguish between eval/arguments and a future-reserved word in arrow function parameter lists. https://codereview.chromium.org/1061983004/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1061983004/diff/1/src/parser.cc#newcode1143 src/parser.cc:1143: DuplicateFinder duplicate_finder(scanner()->unicode_cache()); On 2015/04/13 18:26:38, arv wrote: > Is it worth refactoring to not create a duplicate_finder here? Done by passing a null pointer. https://codereview.chromium.org/1061983004/diff/1/src/parser.cc#newcode3834 src/parser.cc:3834: bool* ok) { On 2015/04/14 07:38:25, marja wrote: > The bool ok seems unnecessary now as this cannot fail. Does it make sense to > remove it, or is it ever imaginable this stage could fail? Good catch, thanks.
conradw, btw, how does this relate to your "is valid strong arrow func param" changes (did you already land all your planned changes wrt that?)
On 2015/04/14 08:24:57, marja wrote: > conradw, btw, how does this relate to your "is valid strong arrow func param" > changes (did you already land all your planned changes wrt that?) Yes, all planned changes are in. This change does away with a nasty workaround I threw in to get the error position in the preparser as well.
lgtm, if arv@ says lgth too https://codereview.chromium.org/1061983004/diff/20001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1061983004/diff/20001/src/parser.cc#newcode3711 src/parser.cc:3711: ZoneList<const AstRawString*>* result, VariableProxy* proxy, "result" is not a very good name, could you change it to something more descriptive? We use results e.g., in this sense: FunctionLiteral* result = nullptr; ... result = ParseFunctionLiteral(...); but here the "result" it's not really the "result" of this operation (recording an arrow function param). declared_params? https://codereview.chromium.org/1061983004/diff/20001/src/parser.cc#newcode3730 src/parser.cc:3730: // TODO(wingo): Fix quadratic check. (Scope::IsDeclaredParameter has the same Nit: Can't you easily use DuplicateFinder one level up? Or, the caller could pass it here... This way you wouldn't even need to iterate over "result" here... (No need to fix in this CL necessarily.)
LGTM if Marja says so :-) I'm out of office today so I cannot take a close look.
I think we'll trust that wingo@ has done arv@'s comments properly. :)
On 2015/04/14 14:05:32, marja wrote: > I think we'll trust that wingo@ has done arv@'s comments properly. :) I'll double-check when I upload the nit-fixing patch. Thanks to you both :)
https://codereview.chromium.org/1061983004/diff/20001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1061983004/diff/20001/src/parser.cc#newcode3711 src/parser.cc:3711: ZoneList<const AstRawString*>* result, VariableProxy* proxy, On 2015/04/14 11:59:48, marja wrote: > "result" is not a very good name, could you change it to something more > descriptive? > > We use results e.g., in this sense: > FunctionLiteral* result = nullptr; > ... > result = ParseFunctionLiteral(...); > > but here the "result" it's not really the "result" of this operation (recording > an arrow function param). declared_params? Done. https://codereview.chromium.org/1061983004/diff/20001/src/parser.cc#newcode3730 src/parser.cc:3730: // TODO(wingo): Fix quadratic check. (Scope::IsDeclaredParameter has the same On 2015/04/14 11:59:48, marja wrote: > Nit: Can't you easily use DuplicateFinder one level up? Or, the caller could > pass it here... This way you wouldn't even need to iterate over "result" here... > > (No need to fix in this CL necessarily.) ACK. Some sort of hash map might work just as well though, given that DuplicateFinder doesn't operate natively on AstRawString.
The CQ bit was checked by wingo@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org, marja@chromium.org Link to the patchset: https://codereview.chromium.org/1061983004/#ps40001 (title: "Rename "result" parameter")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1061983004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/71d3213a3f9da3f2ade37fe22ad02d8a658172c2 Cr-Commit-Position: refs/heads/master@{#27824} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
