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

Implemented transform-remove-undefined plugin. #197

Merged
merged 13 commits into from
Nov 8, 2016
4 changes: 4 additions & 0 deletions packages/babel-plugin-remove-undefined-if-possible/.npmignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
src
__tests__
node_modules
*.log
59 changes: 59 additions & 0 deletions packages/babel-plugin-remove-undefined-if-possible/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# babel-plugin-remove-undefined-if-possible

For variable assignments, this removes rvals that evaluate to `undefined`
(`var`s in functions only).
For functions, this removes return arguments that evaluate to `undefined`.

## Example

**In**

```javascript
let a = void 0;
function foo() {
var b = undefined;
return undefined;
}
```

**Out**

```javascript
let a;
function foo() {
var b;
return;
}
```

## Installation

```sh
$ npm install babel-plugin-remove-undefined-if-possible
```

## Usage

### Via `.babelrc` (Recommended)

**.babelrc**

```json
{
"plugins": ["babel-plugin-remove-undefined-if-possible"]
}
```

### Via CLI

```sh
$ babel --plugins babel-plugin-remove-undefined-if-possible script.js
```

### Via Node API

```javascript
require("babel-core").transform("code", {
plugins: ["babel-plugin-remove-undefined-if-possible"]
});
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
jest.autoMockOff();

const babel = require("babel-core");
const plugin = require("../src/index");

function transform(code) {
return babel.transform(code, {
plugins: [plugin],
}).code;
}

describe("remove-undefined-if-possible-plugin", () => {
it("should remove multiple undefined assignments", () => {
const source = "let a = undefined, b = 3, c = undefined, d;";
const expected = "let a,\n b = 3,\n c,\n d;";
expect(transform(source)).toBe(expected);
});

it("should remove let-assignments to undefined", () => {
const source = "let a = undefined;";
const expected = "let a;";
expect(transform(source)).toBe(expected);
});

it("should remove let-assignments to void 0", () => {
const source = "let a = void 0;";
const expected = "let a;";
expect(transform(source)).toBe(expected);
});

it("should remove undefined return value", () => {
const source = `
Copy link
Member

Choose a reason for hiding this comment

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

...
  ...
      const source = unpad(`
        ...
      `);

This is how all other tests are aligned.

