closure-compiler icon indicating copy to clipboard operation
closure-compiler copied to clipboard

`@pureOrBreakMyCode` can't handle when left-hand side is array pattern

Open jzhan-canva opened this issue 1 year ago • 8 comments

command java -jar closure-compiler.jar --compilation_level ADVANCED_OPTIMIZATIONS --formatting PRETTY_PRINT --js input.js --js_output_file compiled.js

the example code is trimmed from tc39 stage 3 decorator compiled by swc. in the example,@pureOrBreakMyCode is added manually to ({ e: [_init_p3] } = /** @pureOrBreakMyCode */_apply_decs_2203_r(...)

however, because left-hand side is destructuring assignment, so closure-compiler need to traverse the lhs and valueNode respectively, it can't eliminate the pure valueNode. image

reproducible code

/** @noinline */
const applyDecs2203RFactory = () => { return () => {} }
/** @noinline */
const _apply_decs_2203_r = applyDecs2203RFactory()
var _init_p3;

class A {
    constructor(){
        this.p3 = /** @pureOrBreakMyCode */_init_p3(this, "3");
    }
}
({ e: [_init_p3] } = /** @pureOrBreakMyCode */_apply_decs_2203_r(1, 2, 3));

jzhan-canva avatar Jul 11 '24 07:07 jzhan-canva

I'm seeing that the LHS is eliminated even without the @pureOrBreakMyCode annotation. Why do you think this is specific to the annotation?

Screenshot 2024-07-15 at 4 36 32 PM

rishipal avatar Jul 15 '24 23:07 rishipal

Hi @rishipal thanks for looking into this When @pureOrBreakMyCode is used, I expect the rhs gets removed with lhs together, which is the case when lhs is a name in this example, when lhs is name, the whole code can be eliminated image

but when lhs is destructuring assignment, it's not the case image

jzhan-canva avatar Jul 17 '24 00:07 jzhan-canva

It would be nice to have RemoveUnusedCode recognize this annotation and remove the call. Perhaps the /** @pureOrBreakMyCode */ isn't getting attached correctly and needs parens?

In any case, we must better document this annotation for ourselves. @rahul-kamat will take a look at your PR.

rishipal avatar Jul 17 '24 20:07 rishipal

Thanks @rishipal and @rahul-kamat unfortunately my employer Canva doesn't have a CLA signed with Google right now, so I guess my PR can't be used I'm happy to share my findings so far as we know the code working fine when lhs is a name, e.g. (_init_p3 = /** @pureOrBreakMyCode */_apply_decs_2203_r(1, 2, 3)); but can't work when lhs is array pattern, e.g. ([_init_p3] = /** @pureOrBreakMyCode */_apply_decs_2203_r(1, 2, 3)); I think the possible cause is that when lhs is name, CC traverse the assignment as a whole but when lhs is array pattern, CC respectively traverse left first, then traverse right which is a call. but CC doesn't check if a call is pure (I checked, the annotation is still attached to the callNode)

jzhan-canva avatar Jul 18 '24 01:07 jzhan-canva

CLA signed

jzhan-canva avatar Jul 29 '24 06:07 jzhan-canva

This might be a problem with the compiler's RemoveUnusedCode pass

If we can repro this with a unit test in RemoveUnusedCodeTest.java, we can confirm that RemoveUnusedCode is not supporting /** @pureOrBreakMyCode */ on LHS destructed assignment code pattern

rahul-kamat avatar Jul 29 '24 23:07 rahul-kamat

@rahul-kamat this can be a potential unit test

/** @nocollapse */
const createComplexPureFunction1 = () => (x) => {
  return x * 2;
};
/** @nocollapse */
const createComplexPureFunction2 = () => (x) => {
  return { result: [x * 2] };
};
/** @nocollapse */
function complexPureFunction1(x) {
  return (complexPureFunction1 = createComplexPureFunction1())(x)
}
/** @nocollapse */
function complexPureFunction2(x) {
  return (complexPureFunction2 = createComplexPureFunction2())(x)
}

const a = /** @pureOrBreakMyCode */ complexPureFunction1(1);

let b;
({
  result: [b],
} = /** @pureOrBreakMyCode */ complexPureFunction2(2));

in this example, only complexPureFunction1 gets removed completely, complexPureFunction2 still exist despite it has @pureOrBreakMyCode image

jzhan-canva avatar Jul 31 '24 04:07 jzhan-canva

I've spent some time looking into this, your latest example shows complexPureFunction1 completely getting removed but complexPureFunction2 is not removed by RemoveUnusedCode

I also added these 2 examples as unit tests for RemoveUnusedCode example1

example2

example1 shows code being removed, example2 does not. But in RemoveUnusedCode, the only thing removed from both examples is:

class A {
    constructor(){
        this.p3 = /** @pureOrBreakMyCode */_init_p3(this, "3");
    }
}

so these 2 examples have the exact same behavior in RemoveUnusedCode

I'll need to spend more time figuring out what's going on

rahul-kamat avatar Aug 03 '24 00:08 rahul-kamat