Fix var/const/let variable use before declaration #4148
Conversation
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install kzc/rollup#fix-var-use-before-decl-and-tdzor load it into the REPL: |
Codecov Report
@@ Coverage Diff @@
## master #4148 +/- ##
==========================================
+ Coverage 98.29% 98.31% +0.02%
==========================================
Files 201 201
Lines 7149 7194 +45
Branches 2093 2108 +15
==========================================
+ Hits 7027 7073 +46
Misses 58 58
+ Partials 64 63 -1
Continue to review full report at Codecov.
|
|
Not much can be done about this...
npm WARN tarball tarball data for typescript@4.3.2 (sha512-zZ4hShnmnoVnAHpVHWpTcxdv7dWP60S2FsydQLV8V5PbS3FifjWFFRiHSWpDJahly88PRyV5teTSLoq4eG7mKw==) seems to be corrupted. |
|
I couldn't examine the Codecov report in any event: |
|
@lukastaegert This PR successfully runs the tests from #4149 without
|
There was a problem hiding this comment.
Great work, this is really looking very promising. I added some notes as I think especially the pattern handling could be improved.
Also there is one thing bothering me, but maybe this is not actually an issue:
Technically, there can be many tree-shaking passes. The reason is that e.g. when a declaration is first encountered, it is not included as it is unused at that point. Then later we use the declaration, which will include the corresponding variable and trigger another pass, at which point the declaration is then included.
During the second pass, however, the initReached values will be wrong as they retain the values at the end of the first pass.
The reason this is likely not an issue is that the TDZ values are cached anyway the first time an identifier is encountered. But I am not sure I am overlooking something, will need to think a little more about this. But if it should turn out to be an issue anyway, it would be solvable e.g. by putting a Set on the HasEffectsContext and InclusionContext to track which variables are initialized and use a fresh Set for each pass. But as I said, maybe we really do not have an issue here.
|
@lukastaegert Please feel free to modify this PR to your liking. Unfortunately I don't have the time to work on it further. I believe you have a good understanding of how it works and the remaining unresolved issues that are carried over from the previous rollup implementation. I would ask that whatever form this PR takes, that its functionality is the default treeshaking behavior and not be put behind an optional flag. |
|
I think it's worth noting why the regression in |
This analysis is spot on. I added a fix that defers including entry exports after the first tree-shaking pass that indeed fixes this. |
* Also retains TDZ violations present in input * Enabled by default when treeshake is active * Low overhead - uses existing treeshaking simulated execution to mark declarations as reached
without treeshake.correctVarBeforeDeclaration
|
Good stuff - and such a simple fix. What's left to do? This PR already fixes many longstanding tree shaking and ES correctness issues. |
|
I want to have a look at this one #4148 (comment) |
|
At least the bug example above is not a regression. The same erroneous empty output is produced with rollup v2.52.7 as well. Something else appears to be amiss. This PR may not handle every TDZ scenario correctly, but it's a big improvement in any case. Detecting all TDZ issues is not possible without actually executing the code. I'll take a look at these examples tomorrow if you haven't already figured out a solution by that time. |
|
The following patch applied to the latest PR commit will fix the bug in #4148 (comment) and pass --- a/src/ast/nodes/Identifier.ts
+++ b/src/ast/nodes/Identifier.ts
@@ -239,6 +239,20 @@ export default class Identifier extends NodeBase implements PatternNode {
return (this.isTDZAccess = false);
}
+ var decl_id;
+ if (
+ this.variable.kind === 'let' &&
+ this.variable.declarations &&
+ this.variable.declarations.length === 1 &&
+ (decl_id = this.variable.declarations[0] as any) &&
+ this.start < decl_id.start &&
+ closestParentFunctionOrProgram(this) === closestParentFunctionOrProgram(decl_id)
+ ) {
+ // a `let` variable accessed before its declaration
+ // in the same function or at top level of module
+ return (this.isTDZAccess = true);
+ }
+
if (!this.variable.initReached) {
// Either a const/let TDZ violation or
// var use before declaration was encountered.
@@ -248,3 +262,11 @@ export default class Identifier extends NodeBase implements PatternNode {
return (this.isTDZAccess = false);
}
}
+
+function closestParentFunctionOrProgram(node: any): any {
+ while (node && !/^Program|Function/.test(node.type)) {
+ node = node.parent;
+ }
+ // One of: ArrowFunctionExpression, FunctionDeclaration, FunctionExpression or Program.
+ return node;
+}I have not investigated the working but suboptimal test case in #4148 (comment). There are bound to be edge cases, but as long as they are infrequent and produce the correct result they can be tolerated until a future PR fix can be devised. |
|
This patch fixes the deoptimization in #4148 (comment) without any --- a/src/ast/nodes/CallExpression.ts
+++ b/src/ast/nodes/CallExpression.ts
@@ -178,15 +178,18 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
hasEffects(context: HasEffectsContext): boolean {
- if (!this.deoptimized) this.applyDeoptimizations();
- for (const argument of this.arguments) {
- if (argument.hasEffects(context)) return true;
+ try {
+ for (const argument of this.arguments) {
+ if (argument.hasEffects(context)) return true;
+ }
+ if (
+ (this.context.options.treeshake as NormalizedTreeshakingOptions).annotations &&
+ this.annotations
+ )
+ return false;
+ return (
+ this.callee.hasEffects(context) ||
+ this.callee.hasEffectsWhenCalledAtPath(EMPTY_PATH, this.callOptions, context)
+ );
+ } finally {
+ if (!this.deoptimized) this.applyDeoptimizations();
}
- if (
- (this.context.options.treeshake as NormalizedTreeshakingOptions).annotations &&
- this.annotations
- )
- return false;
- return (
- this.callee.hasEffects(context) ||
- this.callee.hasEffectsWhenCalledAtPath(EMPTY_PATH, this.callOptions, context)
- );
}I used try/finally because it was the easiest way to defer Other AST classes should be examined for similar suboptimal code due to child node |
|
This is the same patch as above but it's easier to read using --- a/src/ast/nodes/CallExpression.ts
+++ b/src/ast/nodes/CallExpression.ts
@@ -176,7 +176,7 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
}
hasEffects(context: HasEffectsContext): boolean {
- if (!this.deoptimized) this.applyDeoptimizations();
+ try {
for (const argument of this.arguments) {
if (argument.hasEffects(context)) return true;
}
@@ -189,6 +189,9 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
this.callee.hasEffects(context) ||
this.callee.hasEffectsWhenCalledAtPath(EMPTY_PATH, this.callOptions, context)
);
+ } finally {
+ if (!this.deoptimized) this.applyDeoptimizations();
+ }
}
hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { |
There was a problem hiding this comment.
I added your fix for the CallExpression regression but did not add the other fix as I find it does not really fix the root cause but just my constructed example, and there could be quite a few variations that are not yet fixed. I would rather go for a separate approach to these ones. Which is why I undeprecated the correctVarValueBeforeDeclaration for now.
Still, I think this could be released for now as a definitive improvement.
That's unfortunate. It's a very simple and reliable approach to fix such bugs - any use of a Can you post some examples of variations that are not fixed by #4148 (comment) ? |
I suppose there's no harm in keeping this around as a non-default treeshake option, but as of this latest PR, I'm not aware of any rollup input producing a wrong result for var-use-before-declaration. The patch not applied is a |
|
This PR is a net improvement over the previous release, which is good. But the thing is that the PR only exists because some of rollup's optimization assumptions regarding |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
resolves #4101
Description
Retains code that reads const/let/var variables before their declarations to preserve the original behavior of the rollup input. This PR handles all known issues in #4101.