Skip to content

Commit

Permalink
Use type information to detect special case side-effect free String m…
Browse files Browse the repository at this point in the history 
…ethods.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=129336777
  • Loading branch information
@concavelenz
concavelenz authored and Dimitris Vardoulakis committed Aug 5, 2016
1 parent a33d704 commit 720ec24
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 43 deletions.
54 changes: 40 additions & 14 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -32,9 +32,12 @@
import com.google.javascript.rhino.Token;
import com.google.javascript.rhino.TokenStream;
import com.google.javascript.rhino.TokenUtil;
import com.google.javascript.rhino.TypeI;
import com.google.javascript.rhino.dtoa.DToA;
import com.google.javascript.rhino.jstype.JSType;
import com.google.javascript.rhino.jstype.JSTypeNative;
import com.google.javascript.rhino.jstype.TernaryValue;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand All @@ -43,6 +46,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;

import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -1255,32 +1259,32 @@ && allArgsUnescapedLocal(callNode)) {
return false;
}

Node nameNode = callNode.getFirstChild();
Node callee = callNode.getFirstChild();

// Built-in functions with no side effects.
if (nameNode.isName()) {
String name = nameNode.getString();
if (callee.isName()) {
String name = callee.getString();
if (BUILTIN_FUNCTIONS_WITHOUT_SIDEEFFECTS.contains(name)) {
return false;
}
} else if (nameNode.isGetProp()) {
} else if (callee.isGetProp()) {
if (callNode.hasOneChild()
&& OBJECT_METHODS_WITHOUT_SIDEEFFECTS.contains(
nameNode.getLastChild().getString())) {
callee.getLastChild().getString())) {
return false;
}

if (callNode.isOnlyModifiesThisCall()
&& evaluatesToLocalValue(nameNode.getFirstChild())) {
&& evaluatesToLocalValue(callee.getFirstChild())) {
return false;
}

// Many common Math functions have no side-effects.
// TODO(nicksantos): This is a terrible terrible hack, until
// I create a definitionProvider that understands namespacing.
if (nameNode.getFirstChild().isName() && nameNode.isQualifiedName()
&& nameNode.getFirstChild().getString().equals("Math")) {
switch(nameNode.getLastChild().getString()) {
if (callee.getFirstChild().isName() && callee.isQualifiedName()
&& callee.getFirstChild().getString().equals("Math")) {
switch(callee.getLastChild().getString()) {
case "abs":
case "acos":
case "acosh":
Expand Down Expand Up @@ -1319,12 +1323,15 @@ && evaluatesToLocalValue(nameNode.getFirstChild())) {
}

if (compiler != null && !compiler.hasRegExpGlobalReferences()) {
if (nameNode.getFirstChild().isRegExp()
&& REGEXP_METHODS.contains(nameNode.getLastChild().getString())) {
if (callee.getFirstChild().isRegExp()
&& REGEXP_METHODS.contains(callee.getLastChild().getString())) {
return false;
} else if (nameNode.getFirstChild().isString()) {
String method = nameNode.getLastChild().getString();
Node param = nameNode.getNext();
} else if (isTypedAsString(callee.getFirstChild(), compiler)) {
// 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.getLastChild().getString();
Node param = callee.getNext();
if (param != null) {
if (param.isString()) {
if (STRING_REGEXP_METHODS.contains(method)) {
Expand All @@ -1346,6 +1353,25 @@ && evaluatesToLocalValue(nameNode.getFirstChild())) {
return true;
}

private static boolean isTypedAsString(Node n, AbstractCompiler compiler) {
if (n.isString()) {
return true;
}

if (compiler.getOptions().useTypesForOptimization) {
TypeI type = n.getTypeI();
if (type != null) {
TypeI nativeStringType = compiler.getTypeIRegistry()
.getNativeType(JSTypeNative.STRING_TYPE);
if (type.isEquivalentTo(nativeStringType)) {
return true;
}
}
}

return false;
}

/**
* @return Whether the call has a local result.
*/
Expand Down
3 changes: 2 additions & 1 deletion src/com/google/javascript/jscomp/PureFunctionIdentifier.java
View file
Expand Up @@ -35,6 +35,7 @@
import com.google.javascript.rhino.jstype.FunctionType;
import com.google.javascript.rhino.jstype.JSType;
import com.google.javascript.rhino.jstype.JSTypeNative;

import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
Expand Down Expand Up @@ -415,7 +416,7 @@ public void visit(NodeTraversal traversal, Node node, Node parent) {
return;
}

if (!NodeUtil.nodeTypeMayHaveSideEffects(node) && !node.isReturn()) {
if (!NodeUtil.nodeTypeMayHaveSideEffects(node, compiler) && !node.isReturn()) {
return;
}

Expand Down
39 changes: 12 additions & 27 deletions test/com/google/javascript/jscomp/NodeUtilTest.java
View file
Expand Up @@ -30,12 +30,13 @@
import com.google.javascript.rhino.Token;
import com.google.javascript.rhino.jstype.TernaryValue;

import junit.framework.TestCase;

import java.util.Collection;
import java.util.HashSet;
import java.util.Set;

import junit.framework.TestCase;


/**
* Tests for NodeUtil
*/
Expand Down Expand Up @@ -276,11 +277,7 @@ public void testIsObjectLiteralKey1() throws Exception {
}

private Node parseExpr(String js) {
Compiler compiler = new Compiler();
CompilerOptions options = new CompilerOptions();
options.setLanguageIn(LanguageMode.ECMASCRIPT5);
compiler.initOptions(options);
Node root = compiler.parseTestCode(js);
Node root = parse(js);
return root.getFirstFirstChild();
}

Expand All @@ -289,66 +286,53 @@ private void testIsObjectLiteralKey(Node node, boolean expected) {
}

public void testGetFunctionName1() throws Exception {
Compiler compiler = new Compiler();
Node parent = compiler.parseTestCode("function name(){}");

Node parent = parse("function name(){}");
testGetFunctionName(parent.getFirstChild(), "name");
}

public void testGetFunctionName2() throws Exception {
Compiler compiler = new Compiler();
Node parent = compiler.parseTestCode("var name = function(){}")
Node parent = parse("var name = function(){}")
.getFirstFirstChild();

testGetFunctionName(parent.getFirstChild(), "name");
}

public void testGetFunctionName3() throws Exception {
Compiler compiler = new Compiler();
Node parent = compiler.parseTestCode("qualified.name = function(){}")
Node parent = parse("qualified.name = function(){}")
.getFirstFirstChild();

testGetFunctionName(parent.getLastChild(), "qualified.name");
}

public void testGetFunctionName4() throws Exception {
Compiler compiler = new Compiler();
Node parent = compiler.parseTestCode("var name2 = function name1(){}")
Node parent = parse("var name2 = function name1(){}")
.getFirstFirstChild();

testGetFunctionName(parent.getFirstChild(), "name2");
}

public void testGetFunctionName5() throws Exception {
Compiler compiler = new Compiler();
Node n = compiler.parseTestCode("qualified.name2 = function name1(){}");
Node n = parse("qualified.name2 = function name1(){}");
Node parent = n.getFirstFirstChild();

testGetFunctionName(parent.getLastChild(), "qualified.name2");
}

public void testGetBestFunctionName1() throws Exception {
Compiler compiler = new Compiler();
Node parent = compiler.parseTestCode("function func(){}");
Node parent = parse("function func(){}");

assertEquals("func",
NodeUtil.getNearestFunctionName(parent.getFirstChild()));
}

public void testGetBestFunctionName2() throws Exception {
Compiler compiler = new Compiler();
CompilerOptions options = new CompilerOptions();
options.setLanguageIn(LanguageMode.ECMASCRIPT6);
compiler.initOptions(options);

Node parent = compiler.parseTestCode("var obj = {memFunc(){}}")
Node parent = parse("var obj = {memFunc(){}}")
.getFirstFirstChild().getFirstFirstChild();

assertEquals("memFunc",
NodeUtil.getNearestFunctionName(parent.getLastChild()));
}


private void testGetFunctionName(Node function, String name) {
assertEquals(Token.FUNCTION, function.getType());
assertEquals(name, NodeUtil.getName(function));
Expand All @@ -374,6 +358,7 @@ private void assertSideEffect(boolean se, String js) {
private void assertSideEffect(boolean se, String js, boolean globalRegExp) {
Node n = parse(js);
Compiler compiler = new Compiler();
compiler.initCompilerOptionsIfTesting();
compiler.setHasRegExpGlobalReferences(globalRegExp);
assertEquals(se, NodeUtil.mayHaveSideEffects(n.getFirstChild(), compiler));
}
Expand Down
28 changes: 27 additions & 1 deletion test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java
View file
Expand Up @@ -23,6 +23,7 @@
import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
import com.google.javascript.rhino.Node;

import java.util.ArrayList;
import java.util.List;

Expand Down Expand Up @@ -570,6 +571,30 @@ public void testNoSideEffectsSimple() throws Exception {
prefix + "return externObj.foo" + suffix, expected);
}

public void testNoSideEffectsSimple2() throws Exception {
regExpHaveSideEffects = false;

checkMarkedCalls(
LINE_JOINER.join(
"function f() {",
" return ''.replace(/xyz/g, '');",
"}",
"f()"),
ImmutableList.of("STRING STRING replace", "f"));
}

public void testNoSideEffectsSimple3() throws Exception {
regExpHaveSideEffects = false;

checkMarkedCalls(
LINE_JOINER.join(
"function f(/** string */ str) {",
" return str.replace(/xyz/g, '');",
"}",
"f('')"),
ImmutableList.of("str.replace", "f"));
}

public void testResultLocalitySimple() throws Exception {
String prefix = "var g; function f(){";
String suffix = "} f()";
Expand Down Expand Up @@ -1373,6 +1398,7 @@ private class NoSideEffectCallEnumerator
@Override
public void process(Node externs, Node root) {
compiler.setHasRegExpGlobalReferences(regExpHaveSideEffects);
compiler.getOptions().setUseTypesForOptimization(true);
NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler);
defFinder.process(externs, root);
PureFunctionIdentifier passUnderTest =
Expand All @@ -1393,7 +1419,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
noSideEffectCalls.add(generateNameString(n.getFirstChild()));
}
} else if (n.isCall()) {
if (!NodeUtil.functionCallHasSideEffects(n)) {
if (!NodeUtil.functionCallHasSideEffects(n, compiler)) {
noSideEffectCalls.add(generateNameString(n.getFirstChild()));
}
if (NodeUtil.callHasLocalResult(n)) {
Expand Down

0 comments on commit 720ec24

Please sign in to comment.