if containing only return is dropped

[Conduitry]

Regression in 0.49.

function foo(x) {
	if (x) {
		return y;
	}
	bar();
}

foo(z);

produces

function foo(x) {
	bar();
}

foo(z);

[kzc]

Patch below fixes this particular issue with no test regressions:

--- a/src/ast/Node.js
+++ b/src/ast/Node.js
@@ -30,7 +30,7 @@ export default class Node {
        }
 
        hasEffects () {
-               return this.included || this.someChild( child => child.hasEffects() );
+               return this.included || this.someChild( child => child.shouldBeIncluded() );
        }
 
        hasEffectsWhenAssigned () {

@lukastaegert Does that match your intention?

[lukastaegert]

You are right, but if you change the logic here, then any function that only contains a return statement will be kept as well even though it clearly has no side-effects.
The reason is that return statements have no effects but .shouldBeIncluded() when they are encountered. I though this was enough, but apparently it is knot :)

I'm looking into this now, thanks to @Conduitry for this quick observation! If do not find a good solution soon, your patch is at least a working fix.

[kzc]

You are right, but if you change the logic here, then any function that only contains a return statement will be kept as well even though it clearly has no side-effects.

Not the case. 16 tests like the following still pass with the patch:

var x = true ? foo () : bar();

function foo () {
	return 'should be removed, because x is unused';
}

https://github.com/rollup/rollup/blob/master/test/form/samples/side-effect-q/main.js

[lukastaegert]

You are absolutely right, I just saw this myself! Nevertheless, let me check if it is not better to change the logic of the return statement instead.

[lukastaegert]

Ok, I understand it now, the reason your fix works is because the algorithm that determines if function calls have effects is the custom logic here that I was planning to replace on my next pull request. In so far, there is no reason not to do it this way for now, I will just have to find a better solution when I actually start refactoring this code :) I will push a new pull request with new tests in a second.

[lukastaegert]

Ah, again, it is not that easy. This is not removed now though it should have been:

function isRemoved ( x ) {
	if ( x ) {
		return 2;
	}
	return 1;
}

isRemoved( true );

Back to the drawing board.

[kzc]

It's better to err on the side of caution. Dropping necessary code is far worse than including unnecessary code.

[lukastaegert]

I totally agree. But I think I have a good solution now, should take a few minutes, though :)
My idea is to add an options object to .hasEffects...(options) calls which I wanted to add at some later point anyways. This will tell the algorithm if we are checking nested function calls for effects or not. In the first case, we ignore the effects of return statements, in the other case we don't.

[lukastaegert]

Should be fixed in a nice way by #1586.