Skip to content

Commit e94a25d

Browse files
lauraharkercopybara-github
lauraharker
authored and
copybara-github
committed
Only assume any built-in methods/functions are pure when options.setAssumePropertiesAreStaticallyAnalyzable is enabled
This is still the default behavior; we only change it for @closureUnaware code. PiperOrigin-RevId: 853730717
1 parent 11a60ac commit e94a25d

File tree

4 files changed

+65
-8
lines changed

4 files changed

+65
-8
lines changed

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

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ public class AstAnalyzer {
7676
private final AccessorSummary accessorSummary;
7777
private final boolean useTypesForLocalOptimization;
7878
private final boolean assumeGettersArePure;
79+
private final boolean assumeKnownBuiltinsArePure;
7980
private final boolean hasRegexpGlobalReferences;
8081

8182
AstAnalyzer(
@@ -90,12 +91,14 @@ public class AstAnalyzer {
9091
this.useTypesForLocalOptimization = options.useTypesForLocalOptimization;
9192
this.assumeGettersArePure = options.assumeGettersArePure;
9293
this.hasRegexpGlobalReferences = options.hasRegexpGlobalReferences;
94+
this.assumeKnownBuiltinsArePure = options.assumeKnownBuiltinsArePure;
9395
}
9496

9597
static record Options(
9698
boolean useTypesForLocalOptimization,
9799
boolean assumeGettersArePure,
98-
boolean hasRegexpGlobalReferences) {
100+
boolean hasRegexpGlobalReferences,
101+
boolean assumeKnownBuiltinsArePure) {
99102
@AutoBuilder
100103
static interface Builder {
101104
Builder setUseTypesForLocalOptimization(boolean value);
@@ -104,6 +107,8 @@ static interface Builder {
104107

105108
Builder setHasRegexpGlobalReferences(boolean value);
106109

110+
Builder setAssumeKnownBuiltinsArePure(boolean value);
111+
107112
Options build();
108113
}
109114

@@ -154,11 +159,12 @@ boolean functionCallHasSideEffects(Node callNode) {
154159
// Built-in functions with no side effects.
155160
if (callee.isName()) {
156161
String name = callee.getString();
157-
if (BUILTIN_FUNCTIONS_WITHOUT_SIDEEFFECTS.contains(name)) {
162+
if (assumeKnownBuiltinsArePure && BUILTIN_FUNCTIONS_WITHOUT_SIDEEFFECTS.contains(name)) {
158163
return false;
159164
}
160165
} else if (callee.isGetProp() || callee.isOptChainGetProp()) {
161166
if (callNode.hasOneChild()
167+
&& assumeKnownBuiltinsArePure
162168
&& OBJECT_METHODS_WITHOUT_SIDEEFFECTS.contains(callee.getString())) {
163169
return false;
164170
}
@@ -171,7 +177,8 @@ boolean functionCallHasSideEffects(Node callNode) {
171177
// Many common Math functions have no side-effects.
172178
// TODO(nicksantos): This is a terrible terrible hack, until
173179
// I create a definitionProvider that understands namespacing.
174-
if (callee.getFirstChild().isName()
180+
if (assumeKnownBuiltinsArePure
181+
&& callee.getFirstChild().isName()
175182
&& callee.isQualifiedName()
176183
&& callee.getFirstChild().getString().equals("Math")) {
177184
switch (callee.getString()) {
@@ -218,7 +225,7 @@ boolean functionCallHasSideEffects(Node callNode) {
218225
}
219226
}
220227

221-
if (!hasRegexpGlobalReferences) {
228+
if (!hasRegexpGlobalReferences && assumeKnownBuiltinsArePure) {
222229
if (callee.getFirstChild().isRegExp() && REGEXP_METHODS.contains(callee.getString())) {
223230
return false;
224231
} else if (isTypedAsString(callee.getFirstChild())) {
@@ -516,7 +523,9 @@ boolean constructorCallHasSideEffects(Node newNode) {
516523
}
517524

518525
Node nameNode = newNode.getFirstChild();
519-
return !nameNode.isName() || !CONSTRUCTORS_WITHOUT_SIDE_EFFECTS.contains(nameNode.getString());
526+
return !nameNode.isName()
527+
|| !assumeKnownBuiltinsArePure
528+
|| !CONSTRUCTORS_WITHOUT_SIDE_EFFECTS.contains(nameNode.getString());
520529
}
521530

522531
/**

‎src/com/google/javascript/jscomp/Compiler.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1477,6 +1477,7 @@ public AstAnalyzer getAstAnalyzer() {
14771477
.setUseTypesForLocalOptimization(options.shouldUseTypesForLocalOptimization())
14781478
.setAssumeGettersArePure(options.getAssumeGettersArePure())
14791479
.setHasRegexpGlobalReferences(this.hasRegExpGlobalReferences())
1480+
.setAssumeKnownBuiltinsArePure(options.getAssumePropertiesAreStaticallyAnalyzable())
14801481
.build();
14811482
return new AstAnalyzer(analysisSettings, typeRegistry, getAccessorSummary());
14821483
}

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

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@
8080

8181
import com.google.common.collect.ImmutableList;
8282
import com.google.common.collect.ImmutableMap;
83+
import com.google.errorprone.annotations.CanIgnoreReturnValue;
8384
import com.google.javascript.jscomp.AccessorSummary.PropertyAccessKind;
8485
import com.google.javascript.jscomp.colors.StandardColors;
8586
import com.google.javascript.rhino.Node;
@@ -113,32 +114,44 @@ private static final class AnalysisCase {
113114
Token token;
114115
boolean globalRegExp;
115116
boolean assumeGettersArePure;
117+
boolean assumeBuiltinsPure = true;
116118

119+
@CanIgnoreReturnValue
117120
AnalysisCase expect(boolean b) {
118121
this.expect = b;
119122
return this;
120123
}
121124

125+
@CanIgnoreReturnValue
122126
AnalysisCase js(String s) {
123127
this.js = s;
124128
return this;
125129
}
126130

131+
@CanIgnoreReturnValue
127132
AnalysisCase token(Token t) {
128133
this.token = t;
129134
return this;
130135
}
131136

137+
@CanIgnoreReturnValue
132138
AnalysisCase globalRegExp(boolean b) {
133139
this.globalRegExp = b;
134140
return this;
135141
}
136142

143+
@CanIgnoreReturnValue
137144
AnalysisCase assumeGettersArePure(boolean b) {
138145
this.assumeGettersArePure = b;
139146
return this;
140147
}
141148

149+
@CanIgnoreReturnValue
150+
AnalysisCase assumeBuiltinsPure(boolean b) {
151+
this.assumeBuiltinsPure = b;
152+
return this;
153+
}
154+
142155
@Override
143156
public String toString() {
144157
return String.format("%s node in `%s` -> %s", token, js, expect);
@@ -154,6 +167,7 @@ private static final class ParseHelper {
154167
private boolean useTypesForLocalOptimizations = false;
155168
private boolean hasGlobalRegexpReferences = true;
156169
private boolean assumeGettersArePure = true;
170+
private boolean assumeBuiltinsPure = true;
157171
private JSTypeRegistry typeRegistry;
158172
private final AccessorSummary accessorSummary =
159173
AccessorSummary.create(
@@ -195,6 +209,7 @@ Node parseFirst(Token token, String js) {
195209
Node parseCase(AnalysisCase kase) {
196210
this.hasGlobalRegexpReferences = kase.globalRegExp;
197211
this.assumeGettersArePure = kase.assumeGettersArePure;
212+
this.assumeBuiltinsPure = kase.assumeBuiltinsPure;
198213
Node root = parseInternal(kase.js);
199214
if (kase.token == null) {
200215
return root.getFirstChild();
@@ -209,6 +224,7 @@ AstAnalyzer getAstAnalyzer() {
209224
.setUseTypesForLocalOptimization(useTypesForLocalOptimizations)
210225
.setHasRegexpGlobalReferences(hasGlobalRegexpReferences)
211226
.setAssumeGettersArePure(assumeGettersArePure)
227+
.setAssumeKnownBuiltinsArePure(assumeBuiltinsPure)
212228
.build(),
213229
typeRegistry,
214230
accessorSummary);
@@ -372,6 +388,7 @@ public static ImmutableList<AnalysisCase> cases() {
372388
// Cases in need of differentiation.
373389
kase().expect(false).js("[1]"),
374390
kase().expect(false).js("[1, 2]"),
391+
kase().expect(false).js("[1, 2]").assumeBuiltinsPure(false),
375392
kase().expect(true).js("i++"),
376393
kase().expect(true).js("[b, [a, i++]]"),
377394
kase().expect(true).js("i=3"),
@@ -404,6 +421,7 @@ public static ImmutableList<AnalysisCase> cases() {
404421
kase().expect(false).js("(class Foo extends Bar { })"),
405422
kase().expect(true).js("(class extends foo() { })"),
406423
kase().expect(false).js("a"),
424+
kase().expect(false).js("a").assumeBuiltinsPure(false),
407425
kase().expect(false).js("a.b"),
408426
kase().expect(false).js("a.b.c"),
409427
kase().expect(false).js("[b, c [d, [e]]]"),
@@ -436,6 +454,7 @@ public static ImmutableList<AnalysisCase> cases() {
436454
kase().expect(false).js("new Array"),
437455
kase().expect(false).js("new Array(4)"),
438456
kase().expect(false).js("new Array('a', 'b', 'c')"),
457+
kase().expect(true).js("new Array()").assumeBuiltinsPure(false),
439458
kase().expect(true).js("new SomeClassINeverHeardOf()"),
440459
kase().expect(true).js("new SomeClassINeverHeardOf()"),
441460
kase().expect(false).js("({}).foo = 4"),
@@ -456,7 +475,11 @@ public static ImmutableList<AnalysisCase> cases() {
456475
kase().expect(false).js("({},[]).foo = 2;"),
457476
kase().expect(true).js("delete a.b"),
458477
kase().expect(false).js("Math.random();"),
478+
kase().expect(true).js("Math.random();").assumeBuiltinsPure(false),
459479
kase().expect(true).js("Math.random(seed);"),
480+
kase().expect(true).js("Math.random(seed);").assumeBuiltinsPure(false),
481+
kase().expect(false).js("Math.sin(1, 10);"),
482+
kase().expect(true).js("Math.sin(1, 10);").assumeBuiltinsPure(false),
460483
kase().expect(false).js("[1, 1].foo;"),
461484
kase().expect(true).js("export var x = 0;"),
462485
kase().expect(true).js("export let x = 0;"),
@@ -632,9 +655,13 @@ public static ImmutableList<AnalysisCase> cases() {
632655
kase().expect(false).token(SUPER).js("super.foo()"),
633656

634657
// IObject methods
635-
// "toString" and "valueOf" are assumed to be side-effect free
658+
// "toString" and "valueOf" are by default assumed to be side-effect free
636659
kase().expect(false).js("o.toString()"),
637660
kase().expect(false).js("o.valueOf()"),
661+
// "toString" and "valueOf" are not assumed to be side-effect free when
662+
// assumeKnownBuiltinsArePure is false.
663+
kase().expect(true).js("o.toString()").assumeBuiltinsPure(false),
664+
kase().expect(true).js("o.valueOf()").assumeBuiltinsPure(false),
638665
// other methods depend on the extern definitions
639666
kase().expect(true).js("o.watch()"),
640667

@@ -644,9 +671,15 @@ public static ImmutableList<AnalysisCase> cases() {
644671

645672
// RegExp instance methods have global side-effects, so whether they are
646673
// considered side-effect free depends on whether the global properties
647-
// are referenced.
674+
// are referenced and whether we assume that the built-in methods are not overwritten
675+
// with impure variants.
648676
kase().expect(true).js("(/abc/gi).test('')").globalRegExp(true),
649677
kase().expect(false).js("(/abc/gi).test('')").globalRegExp(false),
678+
kase()
679+
.expect(true)
680+
.js("(/abc/gi).test('')")
681+
.globalRegExp(false)
682+
.assumeBuiltinsPure(false),
650683
kase().expect(true).js("(/abc/gi).test(a)").globalRegExp(true),
651684
kase().expect(false).js("(/abc/gi).test(b)").globalRegExp(false),
652685
kase().expect(true).js("(/abc/gi).exec('')").globalRegExp(true),
@@ -915,11 +948,20 @@ public static final ImmutableList<Object[]> cases() {
915948
}
916949

917950
@Test
918-
public void test() {
951+
public void test_assumingPureBuiltins() {
919952
ParseHelper parseHelper = new ParseHelper();
953+
parseHelper.assumeBuiltinsPure = true;
920954
Node func = parseHelper.parseFirst(CALL, String.format("%s(1);", functionName));
921955
assertThat(parseHelper.getAstAnalyzer().functionCallHasSideEffects(func)).isFalse();
922956
}
957+
958+
@Test
959+
public void test_noAssumePureBuiltins() {
960+
ParseHelper parseHelper = new ParseHelper();
961+
parseHelper.assumeBuiltinsPure = false;
962+
Node func = parseHelper.parseFirst(CALL, String.format("%s(1);", functionName));
963+
assertThat(parseHelper.getAstAnalyzer().functionCallHasSideEffects(func)).isTrue();
964+
}
923965
}
924966

925967
@RunWith(JUnit4.class)

‎test/com/google/javascript/jscomp/TranspileAndOptimizeClosureUnawareTest.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,11 @@ public void testAssumeRegexpMayHaveGlobalSideEffects() {
245245
test(closureUnaware("/abc/.match(str); /unused/"), expectedClosureUnaware("/abc/.match(str);"));
246246
}
247247

248+
@Test
249+
public void testDontAssumeToStringAndValueOfArePure() {
250+
testSame(closureUnaware("x.toString(); x.valueOf();"));
251+
}
252+
248253
@Test
249254
public void transpilesExponentialOperator_ifTargetingES2015() {
250255
setLanguageOut(CompilerOptions.LanguageMode.ECMASCRIPT_2015);

0 commit comments

Comments
 (0)