Skip to content

Mark (bigint|number).toString(radix) nosideeffects (fixes #4169) #4308

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.

Sign up for GitHub

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

Merged
copybara-service merged 1 commit into from
Apr 30, 2026
+45 −12
Merged

Mark (bigint|number).toString(radix) nosideeffects (fixes #4169) #4308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/com/google/javascript/jscomp/AstAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,6 @@ public class AstAnalyzer {
private static final ImmutableSet<String> BUILTIN_FUNCTIONS_WITHOUT_SIDEEFFECTS =
ImmutableSet.of(
"Object", "Array", "String", "Number", "BigInt", "Boolean", "RegExp", "Error");
private static final ImmutableSet<String> OBJECT_METHODS_WITHOUT_SIDEEFFECTS =
ImmutableSet.of("toString", "valueOf");
private static final ImmutableSet<String> REGEXP_METHODS = ImmutableSet.of("test", "exec");
private static final ImmutableSet<String> STRING_REGEXP_METHODS =
ImmutableSet.of("match", "replace", "search", "split");
Expand Down Expand Up @@ -163,9 +161,12 @@ boolean functionCallHasSideEffects(Node callNode) {
return false;
}
} else if (callee.isGetProp() || callee.isOptChainGetProp()) {
if (callNode.hasOneChild()
&& assumeKnownBuiltinsArePure
&& OBJECT_METHODS_WITHOUT_SIDEEFFECTS.contains(callee.getString())) {
String method = callee.getString();
if (assumeKnownBuiltinsArePure
&& ("valueOf".equals(method)
? callNode.hasOneChild()
: "toString".equals(method)
&& (callNode.hasOneChild() || callNode.hasTwoChildren()))) {
return false;
}

Expand All @@ -181,7 +182,7 @@ boolean functionCallHasSideEffects(Node callNode) {
&& callee.getFirstChild().isName()
&& callee.isQualifiedName()
&& callee.getFirstChild().getString().equals("Math")) {
switch (callee.getString()) {
switch (method) {
case "abs",
"acos",
"acosh",
Expand Down Expand Up @@ -226,13 +227,12 @@ boolean functionCallHasSideEffects(Node callNode) {
}

if (!hasRegexpGlobalReferences && assumeKnownBuiltinsArePure) {
if (callee.getFirstChild().isRegExp() && REGEXP_METHODS.contains(callee.getString())) {
if (callee.getFirstChild().isRegExp() && REGEXP_METHODS.contains(method)) {
return false;
} else if (isTypedAsString(callee.getFirstChild())) {
// Unlike regexs, string methods don't need to be hosted on a string literal
// to avoid leaking mutating global state changes, it is just necessary that
// the regex object can't be referenced.
String method = callee.getString();
Node param = callee.getNext();
if (param != null) {
if (param.isStringLit()) {
Expand Down
21 changes: 17 additions & 4 deletions test/com/google/javascript/jscomp/AstAnalyzerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,13 +654,26 @@ public static ImmutableList<AnalysisCase> cases() {
kase().expect(false).token(SUPER).js("super()"),
kase().expect(false).token(SUPER).js("super.foo()"),

// IObject methods
// "toString" and "valueOf" are by default assumed to be side-effect free
// Some common Object methods are assumed to be side-effect free when called
// with their standard builtin arity.
kase().expect(false).js("o.toString()"),
kase().expect(false).js("o.toString(16)"),
kase().expect(false).js("(0).toString(16)"),
kase().expect(false).js("(0n).toString(16)"),
kase().expect(false).js("o?.toString(16)"),
kase().expect(false).js("(0)?.toString(16)"),
kase().expect(false).js("(0n)?.toString(16)"),
kase().expect(false).js("o.valueOf()"),
// "toString" and "valueOf" are not assumed to be side-effect free when
// assumeKnownBuiltinsArePure is false.
kase().expect(true).js("o.toString(16, 2)"),
kase().expect(true).js("o.valueOf(1)"),
// These builtin-method assumptions are disabled when assumeKnownBuiltinsArePure is false.
kase().expect(true).js("o.toString()").assumeBuiltinsPure(false),
kase().expect(true).js("o.toString(16)").assumeBuiltinsPure(false),
kase().expect(true).js("(0).toString(16)").assumeBuiltinsPure(false),
kase().expect(true).js("(0n).toString(16)").assumeBuiltinsPure(false),
kase().expect(true).js("o?.toString(16)").assumeBuiltinsPure(false),
kase().expect(true).js("(0)?.toString(16)").assumeBuiltinsPure(false),
kase().expect(true).js("(0n)?.toString(16)").assumeBuiltinsPure(false),
kase().expect(true).js("o.valueOf()").assumeBuiltinsPure(false),
// other methods depend on the extern definitions
kase().expect(true).js("o.watch()"),
Expand Down
10 changes: 10 additions & 0 deletions test/com/google/javascript/jscomp/CheckSideEffectsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,4 +452,14 @@ public void testIgnoresRuntimeLibRequireStatementsOnly() {
srcs(SourceFile.fromCode("other/file.js", "'require base'")),
warning(CheckSideEffects.USELESS_CODE_ERROR));
}
Copy link
Copy Markdown
Contributor

@brad4d brad4d Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@Test
public void testToStringWithRadixIsUselessCode() {
testWarning("({}).toString(16);", e);
}

@Test
public void testOptionalChainToStringWithRadixIsUselessCode() {
testWarning("({})?.toString(16);", e);
}
}
10 changes: 10 additions & 0 deletions test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3668,6 +3668,16 @@ public void testCallCache_propagatesSideEffects() {
});
}

Copy link
Copy Markdown
Contributor

@brad4d brad4d Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@L3P3 L3P3 Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@mnsaglam mnsaglam Mar 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@brad4d brad4d Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Add the tests as I originally suggested.
  2. eliminate the OBJECT_METHODS_WITHOUT_SIDE_EFFECTS and make the logic explicitly check for either valueOf with no arguments or toString with at most 1 argument.
  3. 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.

Copy link
Copy Markdown
Contributor Author

@mnsaglam mnsaglam Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@mnsaglam mnsaglam Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@Test
public void testToStringWithRadixIsPure() {
assertPureCallsMarked("var x = {}; x.toString(16);", ImmutableList.of("x.toString"));
}

@Test
public void testOptionalChainToStringWithRadixIsPure() {
assertPureCallsMarked("var x = {}; x?.toString(16);", ImmutableList.of("x?.toString"));
}

@Test
public void testDynamicImport() {
ignoreWarnings(DiagnosticGroups.MODULE_LOAD);
Expand Down