ECMASCRIPT_2015+ output omits return in some contexts

[bogdanb]

The following test case is compiled incorrectly when language_out is ECMASCRIPT_2015 or later. (This is the smallest I could make it and still reproduce the issue. I found it in the middle of much more complicated code.)

function test(a, b) { 
    "use strict";

    console.log(b);

    if ('123' == b) {
        const Y = a.c;

        if ('abc' == Y)
            a.Z = a.d;

        if ('abc' != Y)
            return Y;
    }

    return b;
}

The result of running

java -jar closure-compiler-v20190819.jar --language_out=ECMASCRIPT_2015 test.js

is as follows:

'use strict';function test2(b,a){console.log(a);if("123"==a){const a=b.c;"abc"==a&&(b.Z=b.d)}return a};

You might be able to see that the output is missing the “if('abc' != Y) return Y” branch. When calling the method with arguments ({c:'xyz'},'123'), the correct result is 'xyz', but the compiled method returns '123'.

(The output also moves “use strict” outside the function, even if you add another non-strict function to the input file. This is wrong, since it’ll turn the other functions into strict ones. But that’s probably a different issue.)

Using ECMASCRIPT5 for the output language generates correct code.

[lauraharker]

Here's the output with --print_source_after_each_pass:

// parseInputs yields:
// ************************************
'use strict';window["test"]=function test(a,b){console.log(b);if("123"==b){const Y=a.c;if("abc"==Y)a.Z=a.d;if("abc"!=Y)return Y}return b};
// ************************************

// removeUnusedCode yields:
// ************************************
'use strict';window["test"]=function(a,b){console.log(b);if("123"==b){const Y=a.c;if("abc"==Y)a.Z=a.d;if("abc"!=Y)return Y}return b};
// ************************************

// convertToDottedProperties yields:
// ************************************
'use strict';window.test=function(a,b){console.log(b);if("123"==b){const Y=a.c;if("abc"==Y)a.Z=a.d;if("abc"!=Y)return Y}return b};
// ************************************

// renameVars yields:
// ************************************
'use strict';window.test=function(b,a){console.log(a);if("123"==a){const a=b.c;if("abc"==a)b.Z=b.d;if("abc"!=a)return a}return a};
// ************************************

// latePeepholeOptimizations yields:
// ************************************
'use strict';window.test=function(b,a){console.log(a);if("123"==a){const a=b.c;"abc"==a&&(b.Z=b.d)}return a};
// ************************************
'use strict';window.test=function(b,a){console.log(a);if("123"==a){const a=b.c;"abc"==a&&(b.Z=b.d)}return a};

so it's the latePeepholeOptimizations that's breaking this.

Probably one of the optimizations sees two return a; statements in the function and doesn't realize that they may refer to different variables, so eliminates the seemingly unnecessary return a;.

[lauraharker]

Here's a smaller repro:

function test(a) { 
    if (a) {
        const a = Math.random();

        if (a < 0.5) {
            return a;
        }
    }

    return a;
}

becomes

function test(a) {
  if (a) {
    const a = Math.random();
  }
  return a;
}

[brad4d]

Internal issue created http://b/140946919