Merge pull request #4308 from KimlikDAO-bot:toStringRadix

PiperOrigin-RevId: 908417323

Author: copybara-github

src/com/google/javascript/jscomp/AstAnalyzer.java

@@ -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");
@@ -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;
       }
 
@@ -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",
@@ -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()) {

test/com/google/javascript/jscomp/AstAnalyzerTest.java

@@ -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()"),

test/com/google/javascript/jscomp/CheckSideEffectsTest.java

@@ -452,4 +452,14 @@ public void testIgnoresRuntimeLibRequireStatementsOnly() {
         srcs(SourceFile.fromCode("other/file.js", "'require base'")),
         warning(CheckSideEffects.USELESS_CODE_ERROR));
   }
+
+  @Test
+  public void testToStringWithRadixIsUselessCode() {
+    testWarning("({}).toString(16);", e);
+  }
+
+  @Test
+  public void testOptionalChainToStringWithRadixIsUselessCode() {
+    testWarning("({})?.toString(16);", e);
+  }
 }

test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java

@@ -3668,6 +3668,16 @@ public void testCallCache_propagatesSideEffects() {
         });
   }
 
+  @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);