Fix modifying vars passed as function params

[alexmalcoci]

Fixes https://github.com/google/closure-compiler/issues/4129 where passing a variable as one param, then changing it as second param would equate the two refs.

[niloc132]

I don't see an explicit test for it, but does this also resolve https://github.com/google/closure-compiler/issues/4115?

[alexmalcoci]

That issue revealed that looking for assignments wasn't enough, we have to track all usages of vars, and if it happens to be assigned later on, add temps for all usages. I have added a few more tests and fixed one test related to that issue by adding an extra temp. 3edba2e should cover all use cases. Thanks @niloc132

[alexmalcoci]

@rishipal Any updates on this? It's a bit of a blocker for us.

[alexmalcoci]

I've uncovered another edge case where one of the args is a variable, and a subsequent arg is a function call that eventually modifies the value of the first variable. What happens is that the call arg is temped, but the first one is not, so the first one ends up with the updated value instead of the original. Below is a contrived example illustrating this (most of the complexity is to prevent the compiler from optimizing it into a form that no longer has the bug).

Example of first param getting a wrong value

```js let text = 'abc' let index = 1 // This function is necessary. Inlining the slice call will result in both args being temped function slice(begin, end) { return text.slice(begin, end); } function getEndIndex() { if (index) // This line is the only relevant line in this function -- the index is being modified. // Everything else is artificial complexity to prevent the function from being inlined return ++index; return getEndIndex(); } window.bug = slice(index - 1, getEndIndex()) ``` `java -jar closure-compiler-v20230802.jar --js in.js --formatting PRETTY_PRINT --compilation_level ADVANCED` => ```js let a = 1; function b() { return a ? ++a : b(); } var c = window, d = b(); c.g = "abc".slice(a - 1, d); ```

Notice how b() gets invoked first which modifies he value of a, and only then it's used in the .slice() call. A simpler version of this example would be below, but then the function call is inlined and breaks the example.

A simpler (but broken) version of the example above

```js let index = 1 function getEndIndex() { return ++index; } window.bug = 'abc'.slice(index - 1, getEndIndex()) ```

I propose a fix in 99548d5 where we keep track of all args that contain references to vars, then if subsequently we encounter a function call, unconditionally temp all tracked args before. Ideally only the affected args would be temped, but since there's no way to see what the function call does, we have to temp all previous args. As always the tests are green, and added a few additional tests.

[rishipal]

PTAL at my comments on https://github.com/google/closure-compiler/issues/4115. I believe all 3 issues (this PR https://github.com/google/closure-compiler/pull/4131 and issues https://github.com/google/closure-compiler/issues/4129 and https://github.com/google/closure-compiler/issues/4115) reference the same underlying problem in JSCompiler (being fixed in https://github.com/google/closure-compiler/issues/4115).

[alexmalcoci]

Thanks, @rishipal, it does seem like your approach is similar to mine. Can you post the patch file with the fix here, or add my tests to your fix? I just want to make sure that your fix fixes all my use cases as well.

[rishipal]

Copying over the update from https://github.com/google/closure-compiler/issues/4115):

1. I've incorporated all tests from the upstream PR https://github.com/google/closure-compiler/pull/4131 into the internal CL. They both do the same thing essentially, but the difference is that upstream PR traverses args to find the names used in each arg and only hoists them conditionally, whereas my implementation hoists all previous args (for simplicity and avoiding arg traversals for performance).

2. The fix produces a small code size increase (<0.1% diff) for 2 targets in google3. I've sent the CL to the affected owners to LGTM. Awaiting review.

[rishipal]

I've submitted the fix internally and it should get auto-pushed to GitHub by copybara in a few ~hours.

[alexmalcoci]

Thank you, @rishipal , I'll close this PR.

[rishipal]

Sg, for future readers this got fixed with bebf834e32d61a14baa608bc49b4081ce0b1ecbc.