Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1864553002:
[destructuring] don't attempt to visit contents of FunctionLiterals (Closed)
|
Created:
4 years, 8 months ago by caitp (gmail) Modified:
4 years, 8 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[destructuring] don't attempt to visit contents of FunctionLiterals
The parser eagerly rewrites destructuring assignments occuring
in formal parameter initializers, because not doing so would
cause the BindingPattern rewriting to be confused and do the
wrong thing.
This change prevents this rewriting from descending into the
bodies of lazily parsed functions.
In general, it's a mistake to descend into the bodies of function
literals anyways, since they are rewritten separately on their
own time, so there is no distinction made between lazily
"throw away" eagerly parsed functions in the temporary parser
arena, or "real" eagerly parsed functions that will be compiled.
BUG=chromium:594084, v8:811
LOG=N
R=adamk@chromium.org, littledan@chromium.org
Committed: https://crrev.com/f60048c5563761863463a5b685685ee1af8de04a
Cr-Commit-Position: refs/heads/master@{#35277}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use adam's preferred approach #
Total comments: 1
Patch Set 3 : and add a comment explaining why #
Messages
Total messages: 15 (5 generated)
PTAL --- the approach taken to fix this might need adjustments depending on if it breaks any destructuring assignment tests, but AFAIK it should work fine.
Description was changed from ========== [destructuring] don't attempt to visit contents of FunctionLiterals The parser eagerly rewrites destructuring assignments occuring in formal parameters, because not doing so would cause the BindingPattern rewriting to be confused and do the wrong thing. This change prevents this rewriting from descending into the bodies of lazily parsed functions. In general, it's a mistake to descend into the bodies of function literals anyways, since they are rewritten separately on their own time, so there is no distinction made between lazily "throw away" eagerly parsed functions in the temporary parser arena, or "real" eagerly parsed functions that will be compiled. BUG=chromium:594084, v8:811 LOG=N R=adamk@chromium.org, littledan@chromium.org ========== to ========== [destructuring] don't attempt to visit contents of FunctionLiterals The parser eagerly rewrites destructuring assignments occuring in formal parameter initializers, because not doing so would cause the BindingPattern rewriting to be confused and do the wrong thing. This change prevents this rewriting from descending into the bodies of lazily parsed functions. In general, it's a mistake to descend into the bodies of function literals anyways, since they are rewritten separately on their own time, so there is no distinction made between lazily "throw away" eagerly parsed functions in the temporary parser arena, or "real" eagerly parsed functions that will be compiled. BUG=chromium:594084, v8:811 LOG=N R=adamk@chromium.org, littledan@chromium.org ==========
Description was changed from ========== [destructuring] don't attempt to visit contents of FunctionLiterals The parser eagerly rewrites destructuring assignments occuring in formal parameter initializers, because not doing so would cause the BindingPattern rewriting to be confused and do the wrong thing. This change prevents this rewriting from descending into the bodies of lazily parsed functions. In general, it's a mistake to descend into the bodies of function literals anyways, since they are rewritten separately on their own time, so there is no distinction made between lazily "throw away" eagerly parsed functions in the temporary parser arena, or "real" eagerly parsed functions that will be compiled. BUG=chromium:594084, v8:811 LOG=N R=adamk@chromium.org, littledan@chromium.org ========== to ========== [destructuring] don't attempt to visit contents of FunctionLiterals The parser eagerly rewrites destructuring assignments occuring in formal parameter initializers, because not doing so would cause the BindingPattern rewriting to be confused and do the wrong thing. This change prevents this rewriting from descending into the bodies of lazily parsed functions. In general, it's a mistake to descend into the bodies of function literals anyways, since they are rewritten separately on their own time, so there is no distinction made between lazily "throw away" eagerly parsed functions in the temporary parser arena, or "real" eagerly parsed functions that will be compiled. BUG=chromium:594084, v8:811 LOG=N R=adamk@chromium.org, littledan@chromium.org ==========
https://codereview.chromium.org/1864553002/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1864553002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4388: class InitializerRewriter : public AstExpressionVisitor { It seems like a more minimal fix for the problem at hand would be to override VisitFunctionLiteral in this class, especially since the fact that this rewriter shouldn't descend into function bodies (even if eagerly parsed) is specific to it.
https://codereview.chromium.org/1864553002/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1864553002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4388: class InitializerRewriter : public AstExpressionVisitor { On 2016/04/05 17:54:30, adamk wrote: > It seems like a more minimal fix for the problem at hand would be to override > VisitFunctionLiteral in this class, especially since the fact that this rewriter > shouldn't descend into function bodies (even if eagerly parsed) is specific to > it. involves a lot more actual changes to do it that way (making the Visit##Type methods protected instead of private, etc). If that's preferred, will do
https://codereview.chromium.org/1864553002/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1864553002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4388: class InitializerRewriter : public AstExpressionVisitor { On 2016/04/05 18:02:05, caitp wrote: > On 2016/04/05 17:54:30, adamk wrote: > > It seems like a more minimal fix for the problem at hand would be to override > > VisitFunctionLiteral in this class, especially since the fact that this > rewriter > > shouldn't descend into function bodies (even if eagerly parsed) is specific to > > it. > > involves a lot more actual changes to do it that way (making the Visit##Type > methods protected instead of private, etc). > > If that's preferred, will do You shouldn't need to touch the base class at all: overriding private methods works fine in C++. All you should need is: void VisitFunctionLiteral(FunctionLiteral* expr) override { } (since a FunctionLiteral is never a RewritableExpression)
https://codereview.chromium.org/1864553002/diff/1/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1864553002/diff/1/src/parsing/parser.cc#newco... src/parsing/parser.cc:4388: class InitializerRewriter : public AstExpressionVisitor { On 2016/04/05 18:06:26, adamk wrote: > On 2016/04/05 18:02:05, caitp wrote: > > On 2016/04/05 17:54:30, adamk wrote: > > > It seems like a more minimal fix for the problem at hand would be to > override > > > VisitFunctionLiteral in this class, especially since the fact that this > > rewriter > > > shouldn't descend into function bodies (even if eagerly parsed) is specific > to > > > it. > > > > involves a lot more actual changes to do it that way (making the Visit##Type > > methods protected instead of private, etc). > > > > If that's preferred, will do > > You shouldn't need to touch the base class at all: overriding private methods > works fine in C++. All you should need is: > > void VisitFunctionLiteral(FunctionLiteral* expr) override { } > > (since a FunctionLiteral is never a RewritableExpression) Done.
https://codereview.chromium.org/1864553002/diff/20001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1864553002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:4405: void VisitFunctionLiteral(FunctionLiteral* expr) override {} Please add a comment explaining why this is OK.
lgtm
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864553002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864553002/40001
Message was sent while issue was closed.
Description was changed from ========== [destructuring] don't attempt to visit contents of FunctionLiterals The parser eagerly rewrites destructuring assignments occuring in formal parameter initializers, because not doing so would cause the BindingPattern rewriting to be confused and do the wrong thing. This change prevents this rewriting from descending into the bodies of lazily parsed functions. In general, it's a mistake to descend into the bodies of function literals anyways, since they are rewritten separately on their own time, so there is no distinction made between lazily "throw away" eagerly parsed functions in the temporary parser arena, or "real" eagerly parsed functions that will be compiled. BUG=chromium:594084, v8:811 LOG=N R=adamk@chromium.org, littledan@chromium.org ========== to ========== [destructuring] don't attempt to visit contents of FunctionLiterals The parser eagerly rewrites destructuring assignments occuring in formal parameter initializers, because not doing so would cause the BindingPattern rewriting to be confused and do the wrong thing. This change prevents this rewriting from descending into the bodies of lazily parsed functions. In general, it's a mistake to descend into the bodies of function literals anyways, since they are rewritten separately on their own time, so there is no distinction made between lazily "throw away" eagerly parsed functions in the temporary parser arena, or "real" eagerly parsed functions that will be compiled. BUG=chromium:594084, v8:811 LOG=N R=adamk@chromium.org, littledan@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [destructuring] don't attempt to visit contents of FunctionLiterals The parser eagerly rewrites destructuring assignments occuring in formal parameter initializers, because not doing so would cause the BindingPattern rewriting to be confused and do the wrong thing. This change prevents this rewriting from descending into the bodies of lazily parsed functions. In general, it's a mistake to descend into the bodies of function literals anyways, since they are rewritten separately on their own time, so there is no distinction made between lazily "throw away" eagerly parsed functions in the temporary parser arena, or "real" eagerly parsed functions that will be compiled. BUG=chromium:594084, v8:811 LOG=N R=adamk@chromium.org, littledan@chromium.org ========== to ========== [destructuring] don't attempt to visit contents of FunctionLiterals The parser eagerly rewrites destructuring assignments occuring in formal parameter initializers, because not doing so would cause the BindingPattern rewriting to be confused and do the wrong thing. This change prevents this rewriting from descending into the bodies of lazily parsed functions. In general, it's a mistake to descend into the bodies of function literals anyways, since they are rewritten separately on their own time, so there is no distinction made between lazily "throw away" eagerly parsed functions in the temporary parser arena, or "real" eagerly parsed functions that will be compiled. BUG=chromium:594084, v8:811 LOG=N R=adamk@chromium.org, littledan@chromium.org Committed: https://crrev.com/f60048c5563761863463a5b685685ee1af8de04a Cr-Commit-Position: refs/heads/master@{#35277} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f60048c5563761863463a5b685685ee1af8de04a Cr-Commit-Position: refs/heads/master@{#35277} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
