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

Shorten assignments to itself #230

Merged
merged 16 commits into from
Nov 13, 2016
Merged

Shorten assignments to itself #230

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3344baa
Simplify assignments to same identifier
kangax Oct 27, 2016
a4d10bc
Add tests
kangax Oct 27, 2016
c666da1
Bump versions
kangax Oct 27, 2016
c6ec99d
Remove versioning
kangax Oct 27, 2016
6b913fe
Remove version
kangax Oct 27, 2016
c1aa0ca
Add support for optimizing member expressions
kangax Nov 1, 2016
6dfe218
Fix test name
kangax Nov 1, 2016
6813842
Remove comment
kangax Nov 1, 2016
5ddc5f8
Simplify ternary
kangax Nov 1, 2016
2c7a0d5
Fix bugs, simplify check
kangax Nov 1, 2016
4f7e8eb
Change postfix to prefix
kangax Nov 1, 2016
64ed8d9
Merge master
kangax Nov 10, 2016
8c0a456
Exclude operators for member expressions too
kangax Nov 10, 2016
6eaded6
includes -> indexOf
kangax Nov 11, 2016
edd236f
Fix lint
boopathi Nov 11, 2016
b058bd0
Improve method name
kangax Nov 13, 2016
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
132 changes: 132 additions & 0 deletions packages/babel-plugin-minify-simplify/__tests__/simplify-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2352,4 +2352,136 @@ describe("simplify-plugin", () => {
const expected = source;
expect(transform(source)).toBe(expected);
});

