Improve if statement rendering #2146

lukastaegert · 2018-04-21T17:47:48Z

Resolves #2134

// if test is not included, it is impossible that alternate===null even though it is the retained branch

With regard to this (actually, my) cocky code comment, stating that something is impossible does not make it impossible 😜.

This refactors and improves the if statement logic so that

The test, consequent and alternate branches are now included separately and only if both there is a chance the code is actually executed and they have side-effects
alternate branches that are not included are always completely removed instead of sometimes leaving an empty block
consequent branches that are not included but are necessary for rendering because the test is included are replaced by an empty block {} (I also could have gone for an empty statement ; but this irrationally felt somewhat safer)

As this is essentially a bug fix for #2134, I branched this off the latest patch release so this can again be released as a patch release.

guybedford

Nice work on the quick fix. I know you're time-pressed so no pressure on all the feedback to get the bug fix in.

guybedford · 2018-04-22T09:05:59Z

src/ast/nodes/IfStatement.ts

+			if (this.consequent.included) {
+				this.consequent.render(code, options);
+			} else {
+				code.overwrite(this.consequent.start, this.consequent.end, '{}');


Can we not keep this as ; as the tests are currently? It still looks much nicer as an empty statement to me.

I guess you are right.

guybedford · 2018-04-22T09:31:02Z

src/ast/nodes/IfStatement.ts

@@ -45,33 +50,41 @@ export default class IfStatement extends StatementBase {
 	}

 	render(code: MagicString, options: RenderOptions) {
-		const testValue = this.test.getValue();
+		const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.test.getValue();
 		if (
 			!this.context.treeshake ||


I'm not quite sure I follow why this check is being done since it seems like we are performance treeshaking in this block?

You are absolutely right, this check is totally unnecessary as in the no-treeshaking-case, all nodes are included a-priori anyway. I have also adjusted the few other places where such a check was made so that the no-treeshaking logic is now very localized indeed.

guybedford · 2018-04-22T09:31:55Z

src/ast/nodes/IfStatement.ts

 		} else {
-			// if test is not included, it is impossible that alternate===null even though it is the retained branch
+			// if test is not included, then the if statement is included because
+			// there is exactly one included branch
 			const branchToRetain = testValue ? this.consequent : this.alternate;
 			code.remove(this.start, branchToRetain.start);
 			code.remove(branchToRetain.end, this.end);
 			branchToRetain.render(code, options);


It might be nice to try simplify this render logic a little.

Perhaps flip the parity and put this case at the top of render to catch first, with the else as the top block, so it's easier to see the fallthrough?

if (this.context.treeshake && !test.included && testValue ? this.alternate === null || !this.alternate.included : !consequent.included) { const branchToRetain = testValue ? this.consequent : this.alternate; // ... } else { }

You are right, flipping the logic makes it look much nicer. I also discovered another unchecked case where the test was not included even though both branches were included. This is now handled by overwriting the test with the actual stringified test value, further simplifying the output.

guybedford · 2018-04-22T09:33:27Z

src/ast/nodes/IfStatement.ts

 		}
-		return testValue
-			? predicateFunction(this.consequent)
-			: this.alternate !== null && predicateFunction(this.alternate);


Nice simplification to remove this!

* Flip render logic * Handle side-effect free non-trivial tests in if statements * Inline "relevantBranch" logic everywhere

lukastaegert · 2018-04-23T08:00:24Z

Thanks for the quick feedback! I have made the adjustments and also reworked conditional and logical expressions to get rid of the "relevantBranch" logic and adjust the rendering functions so that the three nodes are now much more similar.

guybedford

Looks great - again thanks for the quick feedback cycle here! Main thing would just be to clarify that one question about value inlining if you can for me.

guybedford · 2018-04-23T13:20:53Z

src/ast/nodes/IfStatement.ts

-			this.test.included ||
-			(testValue ? this.alternate !== null && this.alternate.included : this.consequent.included)
+			!this.test.included &&
+			(testValue ? this.alternate === null || !this.alternate.included : !this.consequent.included)


Perhaps put brackets around the (A || B) clause here for clarity (although I usually like to skip brackets for && though out of brevity provided those trained by C compilers can get past their reservations... :P)

Try this and you might learn something new about prettier 😜

Well there goes that debate...!

guybedford · 2018-04-23T13:35:01Z

src/ast/nodes/IfStatement.ts

+			if (this.test.included) {
+				this.test.render(code, options);
+			} else {
+				code.overwrite(this.test.start, this.test.end, JSON.stringify(testValue));


Is this our first implementation of value inlining? 😮

Although we probably wouldn't want this if values ever track more complex objects. That said getValueAtPath neatly avoids this problem.

What if the value was a really long string? Would we still be inlining it?

Also surely we only need to inline the truthiness or falsyness of the value over the actual value itself?

Yes, getValue(AtPath) can and only ever should return primitive values or UNKNOWN_VALUE. That being said, you are right, it would make much more sense to just always write true or false here.

guybedford · 2018-04-23T13:37:50Z

test/function/samples/handle-statically-resolvable-if-statements/main.js

+	x = 1;
+} else if (12 !== 12) {
+	x = 2;
+}


I found myself really wanting to be able to review the output on this one - form tests would be preferable once we've got these set up on a single module format for these analysis scenarios.

Note that for function tests, you can always add show: true to the config to inspect the output.

I wanted to have at least one of those tests as it is really easy in form tests to overlook invalid syntax.

There are in fact a lot of form tests covering the combinatorics of if statements.

Yes I understand, function is super important to catch things too. I wanted to check the JSON.stringify(getValue() case tests which this seemed to be and it helps to review to see the cases without having to clone. So that's why I was wondering if getting those analysis form test structures going would help here.

BTW the stringify/overwrite logic does in fact not cover your case. The only situation where this logic is applied at the moment is if both branches are included but the test is not included. This is only possible if

the statement is statically resolvable, but

the dead branch is included because it contains a used variable declaration

This would be an example:
https://rollupjs.org/repl?version=0.58.2&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMGxldCUyMHglMjAlM0QlMjAwJTNCJTVDbiU1Q25pZiUyMCgxMiUyMCUzRCUzRCUzRCUyMDEyKSUyMCU3QiU1Q24lNUN0eCUyMCUzRCUyMDElM0IlNUNuJTdEJTIwZWxzZSUyMCU3QiU1Q24lNUN0dmFyJTIweSUzQiU1Q24lN0QlNUNuJTVDbmV4cG9ydCUyMCU3QnklN0QlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

In all other situations, either the test is not rendered at all or it needs to be rendered as it is because the value is unknown.

Interesting, yes that fills in the gap I was missing thanks.

guybedford · 2018-04-23T13:44:50Z

src/ast/nodes/ConditionalExpression.ts

+		if (testValue === UNKNOWN_VALUE) {
+			return this.consequent.hasEffects(options) || this.alternate.hasEffects(options);
+		}
+		return testValue ? this.consequent.hasEffects(options) : this.alternate.hasEffects(options);
 	}

 	hasEffectsWhenAccessedAtPath(path: ObjectPath, options: ExecutionPathOptions): boolean {


I've always wondered what this means - is it referring to obj.path[c ? d : e]? Or something else?

No, it means you access e.g. (c ? d : e).the.path. In this example, the path ['the', 'path'] of c ? d : e is accessed.

For obj.path[c ? d : e], we would just be accessing the path [UNKNOWN_KEY] of obj.path. The "unknown" is because non-literal computed keys are never evaluated at the moment (we might extend getValueAtPath to be used here as well, though).

Thanks that helps a lot... I may start PR'ing some comments if I work on these parts to help those who follow.

guybedford · 2018-04-23T13:54:08Z

src/ast/nodes/IfStatement.ts

+			if (this.consequent.included) {
+				this.consequent.render(code, options);
+			} else {
+				code.overwrite(this.consequent.start, this.consequent.end, ';');


Not expecting to get this in on this PR, but how about optimizing this a little further?

if (this.alternate === null || !this.alternate.included) { code.overwrite(this.start, this.start + 3, ''); code.overwrite(this.consequent.start - 1, this.consequent.end, ';'); } else { code.insertLeft(this.test.start, '!'); code.overwrite(this.consequent.start, this.consequent.end + 'false'.length, ''); }

So if I see this correctly, if you have both the test and only one branch included, you want to render them as just two separate statements?
That's actually a good idea but I would refrain from this for this bug fix as I see some potential for introducing new bugs here. Maybe just make it a separate PR?

It was actually meant in the cases of the test not being included to have:

if (true) effect();

become effect();, and

if (false) ; else effect();

become effect().

I can certainly look at a separate PR though!

Also, if (effect()); to become effect();.

Ahh I see - the two statement variation would also be worthwhile here yes.

If the test is not included, it already works this way! This is the first branch with the code.remove statements.

So missing (for a future PR?) are:

Only the test is included -> render it as a statment

The test and one other branch that is always executed is included -> render as two statements

…tions that have a defined value.

lukastaegert · 2018-04-23T15:30:10Z

Released as 0.58.2

Improve if statement rendering; resolves #2134

3d4fda0

lukastaegert requested a review from guybedford April 21, 2018 17:47

lukastaegert mentioned this pull request Apr 21, 2018

Static if statements fail to build in specific cases. #2134

Closed

guybedford reviewed Apr 22, 2018

View reviewed changes

* Do not unnecessarily check if tree-shaking is enabled

c1bc6ca

* Flip render logic * Handle side-effect free non-trivial tests in if statements * Inline "relevantBranch" logic everywhere

guybedford approved these changes Apr 23, 2018

View reviewed changes

lukastaegert added 3 commits April 23, 2018 16:18

Directly use true and false when replacing side-effect-free condi…

6c002d1

…tions that have a defined value.

Update changelog

8e3495f

0.58.2

d21b935

lukastaegert merged commit d21b935 into master Apr 23, 2018

lukastaegert deleted the fix-if-statements branch April 23, 2018 15:28

steve-taylor mentioned this pull request Aug 6, 2018

TypeError: Cannot read property 'start' of null #2381

Closed

Improve if statement rendering #2146

Improve if statement rendering #2146

Conversation

lukastaegert commented Apr 21, 2018 • edited

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert commented Apr 23, 2018

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford Apr 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert commented Apr 23, 2018

lukastaegert commented Apr 21, 2018 •

edited

guybedford Apr 23, 2018 •

edited