fix delete corner cases

[alexlamsl]

No description provided.

[alexlamsl]

$ echo 'var a;console.log(delete(a=NaN))' | node
true

$ echo 'var a;console.log(delete(a=NaN))' | bin/uglifyjs -c toplevel
delete NaN;

$ echo 'var a;console.log(delete(a=NaN))' | bin/uglifyjs -c toplevel | node
false

[alexlamsl]

Looks like delete never had much love... 😓

[kzc]

Looks like delete never had much love

Because it's rarely used and most coders don't know what it is supposed to return - including me.

[alexlamsl]

Well, given that we've fixed a lot of the other bugs, I guess there's no harm getting these done and out of the way.

I've got a bunch of sequences related failures from test/ufuzz.js - once I've isolated them I'll decide if I should merge this first or amend this PR.

[kzc]

Another delete bug:

$ cat bug2.js 
for (var NaN, z = 3; delete (Infinity, NaN) && --z > 0;);
console.log(Infinity, NaN, z);

$ node bug2.js
Infinity undefined 0

$ cat bug2.js | node
Infinity NaN 0

$ bin/uglifyjs bug2.js -c | node
Infinity NaN 3

$ bin/uglifyjs bug2.js -c
for(var NaN,z=3;delete NaN&&--z>0;);console.log(1/0,NaN,z);

[alexlamsl]

You've got the same fuzz failures as I did in #1799 (comment)

Anyway, should be all fixed up now:

$ cat bug2.js
for (var NaN, z = 3; delete (Infinity, NaN) && --z > 0;);
console.log(Infinity, NaN, z);
$ bin/uglifyjs bug2.js -co min.js
WARN: Condition left of && always true [bug2.js:1,21]

$ cat bug2.js | node
Infinity NaN 0

$ cat min.js | node
Infinity NaN 0

$ node bug2.js
Infinity undefined 0

$ node min.js
Infinity undefined 0

Let's see if there are anything left...

[kzc]

$ cat bug2.js 
for (var NaN, z = 3; delete (Infinity, NaN) && --z > 0;);
console.log(Infinity, NaN, z);

$ bin/uglifyjs bug2.js -c
WARN: Condition left of && always true [bug2.js:5,21]
for(var NaN,z=3;--z>0;);console.log(1/0,NaN,z);

Trying to understand your fix - you dropped the delete of the local variable NaN altogether because it's a nop?

Nope:

$ echo 'function x(NaN){delete NaN} function y(){var NaN; delete NaN}' | bin/uglifyjs -c
function x(NaN){delete NaN}function y(){var NaN;delete NaN}

[alexlamsl]

delete (a, b) ➡️ true regardless of a and b

So we transform it to (a, b, !0)

[kzc]

delete (a, b) ➡️ true regardless of a and b

There are never any side effects for a delete of a comma expression regardless of what it contains?

[alexlamsl]

Not that I'm aware of:

$ node
> a = 1
1
> a
1
> delete a
true
> a
ReferenceError: 'a' is undefined
   at Global code (repl:1:1)
   ...
> a = 1
1
> delete (0, a)
true
> a
1
> delete "a"
true
> a
1

[kzc]

Interesting.

$ node
> var o = {a:1, b:2};
undefined
> delete (o.a, o.b);
true
> o
{ a: 1, b: 2 }
> delete true && o.b;
2
> o
{ a: 1, b: 2 }

$ echo 'var o = {a:1, b:2}; delete (true && o.b); console.log(o);' | node
{ a: 1, b: 2 }

With this PR:

$ echo 'var o = {a:1, b:2}; delete (true && o.b); console.log(o);' | bin/uglifyjs -c | node
WARN: Condition left of && always true [-:1,27]
{ a: 1, b: 2 }

So a delete of any expression other than obj.prop is a nop?

[alexlamsl]

I'm not the authority on JavaScript, but yeah, feels that way to me. Not even sure why this is allowed in the first place.

Though I suppose nobody would go to the trouble of using delete then put in non-sensical stuff... (Famous Last Words:tm::grey_question:)

[alexlamsl]

Babbling aside, please give me a signal if you are good with this PR and I'll merge it.

[kzc]

LGTM

[kzc]

I believe these delete corner cases were caught by the paren grouping fuzzer change. This particular uglify bug is as old as uglify itself - a rarely used scenario. The problem with hand crafted fuzzing scenarios (as opposed to grammar directed) is it's only as good as your imagination - and you're never sure it will trigger an error.

[alexlamsl]

The other problem is of priority - fuzzer will find bugs, but not necessarily the ones that will affect users the most. And it has a bad habbit of keep hitting the same ones until you fix it (or somehow program the fuzzer to avoid generating such cases)

Luckily, we don't seem to have too many cases left after the initial batch.

[kzc]

And it has a bad habbit of keep hitting the same ones until you fix it (or somehow program the fuzzer to avoid generating such cases)

It's unavoidable unfortunately - can only guide code generation with probabilities but you can't predict what it will produce. Well at least it will help prevent similar regressions.