@kzc would the following tests more preferable? The difference here is they don't rely on unsafe to generate AST_Undefined.

unsafe_undefined: {
    options = {
        if_return: true,
        unsafe: true
    }
    mangle = {}
    input: {
        function f(undefined) {
            return function() {
                if (a)
                    return b;
                if (c)
                    return d;
            }
        }
    }
    expect: {
        function f(n) {
            return function() {
                if (a)
                    return b;
                if (c)
                    return d;
                else
                    return n;
            }
        }
    }
}

keep_fnames: {
    options = {
        if_return: true,
        unsafe: true
    }
    mangle = {
        keep_fnames: true
    }
    input: {
        function f(undefined) {
            return function() {
                function n(a) {
                    return a*a;
                }
                if (a)
                    return b;
                if (c)
                    return d;
            }
        }
    }
    expect: {
        function f(r) {
            return function() {
                function n(n) {
                    return n*n;
                }
                if (a)
                    return b;
                if (c)
                    return d;
                else
                    return r;
            }
        }
    }
}

That would be preferable since the previous expected code is not valid as explained here: #1443 (comment)

The test you had just happened to illustrate why the typeof transform was deemed to be unsafe.

@kzc tests updated and pushed.

These new tests should still be vulnerable to the same problem, i.e. f(1) would still return 1 instead of undefined

I use this particular transformation in angular.js, where they would deliberately pass in one less parameter to make undefined a local variable. So capturing that whenever uglify-js generates new AST_Undefined gives shorter code.

These new tests should still be vulnerable to the same problem, i.e. f(1) would still return 1 instead of undefined

Redefining undefined is fraught with problems. I've read that coffeescript does something similar in generated catch clauses.

If the generated code is known to be incorrect - even with the unsafe=true option - please add a descriptive comment in the test as per:

https://github.com/mishoo/UglifyJS2/blob/7f8d72d9d37396f2da05d5d824f74bd414c30119/test/compress/screw-ie8.js#L49-L50

An issue for another PR: Is there a way to fix this unsafe typeof transform when undefined is redefined?

Will add comment after 🎥 🍿

@kzc comment added

As for your suggestion for that new PR, is the goal being able to apply typeof a == "undefined" without also enabling the local variable rule, which are both guarded behind unsafe at the moment thus loss of granularity?

If so, one suggestion would be to "graduate" the typeof optimisation from unsafe. It already makes sure the expression under test is not an undeclared symbol - are there other corner cases which this transformation would break?

If I'm reading the history correctly, typeof a == "undefined" optimisation is put behind unsafe in #105, and the reason being <DOM Node>.property may throw exceptions in IE<9.

So I propose swapping unsafe for screw_ie8, may be even compressor.option("screw_ie8") || !(self.right.expression instanceof AST_PropAccess)

I was not aware that #105 was the motivation behind the "unsafe" flag use in the typeof === undefined optimization, but in light of the redefining undefined bug it should probably be put behind "unsafe" in any case.

In the wild undefined is often used as a catch parameter as well as in function arguments.

@kzc mind if we carry on the discussion of typeof === undefined in #1446 so it's easier to keep track?