diff --git a/src/com/google/javascript/jscomp/AstAnalyzer.java b/src/com/google/javascript/jscomp/AstAnalyzer.java index f84d8a8d571..b98ec62f1a5 100644 --- a/src/com/google/javascript/jscomp/AstAnalyzer.java +++ b/src/com/google/javascript/jscomp/AstAnalyzer.java @@ -66,8 +66,6 @@ public class AstAnalyzer { private static final ImmutableSet BUILTIN_FUNCTIONS_WITHOUT_SIDEEFFECTS = ImmutableSet.of( "Object", "Array", "String", "Number", "BigInt", "Boolean", "RegExp", "Error"); - private static final ImmutableSet OBJECT_METHODS_WITHOUT_SIDEEFFECTS = - ImmutableSet.of("toString", "valueOf"); private static final ImmutableSet REGEXP_METHODS = ImmutableSet.of("test", "exec"); private static final ImmutableSet 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()) { diff --git a/test/com/google/javascript/jscomp/AstAnalyzerTest.java b/test/com/google/javascript/jscomp/AstAnalyzerTest.java index 066f97f59c9..c2fbb1ff921 100644 --- a/test/com/google/javascript/jscomp/AstAnalyzerTest.java +++ b/test/com/google/javascript/jscomp/AstAnalyzerTest.java @@ -654,13 +654,26 @@ public static ImmutableList 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()"), diff --git a/test/com/google/javascript/jscomp/CheckSideEffectsTest.java b/test/com/google/javascript/jscomp/CheckSideEffectsTest.java index b9c32696a3a..a911f0d2e54 100644 --- a/test/com/google/javascript/jscomp/CheckSideEffectsTest.java +++ b/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); + } } diff --git a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java index 2687204f0f2..cf8bba7341d 100644 --- a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java +++ b/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);