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.

Sign up for GitHub

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

Jump to bottom

Improve if statement rendering #2146

Merged
merged 5 commits into from
Apr 23, 2018
Merged

Improve if statement rendering #2146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d4fda0
Improve if statement rendering; resolves #2134
lukastaegert Apr 21, 2018
c1bc6ca
* Do not unnecessarily check if tree-shaking is enabled
lukastaegert Apr 23, 2018
6c002d1
Directly use `true` and `false` when replacing side-effect-free condi…
lukastaegert Apr 23, 2018
8e3495f
Update changelog
lukastaegert Apr 23, 2018
d21b935
0.58.2
lukastaegert Apr 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# rollup changelog

## 0.58.2
*2018-04-23*
* Fix rendering of certain statically resolvable if statements ([#2146](https://github.com/rollup/rollup/pull/2146))

## 0.58.1
*2018-04-18*
* Fix comment detection ([#2129](https://github.com/rollup/rollup/pull/2129))
Expand Down
2 changes: 1 addition & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "rollup",
"version": "0.58.1",
"version": "0.58.2",
"description": "Next-generation ES6 module bundler",
"main": "dist/rollup.js",
"module": "dist/rollup.es.js",
Expand Down
155 changes: 84 additions & 71 deletions src/ast/nodes/ConditionalExpression.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,62 +23,73 @@ export default class ConditionalExpression extends NodeBase {
callback: ForEachReturnExpressionCallback,
options: ExecutionPathOptions
) {
if (this.hasUnknownTestValue) {
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
if (testValue === UNKNOWN_VALUE || testValue) {
this.consequent.forEachReturnExpressionWhenCalledAtPath(path, callOptions, callback, options);
}
if (testValue === UNKNOWN_VALUE || !testValue) {
this.alternate.forEachReturnExpressionWhenCalledAtPath(path, callOptions, callback, options);
} else {
this.forEachRelevantBranch(node =>
node.forEachReturnExpressionWhenCalledAtPath(path, callOptions, callback, options)
);
}
}

getValue(): any {
const testValue = this.test.getValue();
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
if (testValue === UNKNOWN_VALUE) return UNKNOWN_VALUE;
return testValue ? this.consequent.getValue() : this.alternate.getValue();
}

hasEffects(options: ExecutionPathOptions): boolean {
return (
this.test.hasEffects(options) ||
(this.hasUnknownTestValue
? this.consequent.hasEffects(options) || this.alternate.hasEffects(options)
: this.someRelevantBranch(node => node.hasEffects(options)))
);
if (this.test.hasEffects(options)) return true;
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return (
path.length > 0 &&
(this.hasUnknownTestValue
? this.consequent.hasEffectsWhenAccessedAtPath(path, options) ||
this.alternate.hasEffectsWhenAccessedAtPath(path, options)
: this.someRelevantBranch(node => node.hasEffectsWhenAccessedAtPath(path, options)))
);
if (path.length === 0) return false;
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
if (testValue === UNKNOWN_VALUE) {
return (
this.consequent.hasEffectsWhenAccessedAtPath(path, options) ||
this.alternate.hasEffectsWhenAccessedAtPath(path, options)
);
}
return testValue
? this.consequent.hasEffectsWhenAccessedAtPath(path, options)
: this.alternate.hasEffectsWhenAccessedAtPath(path, options);
}

hasEffectsWhenAssignedAtPath(path: ObjectPath, options: ExecutionPathOptions): boolean {
return (
path.length === 0 ||
(this.hasUnknownTestValue
? this.consequent.hasEffectsWhenAssignedAtPath(path, options) ||
this.alternate.hasEffectsWhenAssignedAtPath(path, options)
: this.someRelevantBranch(node => node.hasEffectsWhenAssignedAtPath(path, options)))
);
if (path.length === 0) return true;
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
if (testValue === UNKNOWN_VALUE) {
return (
this.consequent.hasEffectsWhenAssignedAtPath(path, options) ||
this.alternate.hasEffectsWhenAssignedAtPath(path, options)
);
}
return testValue
? this.consequent.hasEffectsWhenAssignedAtPath(path, options)
: this.alternate.hasEffectsWhenAssignedAtPath(path, options);
}

hasEffectsWhenCalledAtPath(
path: ObjectPath,
callOptions: CallOptions,
options: ExecutionPathOptions
): boolean {
return this.hasUnknownTestValue
? this.consequent.hasEffectsWhenCalledAtPath(path, callOptions, options) ||
this.alternate.hasEffectsWhenCalledAtPath(path, callOptions, options)
: this.someRelevantBranch(node =>
node.hasEffectsWhenCalledAtPath(path, callOptions, options)
);
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
if (testValue === UNKNOWN_VALUE) {
return (
this.consequent.hasEffectsWhenCalledAtPath(path, callOptions, options) ||
this.alternate.hasEffectsWhenCalledAtPath(path, callOptions, options)
);
}
return testValue
? this.consequent.hasEffectsWhenCalledAtPath(path, callOptions, options)
: this.alternate.hasEffectsWhenCalledAtPath(path, callOptions, options);
}

initialise() {
Expand All @@ -88,7 +99,7 @@ export default class ConditionalExpression extends NodeBase {

include() {
this.included = true;
const testValue = this.test.getValue();
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
if (testValue === UNKNOWN_VALUE || this.test.shouldBeIncluded()) {
this.test.include();
this.consequent.include();
Expand All @@ -102,11 +113,12 @@ export default class ConditionalExpression extends NodeBase {

reassignPath(path: ObjectPath, options: ExecutionPathOptions) {
if (path.length > 0) {
if (this.hasUnknownTestValue) {
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
if (testValue === UNKNOWN_VALUE || testValue) {
this.consequent.reassignPath(path, options);
}
if (testValue === UNKNOWN_VALUE || !testValue) {
this.alternate.reassignPath(path, options);
} else {
this.forEachRelevantBranch(node => node.reassignPath(path, options));
}
}
}
Expand All @@ -116,18 +128,18 @@ export default class ConditionalExpression extends NodeBase {
options: RenderOptions,
{ renderedParentType, isCalleeOfRenderedParent }: NodeRenderOptions = BLANK
) {
if (!this.context.treeshake || this.test.included) {
super.render(code, options);
} else {
const branchToRetain = this.consequent.included ? this.consequent : this.alternate;
code.remove(this.start, branchToRetain.start);
code.remove(branchToRetain.end, this.end);
branchToRetain.render(code, options, {
if (!this.test.included) {
const singleRetainedBranch = this.consequent.included ? this.consequent : this.alternate;
code.remove(this.start, singleRetainedBranch.start);
code.remove(singleRetainedBranch.end, this.end);
singleRetainedBranch.render(code, options, {
renderedParentType: renderedParentType || this.parent.type,
isCalleeOfRenderedParent: renderedParentType
? isCalleeOfRenderedParent
: (<CallExpression>this.parent).callee === this
});
} else {
super.render(code, options);
}
}

Expand All @@ -137,43 +149,44 @@ export default class ConditionalExpression extends NodeBase {
predicateFunction: SomeReturnExpressionCallback,
options: ExecutionPathOptions
): boolean {
return this.hasUnknownTestValue
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
if (testValue === UNKNOWN_VALUE) {
return (
this.consequent.someReturnExpressionWhenCalledAtPath(
path,
callOptions,
predicateFunction,
options
) ||
this.alternate.someReturnExpressionWhenCalledAtPath(
path,
callOptions,
predicateFunction,
options
)
);
}
return testValue
? this.consequent.someReturnExpressionWhenCalledAtPath(
path,
callOptions,
predicateFunction,
options
) ||
this.alternate.someReturnExpressionWhenCalledAtPath(
path,
callOptions,
predicateFunction,
options
)
: this.someRelevantBranch(node =>
node.someReturnExpressionWhenCalledAtPath(path, callOptions, predicateFunction, options)
)
: this.alternate.someReturnExpressionWhenCalledAtPath(
path,
callOptions,
predicateFunction,
options
);
}

private forEachRelevantBranch(callback: (node: ExpressionNode) => void) {
const testValue = this.test.getValue();
if (testValue === UNKNOWN_VALUE) {
this.hasUnknownTestValue = true;
callback(this.consequent);
callback(this.alternate);
} else if (testValue) {
callback(this.consequent);
} else {
callback(this.alternate);
}
}

private someRelevantBranch(predicateFunction: (node: ExpressionNode) => boolean): boolean {
const testValue = this.test.getValue();
if (testValue === UNKNOWN_VALUE) {
private getTestValue() {
if (this.hasUnknownTestValue) return UNKNOWN_VALUE;
const value = this.test.getValue();
if (value === UNKNOWN_VALUE) {
this.hasUnknownTestValue = true;
return predicateFunction(this.consequent) || predicateFunction(this.alternate);
}
return testValue ? predicateFunction(this.consequent) : predicateFunction(this.alternate);
return value;
}
}
81 changes: 48 additions & 33 deletions src/ast/nodes/IfStatement.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,27 +14,32 @@ export default class IfStatement extends StatementBase {
private hasUnknownTestValue: boolean;

hasEffects(options: ExecutionPathOptions): boolean {
return (
this.test.hasEffects(options) ||
(this.hasUnknownTestValue
? this.consequent.hasEffects(options) ||
(this.alternate !== null && this.alternate.hasEffects(options))
: this.someRelevantBranch(node => node.hasEffects(options)))
);
if (this.test.hasEffects(options)) return true;
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
if (testValue === UNKNOWN_VALUE) {
return (
this.consequent.hasEffects(options) ||
(this.alternate !== null && this.alternate.hasEffects(options))
);
}
return testValue
? this.consequent.hasEffects(options)
: this.alternate !== null && this.alternate.hasEffects(options);
}

include() {
this.included = true;
const testValue = this.test.getValue();
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.getTestValue();
if (testValue === UNKNOWN_VALUE || this.test.shouldBeIncluded()) {
this.test.include();
}
if (testValue === UNKNOWN_VALUE) {
this.consequent.include();
if (this.alternate !== null) this.alternate.include();
} else if (testValue) {
if ((testValue === UNKNOWN_VALUE || testValue) && this.consequent.shouldBeIncluded()) {
this.consequent.include();
} else if (this.alternate !== null) {
}
if (
this.alternate !== null &&
((testValue === UNKNOWN_VALUE || !testValue) && this.alternate.shouldBeIncluded())
) {
this.alternate.include();
}
}
Expand All @@ -45,33 +50,43 @@ export default class IfStatement extends StatementBase {
}

render(code: MagicString, options: RenderOptions) {
const testValue = this.test.getValue();
// Note that unknown test values are always included
const testValue = this.hasUnknownTestValue ? UNKNOWN_VALUE : this.test.getValue();
if (
!this.context.treeshake ||
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try this and you might learn something new about prettier 😜

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well there goes that debate...!

) {
super.render(code, options);
const singleRetainedBranch = testValue ? this.consequent : this.alternate;
code.remove(this.start, singleRetainedBranch.start);
code.remove(singleRetainedBranch.end, this.end);
singleRetainedBranch.render(code, options);
} else {
// if test is not included, it is impossible that alternate===null even though it is the retained branch
const branchToRetain = testValue ? this.consequent : this.alternate;
code.remove(this.start, branchToRetain.start);
code.remove(branchToRetain.end, this.end);
branchToRetain.render(code, options);
if (this.test.included) {
this.test.render(code, options);
} else {
code.overwrite(this.test.start, this.test.end, testValue ? 'true' : 'false');
}
if (this.consequent.included) {
this.consequent.render(code, options);
} else {
code.overwrite(this.consequent.start, this.consequent.end, ';');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, '');
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can certainly look at a separate PR though!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
if (this.alternate !== null) {
if (this.alternate.included) {
this.alternate.render(code, options);
} else {
code.remove(this.consequent.end, this.alternate.end);
}
}
}
}

private someRelevantBranch(predicateFunction: (node: StatementNode) => boolean): boolean {
const testValue = this.test.getValue();
if (testValue === UNKNOWN_VALUE) {
private getTestValue() {
if (this.hasUnknownTestValue) return UNKNOWN_VALUE;
const value = this.test.getValue();
if (value === UNKNOWN_VALUE) {
this.hasUnknownTestValue = true;
return (
predicateFunction(this.consequent) ||
(this.alternate !== null && predicateFunction(this.alternate))
);
}
return testValue
? predicateFunction(this.consequent)
: this.alternate !== null && predicateFunction(this.alternate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice simplification to remove this!

return value;
}
}