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

Closed
tung-jin-chew-hp opened this issue Jan 22, 2018 · 29 comments

Comments

@tung-jin-chew-hp
Copy link

@tung-jin-chew-hp tung-jin-chew-hp 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
Copy link
Contributor

@kzc kzc commented Jan 22, 2018

Regression from uglify-es@3.3.7.

Workaround:

uglifyjs -m -c inline=false

Loading

@kzc
Copy link
Contributor

@kzc kzc 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

Loading

@tung-jin-chew-hp
Copy link
Author

@tung-jin-chew-hp tung-jin-chew-hp commented Jan 22, 2018

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

Loading

@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl 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).

Loading

@kzc
Copy link
Contributor

@kzc kzc commented Jan 22, 2018

Above my pay grade.

Can you suggest a simple fix?

Loading

@kzc
Copy link
Contributor

@kzc kzc commented Jan 22, 2018

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

Loading

@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl commented Jan 22, 2018

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

Loading

@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl 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.

Loading

@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl 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();
})();

Loading

@kzc
Copy link
Contributor

@kzc kzc 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.

Loading

@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl 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.

Loading

@kzc
Copy link
Contributor

@kzc kzc commented Jan 22, 2018

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

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

Loading

@kzc
Copy link
Contributor

@kzc kzc 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

Loading

@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl 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.

Loading

@krassx
Copy link

@krassx krassx commented Jan 23, 2018

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

Loading

@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl 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).

Loading

@kzc
Copy link
Contributor

@kzc kzc 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.

Loading

@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl 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.

Loading

@kzc
Copy link
Contributor

@kzc kzc 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.

Loading

@kzc
Copy link
Contributor

@kzc kzc commented Jan 23, 2018

And perhaps the best option:

  1. Pay @alexlamsl to fix it.

Loading

@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl 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.

Loading

@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl 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)

Loading

@kzc
Copy link
Contributor

@kzc kzc 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.

Loading

@fabiosantoscode
Copy link
Contributor

@fabiosantoscode fabiosantoscode 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.

Loading

remo5000 added a commit to source-academy/frontend that referenced this issue Jun 21, 2018
* 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/UglifyJS#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
Due to a bug in UglifyJS (mishoo/UglifyJS#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
Due to a bug in UglifyJS (mishoo/UglifyJS#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
Due to a bug in UglifyJS (mishoo/UglifyJS#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.
mhuggins added a commit to nos/client that referenced this issue Aug 18, 2018
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/UglifyJS#2842 (comment)
mhuggins added a commit to nos/client that referenced this issue Aug 18, 2018
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/UglifyJS#2842 (comment)
DalderupMaurice pushed 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/UglifyJS#2842 (comment)

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

@larsgw larsgw 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?

Loading

@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl 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.

Loading

pdonias added a commit to vatesfr/xen-orchestra that referenced this issue Sep 4, 2019
julien-f added a commit to vatesfr/xen-orchestra that referenced this issue Sep 4, 2019
nirlanka added a commit to nirlanka/file-cargo that referenced this issue Sep 6, 2019
To avoid bug in uglify-es@3.3.9 that reuses const names and causes Uncaught TypeError: Assignment to constant variable. Expected as a regression from 3.3.7 according to mishoo/UglifyJS#2842 (comment)
ZauberNerd added a commit to untool/untool that referenced this issue Sep 9, 2019
ZauberNerd added a commit to untool/untool that referenced this issue Sep 9, 2019
ZauberNerd added a commit to untool/untool that referenced this issue Sep 9, 2019
jsmasterdev pushed a commit to jsmasterdev/cadey-react-academ that referenced this issue Nov 28, 2020
* 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/UglifyJS#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
@alexlamsl
Copy link
Collaborator

@alexlamsl alexlamsl commented Feb 27, 2021

Original example now works correctly in uglify-js:

$ uglify-js input.js -bmc
var test = function() {
    var n = {
        children: n = [ 1, 2, 3 ],
        count: n.reduce(function(n, t) {
            return n + t;
        })
    };
    this.someFunction && this.someFunction(n);
}();

Works even if const is preserved:

$ uglify-js input.js -bmc varify=false
var test = function() {
    {
        const t = {
            children: n = [ 1, 2, 3 ],
            count: n.reduce(function(n, t) {
                return n + t;
            })
        };
        this.someFunction && this.someFunction(t);
    }
    var n;
}();

Loading

@alexlamsl alexlamsl closed this Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants