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.

Sign up for GitHub

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

Jump to bottom

fix(es/minifier): Fix code for dropping unreachable statements #6429

Merged
merged 4 commits into from Nov 15, 2022
Merged

fix(es/minifier): Fix code for dropping unreachable statements #6429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e6233df
Add a test
kdy1 Nov 14, 2022
44cb5ab
Small fix
kdy1 Nov 14, 2022
1b62932
Update test refs
kdy1 Nov 14, 2022
58764a0
Merge branch 'main' into issue-6405
swc-bot Nov 15, 2022
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
10 changes: 1 addition & 9 deletions crates/swc_ecma_minifier/src/compress/pure/dead_code.rs
View file
Open in desktop
Expand Up @@ -269,15 +269,7 @@ impl Pure<'_> {
);
}

match stmts[idx].as_stmt() {
Some(Stmt::Return(ReturnStmt { arg: None, .. })) => {
// Exclude return
new_stmts.extend(stmts.drain(..idx));
}
_ => {
new_stmts.extend(stmts.drain(..=idx));
}
}
new_stmts.extend(stmts.drain(..=idx));
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a before and after diff of:

+    if (!!val) {
         if (val.a?.b !== !0) throw Error('second');
         return val;
+    }

What I think this change is doing is keeping the return statement in the new output, which prevents us from optimizing if (!!val) if (val.a?.b !== !0) … into if (val.a?.b !== !0) …. But that optimization is still broken, right? That's not a valid compression, and I think we've missed the real bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

drop_unreachable_stmts is called for all kinds of statements, including stmts of BlockStmt.
And works only if there's an unreachable statement.

if (foo) {
   return;
   throw foo
}

will trigger this function, but

if (foo) {
   return;
}

will not.

It seems like I think it's a function called only for functions while writing this function initially.
But actually this is called for all kind of statements, and if so we should not drop return foo in

if (foo) {
   return foo;
   throw foo
}

return bar

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so that's one bug that's fixed. But I'm still confused how, even if we were to drop the return statement, we end up also dropping the if (!!val) wrapper around the throw?

Copy link
Member Author

Choose a reason for hiding this comment

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

Logs

I removed unrelated logs

Main:

  DEBUG  Dropped `undefined` from `return undefined`, kind: "change"
    at crates/swc_ecma_minifier/src/compress/pure/misc.rs:233
    in apply pure optimizer
    in optimize with pass: 1
    in compress ast
    in minify

  DEBUG  Dropping statements after a control keyword, kind: "change"
    at crates/swc_ecma_minifier/src/compress/pure/dead_code.rs:229
    in apply pure optimizer
    in optimize with pass: 1
    in compress ast
    in minify


  DEBUG  Converting empty block to empty statement, kind: "change"
    at crates/swc_ecma_minifier/src/compress/optimize/mod.rs:1228
    in visit_mut_stmt
    in visit_mut_if_stmt
    in visit_mut_stmt
    in handle_stmt_likes
    in visit_mut_block_stmt
    in visit_mut_arrow_expr
    in visit_mut_var_declarator
    in visit_mut_var_declarators
    in visit_mut_var_decl
    in visit_mut_decl
    in visit_mut_export_decl
    in handle_stmt_likes
    in apply full optimizer
    in optimize with pass: 1
    in compress ast
    in minify

  DEBUG  conditionals: `if (foo);` => `foo` , kind: "change"
    at crates/swc_ecma_minifier/src/compress/optimize/conditionals.rs:221
    in visit_mut_stmt
    in handle_stmt_likes
    in visit_mut_block_stmt
    in visit_mut_arrow_expr
    in visit_mut_var_declarator
    in visit_mut_var_declarators
    in visit_mut_var_decl
    in visit_mut_decl
    in visit_mut_export_decl
    in handle_stmt_likes
    in apply full optimizer
    in optimize with pass: 1
    in compress ast
    in minify

  DEBUG  Compressing boolean literal, kind: "change"
    at crates/swc_ecma_minifier/src/compress/optimize/mod.rs:578
    in visit_mut_bin_expr
    in visit_mut_if_stmt
    in visit_mut_stmt
    in handle_stmt_likes
    in visit_mut_block_stmt
    in visit_mut_arrow_expr
    in visit_mut_var_declarator
    in visit_mut_var_declarators
    in visit_mut_var_decl
    in visit_mut_decl
    in visit_mut_export_decl
    in handle_stmt_likes
    in apply full optimizer
    in optimize with pass: 1
    in compress ast
    in minify

  DEBUG  sequences: Compressing statements as a sequences, kind: "change"
    at crates/swc_ecma_minifier/src/compress/optimize/sequences.rs:157
    in handle_stmt_likes
    in visit_mut_block_stmt
    in visit_mut_arrow_expr
    in visit_mut_var_declarator
    in visit_mut_var_declarators
    in visit_mut_var_decl
    in visit_mut_decl
    in visit_mut_export_decl
    in handle_stmt_likes
    in apply full optimizer
    in optimize with pass: 1
    in compress ast
    in minify

