Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect whether CallExpressions (may) have side-effects before including them #179

Closed
Rich-Harris opened this issue Oct 12, 2015 · 6 comments · Fixed by #1017
Closed

Detect whether CallExpressions (may) have side-effects before including them #179

Rich-Harris opened this issue Oct 12, 2015 · 6 comments · Fixed by #1017

Comments

@Rich-Harris
Copy link
Contributor

All CallExpression nodes that aren't inside a function are assumed to have side-effects, and automatically included. This behaviour is over-cautious in situations like this:

// main.js
import { foo } from './foo.js';
foo();

// foo.js
export const foo = () => console.log( 'foo' );

export const Unused = (function () {
  function Unused () {
    this.willNeverRun = true;
  }

  Unused.prototype = {
    runThisCode () {
      console.log( 'jk. this code will never run' );
    }
  };

  return Unused;
})();

The Unused class is never instantiated, but we included it anyway because it's declared inside an IIFE (i.e a CallExpression). This isn't an academic problem, it's how Babel transpiles classes (for example).

Similarly, a call to a function that either a) doesn't have side-effects, or b) only has side-effects that apply to things we don't need, should not be included, and should not cause the relevant declaration to be included:

export const num = 5;
export const squared = square( num );

function square ( num ) {
  return num * num;
}

The function call on the second line means we have to include that line, plus the num and square declarations, regardless of whether we're importing either num or squared from this module, even though we can statically determine that square has no side-effects.

It may also be possible to determine whether functions are capable of mutating their arguments.

@Victorystick
Copy link
Contributor

Statically analyzing JavaScript is made very difficult by its various features. Take the square function for instance, which multiplies num with itself. If num is a primitive value type, it can't be modified and there should be no side-effects. But if num is an object that defines valueOf all bets are off since arbitrary code could be executed, twice. There could be both side-effects and mutation.

@Victorystick
Copy link
Contributor

If only JavaScript had a way of telling a function was pure...

@Rich-Harris
Copy link
Contributor Author

Oh, I didn't say it'd be easy! I think it's reasonably to be pragmatic though and say that if you've defined valueOf to have side-effects, you deserve to have your programming license revoked. (Maybe if options.paranoid === true we skip these checks or something.)

If we assume that the developer hasn't poked about with valueOf or whatever, we can determine that both the IIFE and square are pure. I suspect there are a lot of cases where it's not possible to know if a function is pure (e.g. inside our locally declared foo function we call bar, imported from an external module, so we have to treat foo as though it has side-effects), but there are definitely some cases where we can confidently remove code.

@mbostock
Copy link
Contributor

It looks like this will be a relevant issue for D3 bundling. If I do a relatively minimal import from d3-scale, for example,

import {category10} from "d3-scale";

export default {
  scale: {
    category10: category10
  }
};

I end up pulling it quite a lot of d3-time and d3-time-format. They’re used by other parts of d3-scale (for time scales, naturally), but it seems the way I’ve structured them is not amenable to exclusion. I can investigate whether there’s a way I can change the code to avoid their superfluous inclusion, but it would be great to have an “unsafe” mode that is more lenient about ignoring code that probably doesn’t have side-effects. (On the other hand, if I intend the code to have side-effects, like with #13… then I want to include it!)

@Pauan
Copy link
Contributor

Pauan commented Oct 1, 2016

Here is a minimal example of some code.

The Foo function is pure because it only assigns to this, therefore it should be removed, but it is not removed.

This is a problem for me because I'm trying to use Rollup together with PureScript, and PureScript generates a lot of code similar to the above example, which means that it's not tree-shaken.

Is this the right issue for this, or should I create a new issue?

@Rich-Harris
Copy link
Contributor Author

We can close this now, a lot of this side-effect detection is implemented. Still work to do but we've come a long way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants