Refactor call handling by lukastaegert · Pull Request #1624 · rollup/rollup

lukastaegert · 2017-09-11T08:40:07Z

So it here is, my second huge refactoring PR. This time, the focus is on refactoring the side-effect detection for calls and instantiations, something I had left out of the previous PR. Besides, quite a few issues will be fixed as well: #674, #677, #1187, #1235, #1289, #1391, #1615, #1622

Improved results

This time the list of tests that have been added/changed and now produce different (and correct) results is REALLY long, so instead I will just list the highlights and you may check for yourself:

Call expression handling now re-uses the same logic as the rest of rollup to determine if a function body has side-effects. An immediate result is that in namespace-optimization-computed-string, the several times re-exported but side-effect free function is now properly removed so the test needed to be adapted :)
Class declarations and expressions are now properly removed unless there are computed keys in the body that cause side-effects or retrieving the super class has a side-effect
Class instantiations are now removed if there is no constructor in the class or any super class or the relevant constructor has no side-effects
Except for import and export declarations for which this remains to be done, all other kinds of declarations have been separated from their concept of a "variable", which is what is stored in the scope now
This made it possible, that variable re-declarations with "var" or "function" are now recognized properly! See e.g. the test redeclarations
Also, "this"-handling has been refactored. Now, "this" is actually a pseudo-variable on the scope. For now, it only assumes one of two pseudo-values, UNKNOWN_LITERAL and UNKNOWN_ASSIGNMENT but in the future we might start putting much more information here. The values of "this" are set during .bind() time.
This also means that we can now properly remove nested mutations of "this" expressions if they do not have an effect!
Generator functions without side-effects are removed as long as iteration has not started yet, see side-effects-generators
New types of scopes like the FunctionScope have been added. Now functions actually create two new scopes: one for their parameters and one for their body. This resolves some elusive bugs.
In the no-treeshake test, the namespace import is now properly replaced. This is handled by actually including ALL nodes when tree-shaking is off which should also resolve other problems related to tree-shaking being disabled.
A crash with cyclic assignments has been resolved (also fixes Rollup 0.49.3 regression bundling React #1631)
Side-effects in default parameter values are now detected properly. Also, there have been many small fixes and improvements to the side-effect detection in pattern assignments and pattern defaults.
Now if a for-in/of loop uses a global loop variable, this will be properly noted as an effect
Tree-shaking inside LabeledStatements now works properly

Other refactorings

Node now has JSDoc comments describing the parameter types, return values and how and when to overwrite the individual methods. This should help others in determining what needs to be changed if they want to fix something via PR.
hasEffects options have been refactored to be handled via a class that ensures immutability
Node.hasEffectsWhenCalled is implemented
VariableDeclarators no longer generate ProxyDeclarations for their children; instead, this has been moved to the individual Identifiers
Variables on the scope are now usually declared by Identifiers and linked to Identifiers
All kinds of variables are now derived from Variable; variables can be tested for certain effects
callHasEffects and gatherPossibleValues could finally be removed
All calls to a function are collected during bind time. For now, this is only used to detect "this" values. In the future, we might want to start collecting call parameters here to determine if a function does not have side-effects for specific parameters. E.g., if a Literal is passed as a parameter it is not a side-effect to mutate it.
This time, there are many more small commits if anyone wants to follow the refactoring flow…
And many more!

Have fun & I hope this works for everyone!

future) that make immutability easier to handle * Properly tree-shake generator functions without side-effects that have been called to provide an iterator at least as long as iteration has not yet started

a little IDE support

identifiers handle the proxy generation

…ements * Fix an issue when tree-shaking is of where namespace imports were not replaced properly

* Move the job of declaring variables on the scope to the individual Identifiers * Extract Variable types and refactor * In order to find out if a variable has an effect when mutated, ask it! * Now var statements should be correctly associated (test pending)

* For nodes: includeDeclaration -> includeWithAllDeclarations * Scope.findDeclaration -> findVariable

or shadowed

values are correctly associated

preserved

* No special handling for this-values for now * Finally eliminate callHasEffects and gatherPossibleValues * Side-effects across namespace imports are now properly detected! (dedicated test is pending)

namespace props.

to make clear these are meant to be called during .bind() * Create dedicated FunctionBodyScope for non-ArrowFunctions that adds arguments and will in the future handle "this" values

included

lukastaegert · 2017-09-11T08:52:39Z

For some reason when rebasing/merging, some commits were added twice so the list of commits might look a little bloated...
Update: Some stashing, resetting, interactive rebasing, unstashing and force-pushing seem to have fixed that ;) Now you can read the commits properly from top to bottom

* Prevent crashes for mutations of cyclic assignments * Resolve rollup#1627

arguments and return value are (should also improve IDE support) and to explain, when and how each function should be overridden

not always register all side effects.

* Resolves rollup#674 * Closes rollup#677 * Resolves rollup#1235

value to their left side has an effect.

* Resolves rollup#1622

patterns, both the right side as well as any initial assignment are assigned to the left side.

TimvdLippe · 2017-09-13T18:37:24Z

Besides, quite a few issues will be fixed as well:

For GitHub to automatically close these issues, you need to explicitly add the keyword Fixes in the PR description for each issue number. E.g. Fixes #674, Fixes #677, etc...

lukastaegert · 2017-09-13T20:48:56Z

Actually, "resolve" and "close" also work quite well but I prefer to do it via the individual commits, that way the commit gets linked to the issue 😉

lukastaegert · 2017-09-13T20:54:25Z

