Uglifyjs: Uglify drops arguments to functions that use `arguments` object

[davidaurelio]

Bug report or feature request?: Bug report

ES5 or ES6+ input?: ES5

Uglify version (uglifyjs -V): uglify-es 3.0.15

JavaScript input

(function() {
function sum() {
  var s = 0;
  for (var i = 0, n = arguments.length; i < n; ++i) {
    s += arguments[i];
  }
  return s;
}

console.log(sum(1, 2, 3, 4));
console.log(sum(5, 6, 7, 8));
}());

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

JavaScript output or error produced. (beautified for convenience)

!function() {
    function o() {
        for (var o = 0, n = 0, l = arguments.length; n < l; ++n) o += arguments[n];
        return o;
    }
    console.log(o()), console.log(o());
}();

the calls to sum (or o in the minified version) must not drop the arguments, as these are accessed via the arguments object inside of the function.

[alexlamsl]

Only affects harmony:

$ cat test.js
(function() {
    function sum() {
        var s = 0;
        for (var i = 0, n = arguments.length; i < n; ++i) {
            s += arguments[i];
        }
        return s;
    }
    console.log(sum(1, 2, 3, 4));
    console.log(sum(5, 6, 7, 8));
})();

$ uglifyjs -V
uglify-js 3.0.16

$ uglifyjs test.js -cb bracketize
!function() {
    function sum() {
        for (var s = 0, i = 0, n = arguments.length; i < n; ++i) {
            s += arguments[i];
        }
        return s;
    }
    console.log(sum(1, 2, 3, 4)), console.log(sum(5, 6, 7, 8));
}();

$ uglifyjs -V
uglify-es 3.0.16

$ uglifyjs test.js -cb bracketize
!function() {
    function sum() {
        for (var s = 0, i = 0, n = arguments.length; i < n; ++i) {
            s += arguments[i];
        }
        return s;
    }
    console.log(sum()), console.log(sum());
}();

[kzc]

Regression introduced in 68138f22810d4a2086569e979f640202adf757d3

[alexlamsl]

@kzc I'm confused. I've reverted the one liner in the patch, and the result is still the same as https://github.com/mishoo/UglifyJS2/issues/2097#issuecomment-308484593?

Edit: backing up further...

$ uglifyjs -V
uglify-es 3.0.10

$ uglifyjs test.js -cb bracketize
!function() {
    function sum() {
        for (var s = 0, i = 0, n = arguments.length; i < n; ++i) {
            s += arguments[i];
        }
        return s;
    }
    console.log(sum()), console.log(sum());
}();

[kzc]

I was wrong about the regression comment above. Got the branches mixed up.

[alexlamsl]

No worries - let me handle this one while you work on await :wink:

