Skip to content

Commit ef510d7

Browse files
committed
Mark (bigint|number).toString(radix) nosideeffects (fixes #4169)
- Object.toString() was marked nosideeffects in AstAnalyzer through a special code path. - We extend this to (number|bigint).toString(radix) as well.
1 parent f032d8c commit ef510d7

4 files changed

Lines changed: 57 additions & 3 deletions

File tree

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

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -163,9 +163,9 @@ boolean functionCallHasSideEffects(Node callNode) {
163163
return false;
164164
}
165165
} else if (callee.isGetProp() || callee.isOptChainGetProp()) {
166-
if (callNode.hasOneChild()
167-
&& assumeKnownBuiltinsArePure
168-
&& OBJECT_METHODS_WITHOUT_SIDEEFFECTS.contains(callee.getString())) {
166+
if (assumeKnownBuiltinsArePure
167+
&& OBJECT_METHODS_WITHOUT_SIDEEFFECTS.contains(callee.getString())
168+
&& (callNode.hasOneChild() || isNumericToStringCall(callee))) {
169169
return false;
170170
}
171171

@@ -277,6 +277,32 @@ private boolean isTypedAsString(Node n) {
277277
return false;
278278
}
279279

280+
private boolean isNumericToStringCall(Node callee) {
281+
return (callee.isGetProp() || callee.isOptChainGetProp())
282+
&& "toString".equals(callee.getString())
283+
&& isTypedAsNumberOrBigInt(callee.getFirstChild());
284+
}
285+
286+
private boolean isTypedAsNumberOrBigInt(Node n) {
287+
if (n.isNumber() || n.isBigInt()) {
288+
return true;
289+
}
290+
if (useTypesForLocalOptimization) {
291+
Color color = n.getColor();
292+
if (color != null) {
293+
return color.equals(StandardColors.NUMBER) || color.equals(StandardColors.BIGINT);
294+
}
295+
JSType type = n.getJSType();
296+
if (type != null) {
297+
return type.isNumberValueType()
298+
|| type.isNumberObjectType()
299+
|| type.isBigIntValueType()
300+
|| type.isBigIntObjectType();
301+
}
302+
}
303+
return false;
304+
}
305+
280306
/**
281307
* Returns true if some node in n's subtree changes application state. If {@code
282308
* checkForNewObjects} is true, we assume that newly created mutable objects (like object

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -657,10 +657,18 @@ public static ImmutableList<AnalysisCase> cases() {
657657
// IObject methods
658658
// "toString" and "valueOf" are by default assumed to be side-effect free
659659
kase().expect(false).js("o.toString()"),
660+
kase().expect(false).js("(0).toString(16)"),
661+
kase().expect(false).js("(0n).toString(16)"),
662+
kase().expect(false).js("(0)?.toString(16)"),
663+
kase().expect(false).js("(0n)?.toString(16)"),
660664
kase().expect(false).js("o.valueOf()"),
661665
// "toString" and "valueOf" are not assumed to be side-effect free when
662666
// assumeKnownBuiltinsArePure is false.
663667
kase().expect(true).js("o.toString()").assumeBuiltinsPure(false),
668+
kase().expect(true).js("(0).toString(16)").assumeBuiltinsPure(false),
669+
kase().expect(true).js("(0n).toString(16)").assumeBuiltinsPure(false),
670+
kase().expect(true).js("(0)?.toString(16)").assumeBuiltinsPure(false),
671+
kase().expect(true).js("(0n)?.toString(16)").assumeBuiltinsPure(false),
664672
kase().expect(true).js("o.valueOf()").assumeBuiltinsPure(false),
665673
// other methods depend on the extern definitions
666674
kase().expect(true).js("o.watch()"),

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -452,4 +452,14 @@ public void testIgnoresRuntimeLibRequireStatementsOnly() {
452452
srcs(SourceFile.fromCode("other/file.js", "'require base'")),
453453
warning(CheckSideEffects.USELESS_CODE_ERROR));
454454
}
455+
456+
@Test
457+
public void testNumberToStringWithRadixIsUselessCode() {
458+
testWarning("(0).toString(16);", e);
459+
}
460+
461+
@Test
462+
public void testOptionalChainNumberToStringWithRadixIsUselessCode() {
463+
testWarning("(0)?.toString(16);", e);
464+
}
455465
}

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3668,6 +3668,16 @@ public void testCallCache_propagatesSideEffects() {
36683668
});
36693669
}
36703670

3671+
@Test
3672+
public void testNumberToStringWithRadixIsPure() {
3673+
assertPureCallsMarked("var x = 0; x.toString(16);", ImmutableList.of("x.toString"));
3674+
}
3675+
3676+
@Test
3677+
public void testOptionalChainNumberToStringWithRadixIsPure() {
3678+
assertPureCallsMarked("var x = 0; x?.toString(16);", ImmutableList.of("x?.toString"));
3679+
}
3680+
36713681
@Test
36723682
public void testDynamicImport() {
36733683
ignoreWarnings(DiagnosticGroups.MODULE_LOAD);

0 commit comments

Comments
 (0)