But good point, added a comment for #1631.

Rich-Harris · 2017-09-16T13:29:36Z

🎉🎉🎉🎉🎉

amazing work as ever, thank you!

lukastaegert added 18 commits September 11, 2017 06:52

* Introduce classes for hasEffects options (and possibly others in the

622cb4e

future) that make immutability easier to handle * Properly tree-shake generator functions without side-effects that have been called to provide an iterator at least as long as iteration has not yet started

Unite and rename options and add a few more more JSDoc comments to have

ef6a1e6

a little IDE support

No longer access VariableDeclarator.proxies from other Nodes

3db5aef

Extract ProxyDeclaration from VariableDeclarator and let the individual

79293b7

identifiers handle the proxy generation

* Separate variables from declarations for all non import/export stat…

4e6a734

…ements * Fix an issue when tree-shaking is of where namespace imports were not replaced properly

* Inline Function again

b3ec33f

* Move the job of declaring variables on the scope to the individual Identifiers * Extract Variable types and refactor * In order to find out if a variable has an effect when mutated, ask it! * Now var statements should be correctly associated (test pending)

* For variables: includeDeclaration -> includeVariable

cabeb4e

* For nodes: includeDeclaration -> includeWithAllDeclarations * Scope.findDeclaration -> findVariable

Close rollup#1289

bfefb89

Resolve rollup#1391

4e2a61a

Add tests to make sure variables in for scopes are correctly associated

fe5fa2c

or shadowed

Separate parameter and body scopes of functions to make sure default

26f305c

values are correctly associated

Add test to make sure side-effects of destructured variables are always

c363e04

preserved

* Finally implement hasEffectsWhenCalled

8802511

* No special handling for this-values for now * Finally eliminate callHasEffects and gatherPossibleValues * Side-effects across namespace imports are now properly detected! (dedicated test is pending)

Add new BlockScope that encapsulates hoisting behaviour

9a6ef62

Improve test to show that tree-shaking now works with computed

3a79776

namespace props.

* Rename assignExpression -> bindAssignment for nodes (not variables)

3cbb81c

to make clear these are meant to be called during .bind() * Create dedicated FunctionBodyScope for non-ArrowFunctions that adds arguments and will in the future handle "this" values

Implement proper "this"-handling

1b9bb03

Do not count function mutations as effects unless the function is

e3dd094

included

This was referenced Sep 11, 2017

Rollup 0.49 includes unused babel-helpers #1595

Closed

Bundling react-dom causes "Maximum callstack size exceeded" error #1627

Closed

lukastaegert added 9 commits September 13, 2017 06:19

* Test cyclic assignment handling for calls and mutations

b8a488a

* Prevent crashes for mutations of cyclic assignments * Resolve rollup#1627

Test nested this expressions are now treated properly

3ae982b

Test side-effects in function parameter defaults are not ignored

a3e2c17

Slightly improve side-effects-in-template-literals test

5fcf92d

Properly track this values through default and namespace exports

4cbe2b9

Add JSDoc comments to important Node functions to show what the

9a69795

arguments and return value are (should also improve IDE support) and to explain, when and how each function should be overridden

Test and fix a bug where assignments to patterns with defaults would

f8ca69d

not always register all side effects.

Implement tree-shaking for class instantiations without side-effects

af62bc7

* Resolves rollup#674 * Closes rollup#677 * Resolves rollup#1235

Rename Class -> ClassNode and extract FunctionNode from Function*s again

e524d13

lukastaegert added 6 commits September 13, 2017 06:20

Make sure for-in and for-of statements have an effect when assigning a

1ffef5c

value to their left side has an effect.

* Make sure not to remove break statements with labels that are included

bd60a82

* Resolves rollup#1622

Even though it currently has no effect, make sure that for assignment

bea9e46

patterns, both the right side as well as any initial assignment are assigned to the left side.

Resolve rollup#1615

b5ca833

Resolve rollup#1187

c25eb2d

Fix improper initialisation of properties

f964dfb

lukastaegert force-pushed the refactor-call-handling branch from 7db70a6 to f964dfb Compare September 13, 2017 04:21

Rich-Harris merged commit 7c1f987 into rollup:master Sep 16, 2017

lukastaegert deleted the refactor-call-handling branch September 17, 2017 10:01

lukastaegert mentioned this pull request Oct 9, 2017

Feature request: Add a "module" entry in package.json to export ES2015 version of React react/react#10021

Open

Navigation Menu

Uh oh!

Uh oh!

Refactor call handling - #1624

Refactor call handling#1624
Rich-Harris merged 33 commits into
rollup:masterfrom
lukastaegert:refactor-call-handling

lukastaegert commented Sep 11, 2017 •

edited

Loading

Uh oh!

lukastaegert commented Sep 11, 2017 •

edited

Loading

Uh oh!

TimvdLippe commented Sep 13, 2017

Uh oh!

lukastaegert commented Sep 13, 2017

Uh oh!

lukastaegert commented Sep 13, 2017

Uh oh!

Rich-Harris commented Sep 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

lukastaegert commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Improved results

Other refactorings

Uh oh!

lukastaegert commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimvdLippe commented Sep 13, 2017

Uh oh!

lukastaegert commented Sep 13, 2017

Uh oh!

lukastaegert commented Sep 13, 2017

Uh oh!

Rich-Harris commented Sep 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lukastaegert commented Sep 11, 2017 •

edited

Loading

lukastaegert commented Sep 11, 2017 •

edited

Loading