Incorrect removal property from object accessed in nested conditions

[WearyMonkey]

We have found a case where a property of a conditionally accessed object is incorrectly removed. This is a critical issue as it's impossible to lint for in a large code base and will fail at runtime.

tested versions:
20201126.0.0-nightly
20201102.0.0

source:

const X = {Y: 1};

function fn(a) {
    if (a) {
        return a ? X : {};
    }
}

console.log(fn(true).Y);

java -jar compiler.jar -O ADVANCED --js test.js

output:

var a;a={};console.log(a.a);

expected output:

var a;a={a:1};console.log(a.a);

Notes:
Remove either of the conditions and it works as expected
Move the X variable declaration into fn and it works as expected.
Inline the fn and it works as expected.

[WearyMonkey]

Looks like an issue with the collapseProperties phase:

java -jar compiler.jar -O ADVANCED  --js test.js --print_source_after_each_pass                                                

// parseInputs yields:
// ************************************
const X={Y:1};function fn(a){if(a)return a?X:{}}console.log(fn(true).Y);
// ************************************

// Es6RewriteBlockScopedDeclaration yields:
// ************************************
var X={Y:1};function fn(a){if(a)return a?X:{}}console.log(fn(true).Y);
// ************************************

// normalize yields:
// ************************************
var X={Y:1};function fn(a$jscomp$1){if(a$jscomp$1)return a$jscomp$1?X:{}}console.log(fn(true).Y);
// ************************************

// collapseProperties yields:
// ************************************
var X$Y=1;var X={};function fn(a$jscomp$1){if(a$jscomp$1)return a$jscomp$1?X:{}}console.log(fn(true).Y);
// ************************************

// earlyInlineVariables yields:
// ************************************
var X={};console.log(function fn(a$jscomp$1){if(a$jscomp$1)return a$jscomp$1?X:{}}(true).Y);
// ************************************

// removeUnusedCode yields:
// ************************************
var X={};console.log(function(a$jscomp$1){if(a$jscomp$1)return a$jscomp$1?X:{}}(true).Y);
// ************************************

// inlineFunctions yields:
// ************************************
var X={};var JSCompiler_inline_result$jscomp$0;{JSCompiler_inline_label_0_2:{if(true){JSCompiler_inline_result$jscomp$0=true?X:{};break JSCompiler_inline_label_0_2}JSCompiler_inline_result$jscomp$0=void 0}}console.log(JSCompiler_inline_result$jscomp$0.Y);
// ************************************

// peepholeOptimizations yields:
// ************************************
var X={};var JSCompiler_inline_result$jscomp$0;JSCompiler_inline_label_0_2:{JSCompiler_inline_result$jscomp$0=X;break JSCompiler_inline_label_0_2;JSCompiler_inline_result$jscomp$0=void 0}console.log(JSCompiler_inline_result$jscomp$0.Y);
// ************************************

// removeUnreachableCode yields:
// ************************************
var X={};var JSCompiler_inline_result$jscomp$0;JSCompiler_inline_label_0_2:JSCompiler_inline_result$jscomp$0=X;console.log(JSCompiler_inline_result$jscomp$0.Y);
// ************************************

// inlineVariables yields:
// ************************************
var JSCompiler_inline_result$jscomp$0;JSCompiler_inline_label_0_2:JSCompiler_inline_result$jscomp$0={};console.log(JSCompiler_inline_result$jscomp$0.Y);
// ************************************

// renameProperties yields:
// ************************************
var JSCompiler_inline_result$jscomp$0;JSCompiler_inline_label_0_2:JSCompiler_inline_result$jscomp$0={};console.log(JSCompiler_inline_result$jscomp$0.a);
// ************************************

// renameVars yields:
// ************************************
var a;JSCompiler_inline_label_0_2:a={};console.log(a.a);
// ************************************

// renameLabels yields:
// ************************************
var a;a={};console.log(a.a);
// ************************************
var a;a={};console.log(a.a);