(Except I bet I'm holding you up with #2095 :sweat_smile:)

[kzc]

This harmony bug is an old one:

$ git checkout harmony-v3.0.0
HEAD is now at 3fac29a... Merge pull request #1876 from alexlamsl/harmony-v3.0.0
$ cat 2097.js | bin/uglifyjs -c | node
0
0

[kzc]

(Except I bet I'm holding you up with #2095 😅)

A little bit, but not a problem.

[kzc]

Getting closer...

$ git checkout harmony-v2.8.10
Previous HEAD position was c7063c1... Merge pull request #1591 from alexlamsl/harmony-v2.8.11
HEAD is now at e9920f7... v2.8.10

$ cat 2097.js | bin/uglifyjs -c | node
10
26

$ git checkout harmony-v2.8.11
Previous HEAD position was e9920f7... v2.8.10
HEAD is now at c7063c1... Merge pull request #1591 from alexlamsl/harmony-v2.8.11

$ cat 2097.js | bin/uglifyjs -c | node
0
0

[davidaurelio]

whoa guys, thank you for the quick investigation. Very impressive!

[alexlamsl]

Proof that I don't know how to use GitHub - this mess does not really give me useful information :confused:

https://github.com/mishoo/UglifyJS2/compare/harmony-v2.8.10...harmony-v2.8.11

[kzc]

Likely related to how this commit was applied to harmony: b633706ce42576b7e2aa85a96c5691bde87e71ac

[alexlamsl]

@kzc thanks for narrowing this down - investigating.

[kzc]

@davidaurelio Workaround is to disable reduce_vars:

$ bin/uglifyjs -V
uglify-es 3.0.15

$ cat 2097.js | bin/uglifyjs -c reduce_vars=true | node
0
0



md5-05f5a29247cc0164053882f76a5cef9c



$ cat 2097.js | bin/uglifyjs -c reduce_vars=false | node
10
26

[davidaurelio]

thank you!

[alexlamsl]

Hmm - looks like uses_argument is broken on harmony:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -4058,6 +4058,7 @@ merge(Compressor.prototype, {
             var d = self.definition();
             var fixed = self.fixed_value();
             if (fixed instanceof AST_Defun) {
+console.error(fixed.TYPE, fixed.uses_arguments, fixed.print_to_string());
                 d.fixed = fixed = make_node(AST_Function, fixed, fixed).clone(true);
             }
             if (compressor.option("unused")

$ uglifyjs -V
uglify-es 3.0.16

$ uglifyjs test.js -cb bracketize
Defun false function sum(){for(var s=0,i=0,n=arguments.length;i<n;++i)s+=arguments[i];return s}
!function() {
    function sum() {
        for (var s = 0, i = 0, n = arguments.length; i < n; ++i) {
            s += arguments[i];
        }
        return s;
    }
    console.log(sum()), console.log(sum());
}();

$ uglifyjs -V
uglify-js 3.0.16

$ uglifyjs test.js -cb bracketize
Defun true function sum(){for(var s=0,i=0,n=arguments.length;i<n;++i)s+=arguments[i];return s}
!function() {
    function sum() {
        for (var s = 0, i = 0, n = arguments.length; i < n; ++i) {
            s += arguments[i];
        }
        return s;
    }
    console.log(sum(1, 2, 3, 4)), console.log(sum(5, 6, 7, 8));
}();

[kzc]

Hmm - looks like uses_argument is broken on harmony:

Yikes - that'd do it.

Begs the question - do uses_eval and uses_with work on harmony?

[kzc]

Proof that I don't know how to use GitHub - this mess does not really give me useful information
harmony-v2.8.10...harmony-v2.8.11

I've never satisfactorily found a way to list just the commits on a specific branch in git.

Those multi-commit merges from master to harmony throw things for a loop.

[alexlamsl]

I ended up staring at #1591 for a bit, but quickly got bored and started poking at the code directly.

[alexlamsl]

Reduced case with patch from https://github.com/mishoo/UglifyJS2/issues/2097#issuecomment-308499525:

$ echo 'function f(){arguments}f();' | uglifyjs -c toplevel
Defun true function f(){}


$ echo 'function f(){for(;arguments;);}f();' | uglifyjs -c toplevel
Defun false function f(){for(;arguments;);}
!function(){for(;arguments;);}();

Looks like a fix is required for lib/scope.js

[alexlamsl]

This is the culprit: https://github.com/mishoo/UglifyJS2/commit/266ddd96399afcdee56d9d58b287f912b8728342

I believe it should read if (sym.scope instanceof AST_Lambda && name == "arguments") {, though I wonder if that breaks master - oh wait, there is catch on master which uses block scope :smiling_imp:

[alexlamsl]

Bingo! This works on master:

$ echo 'function f(){console.log(arguments[0])}f(1)' | uglifyjs -c toplevel
!function(){console.log(arguments[0])}(1);

... but this fails:

$ echo 'function f(){try{throw 0}catch(e){console.log(arguments[0])}}f(1)' | uglifyjs -c toplevel
!function(){try{throw 0}catch(e){console.log(arguments[0])}}();

$ echo 'function f(){try{throw 0}catch(e){console.log(arguments[0])}}f(1)' | node
1

$ echo 'function f(){try{throw 0}catch(e){console.log(arguments[0])}}f(1)' | uglifyjs -c toplevel | node
undefined

[robpalme]

Another repro - the presence of the inner block matters.

$ echo 'console.log(function() {{ return arguments[0]; }}("Keyser Soze"))' | uglifyjs -c
console.log(function(){return arguments[0]}());

[alexlamsl]

@robpalme yes, since ES6 introduces the concept of block scopes, it exposes the otherwise (almost) innocuous bug in https://github.com/mishoo/UglifyJS2/commit/266ddd96399afcdee56d9d58b287f912b8728342

Please feel free to try out #2099 on uglify-es and see if you can break it.

[kzc]

2099 appears to handle it:

$ echo 'console.log(function() {{ return arguments[0]; }}("Keyser Soze"))' | bin/uglifyjs -c
console.log(function(){return arguments[0]}("Keyser Soze"));