fix(minifier): avoid removing function declaration from KeepVar (#4722

)
oxc-project · Aug 7, 2024 · 94d3c31 · 94d3c31
1 parent eaddc8f
commit 94d3c31
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 35 deletions.
diff --git a/crates/oxc_ast_macros/Cargo.toml b/crates/oxc_ast_macros/Cargo.toml
@@ -22,5 +22,5 @@ doctest    = false
 
 [dependencies]
 quote       = { workspace = true }
-syn         = { workspace = true, features = ["full", "parsing", "proc-macro", "printing"] }
+syn         = { workspace = true, features = ["full", "parsing", "printing", "proc-macro"] }
 proc-macro2 = { workspace = true }
diff --git a/crates/oxc_minifier/src/ast_passes/remove_dead_code.rs b/crates/oxc_minifier/src/ast_passes/remove_dead_code.rs
@@ -55,14 +55,29 @@ impl<'a> RemoveDeadCode<'a> {
         }
 
         let Some(index) = index else { return };
+        if index == stmts.len() - 1 {
+            return;
+        }
 
         let mut keep_var = KeepVar::new(self.ast);
 
         for stmt in stmts.iter().skip(index + 1) {
             keep_var.visit_statement(stmt);
         }
 
-        stmts.drain(index + 1..);
+        let mut i = 0;
+        stmts.retain(|s| {
+            i += 1;
+            if i - 1 <= index {
+                return true;
+            }
+            // keep function declaration
+            if matches!(s.as_declaration(), Some(Declaration::FunctionDeclaration(_))) {
+                return true;
+            }
+            false
+        });
+
         if let Some(stmt) = keep_var.get_variable_declaration_statement() {
             stmts.push(stmt);
         }

diff --git a/crates/oxc_minifier/src/keep_var.rs b/crates/oxc_minifier/src/keep_var.rs
@@ -1,31 +1,48 @@
 #[allow(clippy::wildcard_imports)]
 use oxc_ast::{ast::*, syntax_directed_operations::BoundNames, AstBuilder, Visit};
 use oxc_span::{Atom, Span, SPAN};
-use oxc_syntax::scope::ScopeFlags;
 
 pub struct KeepVar<'a> {
     ast: AstBuilder<'a>,
     vars: std::vec::Vec<(Atom<'a>, Span)>,
 }
 
 impl<'a> Visit<'a> for KeepVar<'a> {
