Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
KimlikDAO-bot opened this issue Jun 18, 2024 · 1 comment
Open

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

KimlikDAO-bot opened this issue Jun 18, 2024 · 1 comment
Labels
good first issue Relatively easy, but low priority bugs help wanted

Comments

@KimlikDAO-bot
Copy link
Contributor

KimlikDAO-bot commented Jun 18, 2024

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
Copy link
Contributor

brad4d commented Jun 28, 2024

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.

@brad4d brad4d added help wanted good first issue Relatively easy, but low priority bugs labels Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Relatively easy, but low priority bugs help wanted
Projects
None yet
Development

No branches or pull requests

2 participants