-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: drop unused local variables #2360
Conversation
@@ -255,6 +255,7 @@ async function test_case(esbuild, test, basename) { | |||
'let.js: issue_4229', | |||
'let.js: issue_4245', | |||
'let.js: use_before_init_3', | |||
'let.js: issue_4276_1', |
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.
This test is related to TDZ.
https://github.com/mishoo/UglifyJS/blob/4c227cc6bd795278c085a6b6dfd40a960ec35698/test/compress/let.js#L1486-L1492
// Ignore declarations if the scope is shadowed by a direct "eval" call. | ||
// The eval'd code may indirectly reference this symbol and the actual | ||
// use count may be greater than 0. | ||
if p.currentScope != p.moduleScope && !p.currentScope.ContainsDirectEval { |
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.
If this costs too much, I think this condition could be p.currentScope.Parent == p.moduleScope && !p.currentScope.ContainsDirectEval
instead.
expectPrintedMangle(t, "{ const v = (() => 'foo')() }", "{\n const v = (() => \"foo\")();\n}\n") | ||
expectPrintedMangle(t, "{ const v = /* #__PURE__ */ (() => 'foo')() }", "") | ||
expectPrintedMangle(t, "{ const v = 0; console.log(v) }", "console.log(0);\n") | ||
// expectPrintedMangle(t, "function f() { const v = 0; eval('v') }", "function f() {\n const v = 0;\n eval(\"v\");\n}\n") |
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.
This test fails without this PR.
const v = 0;
is dropped at here.
esbuild/internal/js_parser/js_parser.go
Lines 9312 to 9317 in 698898f
// Only keep this declaration if it's top-level or exported (which | |
// could be in a nested TypeScript namespace), otherwise erase it | |
if p.currentScope.Parent == nil || s.IsExport { | |
s.Decls[end] = d | |
end++ | |
} |
Created an issue: #2361
This PR breaks var hoist. before function test(v) {
var v = 3
return arguments[0]
}
test(1)
// output: 3 after function test(v) {
return arguments[0]
}
test(1)
// output: 1 |
@unbyte That's a good catch. |
This PR partially implements dropping unused local variables.
Multi-level dropping (for example:
a
is unused andb
depends ona
) is not implemented.This is useful for some cases like minifying IIFE wrapped output.
Example
Input
Current Output
New Output