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
Changes from 2 commits
5aaa71f
eccb78b
cf59b05
b33897d
39a30f1
9bf6f64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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": {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
"use strict"; | ||
|
||
const EVAL_SCOPE_MARKER = "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]; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2306,4 +2306,30 @@ 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
|
@@ -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 | ||
|
@@ -710,6 +715,10 @@ module.exports = ({ types: t, traverse }) => { | |
traverse.clearCache(); | ||
path.scope.crawl(); | ||
|
||
if (!isEvalScopesMarked(path.scope)) { | ||
markEvalScopes(path); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Another thing to consider is that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you want to implement them in Babel instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use Symbol instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. Will change. I was using
scope.evalInScope
directly.