Produce the same scopes / variables for parameters (part 3).
This CL fixes the ordering + variable types in PreParser when there are
simple parameters + a rest parameter. In that case, Parser declares
unnamed temporaries for the non-rest params, then the rest param, then
the named variables (which are not parameters) for the non-rest params.
BUG=v8:5516
R=vogelheim@chromium.org
Change-Id: I9b006595039c8002b0508d1d2a200aa9a0f3eae0
Reviewed-on: https://chromium-review.googlesource.com/443527
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Commit-Queue: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#43241}
E.g.,
{ function lazy_inner(b = somevar) { let somevar; } }
If we don't produce the same scopes, PreParser thinks that the unresolved
variable inside the default parameter resolves into the variable declared inside
the function. Thus, it's not correctly recorded as a free variable.
One part is already done by https://codereview.chromium.org/2638333002 . But at
the laziness boundary, we still produced different scopes.
Unlike previously thought, this is also needed for lazy inner function
correctness, not only for "preparser scope analysis" (ie., skipping inner
functions).
BUG=v8:5938
Change-Id: I047cd43ef16478bb0f18d1f114845e7d1ab8c5f2
Reviewed-on: https://chromium-review.googlesource.com/439345
Commit-Queue: Marja Hölttä <marja@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#43044}
Reason for revert:
False alarm, bot hiccup
Original issue's description:
> Revert of [parsing] Fix maybe-assigned for loop variables. (patchset #3 id:40001 of https://codereview.chromium.org/2673403003/ )
>
> Reason for revert:
> Speculative revert because of https://codereview.chromium.org/2679163002/.
>
> Original issue's description:
> > [parsing] Fix maybe-assigned for loop variables.
> >
> > Due to hoisting, the value of a 'var'-declared variable may actually change even
> > if the code contains only the "initial" assignment, namely when that assignment
> > occurs inside a loop. For example:
> >
> > let i = 10;
> > do { var x = i } while (i--):
> >
> > As a simple and very conservative approximation of this, we explicitly mark
> > as maybe-assigned any non-lexical variable whose "declaration" does not
> > syntactically occur in the function scope. (In the example above, it
> > occurs in a block scope.)
> >
> > BUG=v8:5636
> >
> > Review-Url: https://codereview.chromium.org/2673403003
> > Cr-Commit-Position: refs/heads/master@{#42989}
> > Committed: a33fcd663b
>
> TBR=marja@chromium.org,adamk@chromium.org,neis@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=v8:5636
>
> Review-Url: https://codereview.chromium.org/2679263002
> Cr-Commit-Position: refs/heads/master@{#43010}
> Committed: f3ae5ccf57TBR=marja@chromium.org,adamk@chromium.org,neis@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:5636
Review-Url: https://codereview.chromium.org/2686663002
Cr-Commit-Position: refs/heads/master@{#43013}
Reason for revert:
Speculative revert because of https://codereview.chromium.org/2679163002/.
Original issue's description:
> [parsing] Fix maybe-assigned for loop variables.
>
> Due to hoisting, the value of a 'var'-declared variable may actually change even
> if the code contains only the "initial" assignment, namely when that assignment
> occurs inside a loop. For example:
>
> let i = 10;
> do { var x = i } while (i--):
>
> As a simple and very conservative approximation of this, we explicitly mark
> as maybe-assigned any non-lexical variable whose "declaration" does not
> syntactically occur in the function scope. (In the example above, it
> occurs in a block scope.)
>
> BUG=v8:5636
>
> Review-Url: https://codereview.chromium.org/2673403003
> Cr-Commit-Position: refs/heads/master@{#42989}
> Committed: a33fcd663bTBR=marja@chromium.org,adamk@chromium.org,neis@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:5636
Review-Url: https://codereview.chromium.org/2679263002
Cr-Commit-Position: refs/heads/master@{#43010}
Due to hoisting, the value of a 'var'-declared variable may actually change even
if the code contains only the "initial" assignment, namely when that assignment
occurs inside a loop. For example:
let i = 10;
do { var x = i } while (i--):
As a simple and very conservative approximation of this, we explicitly mark
as maybe-assigned any non-lexical variable whose "declaration" does not
syntactically occur in the function scope. (In the example above, it
occurs in a block scope.)
BUG=v8:5636
Review-Url: https://codereview.chromium.org/2673403003
Cr-Commit-Position: refs/heads/master@{#42989}
Turns out is_hidden is not the right condition for "scope should be present in
the preparse data". For now, replaced it with "is hidden leaf scope" (i.e.,
doesn't contain any non-hidden scopes). That's probably not the right condition
either; will be fixed once there's more data to decide what the right condition
is.
BUG=v8:5516
R=vogelheim@chromium.org
Review-Url: https://codereview.chromium.org/2669163002
Cr-Commit-Position: refs/heads/master@{#42909}
The hoist_scope member of DeclarationDescriptor was only used to pass the function
scope for declaration of parameters containing sloppy evals, for example:
function f(x = eval("var y")) { }
In cases like this, "x" is declared in the function scope but "y" is declared in an inner scope.
Rather than passing the function scope as "hoist_scope", we simply ask for the outer_scope()
of the inner scope as needed in PatternRewriter.
This reduces the cognitive overhead of understanding what a DeclarationDescriptor has; for
example, it removes some dead code from the PreParser which never has to deal
with a situation like the example above.
Review-Url: https://codereview.chromium.org/2662183002
Cr-Commit-Position: refs/heads/master@{#42861}
- Declaring a variable called "this" for preparsed functions was unnecessary;
DeclarationScope ctor already adds the variable.
- "arguments" for preparsed scopes need to be declared after parsing the
function, like it's done in the parser.
- Now arguments_ can be the dummy variable, so adapted code to it.
- A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was
incomplete; it had added ParserBase::ParseFunctionBody but
PreParser::ParseFunction didn't call it. This CL completes that work. This is
needed for getting "arguments" declared properly for preparsed functions.
- AllocateVariablesRecursively is already called for preparsed scopes (without
this CL, that is), and it bails out early. However, before the bailout it used
to dcheck num_stack_slots_ == 0; that is no longer true since we've done scope
analysis for preparsed scopes.
- Test fix: we cannot have any lazy inner functions in the test, except the
topmost lazy inner function. Such functions would also be lazy in the parser
case, and the parser would just throw away their variables. Then the test
tries to verify the preparsed data against the scopes without variables and fails.
- Disabled a test w/ a sloppy block function, will get that working again in the
upcoming CLs.
BUG=v8:5516
Review-Url: https://codereview.chromium.org/2655623005
Cr-Commit-Position: refs/heads/master@{#42685}
- Generalize the sloppy block function data structures to allow
PreParser adding and hoisting sloppy block funcs.
- This completes PreParser scope analysis.
BUG=v8:5501, v8:5516
R=verwaest@chromium.org
Review-Url: https://codereview.chromium.org/2636543002
Cr-Commit-Position: refs/heads/master@{#42368}
Now we have declarations too, so it doesn't matter whether preparser
produces the same unresolved variables as the parser.
BUG=v8:5501, v8:5516
R=verwaest@chromium.org
Review-Url: https://codereview.chromium.org/2623583004
Cr-Commit-Position: refs/heads/master@{#42174}
We're still collecting use counter data for this situation.
BUG=v8:4973
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2510873005
Cr-Commit-Position: refs/heads/master@{#41563}
This allows to detect a static property also named 'name', and also makes sure 'name' is added last, to be standards-compliant.
BUG=v8:4199
Review-Url: https://codereview.chromium.org/2423053002
Cr-Commit-Position: refs/heads/master@{#41546}
This makes the context allocation less pessimistic in the following cases:
function outer() {
var a; // Won't be context allocated
function inner1() { var a; a; }
function inner2(a) { a; }
function inner3([a]) { a; }
function inner4({ a: b}) { a; }
}
BUG=v8:5501
Review-Url: https://codereview.chromium.org/2407163003
Cr-Commit-Position: refs/heads/master@{#41521}
They're supposed to be stable across several parse passes, so we'll also
store them in the associated SharedFunctionInfos
To achieve this, the PreParser and Parser need to generated the same number of
FunctionLiterals. To achieve this, we teach the PreParser about desuggaring of
class literals.
For regular functions, the function IDs are assigned in the order they occur in
the source. For arrow functions, however, we only know that it's an arrow function
after parsing the parameter list, and so the ID assigned to the arrow function is
larger than the IDs assigned to functions defined in the parameter list. This
implies that we have to reset the function ID counter to before the parameter list
when re-parsing an arrow function. To be able to do this, we store the number of
function literals found in the parameter list of arrow functions as well.
BUG=v8:5589
Review-Url: https://codereview.chromium.org/2481163002
Cr-Commit-Position: refs/heads/master@{#41309}
... but be less pessimistic about context allocation (see below).
We might have just (pessimistically) context-allocated a variable based
on references coming from an inner function, but after that we still
need to set maybe_assigned (pessimistically).
This makes test-parsing/InnerAssignment pass with
FLAG_lazy_inner_functions.
This was undetected until now because we didn't have lazy parsing enabled
for small scripts.
Less pessimistic approach: now that inner functions laziness decisions
are stable (if we have once compiled a piece of code with lazy inner
functions, we never compile the same code with eager inner functions),
we don't need to be as pessimistic with context allocation as before.
BUG=v8:5501
Review-Url: https://codereview.chromium.org/2521513004
Cr-Commit-Position: refs/heads/master@{#41183}
This shares the pending_error_handler from the parser to the preparser, allowing the preparser to directly log errors to it. This removes LogMessage from the loggers. ParserLogger::LogMessage was already unused, so this also removes error info from the preparse data altogether.
BUG=
Review-Url: https://codereview.chromium.org/2502633002
Cr-Commit-Position: refs/heads/master@{#40984}
This
- removes the ParserRecorder base class,
- devirtualizes the LogFunction and LogMessage functions,
- reuses the SingletonLogger for all preparser calls
In a subsequent step the preparser should probably log directly to the CompleteParserRecorder rather than indirectly through the singleton logger...
BUG=
Review-Url: https://codereview.chromium.org/2474393003
Cr-Commit-Position: refs/heads/master@{#40803}
Parameters of a lazily parsed function used to be parsed eagerly, and parameter
handling was split between Parser::ParseFunctionLiteral and
ParseEagerFunctionBody, leading to inconsistencies.
After this CL, we preparse (lazy parse) the parameters of lazily parsed
functions.
(For arrow functions, we cannot do that ofc.)
This is needed for later features (PreParser with scope analysis).
-- CL adapted from marja's https://codereview.chromium.org/2411793003/
BUG=
Review-Url: https://codereview.chromium.org/2472063002
Cr-Commit-Position: refs/heads/master@{#40771}
It doesn't need to have this logic.
ParseLazyFunctionLiteralBody is basically just ParseStatementList
+ log the function position. But PreParser doesn't need to have
the "which functions to log" logic, since logging the function is
always done exactly when Parser falls back to PreParser. (See
PreParseLazyFunction.)
So in the current state, PreParser would log several functions in
a SingletonLogger, and only the last one would take
effect (that's the one Parser also logs in SkipLazyFunctionBody).
Also updated test-parsing/Regress928 to produce the preparse data
the way we do now (i.e., not running the PreParser directly, but
running the Parser).
Error reporting: when PreParser finds an error, it doesn't need
to ReportUnexpectedToken in PreParseLazyFunction, since it
already has reported the error whenever it found it.
BUG=v8:5515
Review-Url: https://codereview.chromium.org/2421833002
Cr-Commit-Position: refs/heads/master@{#40315}
If an inner function only declares a variable but doesn't use it, Parser
and PreParser produced different unresolved variables, and that confused
the pessimistic context allocation.
This is continuation to https://codereview.chromium.org/2388183003/
This CL fixes more complicated declarations (which are not just one
identifier). For this, PreParser needs to accumulate identifiers used
in expressions.
In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that
we get clusterfuzz coverage for it.
BUG=chromium:650969, v8:5501
Review-Url: https://codereview.chromium.org/2400613003
Cr-Commit-Position: refs/heads/master@{#40112}
If an inner function only declares a variable but doesn't use it, Parser
and PreParser produced different unresolved variables, and that confused
the pessimistic context allocation.
BUG=chromium:650969
Review-Url: https://codereview.chromium.org/2388183003
Cr-Commit-Position: refs/heads/master@{#39947}
Previously we'd have a scope in the main zone, and another in the temp zone. Then we carefully copied back data to the main zone. This CL changes it so that the scope is just fixed up to only contain data from the main zone. That avoids additional copies and additional allocations; while not increasing the care that needs to be taken. This will also make it easier to abort preparsing while parsing using a temp zone.
BUG=
Committed: https://crrev.com/f41e7ebd62b32e861b6aa14ad8bfce3018d03c3c
Review-Url: https://codereview.chromium.org/2368313002
Cr-Original-Commit-Position: refs/heads/master@{#39800}
Cr-Commit-Position: refs/heads/master@{#39828}
Reason for revert:
Revert due to asm.js slowdown
Original issue's description:
> Don't use different function scopes when parsing with temp zones
>
> Previously we'd have a scope in the main zone, and another in the temp zone. Then we carefully copied back data to the main zone. This CL changes it so that the scope is just fixed up to only contain data from the main zone. That avoids additional copies and additional allocations; while not increasing the care that needs to be taken. This will also make it easier to abort preparsing while parsing using a temp zone.
>
> BUG=
>
> Committed: https://crrev.com/f41e7ebd62b32e861b6aa14ad8bfce3018d03c3c
> Cr-Commit-Position: refs/heads/master@{#39800}
TBR=marja@chromium.org,adamk@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=
Review-Url: https://codereview.chromium.org/2379533003
Cr-Commit-Position: refs/heads/master@{#39821}
Previously we'd have a scope in the main zone, and another in the temp zone. Then we carefully copied back data to the main zone. This CL changes it so that the scope is just fixed up to only contain data from the main zone. That avoids additional copies and additional allocations; while not increasing the care that needs to be taken. This will also make it easier to abort preparsing while parsing using a temp zone.
BUG=
Review-Url: https://codereview.chromium.org/2368313002
Cr-Commit-Position: refs/heads/master@{#39800}
This patch moves the following parsing method to ParserBase:
- DesugarAsyncFunctionBody, renamed to ParseAsyncFunctionBody
- ParseAsyncFunctionExpression, renamed to ParseAsyncFunctionLiteral
- ParseAsyncFunctionDeclaration
It renames the parser implementation methods:
- ParseArrowFunctionFormalParameterList -> DeclareArrowFunctionFormalParameters
- ParseArrowFunctionFormalParameters -> AddArrowFunctionFormalParameters
It also eliminates method ParseAsyncArrowSingleExpressionBody.
R=adamk@chromium.org, marja@chromium.org
BUG=
LOG=N
Review-Url: https://codereview.chromium.org/2372733002
Cr-Commit-Position: refs/heads/master@{#39788}
Reason for revert:
Stability thief found, relanding speculative reverts.
Original issue's description:
> Revert of Preparse functions in the scope that was created when parsing of the function was started (patchset #2 id:20001 of https://codereview.chromium.org/2370713003/ )
>
> Reason for revert:
> Needed for https://codereview.chromium.org/2373443003/
>
> Original issue's description:
> > Preparse functions in the scope that was created when parsing of the function was started
> >
> > This reduces the number of scopes for lazily parsed top-level functions from 3 to 1
> >
> > BUG=v8:5209
> >
> > Committed: https://crrev.com/9618d095903c604a032b33792c068f4a6169503c
> > Cr-Commit-Position: refs/heads/master@{#39725}
>
> TBR=marja@chromium.org,verwaest@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=v8:5209
>
> Committed: https://crrev.com/0cef7100da0b609403c9026fb7307192a898a390
> Cr-Commit-Position: refs/heads/master@{#39729}
TBR=marja@chromium.org,verwaest@chromium.org,hablich@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:5209
Review-Url: https://codereview.chromium.org/2377593002
Cr-Commit-Position: refs/heads/master@{#39756}
Reason for revert:
Stability thief found, relanding speculative reverts.
Original issue's description:
> Revert of Preparse inner functions (new try) (patchset #21 id:420001 of https://codereview.chromium.org/2352593002/ )
>
> Reason for revert:
> We currently have some stability issues on Canary. Let's reland this after we verified that we "fixed" Canary again.
>
> Original issue's description:
> > Preparse inner functions (new try)
> >
> > This is an overly pessimistic approach where PreParser only keeps
> > track of unresolved variables, but doesn't declare anything. This
> > will result in context-allocating variables in the outer function
> > unnecessarily, if the variable names clash with variable names
> > used by the inner function (even if the variables are not the
> > same). However, we have been unable to prove that this approach
> > wouldn't be good enough for the practical purposes.
> >
> > Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
> > Keep the context-allocation decision stable when compiling fully eagerly.
> >
> > Tests which exercise this functionality:
> > mjsunit/fixed-context-shapes-when-recompiling.js
> >
> > Design document (chromium):
> >
> > https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiBAdRnTiSYhazseNHFg/edit?usp=sharing
> >
> > BUG=
> >
> > Committed: https://crrev.com/7c73cf32c60484cdf37c84f1d61b4640e87068d7
> > Cr-Commit-Position: refs/heads/master@{#39719}
>
> TBR=verwaest@chromium.org,adamk@chromium.org,marja@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=
>
> Committed: https://crrev.com/1e6296b2a7cfc307fd9e722e619f42965da4a267
> Cr-Commit-Position: refs/heads/master@{#39730}
TBR=verwaest@chromium.org,adamk@chromium.org,marja@chromium.org,hablich@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=
Review-Url: https://codereview.chromium.org/2377513006
Cr-Commit-Position: refs/heads/master@{#39755}
Reason for revert:
We currently have some stability issues on Canary. Let's reland this after we verified that we "fixed" Canary again.
Original issue's description:
> Preparse inner functions (new try)
>
> This is an overly pessimistic approach where PreParser only keeps
> track of unresolved variables, but doesn't declare anything. This
> will result in context-allocating variables in the outer function
> unnecessarily, if the variable names clash with variable names
> used by the inner function (even if the variables are not the
> same). However, we have been unable to prove that this approach
> wouldn't be good enough for the practical purposes.
>
> Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
> Keep the context-allocation decision stable when compiling fully eagerly.
>
> Tests which exercise this functionality:
> mjsunit/fixed-context-shapes-when-recompiling.js
>
> Design document (chromium):
>
> https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiBAdRnTiSYhazseNHFg/edit?usp=sharing
>
> BUG=
>
> Committed: https://crrev.com/7c73cf32c60484cdf37c84f1d61b4640e87068d7
> Cr-Commit-Position: refs/heads/master@{#39719}
TBR=verwaest@chromium.org,adamk@chromium.org,marja@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=
Review-Url: https://codereview.chromium.org/2373443003
Cr-Commit-Position: refs/heads/master@{#39730}
Reason for revert:
Needed for https://codereview.chromium.org/2373443003/
Original issue's description:
> Preparse functions in the scope that was created when parsing of the function was started
>
> This reduces the number of scopes for lazily parsed top-level functions from 3 to 1
>
> BUG=v8:5209
>
> Committed: https://crrev.com/9618d095903c604a032b33792c068f4a6169503c
> Cr-Commit-Position: refs/heads/master@{#39725}
TBR=marja@chromium.org,verwaest@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:5209
Review-Url: https://codereview.chromium.org/2365393002
Cr-Commit-Position: refs/heads/master@{#39729}
