From 720ec246d3c01a10e0ee48ed48d14858649fb606 Mon Sep 17 00:00:00 2001 From: johnlenz Date: Thu, 4 Aug 2016 09:12:50 -0700 Subject: [PATCH] Use type information to detect special case side-effect free String methods. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=129336777 --- .../google/javascript/jscomp/NodeUtil.java | 54 ++++++++++++++----- .../jscomp/PureFunctionIdentifier.java | 3 +- .../javascript/jscomp/NodeUtilTest.java | 39 +++++--------- .../jscomp/PureFunctionIdentifierTest.java | 28 +++++++++- 4 files changed, 81 insertions(+), 43 deletions(-) diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index e15a920b429..78e05046b92 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -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; @@ -43,6 +46,7 @@ import java.util.List; import java.util.Map; import java.util.Set; + import javax.annotation.Nullable; /** @@ -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": @@ -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)) { @@ -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. */ diff --git a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java index 8b1935800bf..4625df0f65c 100644 --- a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java +++ b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java @@ -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; @@ -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; } diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java index 844c142b095..57b9de1ba19 100644 --- a/test/com/google/javascript/jscomp/NodeUtilTest.java +++ b/test/com/google/javascript/jscomp/NodeUtilTest.java @@ -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 */ @@ -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(); } @@ -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)); @@ -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)); } diff --git a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java index 72431905c03..cba036e83cc 100644 --- a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java +++ b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java @@ -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; @@ -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()"; @@ -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 = @@ -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)) {