google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.38k stars 1.15k forks source link

`Number.toString(base)` does not have `@nosideeffects` semantics #4169

Open KimlikDAO-bot opened 3 months ago

KimlikDAO-bot commented 3 months ago

It is mentioned in various places that .toString() method has a hardcoded @nosideeffects property. (e.g., Angular nosideeffect annotation)) (On a side note, is this hack still relevant after the @nosideeffects being allowed in non-externs code?)

However the same is not true for Number.toString(base) variant, which writes the number in bases other than base 10.

// a.js
for (let i = 0; i < 10; ++i)
  i.toString();
for (let j = 0; j < 10; ++j)
  j.toString(16);
google-closure-compiler -O ADVANCED a.js

a.js:2:2: WARNING - [JSC_USELESS_CODE] Suspicious code. This code lacks side-effects. Is there a bug?
  2|   i.toString();
       ^^^^^^^^^^^^

0 error(s), 1 warning(s), 94.7% typed

The first one is detected as suspicious code, the second one isn't.

The relevant externs definition appears correct, however it looks like there some special handling of .toString() in the java code.

This is important because in many fields (crypto, verifiable computation) some constants need to be generated (instead of being written out as literals). If GCC can eliminate the ones that aren't used, it will result in significant reductions. However when the generation code uses toString(16) elimination does not happen.

brad4d commented 3 months ago

We've added this issue to an internal list of possible optimization improvements, but it isn't a high priority.

We would likely accept a pull request.