Uglifyjs: [ES6] Array Destructuring assignment breaks when assigning to property of "this"

[EthanRutherford]

Bug with UglifyEs 3.0.19

var uglifyEs = require("uglify-es");
uglifyEs.minify("[this.a] = [0]");

results in TypeError: Cannot set property 'fixed' of undefined

[kzc]

Temporary workaround:

$ echo '[this.a] = [0];' | bin/uglifyjs -m -c reduce_vars=false
[this.a]=[0];

[kzc]

Here's an outright bug with toplevel:

$ echo 'var t = this; console.log([t.a] = [0]);' | node
[ 0 ]

$ echo 'var t = this; console.log([t.a] = [0]);' | bin/uglifyjs -c toplevel | node
console.log([t.a]=[0]);
             ^

ReferenceError: t is not defined

[alexlamsl]

@kzc your toplevel example is triggering a bug in unused:

$ echo '!function(){var t=1;console.log([t.a]=[0])}()' | uglifyjs -c
!function(){console.log([t.a]=[0])}();

$ echo '!function(){var t=1;console.log([t.a]=[0])}()' | uglifyjs -c unused=0
!function(){var t=1;console.log([t.a]=[0])}();

[alexlamsl]

@EthanRutherford thanks for the report - investigating.

TIL :open_mouth:

$ echo 'for((1,2,a) in b)console.log(a);' | uglifyjs
for(1,2,a in b)console.log(a);

[kzc]

That goes way back...

$ uglifyjs -V
uglify-js 2.7.5

$ echo 'for((1,2,a) in b)console.log(a);' | uglifyjs
for(1,2,a in b)console.log(a);

$ echo 'for((1,2,a) in b)console.log(a);' | babel
SyntaxError: unknown: Assigning to rvalue (1:5)
for((1,2,a) in b)console.log(a);



md5-a630ce5ac32b45a5d17c5bf1a24ab045



$ echo 'for({a, b: c = 1} in b)console.log(a);' | babel
"use strict";

for (var _ref in b) {
  var a = _ref.a;
  var _ref$b = _ref.b;
  var c = _ref$b === undefined ? 1 : _ref$b;
  console.log(a);
}



md5-565237e8cdd842d5ee946b6e0e286c60



$ echo 'for([a,,b] in b)console.log(a);' | babel -L
"use strict";

for (var _ref in b) {
  var a = _ref[0];
  var b = _ref[2];
  console.log(a);
}

[alexlamsl]

Could we have uglify spit out the same error in that situation?

Most definitely - even with GIGO there is a limit on what _G_ we are willing to admit... :see_no_evil:

[kzc]

Also be aware of the declaration forms of destructuring for for in/of:

for(const {a, b: c = 1} of b)console.log(a);

[alexlamsl]

Declarations are fine and taken care of:
https://github.com/mishoo/UglifyJS2/blob/71556d00b5ceab4304077a9898c7ed0264041a76/lib/parse.js#L1225-L1227

It's the alternative case which is surprisingly loose:
https://github.com/mishoo/UglifyJS2/blob/71556d00b5ceab4304077a9898c7ed0264041a76/lib/parse.js#L1228

[alexlamsl]

This is actually valid JavaScript, even for ES5:

$ echo 'var a={};for(a.b in [1,2])console.log(a);' | node
{ b: '0' }
{ b: '1' }

[kzc]

This is actually valid JavaScript, even for ES5:
$ echo 'var a={};for(a.b in [1,2])console.log(a);' | node

Never considered it, but it makes sense. We should get test/ufuzz.js to exercise it.

[kzc]

BTW, this for loop init stuff is a bit of tangent from destructuring assignment using property of this, is it not? ;-)

[alexlamsl]

@kzc I was hoping you'd ask at some point :wink:

The bug at the top of this issue is due to reset_opt_flags() trying to exclude all the AST_Symbol within AST_Destructuring from reduce_vars. The way it does it is to utilise suppressor, so the fix, which is to account for AST_This being an AST_Symbol but without any SymbolDef attached, would make most sense being done on master.

Now there is exactly one user of suppressor on master, and in looking up to see if I can throw in some tests for that PR, I summoned the infamous can of worms. :nauseated_face:

[kzc]

I was hoping you'd ask at some point

Everything is connected with everything - the circle of life.

[EthanRutherford]

Thanks for looking into this. guys. Those are some fascinating finds, regardless of how tangential to the issue at hand ;)