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

Assume we always have all of the new effects unless specified otherwise #1604

Merged
merged 5 commits into from Sep 8, 2017

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Aug 31, 2017

This will resolve #1599, #1601 and #1603.

This is a very important patch as it will also prevent further bugs like the two mentioned above by setting .hasEffectsWhenAssigned() and .hasEffectsWhenMutated() to true for all nodes unless they explicitly specify it otherwise. Also, I made Promise() impure until we have a better solution. For safety reasons, I also made Promise.reject() impure since it can cause uncaught Promise rejections. Not sure if we will ever be able to fix this properly.

New tests

assignment-to-array-buffer-view

Here the mutation of the buffer via the second view was dropped. For now, this is solved by assuming that mutating the result of a NewExpression (or CallExpression for that matter) always has an effect. This will probably prevent tree-shaking in quite a few cases but is certainly necessary for now. The situation may improve at least for non-global functions once we have function return value assignment tracking in place (will take some time, though).

Output of v0.49.2
var buffer = new ArrayBuffer( 8 );

var view8 = new Int8Array( buffer );

export { view8 };
New output
var buffer = new ArrayBuffer( 8 );

var view8 = new Int8Array( buffer );
var view16 = new Int16Array( buffer );

view16[ 0 ] = 3;

export { view8 };

mutate-logical-expression

This was completely broken. However, instead of doing the same as for CallExpressions, I decided to implement the proper logic for LogicalExpressions. This means that if the LogicalExpression can be partially evaluated, mutating it has only an effect if mutating the result of the expression has an effect. In the future we might implement similar behaviour for ConditionalExpressions, but I decided to stick with the problem at hand for now.

Output of v0.49.2
var aExp = {};
var bExp = {};
var cExp = {};
var dExp = {};
var eExp = {};

export { aExp, bExp, cExp, dExp, eExp };
New output
var aExp = {};
var logicalAExp = aExp || true;
logicalAExp.bar = 1;

var bExp = {};
var cExp = {};
var logicalCExp = false || cExp;
logicalCExp.bar = 1;

var dExp = {};
var logicalDExp = true && dExp;
logicalDExp.bar = 1;

var eExp = {};

export { aExp, bExp, cExp, dExp, eExp };

promises

By making Promise impure, we no longer suppress side-effects of a Promise callback. This will for now also include Promises that do not have side-effects, but I am quite sure there will be a solution for this in the future.

Output of v0.49.2
const p2 = new Promise( () => {
	console.info( 'forget me as well' );
} );

const p3 = new Promise( () => {
	console.info( 'and me too' );
} );

const allExported = Promise.all([p2, p3]);

export { allExported };
New output
const p1 = new Promise( () => {
	console.log( 'fire & forget' );
} );

const p2 = new Promise( () => {
	console.info( 'forget me as well' );
} );

const p3 = new Promise( () => {
	console.info( 'and me too' );
} );

const p5 = Promise.reject('should be kept for uncaught rejections');

const allExported = Promise.all([p2, p3]);

export { allExported };

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

Successfully merging this pull request may close these issues.

Bug: Treeshaking destroys valid function contents > 0.49.0
2 participants