Renaming causes bug to occur on iOS device

[leidegre]

The following JavaScript

"use strict";
const selectors_1 = require("../../../shared/selectors");
module.exports = async (dispatch, getState, { match, serverRes, appContext }) => {
    const { fetchProjectDetails } = await appContext.import('../actions', __filename, __dirname);
    await dispatch(fetchProjectDetails(match.params.projectHId));
    if (typeof process !== 'undefined') {
        if (selectors_1.getRequestErrorCode(getState(), { operationId: `/projects/${match.params.projectHId}` }) === 'NOT_FOUND') {
            serverRes && serverRes.status(404);
        }
    }
};

Compiled like this

java -jar closure-compiler-v20181125.jar --language_in ECMASCRIPT_2017 --language_out ECMASCRIPT_2017 --js x.js --formatting PRETTY_PRINT

Results in the following code

'use strict';
const selectors_1 = require("../../../shared/selectors");
module.exports = async(d, e, {match:b, serverRes:c, appContext:a}) => {
  var {fetchProjectDetails:a} = await a.import("../actions", __filename, __dirname);
  await d(a(b.params.projectHId));
  "undefined" !== typeof process && "NOT_FOUND" === selectors_1.getRequestErrorCode(e(), {operationId:`/projects/${b.params.projectHId}`}) && c && c.status(404);
};

This line

var {fetchProjectDetails:a} = await a.import("../actions", __filename, __dirname);

Both appContext and fetchProjectDetails was renamed to a which appears to confuse iOS. The error we get is undefined is not an object (evaluating 'a.import'). This is a consistent issue across all iOS devices we've been able to test with and I'm wondering whether there's a way to get around this issue without switching to WHITESPACE_ONLY.

[EatingW]

Created Google internal issue b/120558057

[EatingW]

This doesn't seem specific to iOS. The code will cause the same error in any environment.

It might be a destructuring-related bug in the CoalesceVariableNames pass.

[leidegre]

@EatingW we did not experience any problems with V8 (Node.js/Chrome/Chromium). I don't know the language spec well enough to say whether this construction is legal or not but it is definitely not ideal from my point of view because it's confusing.

I don't think the Closure Compiler should repurpose any variable name in scope. I don't think the trade off is worth it. But this is just my opinion.

[lauraharker]

Sorry, Yiting's earlier comment was my mistake. This code is valid in the language. You can workaround this by with options.setCoalesceVariableNames(false) in the Java API.

Here's a smaller repro that fails in Safari v12.0.1 for me on MacOS, but works in other browsers:
(async (x) => { var x; x.y })({})
The var x declaration seems to cause the parameter x to be put in a separate scope from the body. For me, this is only happen with async arrows, so maybe we could update the pass to not rename variables in async arrow fns only. I'll see if there's an existing WebKit bug for this.

Could you explain what trade offs of variable name repurposing you're referring to? Just this one bug, or have you run into other issues with the pass?

[lauraharker]

Looks like https://bugs.webkit.org/show_bug.cgi?id=182414.

[leidegre]

Sorry, Yiting's earlier comment was my mistake. This code is valid in the language. You can workaround this by with options.setCoalesceVariableNames(false) in the Java API.

Great, I'll use this for now.

[leidegre]

Could you explain what trade offs of variable name repurposing you're referring to? Just this one bug, or have you run into other issues with the pass?

I was thinking about the space saving of minification trade off. That while reusing a here is legal, b would be equally good and not as confusing. If I ever have to debug the minified output overly aggressive compact naming does make that harder. I understand that there are situations where this probably is warranted but that was what I had in mind, when I wrote that.

[lauraharker]

I'm implementing a workaround for the Safari bug.

Re debugging minified code - I agree that this makes debugging harder. Since it benefits code size it still seems worth having on by default to me. (As long as the source map information is corret.)

[leidegre]

I agree that this makes debugging harder. Since it benefits code size it still seems worth having on by default to me. (As long as the source map information is corret.)

Agreed.

[lauraharker]

One more debugging item I forgot about earlier:

If you enable options.generatePseudoNames = true, the compiler will generate a new name that fuses the two combined variable names, instead of just using the first name. Something like:
function f(a) { let b = 3; return b; } -> function f(a_b) { let a_b = 3; return a_b; }