-    fn visit_variable_declaration(&mut self, decl: &VariableDeclaration<'a>) {
-        if decl.kind.is_var() {
-            decl.bound_names(&mut |ident| {
-                self.vars.push((ident.name.clone(), ident.span));
-            });
+    fn visit_statement(&mut self, it: &Statement<'a>) {
+        // Only visit blocks where vars could be hoisted
+        match it {
+            Statement::BlockStatement(it) => self.visit_block_statement(it),
+            Statement::BreakStatement(it) => self.visit_break_statement(it),
+            Statement::ContinueStatement(it) => self.visit_continue_statement(it),
+            // Statement::DebuggerStatement(it) => self.visit_debugger_statement(it),
+            Statement::DoWhileStatement(it) => self.visit_do_while_statement(it),
+            // Statement::EmptyStatement(it) => self.visit_empty_statement(it),
+            // Statement::ExpressionStatement(it) => self.visit_expression_statement(it),
+            Statement::ForInStatement(it) => self.visit_for_in_statement(it),
+            Statement::ForOfStatement(it) => self.visit_for_of_statement(it),
+            Statement::ForStatement(it) => self.visit_for_statement(it),
+            Statement::IfStatement(it) => self.visit_if_statement(it),
+            Statement::LabeledStatement(it) => self.visit_labeled_statement(it),
+            // Statement::ReturnStatement(it) => self.visit_return_statement(it),
+            Statement::SwitchStatement(it) => self.visit_switch_statement(it),
+            // Statement::ThrowStatement(it) => self.visit_throw_statement(it),
+            Statement::TryStatement(it) => self.visit_try_statement(it),
+            Statement::WhileStatement(it) => self.visit_while_statement(it),
+            Statement::WithStatement(it) => self.visit_with_statement(it),
+            // match_declaration!(Statement) => visitor.visit_declaration(it.to_declaration()),
+            // match_module_declaration!(Statement) => {
+            // visitor.visit_module_declaration(it.to_module_declaration())
+            // }
+            Statement::VariableDeclaration(decl) => {
+                if decl.kind.is_var() {
+                    decl.bound_names(&mut |ident| {
+                        self.vars.push((ident.name.clone(), ident.span));
+                    });
+                }
+            }
+            _ => {}
         }
     }
-
-    fn visit_function(&mut self, _it: &Function<'a>, _flags: ScopeFlags) {
-        /* skip functions */
-    }
-
-    fn visit_arrow_function_expression(&mut self, _it: &ArrowFunctionExpression<'a>) {}
-
-    fn visit_class(&mut self, _it: &Class<'a>) {
-        /* skip classes */
-    }
 }
 
 impl<'a> KeepVar<'a> {

diff --git a/crates/oxc_minifier/tests/oxc/remove_dead_code.rs b/crates/oxc_minifier/tests/oxc/remove_dead_code.rs
@@ -24,12 +24,6 @@ fn test(source_text: &str, expected: &str) {
     assert_eq!(minified, expected, "for source {source_text}");
 }
 
-fn test_same(source_text: &str) {
-    let minified = print(source_text, true);
-    let expected = print(source_text, false);
-    assert_eq!(minified, expected, "for source {source_text}");
-}
-
 #[test]
 fn dce_if_statement() {
     test("if (true) { foo }", "{ foo }");
@@ -127,18 +121,36 @@ fn dce_logical_expression() {
 
 #[test]
 fn dce_var_hoisting() {
-    test_same(
+    test(
+        "function f() {
+          return () => {
+            var x;
+          }
+          REMOVE;
+          function KEEP() {}
+          REMOVE;
+        }",
         "function f() {
           return () => {
             var x;
           }
+          function KEEP() {}
         }",
     );
-    test_same(
+    test(
+        "function f() {
+          return function g() {
+            var x;
+          }
+          REMOVE;
+          function KEEP() {}
+          REMOVE;
+        }",
         "function f() {
           return function g() {
             var x;
           }
+          function KEEP() {}
         }",
     );
 }

diff --git a/tasks/minsize/minsize.snap b/tasks/minsize/minsize.snap
@@ -2,25 +2,25 @@ Original   | Minified   | esbuild    | Gzip       | esbuild
 
 72.14 kB   | 24.32 kB   | 23.70 kB   | 8.72 kB    | 8.54 kB    | react.development.js
 
-173.90 kB  | 61.80 kB   | 59.82 kB   | 19.57 kB   | 19.33 kB   | moment.js 
+173.90 kB  | 61.80 kB   | 59.82 kB   | 19.58 kB   | 19.33 kB   | moment.js 
 
 287.63 kB  | 92.91 kB   | 90.07 kB   | 32.33 kB   | 31.95 kB   | jquery.js 
 
-342.15 kB  | 122.97 kB  | 118.14 kB  | 45.08 kB   | 44.37 kB   | vue.js    
+342.15 kB  | 122.97 kB  | 118.14 kB  | 45.05 kB   | 44.37 kB   | vue.js    
 
-544.10 kB  | 74.71 kB   | 72.48 kB   | 26.24 kB   | 26.20 kB   | lodash.js 
+544.10 kB  | 74.71 kB   | 72.48 kB   | 26.22 kB   | 26.20 kB   | lodash.js 
 
-555.77 kB  | 274.92 kB  | 270.13 kB  | 91.50 kB   | 90.80 kB   | d3.js     
+555.77 kB  | 274.82 kB  | 270.13 kB  | 91.39 kB   | 90.80 kB   | d3.js     
 
-1.01 MB    | 471.72 kB  | 458.89 kB  | 127.60 kB  | 126.71 kB  | bundle.min.js
+1.01 MB    | 471.74 kB  | 458.89 kB  | 127.57 kB  | 126.71 kB  | bundle.min.js
 
-1.25 MB    | 673.73 kB  | 646.76 kB  | 166.77 kB  | 163.73 kB  | three.js  
+1.25 MB    | 673.75 kB  | 646.76 kB  | 166.76 kB  | 163.73 kB  | three.js  
 
-2.14 MB    | 743.50 kB  | 724.14 kB  | 182.06 kB  | 181.07 kB  | victory.js
+2.14 MB    | 743.40 kB  | 724.14 kB  | 181.95 kB  | 181.07 kB  | victory.js
 
-3.20 MB    | 1.03 MB    | 1.01 MB    | 332.77 kB  | 331.56 kB  | echarts.js
+3.20 MB    | 1.03 MB    | 1.01 MB    | 332.46 kB  | 331.56 kB  | echarts.js
 
 6.69 MB    | 2.42 MB    | 2.31 MB    | 503.31 kB  | 488.28 kB  | antd.js   
 
-10.95 MB   | 3.57 MB    | 3.49 MB    | 912.65 kB  | 915.50 kB  | typescript.js
+10.95 MB   | 3.57 MB    | 3.49 MB    | 912.42 kB  | 915.50 kB  | typescript.js