Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ECMASCRIPT_2015+ output omits return in some contexts #3462

Closed
bogdanb opened this issue Sep 5, 2019 · 3 comments
Closed

ECMASCRIPT_2015+ output omits return in some contexts #3462

bogdanb opened this issue Sep 5, 2019 · 3 comments
Labels
internal-issue-created An internal Google issue has been created to track this GitHub issue

Comments

@bogdanb
Copy link

bogdanb commented Sep 5, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

brad4d commented Sep 12, 2019

Internal issue created http://b/140946919

@brad4d brad4d added the internal-issue-created An internal Google issue has been created to track this GitHub issue label Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal-issue-created An internal Google issue has been created to track this GitHub issue
Projects
None yet
Development

No branches or pull requests

3 participants