Mark (bigint|number).toString(radix) nosideeffects (fixes #4169)#4308
Mark (bigint|number).toString(radix) nosideeffects (fixes #4169)#4308mnsaglam wants to merge 1 commit intogoogle:masterfrom
Conversation
mnsaglam
commented
Mar 19, 2026
mnsaglam
commented
- Object.toString() was marked nosideeffects in AstAnalyzer through a special code path.
- We extend this to (number|bigint).toString(radix) as well.
|
@brad4d Does this look about right? |
|
@KimlikDAO-bot I think this is OK, but I'm having trouble importing it due to a completely unrelated problem that it's not worth explaining. Could you please rebase this PR to the current head? That should resolve the problem for me. |
|
@brad4d It's synced to the head now. Let me know if the direction is right, if so, happy to add more tests. |
|
Thanks, that resolved the problem. |
| @@ -452,4 +452,14 @@ public void testIgnoresRuntimeLibRequireStatementsOnly() { | |||
| srcs(SourceFile.fromCode("other/file.js", "'require base'")), | |||
| warning(CheckSideEffects.USELESS_CODE_ERROR)); | |||
| } | |||
There was a problem hiding this comment.
What we're missing is a test case to confirm that the call will not be eliminated when the this object is not numerical.
I think we should have that before I try to run this through extensive testing.
| @@ -3668,6 +3668,16 @@ public void testCallCache_propagatesSideEffects() { | |||
| }); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
What we're missing is a test case to confirm that the call will not be considered pure when the this object is not numerical.
I think we should have that before I try to run this through extensive testing.
There was a problem hiding this comment.
Just for learning: So you mean that if the compiler knows for sure that its numerical, the call can be removed. And if it does not know for sure (any type) its kept? Why wouldn't we say that toString will always only have the purpose to output a string and if anyone calls another function toString, its an idiot? Thanks.
There was a problem hiding this comment.
Right, we may be way overly cautious here. Instead of
if (assumeKnownBuiltinsArePure
&& OBJECT_METHODS_WITHOUT_SIDEEFFECTS.contains(callee.getString())
&& (callNode.hasOneChild() || isNumericToStringCall(callee))) {maybe one can decide the call to be pure if its a built-in with 0 arguments or toString(...) with any number of arguments, regardless of the node type. Something like
if (assumeKnownBuiltinsArePure
&& OBJECT_METHODS_WITHOUT_SIDEEFFECTS.contains(callee.getString())
&& (callNode.hasOneChild() || "toString".equals(callee.getString()))) {Whichever is the desired behavior, I can change the code to that.
There was a problem hiding this comment.
OK, I did a bit of historical digging with the intent of finding out whether the callNode.hasOneChild() condition was added in response to the discovery of some bug.
The answer was "no". This condition was there when the logic was first added in 2010, and never had a comment attached to say why it was there. I assume it was just an extra signal to be sure that the valueOf or toString call didn't look suspiciously different from the standard usage.
I'll point out that OBJECT_METHODS_WITHOUT_SIDEEFFECTS has always had only valueOf and toString in it and is only used in this one location.
So, I'll suggest 3 options for you.
- Add the tests as I originally suggested.
- eliminate the
OBJECT_METHODS_WITHOUT_SIDE_EFFECTSand make the logic explicitly check for eithervalueOfwith no arguments ortoStringwith at most 1 argument. - drop all checks for number of arguments
Options 2 and 3 assume you would also remove the type-checking code, of course.
Number 3 keeps the logic the simplest, but adds a new risk of breaking code using user-written valueOf or toString that have side-effects. OTOH, it would enable elimination of all those calls when their results are unused.
Number 2 adds very little risk, so I'd be inclined to go with that one.
I think I prefer both of those options over number 1 now that I've had more time to reflect on it, because they're simpler and don't require type information. But, if you decide to add tests, I'm still OK with merging it.
There was a problem hiding this comment.
Awesome, thanks a lot! I'll get to this later today.
Let me research this better but I remember seeing toString() pure assumption in other tools as well (oxc, Uglify). If so, that may make 3 safer than it initially seems: most things that can be broken by this have already been broken & likely fixed.
Re: eliminating OBJECT_METHODS_WITHOUT_SIDE_EFFECTS , I've been thinking of ways to transform Object.keys(OBJECT_LITERAL) -> array literal (which would motivate keeping this map) but this is quite tricky in gcc due to prop renaming happening later. Let me understand this better.
There was a problem hiding this comment.
@brad4d I implemented Option 2 with minimal changes. Can you take another look?
Though, it could be nice to rearrange functionCallHasSideEffects() to have the following structure:
test callNode.isNoSideEffectsCall()
test callNode.isOnlyModifiesArgumentsCall() && NodeUtil.allArgsUnescapedLocal(callNode)
test callNode.isOnlyModifiesThisCall() && NodeUtil.evaluatesToLocalValue(callee.getFirstChild())
then test for built-ins
- Object.toString() was marked nosideeffects in AstAnalyzer through a special code path. - We extend this to (number|bigint).toString(radix).
|
I've pulled in the latest version of this PR for testing and review. FYI, I'll be on vacation April 15 - 27. |
3 participants
