Skip to content

Missing side-effects when rolling up acorn #96

Description

@Rich-Harris

This follows #88 and #95. Rollup doesn't account for this pattern:

import {Parser} from './state';

/* some code happens... */

const pp = Parser.prototype

pp.initialContext = function() {
  return [types.b_stat]
}

/* a bunch more methods get added to pp... */

Rollup is smart enough to recognise that changes to Parser.prototype should be included, if we're dependent on Parser, but not smart enough to recognise that changes to pp should also be included. So we need to expand the search for 'modifier statements' to include aliases (where pp is treated as an alias of Parser by virtue of the fact that Parser is one of the one-or-more names contained in the right node of an AssignmentExpression where the left node is an Identifier whose name is pp, or the init node of a VariableDeclarator whose id.name is pp), and aliases of aliases. Tricky but doable (he said confidently).

It might yield false positives unless later on we distinguish between reassignments and mutations – at the moment they're essentially regarded as the same thing. In other words:

// if this is included
var a = something;

var b = a;

// this should be included, because `b` is an alias of `a`, and is mutated
b.prop = 'whatever';

// but this shouldn't be, because we can't indirectly mutate `a` like this
b = c;

But that can be solved later; false positives are a lower priority than false negatives.

This isn't a complete solution, because (and I suppose deep down I always knew this...) it's possible for imported-but-ignored modules to have side-effects:

// main.js
import { foo } from './foo';
import { bar } from './bar';

assert.ok( foo.wasMutated );

// foo.js
export var foo = {};

// bar.js
import { foo } from './foo';

var f = foo;
f.wasMutated = true;

export var bar = 'whatever';

Rollup would ignore bar.js because the bar binding is never used in main.js, so it would never 'discover' the side-effect even if it knew what to do with it. This might be a rare (and ill-advised) pattern, but if it exists in the wild we have to deal with it: the entire dependency graph as described by import statements will need to be loaded and parsed, regardless of whether the bindings described by those imports are actually used. (This would only increase bundle size in situations where side-effects do exist; the reason it's regrettable is purely that it will slow Rollup down a bit in cases where it was previously able to get away with not parsing some code.)

The upside of such a change is that the codebase could be made a good bit simpler – instead of having to accommodate asynchronous loaders at various places, we can confine the asynchrony to the initial loading phase and trace bindings synchronously thereafter.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions