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

Add mark eval scopes helper and deopt DCE fn unused params #371

Merged
merged 6 commits into from Jan 19, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions packages/babel-helper-mark-eval-scopes/README.md
@@ -0,0 +1,9 @@
# babel-helper-mark-eval-scopes

Traverse through input path and mark all scopes that contain Direct eval (`eval("")`) calls.

## Installation

```sh
npm install babel-helper-mark-eval-scopes
```
@@ -0,0 +1,48 @@
jest.autoMockOff();

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

function getPath(source) {
let path;

babel.transform(source, {
babelrc: false,
plugins: [
function ({traverse}) {
traverse.clearCache();
return {
visitor: {
Program(programPath) {
path = programPath;
}
}
};
}
]
});

return path;
}

describe("babel-helper-mark-eval-scopes", () => {
it("getEvalScopes - should give a set of scopes which contains eval", () => {
const source = `
function foo() {
function bar() {
eval(";");
}
function baz() {
noeval();
}
}
`;

const program = getPath(source);
const evalScopes = [...helper.getEvalScopes(program)];

expect(evalScopes).toContain(program.scope);
expect(evalScopes).toContain(program.get("body.0.body.body.0").scope);
expect(evalScopes).not.toContain(program.get("body.0.body.body.1").scope);
});
});
17 changes: 17 additions & 0 deletions packages/babel-helper-mark-eval-scopes/package.json
@@ -0,0 +1,17 @@
{
"name": "babel-helper-mark-eval-scopes",
"version": "0.0.1",
"description": "Mark scopes for deopt which contain a direct eval call",
"homepage": "https://github.com/babel/babili#readme",
"repository": "https://github.com/babel/babili/tree/master/packages/babel-helper-mark-eval-scopes",
"bugs": "https://github.com/babel/babili/issues",
"author": "boopathi",
"license": "MIT",
"main": "lib/index.js",
"keywords": [
"babel-plugin",
"babili"
],
"dependencies": {},
"devDependencies": {}
}
52 changes: 52 additions & 0 deletions packages/babel-helper-mark-eval-scopes/src/index.js
@@ -0,0 +1,52 @@
"use strict";

const EVAL_SCOPE_MARKER = Symbol("evalInScope");

module.exports = {
EVAL_SCOPE_MARKER,
getEvalScopes,
markEvalScopes,
isMarked,
hasEval,
};

function getEvalScopes(path) {
const evalScopes = new Set;

function add(scope) {
let evalScope = scope;
do {
evalScopes.add(evalScope);
} while (evalScope = evalScope.parent);
}

path.traverse({
CallExpression(evalPath) {
const callee = evalPath.get("callee");

if (callee.isIdentifier() && callee.node.name === "eval" && !callee.scope.getBinding("eval")) {
add(callee.scope);
}
}
});

return evalScopes;
}

function markEvalScopes(path, key = EVAL_SCOPE_MARKER) {
const evalScopes = getEvalScopes(path);
[...evalScopes].forEach((scope) => {
scope[key] = true;
});
}

function isMarked(scope, key = EVAL_SCOPE_MARKER) {
return Object.prototype.hasOwnProperty.call(scope, key);
}

function hasEval(scope, key = EVAL_SCOPE_MARKER) {
if (!isMarked(scope, key)) {
markEvalScopes(scope, key);
}
return scope[key];
}
Expand Up @@ -2306,4 +2306,119 @@ describe("dce-plugin", () => {
`);
expect(transformWithSimplify(source)).toBe(expected);
});

it("should not remove params from functions containing direct eval", () => {
const source = unpad(`
function a(b, c, d) {
eval(";");
return b;
}
function b(c, d, e) {
(1, eval)(";");
return c;
}
`);

const expected = unpad(`
function a(b, c, d) {
Copy link
Member

Choose a reason for hiding this comment

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

Will this work with nested functions too? i.e. making sure it doesn't mangle top-level vars either.

Copy link
Member Author

Choose a reason for hiding this comment

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

added more tests

eval(";");
return b;
}
function b(c) {
(1, eval)(";");
return c;
}
`);

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

it("should not remove params/vars from functions containing direct eval", () => {
const source = unpad(`
function foo(bar, baz) {
function foox(a, b, c) {
x.then((data, unused) => {
let unused1;
eval(data);
foox1();
{
var unused2;
}
});

function foox1(unused) {
console.log("foox1");
}
}
function fooy(unused1, unused2) {
console.log("fooy");
}
}
`);

const expected = unpad(`
function foo(bar, baz) {
function foox(a, b, c) {
x.then((data, unused) => {
let unused1;
eval(data);
foox1();
{
var unused2;
}
});

function foox1() {
console.log("foox1");
}
}
function fooy() {
console.log("fooy");
}
}
`);

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

it("should not optimize/remove vars from functions containing direct eval", () => {
const source = unpad(`
function foo() {
bar();

var x = 5;
return x;

function bar() {
eval(";");
return 5;
}

function baz() {
let x = 10;
return x;
}
}
`);

const expected = unpad(`
function foo() {
bar();

var x = 5;
return x;

function bar() {
eval(";");
return 5;
}

function baz() {
return 10;
}
}
`);

expect(transform(source)).toBe(expected);
});
});
Expand Up @@ -12,6 +12,7 @@
"babel-plugin"
],
"dependencies": {
"babel-helper-mark-eval-scopes": "^0.0.1",
"babel-helper-remove-or-void": "^0.0.1",
"lodash.some": "^4.6.0"
},
Expand Down
@@ -1,6 +1,7 @@
"use strict";

const some = require("lodash.some");
const { markEvalScopes, isMarked: isEvalScopesMarked, hasEval } = require("babel-helper-mark-eval-scopes");

module.exports = ({ types: t, traverse }) => {
const removeOrVoid = require("babel-helper-remove-or-void")(t);
Expand Down Expand Up @@ -104,6 +105,10 @@ module.exports = ({ types: t, traverse }) => {
return;
}

if (hasEval(path.scope)) {
return;
}

const { scope } = path;

// if the scope is created by a function, we obtain its
Expand Down Expand Up @@ -712,6 +717,10 @@ module.exports = ({ types: t, traverse }) => {
traverse.clearCache();
path.scope.crawl();

if (!isEvalScopesMarked(path.scope)) {
markEvalScopes(path);
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to merge this for now.

Just one suggestion — let's move isEvalScopesMarked into markEvalScopes (checking it within and returning if scope is already marked). That way API becomes simpler (markEvalScopes(path)), there's less vars to exports, etc.

Another thing to consider is that markEvalScopes operates on path, and hasEval operates on scope. In an ideal world, that means these methods belong to those objects — path.markScopesWithEval, scope.hasDirectEvals.

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 didn't put isMarked inside markEvalScopes because if something is changed after marking - we need to remark.

Copy link
Member Author

Choose a reason for hiding this comment

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

that means these methods belong to those objects — path.markScopesWithEval, scope.hasDirectEvals

Yes!!

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to implement them in Babel instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @hzoo what do you think ?

}

// We need to run this plugin in isolation.
path.traverse(main, {
functionToBindings: new Map(),
Expand Down
4 changes: 3 additions & 1 deletion packages/babel-plugin-minify-mangle-names/package.json
Expand Up @@ -11,6 +11,8 @@
"keywords": [
"babel-plugin"
],
"dependencies": {},
"dependencies": {
"babel-helper-mark-eval-scopes": "^0.0.1"
},
"devDependencies": {}
}
53 changes: 25 additions & 28 deletions packages/babel-plugin-minify-mangle-names/src/index.js
@@ -1,3 +1,11 @@
"use strict";

const {
markEvalScopes,
isMarked: isEvalScopesMarked,
hasEval,
} = require("babel-helper-mark-eval-scopes");

module.exports = ({ types: t, traverse }) => {
const hop = Object.prototype.hasOwnProperty;

Expand Down Expand Up @@ -49,39 +57,28 @@ module.exports = ({ types: t, traverse }) => {
collect() {
const mangler = this;

const collectVisitor = {
// capture direct evals
CallExpression(path) {
const callee = path.get("callee");

if (callee.isIdentifier()
&& callee.node.name === "eval"
&& !callee.scope.getBinding("eval")
) {
mangler.markUnsafeScopes(path.scope);
}
}
};
if (!isEvalScopesMarked(mangler.program.scope)) {
markEvalScopes(mangler.program);
}

if (this.charset.shouldConsider) {
// charset considerations
collectVisitor.Identifier = function Identifier(path) {
const { node } = path;

if ((path.parentPath.isMemberExpression({ property: node })) ||
(path.parentPath.isObjectProperty({ key: node }))
) {
mangler.charset.consider(node.name);
const collectVisitor = {
Identifier(path) {
const { node } = path;

if ((path.parentPath.isMemberExpression({ property: node })) ||
(path.parentPath.isObjectProperty({ key: node }))
) {
mangler.charset.consider(node.name);
}
},
Literal({ node }) {
mangler.charset.consider(String(node.value));
}
};

// charset considerations
collectVisitor.Literal = function Literal({ node }) {
mangler.charset.consider(String(node.value));
};
mangler.program.traverse(collectVisitor);
}

this.program.traverse(collectVisitor);
}

mangle() {
Expand All @@ -91,7 +88,7 @@ module.exports = ({ types: t, traverse }) => {
Scopable(path) {
const {scope} = path;

if (!mangler.eval && mangler.unsafeScopes.has(scope)) return;
if (!mangler.eval && hasEval(scope)) return;

if (mangler.visitedScopes.has(scope)) return;
mangler.visitedScopes.add(scope);
Expand Down