Assignments to side-effecting getters in class prototypes can be incorrectly tree-shaken #4016
Comments
|
(I have a fix for this in my upcoming patch, waiting on #4011 to merge because it depends on the new method added by that.) |
marijnh
added a commit
to marijnh/rollup
that referenced
this issue
Mar 26, 2021
This aims to improve tree-shaking of code that uses static class properties (rollup#3989) and to improve detection of side effects through class getters/setters (rollup#4016). The first part works by keeping a map of positively known static properties (methods and simple getters) in `ClassNode.staticPropertyMap`, along with a flag (`ClassNode.deoptimizedStatic`) that indicates that something happened that removed our confidence that we know anything about the class object. Access and calls to these known static properties are handled by routing the calls to `getLiteralValueAtPath`, `getReturnExpressionWhenCalledAtPath`, and `hasEffectsWhenCalledAtPath` to the known values in the properties. In contrast to `ObjectExpression`, this class does not try to keep track of multiple expressions associated with a property, since that doesn't come up a lot on classes. The handling of side effect detection through getters and setters is done by, _if_ the entire class object (or its prototype in case of access to the prototype) hasn't been deoptimized, scanning through the directly defined getters and setters to see if one exists (calling through to superclasses as appropriate). I believe that this is solid because any code that would be able to change the set of getters and setters on a class would cause the entire object to be deoptimized.
marijnh
added a commit
to marijnh/rollup
that referenced
this issue
Mar 26, 2021
This aims to improve tree-shaking of code that uses static class properties (rollup#3989) and to improve detection of side effects through class getters/setters (rollup#4016). The first part works by keeping a map of positively known static properties (methods and simple getters) in `ClassNode.staticPropertyMap`, along with a flag (`ClassNode.deoptimizedStatic`) that indicates that something happened that removed our confidence that we know anything about the class object. Access and calls to these known static properties are handled by routing the calls to `getLiteralValueAtPath`, `getReturnExpressionWhenCalledAtPath`, and `hasEffectsWhenCalledAtPath` to the known values in the properties. In contrast to `ObjectExpression`, this class does not try to keep track of multiple expressions associated with a property, since that doesn't come up a lot on classes. The handling of side effect detection through getters and setters is done by, _if_ the entire class object (or its prototype in case of access to the prototype) hasn't been deoptimized, scanning through the directly defined getters and setters to see if one exists (calling through to superclasses as appropriate). I believe that this is solid because any code that would be able to change the set of getters and setters on a class would cause the entire object to be deoptimized.
marijnh
added a commit
to marijnh/rollup
that referenced
this issue
Mar 28, 2021
This aims to improve tree-shaking of code that uses static class properties (rollup#3989) and to improve detection of side effects through class getters/setters (rollup#4016). The first part works by keeping a map of positively known static properties (methods and simple getters) in `ClassNode.staticPropertyMap`, along with a flag (`ClassNode.deoptimizedStatic`) that indicates that something happened that removed our confidence that we know anything about the class object. Access and calls to these known static properties are handled by routing the calls to `getLiteralValueAtPath`, `getReturnExpressionWhenCalledAtPath`, and `hasEffectsWhenCalledAtPath` to the known values in the properties. In contrast to `ObjectExpression`, this class does not try to keep track of multiple expressions associated with a property, since that doesn't come up a lot on classes. The handling of side effect detection through getters and setters is done by, _if_ the entire class object (or its prototype in case of access to the prototype) hasn't been deoptimized, scanning through the directly defined getters and setters to see if one exists (calling through to superclasses as appropriate). I believe that this is solid because any code that would be able to change the set of getters and setters on a class would cause the entire object to be deoptimized.
lukastaegert
added a commit
that referenced
this issue
May 23, 2021
* Move logic from ClassBody into ClassNode So that it sits in one place and is easier to extend. * Track static class fields and improve handling of class getters/setters This aims to improve tree-shaking of code that uses static class properties (#3989) and to improve detection of side effects through class getters/setters (#4016). The first part works by keeping a map of positively known static properties (methods and simple getters) in `ClassNode.staticPropertyMap`, along with a flag (`ClassNode.deoptimizedStatic`) that indicates that something happened that removed our confidence that we know anything about the class object. Access and calls to these known static properties are handled by routing the calls to `getLiteralValueAtPath`, `getReturnExpressionWhenCalledAtPath`, and `hasEffectsWhenCalledAtPath` to the known values in the properties. In contrast to `ObjectExpression`, this class does not try to keep track of multiple expressions associated with a property, since that doesn't come up a lot on classes. The handling of side effect detection through getters and setters is done by, _if_ the entire class object (or its prototype in case of access to the prototype) hasn't been deoptimized, scanning through the directly defined getters and setters to see if one exists (calling through to superclasses as appropriate). I believe that this is solid because any code that would be able to change the set of getters and setters on a class would cause the entire object to be deoptimized. * Remove ClassNode.deoptimizeCache * Keep a table for class property effects * Add comment explaining property map * Fix types * Make getReturnExpression and getLiteralValue more similar for objects * Use common logic for return expression and literal value * Use common logic for return access and call effects * Extract shared logic from ObjectExpression * Use an object for better performance * Simplify handling for setters and other properties * Small simplification * Work towards better class handling * merge ObjectPathHandler into ObjectEntity * Slightly refactor default values * Separate unknown nodes from other Nodes to avoid future circular dependencies * Introduce new prototype tracking * Improve coverage * Fix class deoptimization in arrow functions via this/super * Simplify and merge property and method definition * Improve coverage * Replace deoptimizeProperties by deoptimizing a double unknown path * Assume functions can add getters to parameters * Improve class.prototype handling * Assume created instance getters have side-effects * Base all expressions on a base class * Only deoptimize necessary paths when deoptimizing "this" * Handle deoptimizing "this" in getters * Handle deoptimizing "this" in setters * Unify this deoptimization * Simplify recursion tracking * Get rid of deoptimizations during bind phase * Get rid of unneeded double-binding checks * Inline deoptimizations into NodeBase for simple cases * Add more efficient way to create object entities * Add thisParameter to CallOptions * Move NodeEvents to separate file * Track array elements * Simplify namespace handling * Use Object.values and Object.entries instead of Object.keys where useful * Improve code and simplify literal handling * Improve coverage * Improve coverage * Improve coverage in conditional and logical expressions * Improve coverage * 2.49.0-0 * Fix test to support pre-release versions * Fix failed deoptimization of array props * 2.49.0-1 Co-authored-by: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com> Co-authored-by: Lukas Taegert-Atkinson <lukastaegert@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Expected Behavior
The code should exhibit a side effect (running
console.log).Actual Behavior
Rollup removes the entire program and emits an empty chunk.
This is caused by
ClassNode.hasEffectsWhenAccessedAtPathbeing too liberal, but I expect just tightening that up the obvious way will cause significant regressions in tree-shaking on some code. We may have to start tracking class instance properties to properly fix this without regressing.The text was updated successfully, but these errors were encountered: