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

[ES6] bug: minifying const variable with inline parameter reuses same variable name for const and var, giving 'n is not defined' error #2842

Open
tung-jin-chew-hp opened this issue Jan 22, 2018 · 28 comments

Comments

@tung-jin-chew-hp
Copy link

commented Jan 22, 2018

Bug report or feature request?

bug

ES5 or ES6+ input?

ES6

Uglify version (uglifyjs -V)

uglify-es 3.3.8

JavaScript input

var test = (function(){
    function inlinedFunction(data) {
        return {
            children: data,
            count: data.reduce(function(a, b) {
                return a + b;
            })
        }
    }

    function testMinify(){
        if(true) {
            const data = inlinedFunction([1,2,3]);

            if(this.someFunction) {
                this.someFunction(data);
            }
        }
    }

    return testMinify();
})();

The uglifyjs CLI command executed or minify() options used.

uglifyjs -b -c -m -- test.js

JavaScript output or error produced.

Chrome gives an 'Uncaught ReferenceError: n is not defined' error on line 5 when the minified output is pasted into the console and run, because it's trying to assign to 'n' as a variable and a constant.

The minified output looks like the following:

var test = function() {
    return function() {
        {
            const n = {
                children: n = [ 1, 2, 3 ],
                count: n.reduce(function(n, t) {
                    return n + t;
                })
            };
            this.someFunction && this.someFunction(n);
        }
        var n;
    }();
}();

Actually encountered it elsewhere, but this is as simple as I can get the test case. Both the inlined function and the containing function seem to need to use their variables twice, otherwise it's all optimized away. The conditional is also necessary, though it doesn't have to be if(true) , just using that as an example.

@kzc

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

Regression from uglify-es@3.3.7.

Workaround:

uglifyjs -m -c inline=false
@kzc

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

@alexlamsl This is a case where the rename pass in uglify-es@3.3.7 masked the issue.

$ bin/uglifyjs -V
uglify-es 3.3.7
$ 
$ cat t2842.js | bin/uglifyjs -bmc | node
$ 
$ cat t2842.js | bin/uglifyjs -bmc --no-rename | node
[stdin]:5
                children: n = [ 1, 2, 3 ],
                            ^
ReferenceError: n is not defined
@tung-jin-chew-hp

This comment has been minimized.

Copy link
Author

commented Jan 22, 2018

Thanks, that's useful to know! Both inline=false and inline=1 work, it only gives errors on inline >= 2.

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2018

@kzc may be the variable name collision logic of inline not working for AST_SymbolBlockDeclaration inside AST_Block (as opposed to AST_Scope).

@kzc

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

Above my pay grade.

Can you suggest a simple fix?

@kzc

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

OT - function inlining is a lot trickier than I first thought. So many bloody edge cases.

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2018

Let me finish my $day_job in a couple of hours, and I'll see what I can do.

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2018

@kzc AST_Node.block_scope is easily lost during compress, so unless we track down and propagate this there is no way inline can safely operate.

So the simplest fix would be to change inline default value to 1 and call it a day.

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2018

FWIW, here's a slighly more reduced test case:

(function() {
    function inlinedFunction(data) {
        return data[data[0]];
    }
    function testMinify() {
        if (true) {
            const data = inlinedFunction([1, 2, 3]);
            console.log(data);
        }
    }
    return testMinify();
})();
@kzc

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

So the simplest fix would be to change inline default value to 1 and call it a day.

Then we'd lose inline=2 for any ES5 code processed with uglify-es.

There must be way to identify whether symbols within the function to be inlined conflict with any non-var variable names in the caller at the point of the call.

Let me think about it.

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2018

Then we'd lose inline=2 for any ES5 code processed with uglify-es.

They should be using uglify-js in the first place.

@kzc

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

They should be using uglify-js in the first place.

Most new projects favor uglify-es over uglify-js@3.

@kzc

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2018

I haven't looked at the uglify-es download count in a while. Seems to be getting a fair bit of use.

186 170 downloads in the last day
920 228 downloads in the last week

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Jan 23, 2018

Most new projects favor uglify-es over uglify-js@3.

In which case, they are on their own - I have no plans to support harmony in the foreseeable future.

@krassx

This comment has been minimized.

Copy link

commented Jan 23, 2018

@alexlamsl so, does that mean uglify-es will not get bug fixes/new features from now on?

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Jan 23, 2018

@krassx you are more than welcomed to work on harmony (uglify-es) - this is an OpenSource project.

I will review and take Pull Request on both branches, but my primary focus is a stable and usable master (uglify-js).

@kzc

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2018

@alexlamsl Correct me if I'm wrong...

For the benefit of those not familiar with the uglify code base, this harmony branch bug (a.k.a. uglify-es) can be fixed in one of three ways from least to most effort:

  1. Limit inline to 1 as discussed above. This is the easiest stop-gap measure but has the disadvantage of not inlining ES5 single-use functions with arguments and/or variables. This is the safest option.
  2. Avoid single-use function inlining when there is a variable/argument name collision with block scoped (let/const) variables in the calling scope.
  3. Fix and reintroduce the symbol rename pass (AST_Toplevel.expand_names) to respect ES6 constructs and block scope. This allows for the inlining of more single-use functions than the other options.
@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Jan 23, 2018

@kzc (3) does not really fix the underlying cause of this issue - it just so happens the order of optimize() being called in the particular test case above.

@kzc

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2018

I was just in the process of making a comment edit. ;-)

Edit: Technically option (2) would be also be required if option (3) were to be implemented.

@kzc

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2018

And perhaps the best option:

  1. Pay @alexlamsl to fix it.
@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Jan 23, 2018

Jokes aside - I think we are getting close to the point where the issue of block scopes can no longer be relied on half-hearted workarounds within harmony.

Certainly not something I would undertake lightly.

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Jan 23, 2018

@kzc if you really want to tackle (2), here's what I can contribute to your efforts:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -4718,10 +4718,15 @@ merge(Compressor.prototype, {
                     in_loop = [];
                 } else if (scope instanceof AST_SymbolRef) {
                     if (scope.fixed_value() instanceof AST_Scope) return false;
+                } else if (scope.block_scope) {
+                    scope = scope.block_scope;
                 }
             } while (!(scope instanceof AST_Scope) || scope instanceof AST_Arrow);
             var safe_to_inject = !(scope instanceof AST_Toplevel) || compressor.toplevel.vars;
             var inline = compressor.option("inline");
+console.error(self.print_to_string());
+console.error(scope.TYPE, scope.var_names());
+console.error();
             if (!can_inject_vars(catches, inline >= 3 && safe_to_inject)) return false;
             if (!can_inject_args(catches, inline >= 2 && safe_to_inject)) return false;
             return !in_loop || in_loop.length == 0 || !is_reachable(fn, in_loop);
$ uglifyjs test.js -bc
function(data){return data[data[0]]}([1,2,3])
Scope { data: true, inlinedFunction: true, console: true }

function(data){return data[data[0]]}([1,2,3])
Function { inlinedFunction: true, console: true, arguments: true }

!function() {
    (function() {
        {
            const data = (data = [ 1, 2, 3 ])[data[0]];
            console.log(data);
        }
        var data;
    })();
}();

This is what I meant in #2842 (comment)

@kzc

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2018

Jokes aside

@alexlamsl I wasn't kidding. You're doing a fantastic job maintaining and improving uglify in your free time and you should be compensated for it.

if you really want to tackle (2), here's what I can contribute to your efforts

I'll let someone else deal with it. Now that uglify-es is out the door and is on solid footing I was going to step back from this project anyway and now is as good time as any. People using this software every day should step up and contribute.

@fabiosantoscode

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2018

I found a nice way to fix this. But it's going to take quite some work.

There's already block-scope support in inlining functions, but it's only used for catch blocks.

function can_inject_symbols() {         
    var catches = Object.create(null);  
(...)
        if (scope instanceof AST_Catch) {        
            catches[scope.argname.name] = true;  

I propose changing that variable name to block_scoped and adding variables from block scopes to it.

} else if (scope instanceof AST_Block && !(scope instanceof AST_Lambda)) {     
    scope.block_scope.variables.each(function (variable) {                     
        catches[variable.name] = true;                                         
    });                                                                        

However, it will be quite some work because compress.js destroys and creates blocks like they don't have any scope! For example, our if (true) in this issue, since it always evaluates to true, is compressed to a AST_BlockStatement by a simple make_node call. However, the block_scope property which was duly integrated in scope.js into the if body isn't added to the newly created block statement. Therefore, all information about the block scope is lost.

I'm currently working on finding places where make_node(AST_BlockStatement is called, and passing the block_scope property to it.

petemill added a commit to brave/browser-laptop that referenced this issue May 10, 2018

webpack - only inline simple functions
uglify-es has bugs with function inlining which can cause assignment to const variables, as well as other issues. See mishoo/UglifyJS2#2842

petemill added a commit to brave/browser-laptop that referenced this issue May 10, 2018

webpack - only inline simple functions
uglify-es has bugs with function inlining which can cause assignment to const variables, as well as other issues. See mishoo/UglifyJS2#2842

petemill added a commit to brave/browser-laptop that referenced this issue May 10, 2018

webpack - only inline simple functions
uglify-es has bugs with function inlining which can cause assignment to const variables, as well as other issues. See mishoo/UglifyJS2#2842

petemill added a commit to brave/browser-laptop that referenced this issue May 11, 2018

webpack - only inline simple functions
uglify-es has bugs with function inlining which can cause assignment to const variables, as well as other issues. See mishoo/UglifyJS2#2842

lbogdan referenced this issue in codesandbox/codesandbox-client May 11, 2018

oshima added a commit to oshima/phraselog_web that referenced this issue May 20, 2018

ericpyle added a commit to ubsicap/dbl.local.electron that referenced this issue Jun 11, 2018

fix search to remove unmatched rows
fix crash in search (on production build another "TypeError: Assignment to constant variable. see mishoo/UglifyJS2#2842)
disable regex search (to allow quoting anything)

ningyuansg added a commit to source-academy/cadet-frontend that referenced this issue Jun 20, 2018

Hotfix errors due to minifying js assets
1. It seems that using shorthand generator function definitions in objects (for
   src/slang/interpreter.ts) will result in a minified file where the generator
   method replaced by a function. `yield` calls in this generator therefore
   raise an uncaught syntax error in the browser. See:
   webpack/webpack#7566
2. A known issue with causes reuse of const variables in the same scope. See:
   mishoo/UglifyJS2#2842

remo5000 added a commit to source-academy/cadet-frontend that referenced this issue Jun 21, 2018

Add deploy to staging with terraform (#120)
* Add terraform and deployment scripts

* Hotfix errors due to minifying js assets

1. It seems that using shorthand generator function definitions in objects (for
   src/slang/interpreter.ts) will result in a minified file where the generator
   method replaced by a function. `yield` calls in this generator therefore
   raise an uncaught syntax error in the browser. See:
   webpack/webpack#7566
2. A known issue with causes reuse of const variables in the same scope. See:
   mishoo/UglifyJS2#2842

* Fix workspace-green css

* Add profile variable to terraform config

* Fix runInContext not finishing in minified js

* Fix more instances of method shorthand bugs

* Use dynamic class name for greenscreen

* Format playground component

* Add console.log to report version

ZauberNerd added a commit to untool/untool that referenced this issue Jun 28, 2018

fix(webpack): turn off function inlining to avoid const reassignment
Due to a bug in UglifyJS (mishoo/UglifyJS2#2842)
we should disable function inlining to avoid falling into this issue.

the uglifyjs-webpack-plugin module is considering to move to the
maintained fork of UglifyJS (terser: https://github.com/fabiosantoscode/terser):
- webpack-contrib/uglifyjs-webpack-plugin#264
- webpack-contrib/uglifyjs-webpack-plugin#296

But until that happens I'd propose to disable function inlining.

ZauberNerd added a commit to untool/untool that referenced this issue Jun 28, 2018

fix(webpack): turn off function inlining to avoid const reassignment
Due to a bug in UglifyJS (mishoo/UglifyJS2#2842)
we should disable function inlining to avoid falling into this issue.

the uglifyjs-webpack-plugin module is considering to move to the
maintained fork of UglifyJS (terser: https://github.com/fabiosantoscode/terser):
- webpack-contrib/uglifyjs-webpack-plugin#264
- webpack-contrib/uglifyjs-webpack-plugin#296

But until that happens I'd propose to disable function inlining.

dmbch added a commit to untool/untool that referenced this issue Jun 28, 2018

fix(webpack): turn off function inlining to avoid const reassignment
Due to a bug in UglifyJS (mishoo/UglifyJS2#2842)
we should disable function inlining to avoid falling into this issue.

the uglifyjs-webpack-plugin module is considering to move to the
maintained fork of UglifyJS (terser: https://github.com/fabiosantoscode/terser):
- webpack-contrib/uglifyjs-webpack-plugin#264
- webpack-contrib/uglifyjs-webpack-plugin#296

But until that happens I'd propose to disable function inlining.

ppvg added a commit to ppvg/create-react-app-typescript that referenced this issue Aug 6, 2018

fix: don't let uglify-es inline functions with arguments
Fixes wmonk#346.

This is a workaround for a bug in uglifyjs2
(mishoo/UglifyJS2#2842), which can cause name
collisions when a function with arguments is inlined. This can cause an
unintended shadowing of a `var` or `let`, or a `TypeError: Assignment to
constant variable` in case of a `const`.

mhuggins added a commit to nos/client that referenced this issue Aug 18, 2018

chore(account): fixed TypeError for accounts with 3+ tokens
UglifyJsPlugin will inline single-use functions by default.  This can result in an error
stating `TypeError: Assignment to constant variable`.  To work around this, the function
is reused since there was an opportunity to do so anyway.

mishoo/UglifyJS2#2842 (comment)

mhuggins added a commit to nos/client that referenced this issue Aug 18, 2018

chore(account): fixed TypeError for accounts with 3+ tokens
UglifyJsPlugin will inline single-use functions by default.  This can result in an error
stating `TypeError: Assignment to constant variable`.  To work around this, the function
is reused since there was an opportunity to do so anyway.

mishoo/UglifyJS2#2842 (comment)

DalderupMaurice added a commit to nos/client that referenced this issue Aug 18, 2018

chore(account): fixed TypeError for accounts with 3+ tokens (#455)
* chore(account): fixed TypeError for accounts with 3+ tokens

UglifyJsPlugin will inline single-use functions by default.  This can result in an error
stating `TypeError: Assignment to constant variable`.  To work around this, the function
is reused since there was an opportunity to do so anyway.

mishoo/UglifyJS2#2842 (comment)

* chore(webpack): prevent uglify from inlining single-use functions
@larsgw

This comment has been minimized.

Copy link

commented Aug 23, 2018

Note: this issue is also causing a problem with babel-plugin-transform-typeof-symbol, which generates this function:

function _typeof(obj) {
  if (typeof Symbol === "function" && typeof Symbol.iterator === "symbol") {
    _typeof = function _typeof(obj) {
      return typeof obj;
    };
  } else {
    _typeof = function _typeof(obj) {
      return obj && typeof Symbol === "function" && obj.constructor === Symbol && obj !== Symbol.prototype ? "symbol" : typeof obj;
    };
  }
  return _typeof(obj);
}

This, when inlined, results in something like this (minified):

function u(t) {
    return o = "function" == typeof Symbol && "symbol" == typeof Symbol.iterator ? function o(t) {
        return typeof t
    } : function o(t) {
        return t && "function" == typeof Symbol && t.constructor === Symbol && t !== Symbol.prototype ? "symbol" : typeof t
    }, o(t)
}

When executed in the browser (e.g. with browserify), this can cause all kinds of trouble, like overwriting the local module variable (edit: which shouldn't be happening anyway, should it?).


I don't see this behavior anymore in v3.4.8, is that correct?

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Aug 24, 2018

@larsgw latest version of uglify-js should be working correctly.

If not, please follow the issue template & file a new issue with a reproducible test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.