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

don't remove impure reachable parts of logical expressions #160

Merged
merged 3 commits into from
Sep 14, 2016

Conversation

goto-bus-stop
Copy link
Contributor

@goto-bus-stop goto-bus-stop commented Sep 14, 2016

An attempt at dealing with #159.

Impure statements like function calls were removed from the right-hand side of && expressions if they were inferred to return a falsy value (e.g. when using the void operator).

This patch checks if the thing to be removed is pure. There's a lot more manual checking involved but it also simplifies more stuff.

path.replaceWith(node.left);
}
} else if (node.operator === "||") {
if (left.evaluateTruthy() === false && left.isPure()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, that's not right :P

@goto-bus-stop
Copy link
Contributor Author

I think I fixed my logic mistakes. I'm not entirely sure if all the branches I put in are necessary or if flip-expressions obsoletes some of them.

expect(transform(source)).toBe(source);

source = unpad(`
alert(func() || true);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we remove true from the RHS here since it has no side effects?

Copy link
Member

Choose a reason for hiding this comment

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

a = foo() || true. a should be true if foo() is truthy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if func() is falsy, func() || true would still evaluate to true, so the RHS is significant for the final value

Copy link
Member

Choose a reason for hiding this comment

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

Oh, of course. Missed it.

@kangax kangax merged commit e1b0d31 into babel:master Sep 14, 2016
@hzoo hzoo added the bug Confirmed bug label Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants