Shorten assignments to itself #230

kangax · 2016-10-27T18:58:53Z

No description provided.

boopathi · 2016-10-27T19:00:59Z

packages/babel-plugin-minify-simplify/package.json

@@ -1,6 +1,6 @@
 {
  "name": "babel-plugin-minify-simplify",
-  "version": "0.0.3",
+  "version": "0.0.4",


this should be done by lerna and not manually.

ok, removed

don't need to change package.json either

kangax · 2016-10-27T19:05:34Z

packages/babel-plugin-minify-simplify/src/index.js

@@ -28,6 +28,17 @@ module.exports = ({ types: t }) => {
  const or = (a, b) => t.logicalExpression("||", a, b);
  const and = (a, b) => t.logicalExpression("&&", a, b);

+  const operators = new Set([


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.

boopathi · 2016-10-28T15:51:14Z

packages/babel-plugin-minify-simplify/src/index.js

+        const canShorten = (
+          right.type === 'BinaryExpression' &&
+          operators.has(right.node.operator) &&
+          left.node.name === right.node.left.name


requires isIdentifier check. if left.node and right.node.left are not identifiers, it can end up in undefined === undefined check and canShorten will be true - eg: [a,b] = foo() + 1 - canShorten will be true for this.

boopathi · 2016-10-28T15:51:27Z

packages/babel-plugin-minify-simplify/src/index.js

+        const left = path.get('left');
+
+        const canShorten = (
+          right.type === 'BinaryExpression' &&


right.isBinaryExpression()

Good to know, thanks!

boopathi · 2016-10-28T15:52:52Z

packages/babel-plugin-minify-simplify/src/index.js

+          right.node.right.value === 1 &&
+          updateOperators.has(right.node.operator));
+
+        if (left.node.name === undefined) {


left.isMemberExpression() is better IMO, rather than depending on left not being an identifier.

I wasn't sure if it's always going to be member expression. This felt like a broader/safer check. But sure, probably ok to change it.

boopathi · 2016-10-28T15:53:58Z

packages/babel-plugin-minify-simplify/src/index.js

+
+          const newExpression = t.updateExpression(
+            right.node.operator + right.node.operator,
+            t.identifier(left.node.name));


probably needs to handle member expression here ?

Wonder if we can make a generic utility for handling both Identifier/MemberExpression?

boopathi · 2016-10-28T15:54:10Z

packages/babel-plugin-minify-simplify/src/index.js

+
+          const newExpression = t.assignmentExpression(
+            right.node.operator + '=',
+            t.identifier(left.node.name),


handle memberExpressions

boopathi · 2016-10-28T15:56:23Z

packages/babel-plugin-minify-simplify/__tests__/simplify-test.js

+    ];
+
+    actual.forEach((test, index) => {
+      expect(transform(test)).toBe(expected[index]);


expect(actual.map((s) => transform(s))).toEqual(expected);

here you compare two arrays and even if one fails in the list, the others still run. But when you do forEach when one of them fails, all else that follows won't be run at all.

So you're thinking to wrap them in try/catch?

boopathi · 2016-10-28T15:58:17Z

packages/babel-plugin-minify-simplify/src/index.js

+		    if (!canShorten) return;
+
+        const canBeUpdateExpression = (
+          right.node.right.type === 'NumericLiteral' &&


right.get("right").isNumericLiteral() or t.isNumericLiteral(right.node.right)

kangax · 2016-11-01T00:52:57Z

packages/babel-plugin-minify-simplify/src/index.js

+
+  function isEqual(arr1, arr2) {
+    return arr1.every((value, index) => {
+      return value.toString() === arr2[index].toString();


Not using lodash.isequal here coz of slightly custom logic

kangax · 2016-11-01T00:54:31Z

packages/babel-plugin-minify-simplify/src/index.js

+    const prop = path.get('property');
+    const propNames = [getName(prop.node)];
+
+    while (obj.type !== 'Identifier' &&


@boopathi @shinew lmk if you see a more elegant/robust solution here, with both getPropNames and getName

This is just to traverse the chain of MemberExpressions like a.b.c.d, right? If so, can't we just let the condition be t.isMemberExpression(node)?

kangax · 2016-11-01T01:01:38Z

packages/babel-plugin-minify-simplify/src/index.js

+          const rightPropNames = getPropNames(rightExpr.get('left'));
+
+          if (!leftPropNames ||
+              leftPropNames.includes(undefined) ||


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

.indexOf instead of .includes for node4

shinew · 2016-11-01T17:50:07Z

packages/babel-plugin-minify-simplify/__tests__/simplify-test.js

+      foo *= function () {},
+      foo += 123,
+      foo = 1 + foo,
+      x = x++,


Not equivalent: if x = 0 at the start, then x = x = x + 1 makes x = 1, while x = x++ makes x = 0 (tested in Chrome). Maybe x = ++x is what we want? (see comment about prefix = true below)

shinew · 2016-11-01T18:11:47Z

packages/babel-plugin-minify-simplify/src/index.js

+        const leftExpr = path.get('left');
+
+        const canBeUpdateExpression = (
+          rightExpr.get('right').isNumericLiteral() &&


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

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

shinew · 2016-11-01T18:20:26Z

packages/babel-plugin-minify-simplify/src/index.js

+  }
+
+  function getName(node) {
+    if (node.type === 'ThisExpression') {


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

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?

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.

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

shinew · 2016-11-01T18:30:56Z

packages/babel-plugin-minify-simplify/src/index.js

+    '+', '-'
+  ]);
+
+  function isEqual(arr1, arr2) {


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

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?

shinew · 2016-11-01T18:37:30Z

packages/babel-plugin-minify-simplify/src/index.js

+      if (node) {
+        propNames.push(getName(node));
+      }
+      obj = obj.get('object');


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.

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.

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.

shinew · 2016-11-01T18:45:19Z

packages/babel-plugin-minify-simplify/src/index.js

+    // string/number literals
+    // but still match against each other
+    return node.name
+      ? node.name + '_'


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; }

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.

shinew · 2016-11-01T18:55:22Z

packages/babel-plugin-minify-simplify/src/index.js

+        if (canBeUpdateExpression) {
+          newExpression = t.updateExpression(
+            rightExpr.node.operator + rightExpr.node.operator,
+            t.clone(leftExpr.node));


Adding prefix = true would probably resolve the prefix/postfix issues, since x += 1 and ++x returns the new value of x, while x++ returns the old value of x.

kangax · 2016-11-03T20:30:09Z

@boopathi @shinew fixed some things up, PTAL

kangax · 2016-11-10T03:48:47Z

@boopathi I'd love to merge this ASAP and cut a new release. Could you please take one last look?

boopathi · 2016-11-10T18:44:22Z

I'd try using it with other plugins - especially block-scoping, dce and simplify.

Mangler is in broken state. Let's fix that before a release.

shinew · 2016-11-10T19:10:55Z

LGTM 👍

kangax · 2016-11-10T19:26:32Z

I'm checking this addition on our codebase with all the other plugins.

Agreed about mangler.

boopathi · 2016-11-11T21:08:32Z

packages/babel-plugin-minify-simplify/src/index.js

+          if (!leftPropNames ||
+              leftPropNames.includes(undefined) ||
+              !rightPropNames ||
+              rightPropNames.includes(undefined) ||


boopathi · 2016-11-11T23:26:13Z

packages/babel-plugin-minify-simplify/src/index.js

+    "+", "-"
+  ]);
+
+  function isEqual(arr1, arr2) {


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

kangax added 3 commits October 27, 2016 14:50

Simplify assignments to same identifier

3344baa

Add tests

a4d10bc

Bump versions

c666da1

kangax added this to the 0.0.7 milestone Oct 27, 2016

boopathi reviewed Oct 27, 2016

View reviewed changes

Remove versioning

c6ec99d

kangax commented Oct 27, 2016

View reviewed changes

kangax modified the milestone: 0.0.7 Oct 27, 2016

Remove version

6b913fe

boopathi reviewed Oct 28, 2016

View reviewed changes

boopathi added the Tag: New Feature Pull Request adding a new feature label Oct 31, 2016

Add support for optimizing member expressions

c1aa0ca

kangax changed the title ~~Shorten assignments to same identifier~~ Shorten assignments to itself Nov 1, 2016

Fix test name

6dfe218

kangax commented Nov 1, 2016

View reviewed changes

kangax added 2 commits October 31, 2016 20:55

Remove comment

6813842

Simplify ternary

5ddc5f8

kangax commented Nov 1, 2016

View reviewed changes

shinew reviewed Nov 1, 2016

View reviewed changes

kangax added 2 commits November 1, 2016 19:20

Fix bugs, simplify check

2c7a0d5

Change postfix to prefix

4f7e8eb

Merge master

64ed8d9

Exclude operators for member expressions too

8c0a456

boopathi reviewed Nov 11, 2016

View reviewed changes

kangax and others added 2 commits November 11, 2016 14:32

includes -> indexOf

6eaded6

Fix lint

edd236f

boopathi reviewed Nov 11, 2016

View reviewed changes

Improve method name

b058bd0

kangax merged commit 319d69d into master Nov 13, 2016

kangax deleted the assignment branch November 13, 2016 20:26

kangax added this to the 0.0.9 milestone Nov 13, 2016

Shorten assignments to itself #230

Shorten assignments to itself #230

Conversation

kangax commented Oct 27, 2016

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

Choose a reason for hiding this comment

shinew Nov 1, 2016 • 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

Choose a reason for hiding this comment

shinew Nov 1, 2016 • 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

shinew Nov 1, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shinew Nov 1, 2016 • edited

Choose a reason for hiding this comment

kangax commented Nov 3, 2016

kangax commented Nov 10, 2016

boopathi commented Nov 10, 2016

shinew commented Nov 10, 2016

kangax commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shinew Nov 1, 2016 •

edited

shinew Nov 1, 2016 •

edited

shinew Nov 1, 2016 •

edited

shinew Nov 1, 2016 •

edited