it("should simplify assignments", () => {

const source = unpad(`
x = x + 1,
x = x - 1,
x = x * 1,
x = x % 1,
x = x << 1,
x = x >> 1,
x = x >>> 1,
x = x & 1,
x = x | 1,
x = x ^ 1,
x = x / 1,
x = x ** 1;
`);
const expected = unpad(`
++x,
--x,
x *= 1,
x %= 1,
x <<= 1,
x >>= 1,
x >>>= 1,
x &= 1,
x |= 1,
x ^= 1,
x /= 1,
x **= 1;
`).replace(/\s+/g, ' ');

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

it("should simplify assignments 2", () => {

const source = unpad(`
foo = foo + bar,
foo = foo * function(){},
foo += 123,
foo = 1 + foo,
x = x = x + 1,
foo = foo + bar + baz
`);
const expected = unpad(`
foo += bar,
foo *= function () {},
foo += 123,
foo = 1 + foo,
x = ++x,
foo = foo + bar + baz;
`).replace(/\s+/g, ' ');

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

it("should simplify assignments w. member expressions", () => {

const source = unpad(`
foo.bar = foo.bar + 1,
foo.bar = foo.bar + 2,
foo["x"] = foo[x] + 2,
foo[x] = foo[x] + 2,
foo[x] = foo["x"] + 2,
foo["x"] = foo["x"] + 2,
foo[1] = foo["1"] + 2,
foo["bar"] = foo["bar"] + 2,
foo[bar()] = foo[bar()] + 2,
foo[""] = foo[""] + 2,
foo[2] = foo[2] + 2,
foo[{}] = foo[{}] + 1,
foo[function(){}] = foo[function(){}] + 1,
foo[false] = foo[false] + 1,
foo.bar.baz = foo.bar.baz + 321,
this.hello = this.hello + 1,
foo[null] = foo[null] + 1,
foo[undefined] = foo[undefined] + 1,
foo.bar = foo.bar || {};
`);
// TODO: foo[void 0] = foo[void 0] + 1;
const expected = unpad(`
++foo.bar,
foo.bar += 2,
foo["x"] = foo[x] + 2,
foo[x] += 2,
foo[x] = foo["x"] + 2,
foo["x"] += 2,
foo[1] += 2,
foo["bar"] += 2,
foo[bar()] = foo[bar()] + 2,
foo[""] += 2,
foo[2] += 2,
foo[{}] = foo[{}] + 1,
foo[function () {}] = foo[function () {}] + 1,
++foo[false],
foo.bar.baz += 321,
++this.hello,
++foo[null],
++foo[undefined],
foo.bar = foo.bar || {};
`).replace(/\s+/g, ' ');

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

it("should simplify assignments w. super", () => {

const source = unpad(`
class Foo {
foo() {
super.foo = super.foo + 1;
}
};
`);
const expected = unpad(`
class Foo {
foo() {
++super.foo;
}
};
`);

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

it("should not simplify assignments w. template literals", () => {

const source = unpad('foo[`x`] = foo[`x`] + 1;');

expect(transform(source)).toBe(source);
});
});
107 changes: 107 additions & 0 deletions packages/babel-plugin-minify-simplify/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,62 @@ module.exports = ({ types: t }) => {
const or = (a, b) => t.logicalExpression("||", a, b);
const and = (a, b) => t.logicalExpression("&&", a, b);

const operators = new Set([
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to consider splitting plugins into multiple modules. This kind of mixing of helpers and supplemental stuff is not great ^. I think we could even strive for 1 type-of-transform per file.

'+', '-', '*', '%',
'<<', '>>', '>>>',
'&', '|', '^', '/',
'**'
]);

const updateOperators = new Set([
'+', '-'
]);

function isEqual(arr1, arr2) {
Copy link
Contributor

@shinew shinew Nov 1, 2016 •
edited

Choose a reason for hiding this comment

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

This function should be isEqualToPrefix rather than isEqual. isEqual([1], [1, 2]) === true given this implementation -- is this expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, good catch. So the reason it works is because property names are resolved RTL so foo.bar won't match foo.bar.baz due to arrays being ['bar', 'foo'] and ['baz', 'bar', 'foo']. We can maybe do an additional length check which could be a nice compromise between speed and correctness?

Copy link
Member

Choose a reason for hiding this comment

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

this is specific to this feature, we should have a comment or change name to indicate that, since this is at the top level (nearly).

return arr1.every((value, index) => {
return String(value) === String(arr2[index]);
});
}

function getName(node) {
if (node.type === 'ThisExpression') {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why not t.isThisExpression or t.isSuper?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really care; I would just want us to be consistent in the entire codebase. Do we want abstractions over performance or vice versa? Ditto with .get(...) vs node access. Let's chose one and stick to that?

Copy link
Member

Choose a reason for hiding this comment

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

It's also about being able to use type aliases. So t.isSomething is a better check that you don't have to worry about whether it's an alias or not. If we forget that it's an alias and add some code, it'll be buggy and hard to debug later.

Copy link
Member

Choose a reason for hiding this comment

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

Unless we start compiling t.isThisExpression -> node.type === 'ThisExpression' 😛 - I was saying we could write a few linting rules for these

return 'this';
}
if (node.type === 'Super') {
return 'super';
}
if (node.type === 'NullLiteral') {
return 'null';
}
// augment identifiers so that they don't match
// string/number literals
// but still match against each other
return node.name
? node.name + '_'
Copy link
Contributor

@shinew shinew Nov 1, 2016 •
edited

Choose a reason for hiding this comment

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

This seems a bit hacky -- would it make sense to return an object with "type" and "value" properties instead?

Also, since the type of .property is technically either Expression or Literal, it might be better to be more explicit:

if (t.isIdentifier(node)) {
  return {'type': 'id', 'value': node.name};
} else if (t.isLiteral(node)) {
  return {'type': 'lit', 'value': node.value};
} else {
 return undefined;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about object-based abstraction but wasn't sure about perf. impact of all the "unnecessary" intermediate objects. Definitely would be less hackish.

The reason I went with duck-typing instead of explicit isIdentifier/isLiteral is in case I was forgetting some other abstraction. I'm also not sure about assumption of if something is a literal it will have a value and so on.

I just checked and I see that NullLiteral doesn't have value. Neither does TemplateLiteral :/

null also throws since I call toString on it instead of String(...).

Let me clean this up a little.

: node.value /* Literal */;
}

function getPropNames(path) {
if (!path.isMemberExpression()) {
return;
}

let obj = path.get('object');

const prop = path.get('property');
const propNames = [getName(prop.node)];

while (obj.type === 'MemberExpression') {
const node = obj.get('property').node;
if (node) {
propNames.push(getName(node));
}
obj = obj.get('object');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: .get() calls are more expensive than just directly referencing the nodes' children.

It's probably faster/clearer to use nodes instead of .get(), which returns paths. This doesn't need path-specific methods, like scoping.

Copy link
Member Author

Choose a reason for hiding this comment

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

get was recommended by @boopathi so I tried to stick to that, but see response above. Let's just figure out which to use everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

it's useful to have path in many places especially when there is a bug... we can optimize to nodes later when we have it correct. path has more information and we might not know when we have a bug in the code. I faced issues where some information from path(at least parentPath) would be required to solve the issue.

}
propNames.push(getName(obj.node));

return propNames;
}
const OP_AND = (input) => input === "&&";
const OP_OR = (input) => input === "||";

Expand Down Expand Up @@ -194,6 +250,57 @@ module.exports = ({ types: t }) => {
}
},

AssignmentExpression(path) {

const rightExpr = path.get('right');
const leftExpr = path.get('left');

const canBeUpdateExpression = (
rightExpr.get('right').isNumericLiteral() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the behavior if .get('right') fails?

Copy link
Member

Choose a reason for hiding this comment

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

Should be ok - gives a path anyway? http://astexplorer.net/#/qyjXEtbUxr

rightExpr.get('right').node.value === 1 &&
updateOperators.has(rightExpr.node.operator));

if (leftExpr.isMemberExpression()) {

const leftPropNames = getPropNames(leftExpr);
const rightPropNames = getPropNames(rightExpr.get('left'));

if (!leftPropNames ||
leftPropNames.includes(undefined) ||
Copy link
Member Author

Choose a reason for hiding this comment

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

We bail out whenever we can't resolve "name" of one of the properties in the chain; e.g. foo[bar()]

Copy link
Member

Choose a reason for hiding this comment

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

.indexOf instead of .includes for node4

!rightPropNames ||
rightPropNames.includes(undefined) ||
Copy link
Member

Choose a reason for hiding this comment

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

indexOf

!operators.has(rightExpr.node.operator) ||
!isEqual(leftPropNames, rightPropNames)) {
return;
}
}
else {
if (!rightExpr.isBinaryExpression() ||
!operators.has(rightExpr.node.operator) ||
leftExpr.node.name !== rightExpr.node.left.name) {
return;
}
}

let newExpression;

// special case x=x+1 --> ++x
if (canBeUpdateExpression) {
newExpression = t.updateExpression(
rightExpr.node.operator + rightExpr.node.operator,
t.clone(leftExpr.node),
true /* prefix */);
}
else {
newExpression = t.assignmentExpression(
rightExpr.node.operator + '=',
t.clone(leftExpr.node),
t.clone(rightExpr.node.right));
}

path.replaceWith(newExpression);
},

ConditionalExpression: {
enter: [
// !foo ? 'foo' : 'bar' -> foo ? 'bar' : 'foo'
Expand Down