function foo() {
const a = undefined;
return undefined;
}`;
const expected = `
function foo() {
const a = undefined;
return;
}`;
expect(transform(source)).toBe(expected);
});

it("should remove var declarations in functions", () => {
const source = `
function foo() {
var a = undefined;
}`;
const expected = `
function foo() {
var a;
}`;
expect(transform(source)).toBe(expected);
});

it("should not remove const-assignments to undefined", () => {
const source = "const a = undefined;";
expect(transform(source)).toBe(source);
});

it("should not remove var-assignments in loops", () => {
const source = `
for (var a = undefined;;) {
var b = undefined;
}`;
expect(transform(source)).toBe(source);
});

it("should not remove var-assignments if referenced before", () => {
const source = `
function foo() {
a = 3;
var a = undefined;
}`;
expect(transform(source)).toBe(source);
});
Copy link
Member

Choose a reason for hiding this comment

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

test cases for

  • void foo()

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't convert

function foo() {
  var x = void 0;
  function bar() {
    var x = void 0;
    function baz() {
      var x = void 0;
    }
  }
}

});
96 changes: 96 additions & 0 deletions packages/babel-plugin-remove-undefined-if-possible/src/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
"use strict";

function isInLocalLoop(path, t) {
if (path === null) {
return false;
} else if (t.isLoop(path)) {
return true;
} else if (t.isFunction(path)) {
return false;
} else {
return isInLocalLoop(path.parentPath, t);
}
}

function removeRvalIfUndefined(declaratorPath) {
const rval = declaratorPath.get("init")
.evaluate();
Copy link
Member

Choose a reason for hiding this comment

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

can you push evaluate to the same line. It's not really long to be added to a separate line.

if (rval.confident === true && rval.value === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

This also needs to be pure.

var x = void console.log("hi");
var y = void foo();

shouldn't be evaluated to undefined and be removed.

declaratorPath.node.init = null;
}
}

function isAnyLvalReferencedBefore(declaratorPath, seenIds, t) {
const id = declaratorPath.get("id");
if (t.isIdentifier(id)) {
return seenIds.has(id.node.name);
}
let hasReference = false;
id.traverse({
Copy link
Member

Choose a reason for hiding this comment

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

for non simple declarations, you have getBindingIdentifiers() to get the binding identifiers. .traverse is probably not the best idea.

Identifier(path) {
if (seenIds.has(path.node.name)) {
hasReference = true;
}
}
});
return hasReference;
}

function removeUndefinedAssignments(functionPath, t) {
const seenIds = new Set();
functionPath.traverse({
Function(path) {
path.skip();
},
Identifier(path) {
// potential optimization: only store ids that are lvals of assignments.
seenIds.add(path.node.name);
Copy link
Member

@boopathi boopathi Oct 20, 2016

Choose a reason for hiding this comment

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

Doesn't it also consider this as seen ?

let a = 1;
{
  let a = undefined;
}

?

},
VariableDeclaration(path) {
switch (path.node.kind) {
case "const":
break;
case "let":
path.node.declarations.forEach((declarator, index) => {
Copy link
Member

@boopathi boopathi Oct 20, 2016

Choose a reason for hiding this comment

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

Since you need declarationPath, you can simply do const declarations = path.get("declarations") and just iterate over it, each item will be a declarationPath

const declaratorPath = path.get("declarations")[index];
removeRvalIfUndefined(declaratorPath);
});
break;
case "var":
if (!t.isFunction(functionPath)) {
break;
}
if (isInLocalLoop(path.parentPath, t)) {
break;
}
path.node.declarations.forEach((declarator, index) => {
const declaratorPath = path.get("declarations")[index];
Copy link
Member

@boopathi boopathi Oct 20, 2016

Choose a reason for hiding this comment

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

Same as previous.

Since you need declarationPath, you can simply do const declarations = path.get("declarations") and just iterate over it, each item will be a declarationPath

if (!isAnyLvalReferencedBefore(declaratorPath, seenIds, t)) {
removeRvalIfUndefined(declaratorPath);
}
});
break;
}
},
});
}

module.exports = function({ types: t }) {
return {
name: "remove-undefined-if-possible",
visitor: {
FunctionParent(path) {
removeUndefinedAssignments(path, t);
Copy link
Member

Choose a reason for hiding this comment

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

since this creates 2 more levels of traverse for some paths, can you run this with scripts/plugin-timing if this is taking too much time ?

Copy link
Contributor Author

@shinew shinew Oct 21, 2016

Choose a reason for hiding this comment

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

I ran the timing script after adding this to the preset list:

for file in ./scripts/fixtures/*.js; do
echo $file
./scripts/plugin-timing.js $file
done

./scripts/fixtures/backbone.js

pluginAlias time(ms) # visits time/visit(ms)
minify-simplify 174.445 5669 0.031
minify-mangle-names 88.821 1 88.821
minify-constant-folding 71.037 12515 0.006
remove-undefined-if-possible 67.165 459 0.146
minify-dead-code-elimination 66.802 1 66.802
minify-guarded-expressions 18.205 808 0.023
internal.shadowFunctions 12.601 6406 0.002
foreign$14 11.361 265 0.043
minify-type-constructors 10.955 822 0.013
minify-infinity 7.723 5453 0.001
foreign$15 6.950 5453 0.001
foreign$10 6.535 2024 0.003
foreign$12 3.333 738 0.005
foreign$11 1.797 209 0.009
minify-flip-comparisons 1.457 265 0.005
foreign$13 0.953 236 0.004
internal.blockHoist 0.776 288 0.003
minify-replace 0.153 1 0.153

./scripts/fixtures/flot.js

pluginAlias time(ms) # visits time/visit(ms)
minify-dead-code-elimination 119.034 1 119.034
minify-simplify 89.467 1182 0.076
minify-constant-folding 47.764 6162 0.008
minify-mangle-names 32.701 1 32.701
remove-undefined-if-possible 23.108 54 0.428
minify-guarded-expressions 8.493 180 0.047
internal.shadowFunctions 6.262 2713 0.002
foreign$15 3.823 2713 0.001
minify-infinity 3.684 2713 0.001
minify-type-constructors 2.568 315 0.008
foreign$12 2.458 504 0.005
foreign$10 2.278 1130 0.002
minify-flip-comparisons 1.714 461 0.004
foreign$14 0.819 461 0.002
foreign$11 0.606 61 0.010
foreign$13 0.363 113 0.003
internal.blockHoist 0.278 50 0.006
minify-replace 0.147 1 0.147

./scripts/fixtures/jquery.js

pluginAlias time(ms) # visits time/visit(ms)
minify-simplify 2379.277 54019 0.044
minify-constant-folding 687.636 124589 0.006
minify-mangle-names 645.566 1 645.566
remove-undefined-if-possible 486.537 3746 0.130
minify-guarded-expressions 218.809 11556 0.019
minify-dead-code-elimination 193.202 1 193.202
internal.shadowFunctions 89.802 58084 0.002
minify-infinity 88.526 56758 0.002
foreign$15 87.342 56758 0.002
minify-type-constructors 68.566 7615 0.009
foreign$14 42.466 4852 0.009
foreign$10 29.695 17269 0.002
foreign$12 19.914 9339 0.002
minify-flip-comparisons 18.298 4852 0.004
internal.blockHoist 5.210 2495 0.002
foreign$13 3.870 1336 0.003
foreign$11 3.486 877 0.004
minify-replace 0.298 1 0.298

./scripts/fixtures/react.js

pluginAlias time(ms) # visits time/visit(ms)
minify-simplify 747.820 31213 0.024
minify-mangle-names 480.056 1 480.056
minify-constant-folding 422.067 65514 0.006
minify-dead-code-elimination 410.427 1 410.427
remove-undefined-if-possible 300.603 2157 0.139
minify-guarded-expressions 83.892 3534 0.024
internal.shadowFunctions 52.667 31267 0.002
minify-infinity 48.516 30538 0.002
foreign$15 47.769 30538 0.002
minify-type-constructors 39.374 4199 0.009
foreign$14 33.705 2254 0.015
foreign$12 26.349 7659 0.003
foreign$10 13.478 7812 0.002
foreign$11 8.993 1240 0.007
minify-flip-comparisons 7.981 2254 0.004
foreign$13 7.097 2816 0.003
internal.blockHoist 3.541 1510 0.002
minify-replace 0.151 1 0.151

},
ReturnStatement(path) {
if (path.node.argument !== null) {
const rval = path.get("argument")
.evaluate();
Copy link
Member

Choose a reason for hiding this comment

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

same as above. Can you push evaluate to the same line as previous one?

if (rval.confident === true && rval.value === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

probably requires isPure check here too for

return void console.log("asdf");

path.node.argument = null;
}
}
},
},
};
};