Uglifyjs: join_vars regression since 3.3.6

[eiszfuchs]

This is a Bug report for a regression with ES5 input since version 3.3.6.

I didn't find a report on this problem.

Javascript input

I could reproduce the problem with this script:

var x = {};

x.a = 'foo';
x.b = 'bar';

var y = { a: 'foo', b: 'bar' };

y.a = 'faz';
y.c = 'baz';

window.x = x;
window.y = y;

It doesn't make much sense, but the problem occured in this vendor script I was using: https://github.com/abouolia/sticky-sidebar/blob/master/src/sticky-sidebar.js#L554

The output from version 3.3.6 on breaks strict mode and at least fails in Internet Explorer 11.

Minification script

const uglifyJS = require('uglify-es')

console.log(uglifyJS.minify(fs.readFileSync('my-code.js', 'utf8'), {}).code, 'utf8'))

Outputs

Expected

var x={a:"foo",b:"bar"},y={a:"foo",b:"bar"};y.a="faz",y.c="baz",window.x=x,window.y=y;

Though, technically, this would be correct, too:

var x={a:"foo",b:"bar"},y={a:"foo",b:"bar",c:"baz"};y.a="faz",window.x=x,window.y=y;

[email protected]

var x={};x.a="foo",x.b="bar";var y={a:"foo",b:"bar"};y.a="faz",y.c="baz",window.x=x,window.y=y;

[email protected]

var x={a:"foo",b:"bar"},y={a:"foo",b:"bar",a:"faz",c:"baz"};window.x=x,window.y=y;

[email protected] (current version)

var x={a:"foo",b:"bar"},y={a:"foo",b:"bar",a:"faz",c:"baz"};window.x=x,window.y=y;

[email protected]

var x={};x.a="foo",x.b="bar";var y={a:"foo",b:"bar"};y.a="faz",y.c="baz",window.x=x,window.y=y;

[email protected]

var x={a:"foo",b:"bar"},y={a:"foo",b:"bar",a:"faz",c:"baz"};window.x=x,window.y=y;

[email protected] (current version)

var x={a:"foo",b:"bar"},y={a:"foo",b:"bar",a:"faz",c:"baz"};window.x=x,window.y=y;

Workaround

You can work around the problem by turning join_vars off:

const options = {
    compress: {
        join_vars: false,
    },
}

[alexlamsl]

Interesting - so duplicated property names in object literal only fails under "use strict"; of ES5:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode

Duplicate property names are a syntax error in strict mode:

• This is no longer the case in ECMAScript 2015 (bug 1041128).

[kzc]

How about that...

$ echo '"use strict"; console.log({a:1, b:2, a:3});' | node690
{ a: 3, b: 2 }

$ echo '"use strict"; console.log({a:1, b:2, a:3});' | node0_12_9 
"use strict"; console.log({a:1, b:2, a:3});
                                     ^
SyntaxError: Duplicate data property in object literal not allowed in strict mode



md5-9f22626da4a5f9623dd2565eed299f40



$ echo 'console.log({a:1, b:2, a:3});' | node_modules/.bin/acorn --ecma5 >/dev/null && echo pass
pass



md5-3b15d8470844ad062f72833fb48bd91a



$ echo '"use strict"; console.log({a:1, b:2, a:3});' | node_modules/.bin/acorn --ecma5 >/dev/null && echo pass
Redefinition of property (1:37)

[alexlamsl]

@eiszfuchs thanks for the detailed report - working on the fix :wink:

@kzc I shall shamelessly steal your test case as usual :ghost:

[kzc]

The main issue is that property order is not preserved if the first duplicate is removed. The following programs are not equivalent:

$ echo '"use strict"; var o = {a:1, b:2}; o.a = 3; console.log(o);' | node690
{ a: 3, b: 2 }

$ echo '"use strict"; var o = {b:2, a:3}; console.log(o);' | node690
{ b: 2, a: 3 }

So all you can do in ES5 strict mode is to stop the transform upon encountering the first duplicate key.

In harmony compress for ecma >= 6 this constraint is not necessary.

[alexlamsl]

@kzc I was thinking along the same lines, though was more worried about side effects rather than enumeration - good to know :+1:

[kzc]

If the original object literal is constant and the subsequent property assignment value is side effect free then the duplicate key's value can overwrite the original key's value in situ, thus retaining its property position within the object. But this is a lot of potentially error prone work for little payback.

[alexlamsl]

Yeah, that sounds like collapse_vars in reverse :sweat_smile:

I reckon if that object literal is so full of constant expressions, then hoist_props could probably pick up a lot of slack anyway.