Fixed:

DEBUG  Dropped `undefined` from `return undefined`, kind: "change"
    at crates/swc_ecma_minifier/src/compress/pure/misc.rs:233
    in apply pure optimizer
    in optimize with pass: 1
    in compress ast
    in minify

  DEBUG  Dropping statements after a control keyword, kind: "change"
    at crates/swc_ecma_minifier/src/compress/pure/dead_code.rs:229
    in apply pure optimizer
    in optimize with pass: 1
    in compress ast
    in minify

   DEBUG  if_return: Negating `foo` in `if (foo) return; bar()` to make it `if (!foo) bar()`, kind: "change"
    at crates/swc_ecma_minifier/src/compress/pure/if_return.rs:89
    in postcompress
    in minify

Copy link
Member Author

Choose a reason for hiding this comment

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

So...

previously

export const fn = () => {
    let val;

    if (!val) {
        return undefined;
        // works as expected if comment out below line
        throw new Error('first');
    }

    if (val.a?.b !== true) { // Uncaught TypeError: Cannot read properties of undefined (reading 'a')
        throw Error('second');
    }
    return val;
}

=>

export const fn = () => {
    let val;

    if (!val) {
    }

    if (val.a?.b !== true) { // Uncaught TypeError: Cannot read properties of undefined (reading 'a')
        throw Error('second');
    }
    return val;
}

=>

export const fn = () => {
    let val;

    if (!val) ;

    if (val.a?.b !== true) { // Uncaught TypeError: Cannot read properties of undefined (reading 'a')
        throw Error('second');
    }
    return val;
}

=>

export const fn = () => {
    let val;

    if (val.a?.b !== true) { // Uncaught TypeError: Cannot read properties of undefined (reading 'a')
        throw Error('second');
    }
    return val;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, it's because it's the first if statement that has the return dropped.

Copy link
Member Author

Choose a reason for hiding this comment

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

New version:

export const fn = () => {
    let val;

    if (!val) {
        return;
    }

    if (val.a?.b !== true) { // Uncaught TypeError: Cannot read properties of undefined (reading 'a')
        throw Error('second');
    }
    return val;
}

=> (negation from if_return)

export const fn = () => {
    let val;

    if (val) {
    // condition is negated to remove return token

        if (val.a?.b !== true) { // Uncaught TypeError: Cannot read properties of undefined (reading 'a')
            throw Error('second');
        }
        return val;
     
    }

}


new_stmts.extend(hoisted_fns);

Expand Down
14 changes: 14 additions & 0 deletions crates/swc_ecma_minifier/tests/pass-1/issue-6405/1/input.js
View file
Open in desktop
@@ -0,0 +1,14 @@
export const fn = () => {
let val;

if (!val) {
return undefined;
// works as expected if comment out below line
throw new Error('first');
}

if (val.a?.b !== true) { // Uncaught TypeError: Cannot read properties of undefined (reading 'a')
throw new Error('second');
}
return val;
}
7 changes: 7 additions & 0 deletions crates/swc_ecma_minifier/tests/pass-1/issue-6405/1/output.js
View file
Open in desktop
@@ -0,0 +1,7 @@
export const fn = ()=>{
let val;
if (!!val) {
if (val.a?.b !== !0) throw Error('second');
return val;
}
};