[blickly]

This is a long-standing assumption of how the collapse properties pass works and is one of the sharpest edges of ADVANCED mode right now (along with quoted vs. unquoted properties).

@brad4d may have some longer-term ideas for how to improve usage model here, but in the shorter term the best workaround I know of is use the /** @nocollapse */ annotation, which tells the collapseProperties pass to backoff of collapsing for a specific declaration.

For example, on your given example, it would be used like so:
https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F**%2520%2540nocollapse%2520*%252F%250Aconst%2520X%2520%253D%2520%257BY%253A%25201%257D%253B%250A%250Afunction%2520fn(a)%2520%257B%250A%2520%2520%2520%2520if%2520(a)%2520%257B%250A%2520%2520%2520%2520%2520%2520%2520%2520return%2520a%2520%253F%2520X%2520%253A%2520%257B%257D%253B%250A%2520%2520%2520%2520%257D%250A%257D%250A%250Aconsole.log(fn(true).Y)%253B

[WearyMonkey]

@blickly thanks for your reply. It sounds like you're implying it's by design?

However it fails under such a specific pattern that I can't help but think it's a bug. E.g. swap the if with a ? and it doesn't collapse:

const X = {Y: 1};

function fn(a) {
    if (a) {
       if (a) {
          return  X;
       } else {
          return {};
       }
    }
}

Or introduce a variable, and it works as expected:

const X = {Y: 1};

function fn(a) {
    if (a) {
        const b = a ? X : {};
        return b;
    }
}

but in the shorter term the best workaround I know of is use the /** @nocollapse */ annotation

In a code base with millions of lines of code, and hundreds of developers thats not really an option unless it is very clear what the pattern of incorrect use is. With strings vs dot properties we can say don't mix the two, but not sure how to communicate the anti-pattern that triggers this issue.

As far we know we could be hitting this problem hundreds of times in our code base, and only know when on of our users executes the code in production.

[blickly]

Hmm, that's a very interesting example. For cases like these, I prefer to use the debugger, so that we can see the effects of a single compiler pass in isolation and make sure that the changes aren't caused by other passes that run before, but even in that context, it's easy to reproduce how the CollapseProperties pass is behaving differently with the ?: operator than with other similar patterns:
https://closure-compiler-debugger.appspot.com/#input0%3Dconst%2520X%2520%253D%2520%257BY%253A%25201%257D%253B%250A%250Afunction%2520fn(a)%2520%257B%250A%2520%2520if%2520(a)%2520%257B%250A%2520%2520%2520%2520return%2520a%2520%253F%2520X%2520%253A%2520%257B%257D%253B%250A%2520%2520%257D%250A%257D%250A%250Aconsole.log(fn(true).Y)%253B%26input1%26conformanceConfig%26externs%3Dfunction%2520alert(x)%2520%257B%257D%26refasterjs-template%26COLLAPSE_PROPERTIES%3Dtrue%26PRESERVE_TYPE_ANNOTATIONS%3Dtrue%26PRETTY_PRINT%3Dtrue

I think Bradford would be best able to evaluate if this is likely a small fix to how CollapseProperties operates currently, or part of some bigger/more long term issue.

[brad4d]

@WearyMonkey Thanks a lot for reporting this case.
I'm actively working right now on making CollapseProperties behave in a safer manner, and I'm adding a test case for this situation into those I need to fix.

I expect to discover why ?: is different from if while adding that test case, and I'll update this issue when I have.
It does seem reasonable to hope that we can do a quick fix to plug this particular hole, but I don't know for sure yet.

[brad4d]

Good news!

I found the source of the problem and have a fix pending review and testing internally.

Basically there was a switch statement that failed to check for the RETURN node type and flag that as creating an alias for the returned value.

[WearyMonkey]

Thanks @brad4d ! Appreciate the fast response.

Any chance this will be released soon?

[brad4d]

I believe we'll be doing a new release today.