Uglifyjs: `pure_getters` with 3.1.4 breaks build

[epoberezkin]

ES5 or ES6+ input?

ES5

Uglify version (uglifyjs -V)

3.1.4

JavaScript input

https://github.com/epoberezkin/ajv/issues/593

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

-m -c pure_getters

[kzc]

Is the JS generated by uglify the same for all node versions in the CI tests?

According to this link Node versions 6, 7 and 8 are okay and only Node v4.8.4 fails.

Based on that data alone, it could point to a bug in the version of V8 used in Node v4.8.4 or the webkit VM in PhantomJS. If CI is run again does it fail deterministically?

@epoberezkin Would it be possible for you to isolate and create a small standalone test case for the specific failing test on node v4.8.4 so it could be easily examined?

[epoberezkin]

@kzc The browser test only runs in node 4; 6-8 pass because they don't use uglifyjs.

It works with 3.1.3 of uglifyjs. I didn't have a chance to look deeper yet.

[alexlamsl]

Not sure if related, but currently I'm hunting down the following:

correct

$ echo 'function f(a){var b=a.f;a.f++;return b}' | uglifyjs -c pure_getters
function f(a){var b=a.f;return a.f++,b}

$ echo 'function f(){var a={f:1},b=a.f;a.f++;return b}' | uglifyjs -c pure_getters
function f(){var a={f:1},b=a.f;return a.f++,b}

wrong

$ echo 'function f(){var b=this.f;this.f++;return b}' | uglifyjs -c pure_getters
function f(){return this.f++,this.f}

And my current theory is that #2350 may have exposed a bug.

[alexlamsl]

@epoberezkin if possible can you test if the following options would pass your tests?

• -c (i.e. without pure_getters)
• -c pure_getters,collapse_vars=false

[kzc]

Not sure if related, but currently I'm hunting down the following

Found by fuzzing?

[alexlamsl]

Found by fuzzing?

Nope - the 1MFuzz didn't catch anything this weekend.

I run into the issue when checking uglify-js against my $day_job

[kzc]

I ran into the issue when checking uglify-js against my $day_job

Ouch!

Nope - the 1MFuzz didn't catch anything this weekend.

The fuzzer may not create this.prop++ patterns.

[alexlamsl]

Ouch!

Nah - it's good to get paid for the work once in a while :wink:

The fuzzer may not create this.prop++ patterns.

Possible - or may be just very unprobable. I'll take a look this weekend if you don't beat me to it.

[kzc]

π aside, can the 3.14 in this issue's title to be changed to 3.1.4?

[alexlamsl]

TIL even the PR comment would cause auto-closing of issues :see_no_evil:

[kzc]

Could pure_getters be mentioned in the title?

TIL even the PR comment would cause auto-closing of issues

You seem to relearn that every few months. :-)

[alexlamsl]

You seem to relearn that every few months. :-)

In my defence, I thought ~crossing things out~ should be apparent to any :robot:

[kzc]

@epoberezkin Could you please confirm that reverting https://github.com/mishoo/UglifyJS2/commit/1abe14296e9a11d30935dba3f27682a3e67885f8 (https://github.com/mishoo/UglifyJS2/pull/2350) for the 3.1.4 release will allow your tests to pass with pure_getters enabled?

[alexlamsl]

(continued from https://github.com/mishoo/UglifyJS2/pull/2365#issuecomment-336968187)

@epoberezkin I see, I didn't know which tests failed since the issue you linked to from greenbot doesn't seem to have any output logs at all.

That and getting any of those npm run scripts to work on Windows require quite a bit of editting :sweat_smile:

[epoberezkin]

https://travis-ci.org/epoberezkin/ajv/jobs/288398162

Greenkeeper used to add links to travis builds from issues it creates @Realtin

[epoberezkin]

@kzc when https://github.com/mishoo/UglifyJS2/commit/1abe14296e9a11d30935dba3f27682a3e67885f8 is reverted ajv tests pass

[alexlamsl]

So I tried -c pure_getters against 3.1.3 and 3.1.4 on ajv.bundle.js, and this function does not seem functionally equivalent:

3.1.3

function callValidate() {
    var validate = compilation.validate, result = validate.apply(null, arguments);
    return callValidate.errors = validate.errors, result;
}

3.1.4

function callValidate() {
    var validate = compilation.validate;
    return callValidate.errors = validate.errors, validate.apply(null, arguments);
}

[epoberezkin]

So I tried -c pure_getters against 3.1.3 and 3.1.4 on ajv.bundle.js, and this function does not seem functionally equivalent

The change assumes that a function returned by pure getter is also pure (which it isn't in this case)...

[kzc]

Here are two failing pure_getters test cases:

The first test case does not use arguments:

$ cat t1.js 
function inc(obj) {
    return obj.count++;
}
function foo(bar) {
    var result = inc(bar);
    return foo.amount = bar.count, result;
}
var data = {
    count: 0,
};
var answer = foo(data);
console.log(foo.amount, answer);

$ cat t1.js | node
1 0



md5-1fdf4f17f1afa755d45abe6a0b9845ff



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



md5-e735826383df358ac0e5b12b952d0592



$ cat t2.js
function inc(obj) {
    return obj.count++;
}
function foo() {
    var first = arguments[0];
    var result = inc(first);
    return foo.amount = first.count, result;
}
var data = {
    count: 0,
};
var answer = foo(data);
console.log(foo.amount, answer);



md5-1fdf4f17f1afa755d45abe6a0b9845ff



$ cat t2.js | node
1 0



md5-1fdf4f17f1afa755d45abe6a0b9845ff



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

The second one is easy - I'd advise that if arguments is used in the function then collapse_vars should be disabled in that function.

The first test case is trickier. collapse_vars must not be allowed to skip over any AST_PropAccess with a variable found in the AST of any collapse variable candidate or that of the ASTs of previous local variables.

[kzc]

@alexlamsl The heuristic described above may not take care of all pure_getter+collapse_vars scenarios. We may have to revert #2350.

[alexlamsl]

@kzc keep those cases coming, I'll work out where they are in the code to decide the next course of action, as they may expose existing issues with collapse_vars as well.

[alexlamsl]

Let's see if #2370 would cover this :crossed_fingers:

[kzc]

@alexlamsl Unfortunately #2370 does not cover all cases. Could you please reopen this issue?

Testing against f2b9c11e2a0ab3597f798cf85770e24733609b1f:

$ cat t6.js 
function inc(obj) {
    return obj.count++;
}
function foo(bar, baz) {
    var result = inc(bar);
    return foo.amount = baz.count, result;
}
var data = {
    count: 0,
};
var answer = foo(data, data);
console.log(foo.amount, answer);

$ cat t6.js | node
1 0



md5-1fdf4f17f1afa755d45abe6a0b9845ff



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

[alexlamsl]

@kzc thanks for the new test case - PR coming up

(GitHub is currently view-only on my desktop – Grrr...)