Fix for array constructor with length is not eliminated

[maifeeulasad]

closes #4046

Tested w

bazel test //:test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest --test_filter=testArrayConstructor --test_output=all

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[blickly]

Can you add unit tests for this new case alongside the existing unit tests of the similar new Array() cases in AstAnalyzerTest.java ?

In particular, including the .assumeBuiltinsPure(false) case is something we'd want to include

[maifeeulasad]

@blickly absolutely. Planning to do it in the next 18 hours!

[maifeeulasad]

@blickly care to take look into this please?

• fixed breaking test case
• added builtin assumption to both true and negative for array

Current testing commands are:

bazel test //:test/com/google/javascript/jscomp/AstAnalyzerTest --test_output=all
bazel test //:test/com/google/javascript/jscomp/PeepholeRemoveDeadCodeTest --test_filter=testArrayConstructor --test_output=all

Running the whole thing just to confirm CI doesn't complain any more with, if I find any issues I will resolve in upcoming commits

bazel test //... --test_output=errors

[maifeeulasad]

All tests are passing with

bazel test //... --test_output=errors

[blickly]

Can you put the new unit tests alongside and match the style of the existing tests of new Array having side effects?

[maifeeulasad]

Sorry. I used the exact style I found in ast analyzer, both for positive and negative test. And I already moved the tests into this existing file, where other ast related test lies. Do you want me to move them into some other files? Sorry I couldn't understand. But I am willing to complete this and get it done. Care to explain a bit more please?

@blickly

ref:

• 2b68d90

[maifeeulasad]

Does it require any additional attention? Please let me know if it requires any other modifications or anything like that.

@blickly

[blickly]

Sorry, I meant the tests in the test case called MayEffectMutableStateTest. Do you see how there are existing parameterized test cases that already test new Array? Can we put the new tests alongside those existing ones?

[maifeeulasad]

All done! Care to take a look into it again for me please??

Eagerly waiting for feedbacks and open to comply!

cc: @blickly

[blickly]

If you remove all your changes to src/com/google/javascript/jscomp/AstAnalyzer.java, which of the tests fail?

[maifeeulasad]

If you remove all your changes to src/com/google/javascript/jscomp/AstAnalyzer.java, which of the tests fail?

Nothing fails.

---

Sorry I missed it, cause I jumped into it.

due to the following lines, it is already working:

• https://github.com/maifeeulasad/closure-compiler/blob/51b8eb284619390f25c76f64f8b61f64c76ba146/src/com/google/javascript/jscomp/AstAnalyzer.java#L60
• https://github.com/maifeeulasad/closure-compiler/blob/51b8eb284619390f25c76f64f8b61f64c76ba146/src/com/google/javascript/jscomp/AstAnalyzer.java#L66
• https://github.com/maifeeulasad/closure-compiler/blob/51b8eb284619390f25c76f64f8b61f64c76ba146/src/com/google/javascript/jscomp/AstAnalyzer.java#L162

Do I raise another MR with just the tests introduced? Or do we continue here? What shall we do next??

cc: @blickly