From 08f57404c3a13c74fdd1d2389fa97cdcd735e767 Mon Sep 17 00:00:00 2001 From: bradfordcsmith Date: Wed, 15 May 2019 16:16:45 -0700 Subject: [PATCH] FunctionArgumentInjector should AstAnalyzer.functionCallHasSideEffects This required some refactoring of some classes, but unblocks moving the functionCallHasSideEffects() implementation to AstAnalyzer. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=248426353 --- .../jscomp/FunctionArgumentInjector.java | 82 +++++----- .../javascript/jscomp/FunctionInjector.java | 142 ++++++++++++------ .../jscomp/FunctionToBlockMutator.java | 17 +-- .../javascript/jscomp/InlineFunctions.java | 16 +- .../google/javascript/jscomp/J2clPass.java | 7 +- .../jscomp/J2clPropertyInlinerPass.java | 12 +- .../google/javascript/jscomp/NodeUtil.java | 9 -- .../jscomp/FunctionArgumentInjectorTest.java | 121 +++++++++------ .../jscomp/FunctionInjectorTest.java | 45 +++--- .../javascript/jscomp/NodeUtilTest.java | 12 +- 10 files changed, 278 insertions(+), 185 deletions(-) diff --git a/src/com/google/javascript/jscomp/FunctionArgumentInjector.java b/src/com/google/javascript/jscomp/FunctionArgumentInjector.java index 6d6e35c77b7..8c0a9bcb82f 100644 --- a/src/com/google/javascript/jscomp/FunctionArgumentInjector.java +++ b/src/com/google/javascript/jscomp/FunctionArgumentInjector.java @@ -49,8 +49,10 @@ class FunctionArgumentInjector { static final String OBJECT_PATTERN_MARKER = "object pattern"; - private FunctionArgumentInjector() { - // A private constructor to prevent instantiation. + private final AstAnalyzer astAnalyzer; + + FunctionArgumentInjector(AstAnalyzer astAnalyzer) { + this.astAnalyzer = astAnalyzer; } /** @@ -62,12 +64,11 @@ private FunctionArgumentInjector() { * Nodes. * @return The root node or its replacement. */ - static Node inject( - AbstractCompiler compiler, Node node, Node parent, Map replacements) { + Node inject(AbstractCompiler compiler, Node node, Node parent, Map replacements) { return inject(compiler, node, parent, replacements, /* replaceThis */ true); } - private static Node inject( + private Node inject( AbstractCompiler compiler, Node node, Node parent, @@ -117,10 +118,8 @@ private static Node inject( return node; } - /** - * Get a mapping for function parameter names to call arguments. - */ - static ImmutableMap getFunctionCallParameterMap( + /** Get a mapping for function parameter names to call arguments. */ + ImmutableMap getFunctionCallParameterMap( final Node fnNode, Node callNode, Supplier safeNameIdSupplier) { checkNotNull(fnNode); // Create an argName -> expression map @@ -189,14 +188,18 @@ private static String getUniqueAnonymousParameterName( /** * Retrieve a set of names that can not be safely substituted in place. - * Example: + * + *

Example: 

+   *
    *   function(a) {
    *     a = 0;
    *   }
-   * Inlining this without taking precautions would cause the call site value
-   * to be modified (bad).
+   *
+ * + *

Inlining this without taking precautions would cause the call site value to be modified + * (bad). */ - static Set findModifiedParameters(Node fnNode) { + Set findModifiedParameters(Node fnNode) { ImmutableSet names = getFunctionParameterSet(fnNode); Set unsafeNames = new HashSet<>(); return findModifiedParameters(fnNode.getLastChild(), names, unsafeNames, false); @@ -268,7 +271,7 @@ private static boolean canNameValueChange(Node n) { * @param argMap The argument list for the call to fnNode. * @param namesNeedingTemps The set of names to update. */ - static void maybeAddTempsForCallArguments( + void maybeAddTempsForCallArguments( AbstractCompiler compiler, Node fnNode, ImmutableMap argMap, @@ -359,10 +362,10 @@ static void maybeAddTempsForCallArguments( } /** - * We consider a return or expression trivial if it doesn't contain a conditional expression or - * a function. + * We consider a return or expression trivial if it doesn't contain a conditional expression or a + * function. */ - static boolean bodyMayHaveConditionalCode(Node n) { + boolean bodyMayHaveConditionalCode(Node n) { if (!n.isReturn() && !n.isExprResult()) { return true; } @@ -370,10 +373,9 @@ static boolean bodyMayHaveConditionalCode(Node n) { } /** - * We consider an expression trivial if it doesn't contain a conditional expression or - * a function. + * We consider an expression trivial if it doesn't contain a conditional expression or a function. */ - static boolean mayHaveConditionalCode(Node n) { + boolean mayHaveConditionalCode(Node n) { for (Node c = n.getFirstChild(); c != null; c = c.getNext()) { switch (c.getToken()) { case FUNCTION: @@ -394,13 +396,13 @@ static boolean mayHaveConditionalCode(Node n) { /** * Bootstrap a traversal to look for parameters referenced after a non-local side-effect. * - * NOTE: This assumes no-inner functions. + *

NOTE: This assumes no-inner functions. + * * @param parameters The set of parameter names. * @param root The function code block. - * @return The subset of parameters referenced after the first - * seen non-local side-effect. + * @return The subset of parameters referenced after the first seen non-local side-effect. */ - private static ImmutableSet findParametersReferencedAfterSideEffect( + private ImmutableSet findParametersReferencedAfterSideEffect( ImmutableSet parameters, Node root) { // TODO(johnlenz): Consider using scope for this. @@ -419,24 +421,24 @@ private static ImmutableSet findParametersReferencedAfterSideEffect( /** * Collect parameter names referenced after a non-local side-effect. * - * Assumptions: - * - We assume parameters are not modified in the function body - * (that is checked separately). - * - There are no inner functions (also checked separately). + *

Assumptions: + * + *

    + *
  • We assume parameters are not modified in the function body (that is checked separately). + *
  • There are no inner functions (also checked separately). + *
* - * As we are trying to replace parameters with there passed in values - * we are interested in anything that may affect those value. So, ignoring - * changes to local variables, we look for things that may affect anything - * outside the local-state. Once such a side-effect is seen any following - * reference to the function parameters are collected. These will need - * to be assigned to temporaries to prevent changes to their value as would - * have happened during the function call. + *

As we are trying to replace parameters with there passed in values we are interested in + * anything that may affect those value. So, ignoring changes to local variables, we look for + * things that may affect anything outside the local-state. Once such a side-effect is seen any + * following reference to the function parameters are collected. These will need to be assigned to + * temporaries to prevent changes to their value as would have happened during the function call. * - * To properly handle loop structures all references to the function - * parameters are recorded and the decision to keep or throw away those - * references is deferred until exiting the loop structure. + *

To properly handle loop structures all references to the function parameters are recorded + * and the decision to keep or throw away those references is deferred until exiting the loop + * structure. */ - private static class ReferencedAfterSideEffect implements Visitor, Predicate { + private class ReferencedAfterSideEffect implements Visitor, Predicate { private final ImmutableSet parameters; private final ImmutableSet locals; private boolean sideEffectSeen = false; @@ -520,7 +522,7 @@ private boolean hasNonLocalSideEffect(Node n) { sideEffect = true; } } else if (type == Token.CALL) { - sideEffect = NodeUtil.functionCallHasSideEffects(n); + sideEffect = astAnalyzer.functionCallHasSideEffects(n); } else if (type == Token.NEW) { sideEffect = NodeUtil.constructorCallHasSideEffects(n); } else if (type == Token.DELPROP) { diff --git a/src/com/google/javascript/jscomp/FunctionInjector.java b/src/com/google/javascript/jscomp/FunctionInjector.java index 6dd5ee9d20d..3b094a32a83 100644 --- a/src/com/google/javascript/jscomp/FunctionInjector.java +++ b/src/com/google/javascript/jscomp/FunctionInjector.java @@ -57,51 +57,96 @@ public String get() { return String.valueOf(nextId++); } }; - - public enum Decomposition { - DISABLED, - ENABLED, - // TODD(b/124253050): consider removing this option. - ENABLED_WITHOUT_METHOD_CALL_DECOMPOSING; + private final FunctionArgumentInjector functionArgumentInjector; + + private FunctionInjector(Builder builder) { + this.compiler = checkNotNull(builder.compiler); + this.safeNameIdSupplier = checkNotNull(builder.safeNameIdSupplier); + this.assumeStrictThis = builder.assumeStrictThis; + this.assumeMinimumCapture = builder.assumeMinimumCapture; + this.allowDecomposition = builder.allowDecomposition; + this.allowMethodCallDecomposing = builder.allowMethodCallDecomposing; + this.functionArgumentInjector = checkNotNull(builder.functionArgumentInjector); + checkState( + !this.allowMethodCallDecomposing || this.allowDecomposition, + "Cannot allow method call decomposition when decomposition in general is not allowed."); } - /** - * @param decomposition Whether an effort should be made to break down expressions into simpler - * expressions to allow functions to be injected where they would otherwise be disallowed. - */ - public FunctionInjector( - AbstractCompiler compiler, - Supplier safeNameIdSupplier, - Decomposition decomposition, - boolean assumeStrictThis, - boolean assumeMinimumCapture) { - checkNotNull(compiler); - checkNotNull(safeNameIdSupplier); - this.compiler = compiler; - this.safeNameIdSupplier = safeNameIdSupplier; - this.assumeStrictThis = assumeStrictThis; - this.assumeMinimumCapture = assumeMinimumCapture; - this.allowDecomposition = !decomposition.equals(Decomposition.DISABLED); - this.allowMethodCallDecomposing = decomposition.equals(Decomposition.ENABLED); - } + static class Builder { - /** - * @param allowDecomposition Whether an effort should be made to break down expressions into - * simpler expressions to allow functions to be injected where they would otherwise be - * disallowed. - */ - public FunctionInjector( - AbstractCompiler compiler, - Supplier safeNameIdSupplier, - boolean allowDecomposition, - boolean assumeStrictThis, - boolean assumeMinimumCapture) { - this( - compiler, - safeNameIdSupplier, - allowDecomposition ? Decomposition.ENABLED : Decomposition.DISABLED, - assumeStrictThis, - assumeMinimumCapture); + private final AbstractCompiler compiler; + private Supplier safeNameIdSupplier = null; + private boolean assumeStrictThis = true; + private boolean assumeMinimumCapture = true; + private boolean allowDecomposition = true; + private boolean allowMethodCallDecomposing = true; + private FunctionArgumentInjector functionArgumentInjector = null; + + Builder(AbstractCompiler compiler) { + this.compiler = checkNotNull(compiler); + } + + /** + * Provide the name supplier to use for injection. + * + *

If this method is not called, {@code compiler.getUniqueNameIdSupplier()} will be used. + */ + Builder safeNameIdSupplier(Supplier safeNameIdSupplier) { + this.safeNameIdSupplier = checkNotNull(safeNameIdSupplier); + return this; + } + + /** + * Allow decomposition of expressions. + * + *

Default is {@code true}. + */ + Builder allowDecomposition(boolean allowDecomposition) { + this.allowDecomposition = allowDecomposition; + return this; + } + + /** + * Allow decomposition of method calls. + * + *

Default is {@code true}. May be disabled independently of decomposition in general. It's + * invalid to enable this when allowDecomposition is disabled. + */ + Builder allowMethodCallDecomposing(boolean allowMethodCallDecomposing) { + this.allowMethodCallDecomposing = allowMethodCallDecomposing; + return this; + } + + Builder assumeStrictThis(boolean assumeStrictThis) { + this.assumeStrictThis = assumeStrictThis; + return this; + } + + Builder assumeMinimumCapture(boolean assumeMinimumCapture) { + this.assumeMinimumCapture = assumeMinimumCapture; + return this; + } + + /** + * Specify the {@code FunctionArgumentInjector} to be used. + * + *

Default is for the builder to create this. This method exists for testing purposes. + */ + public Builder functionArgumentInjector(FunctionArgumentInjector functionArgumentInjector) { + this.functionArgumentInjector = checkNotNull(functionArgumentInjector); + return this; + } + + public FunctionInjector build() { + if (safeNameIdSupplier == null) { + safeNameIdSupplier = compiler.getUniqueNameIdSupplier(); + } + if (functionArgumentInjector == null) { + functionArgumentInjector = + new FunctionArgumentInjector(checkNotNull(compiler.getAstAnalyzer())); + } + return new FunctionInjector(this); + } } /** The type of inlining to perform. */ @@ -336,7 +381,7 @@ private Node inlineReturnValue(Reference ref, Node fnNode) { // Create an argName -> expression map, checking for side effects. Map argMap = - FunctionArgumentInjector.getFunctionCallParameterMap( + functionArgumentInjector.getFunctionCallParameterMap( fnNode, callNode, this.safeNameIdSupplier); Node newExpression; @@ -349,8 +394,7 @@ private Node inlineReturnValue(Reference ref, Node fnNode) { // Clone the return node first. Node safeReturnNode = returnNode.cloneTree(); - Node inlineResult = FunctionArgumentInjector.inject( - null, safeReturnNode, null, argMap); + Node inlineResult = functionArgumentInjector.inject(null, safeReturnNode, null, argMap); checkArgument(safeReturnNode == inlineResult); newExpression = safeReturnNode.removeFirstChild(); NodeUtil.markNewScopesChanged(newExpression, compiler); @@ -757,13 +801,13 @@ public boolean apply(Node n) { // additional VAR declarations because aliasing is needed. if (forbidTemps) { ImmutableMap args = - FunctionArgumentInjector.getFunctionCallParameterMap( + functionArgumentInjector.getFunctionCallParameterMap( fnNode, ref.callNode, this.safeNameIdSupplier); boolean hasArgs = !args.isEmpty(); if (hasArgs) { // Limit the inlining Set allNamesToAlias = new HashSet<>(namesToAlias); - FunctionArgumentInjector.maybeAddTempsForCallArguments( + functionArgumentInjector.maybeAddTempsForCallArguments( compiler, fnNode, args, allNamesToAlias, compiler.getCodingConvention()); if (!allNamesToAlias.isEmpty()) { return false; @@ -813,13 +857,13 @@ private CanInlineResult canInlineReferenceDirectly( } ImmutableMap args = - FunctionArgumentInjector.getFunctionCallParameterMap( + functionArgumentInjector.getFunctionCallParameterMap( fnNode, callNode, this.throwawayNameSupplier); boolean hasArgs = !args.isEmpty(); if (hasArgs) { // Limit the inlining Set allNamesToAlias = new HashSet<>(namesToAlias); - FunctionArgumentInjector.maybeAddTempsForCallArguments( + functionArgumentInjector.maybeAddTempsForCallArguments( compiler, fnNode, args, allNamesToAlias, compiler.getCodingConvention()); if (!allNamesToAlias.isEmpty()) { return CanInlineResult.NO; diff --git a/src/com/google/javascript/jscomp/FunctionToBlockMutator.java b/src/com/google/javascript/jscomp/FunctionToBlockMutator.java index 893929f68de..b434467f532 100644 --- a/src/com/google/javascript/jscomp/FunctionToBlockMutator.java +++ b/src/com/google/javascript/jscomp/FunctionToBlockMutator.java @@ -45,11 +45,13 @@ class FunctionToBlockMutator { private final AbstractCompiler compiler; private final Supplier safeNameIdSupplier; + private final FunctionArgumentInjector functionArgumentInjector; FunctionToBlockMutator( AbstractCompiler compiler, Supplier safeNameIdSupplier) { - this.compiler = compiler; + this.compiler = checkNotNull(compiler); this.safeNameIdSupplier = safeNameIdSupplier; + this.functionArgumentInjector = new FunctionArgumentInjector(compiler.getAstAnalyzer()); } /** @@ -139,14 +141,13 @@ private Node mutateInternal( } // TODO(johnlenz): Mark NAME nodes constant for parameters that are not modified. - Set namesToAlias = - FunctionArgumentInjector.findModifiedParameters(newFnNode); + Set namesToAlias = functionArgumentInjector.findModifiedParameters(newFnNode); ImmutableMap args = - FunctionArgumentInjector.getFunctionCallParameterMap( + functionArgumentInjector.getFunctionCallParameterMap( newFnNode, callNode, this.safeNameIdSupplier); boolean hasArgs = !args.isEmpty(); if (hasArgs) { - FunctionArgumentInjector.maybeAddTempsForCallArguments( + functionArgumentInjector.maybeAddTempsForCallArguments( compiler, newFnNode, args, namesToAlias, compiler.getCodingConvention()); } @@ -312,8 +313,7 @@ private Node aliasAndInlineArguments( if (namesToAlias == null || namesToAlias.isEmpty()) { // There are no names to alias, just inline the arguments directly. - Node result = FunctionArgumentInjector.inject( - compiler, fnTemplateRoot, null, argMap); + Node result = functionArgumentInjector.inject(compiler, fnTemplateRoot, null, argMap); checkState(result == fnTemplateRoot); return result; } else { @@ -362,8 +362,7 @@ private Node aliasAndInlineArguments( } // Inline the arguments. - Node result = FunctionArgumentInjector.inject( - compiler, fnTemplateRoot, null, newArgMap); + Node result = functionArgumentInjector.inject(compiler, fnTemplateRoot, null, newArgMap); checkState(result == fnTemplateRoot); // Now that the names have been replaced, add the new aliases for diff --git a/src/com/google/javascript/jscomp/InlineFunctions.java b/src/com/google/javascript/jscomp/InlineFunctions.java index 614ee3bb338..3f974ad0332 100644 --- a/src/com/google/javascript/jscomp/InlineFunctions.java +++ b/src/com/google/javascript/jscomp/InlineFunctions.java @@ -68,6 +68,7 @@ class InlineFunctions implements CompilerPass { private final AbstractCompiler compiler; private final FunctionInjector injector; + private final FunctionArgumentInjector functionArgumentInjector; private final Reach reach; private final boolean assumeMinimumCapture; @@ -101,13 +102,14 @@ class InlineFunctions implements CompilerPass { // the function itself is removed. The function inliner need to be made // aware of these new calls in order to enble it. + this.functionArgumentInjector = new FunctionArgumentInjector(compiler.getAstAnalyzer()); this.injector = - new FunctionInjector( - compiler, - safeNameIdSupplier, - FunctionInjector.Decomposition.ENABLED_WITHOUT_METHOD_CALL_DECOMPOSING, - assumeStrictThis, - assumeMinimumCapture); + new FunctionInjector.Builder(compiler) + .safeNameIdSupplier(safeNameIdSupplier) + .assumeStrictThis(assumeStrictThis) + .assumeMinimumCapture(assumeMinimumCapture) + .functionArgumentInjector(this.functionArgumentInjector) + .build(); } FunctionState getOrCreateFunctionState(String fnName) { @@ -311,7 +313,7 @@ void maybeAddFunction(Function fn, JSModule module) { if (functionState.canInline()) { functionState.setModule(module); - Set namesToAlias = FunctionArgumentInjector.findModifiedParameters(fnNode); + Set namesToAlias = functionArgumentInjector.findModifiedParameters(fnNode); if (!namesToAlias.isEmpty()) { functionState.inlineDirectly(false); functionState.setNamesToAlias(namesToAlias); diff --git a/src/com/google/javascript/jscomp/J2clPass.java b/src/com/google/javascript/jscomp/J2clPass.java index 8cdf5ebb707..892afbee02a 100644 --- a/src/com/google/javascript/jscomp/J2clPass.java +++ b/src/com/google/javascript/jscomp/J2clPass.java @@ -108,7 +108,12 @@ private ClassStaticFunctionsInliner( this.fnNamesToInline = fnNamesToInline; this.inliningMode = inliningMode; - this.injector = new FunctionInjector(compiler, safeNameIdSupplier, true, true, true); + this.injector = + new FunctionInjector.Builder(compiler) + .safeNameIdSupplier(safeNameIdSupplier) + .assumeStrictThis(true) + .assumeMinimumCapture(true) + .build(); this.injector.setKnownConstants(fnNamesToInline); } diff --git a/src/com/google/javascript/jscomp/J2clPropertyInlinerPass.java b/src/com/google/javascript/jscomp/J2clPropertyInlinerPass.java index 65c3386b8f9..9786050fa50 100644 --- a/src/com/google/javascript/jscomp/J2clPropertyInlinerPass.java +++ b/src/com/google/javascript/jscomp/J2clPropertyInlinerPass.java @@ -381,8 +381,10 @@ public void visit(NodeTraversal t, Node n, Node parent) { J2clProperty prop = propertiesByName.get(accessName); if (prop != null && prop.isSafeToInline) { FunctionInjector injector = - new FunctionInjector( - compiler, compiler.getUniqueNameIdSupplier(), true, true, true); + new FunctionInjector.Builder(compiler) + .assumeStrictThis(true) + .assumeMinimumCapture(true) + .build(); Node inlinedCall = injector.inline( new Reference(n, t.getScope(), t.getModule(), InliningMode.DIRECT), @@ -400,8 +402,10 @@ public void visit(NodeTraversal t, Node n, Node parent) { J2clProperty prop = propertiesByName.get(accessName); if (prop != null && prop.isSafeToInline) { FunctionInjector injector = - new FunctionInjector( - compiler, compiler.getUniqueNameIdSupplier(), true, true, true); + new FunctionInjector.Builder(compiler) + .assumeStrictThis(true) + .assumeMinimumCapture(true) + .build(); assignmentValue.detach(); Node functionCall = IR.call(IR.empty(), assignmentValue); parent.replaceChild(n, functionCall); diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index c69f7603902..85f906951c5 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -1346,15 +1346,6 @@ static boolean constructorCallHasSideEffects(Node callNode) { private static final ImmutableSet STRING_REGEXP_METHODS = ImmutableSet.of("match", "replace", "search", "split"); - /** - * Returns true if calls to this function have side effects. - * - * @param callNode - function call node - */ - static boolean functionCallHasSideEffects(Node callNode) { - return functionCallHasSideEffects(callNode, null); - } - /** * Returns true if calls to this function have side effects. * diff --git a/test/com/google/javascript/jscomp/FunctionArgumentInjectorTest.java b/test/com/google/javascript/jscomp/FunctionArgumentInjectorTest.java index 4940937272c..34ab79d5caa 100644 --- a/test/com/google/javascript/jscomp/FunctionArgumentInjectorTest.java +++ b/test/com/google/javascript/jscomp/FunctionArgumentInjectorTest.java @@ -18,10 +18,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.truth.Truth.assertThat; import static com.google.javascript.jscomp.CompilerOptions.LanguageMode.ECMASCRIPT_2017; -import static com.google.javascript.jscomp.FunctionArgumentInjector.findModifiedParameters; -import static com.google.javascript.jscomp.FunctionArgumentInjector.getFunctionCallParameterMap; -import static com.google.javascript.jscomp.FunctionArgumentInjector.inject; -import static com.google.javascript.jscomp.FunctionArgumentInjector.maybeAddTempsForCallArguments; import static com.google.javascript.jscomp.NodeUtil.getFunctionBody; import static com.google.javascript.rhino.testing.NodeSubject.assertNode; @@ -31,6 +27,7 @@ import com.google.javascript.rhino.Node; import java.util.HashSet; import java.util.Set; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -45,11 +42,23 @@ public final class FunctionArgumentInjectorTest { private static final ImmutableSet EMPTY_STRING_SET = ImmutableSet.of(); + private Compiler compiler; + private FunctionArgumentInjector functionArgumentInjector; + + @Before + public void setUp() { + compiler = new Compiler(); + CompilerOptions options = new CompilerOptions(); + options.setLanguageIn(ECMASCRIPT_2017); + + compiler.initOptions(options); + functionArgumentInjector = new FunctionArgumentInjector(compiler.getAstAnalyzer()); + } + @Test public void testInject0() { - Compiler compiler = getCompiler(); Node result = - inject( + functionArgumentInjector.inject( compiler, getFunctionBody(parseFunction("function f(x) { alert(x); }")), null, @@ -59,9 +68,8 @@ public void testInject0() { @Test public void testInject1() { - Compiler compiler = getCompiler(); Node result = - inject( + functionArgumentInjector.inject( compiler, getFunctionBody(parseFunction("function f() { alert(this); }")), null, @@ -73,72 +81,94 @@ public void testInject1() { @Test public void testFindModifiedParameters0() { - assertThat(findModifiedParameters(parseFunction("function f(a){ return a; }"))).isEmpty(); + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a){ return a; }"))) + .isEmpty(); } @Test public void testFindModifiedParameters1() { - assertThat(findModifiedParameters(parseFunction("function f(a){ return a==0; }"))).isEmpty(); + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a){ return a==0; }"))) + .isEmpty(); } @Test public void testFindModifiedParameters2() { - assertThat(findModifiedParameters(parseFunction("function f(a){ b=a }"))).isEmpty(); + assertThat( + functionArgumentInjector.findModifiedParameters(parseFunction("function f(a){ b=a }"))) + .isEmpty(); } @Test public void testFindModifiedParameters3() { - assertThat(findModifiedParameters(parseFunction("function f(a){ a=0 }"))).containsExactly("a"); + assertThat( + functionArgumentInjector.findModifiedParameters(parseFunction("function f(a){ a=0 }"))) + .containsExactly("a"); } @Test public void testFindModifiedParameters4() { - assertThat(findModifiedParameters(parseFunction("function f(a,b){ a=0;b=0 }"))) + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a,b){ a=0;b=0 }"))) .containsExactly("a", "b"); } @Test public void testFindModifiedParameters5() { - assertThat(findModifiedParameters(parseFunction("function f(a,b){ a; if (a) b=0 }"))) + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a,b){ a; if (a) b=0 }"))) .containsExactly("b"); } @Test public void testFindModifiedParameters6() { - assertThat(findModifiedParameters(parseFunction("function f(a,b){ function f(){ a;b; } }"))) + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a,b){ function f(){ a;b; } }"))) .containsExactly("a", "b"); } @Test public void testFindModifiedParameters7() { - assertThat(findModifiedParameters(parseFunction("function f(a,b){ a; function f(){ b; } }"))) + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a,b){ a; function f(){ b; } }"))) .containsExactly("b"); } @Test public void testFindModifiedParameters8() { assertThat( - findModifiedParameters( + functionArgumentInjector.findModifiedParameters( parseFunction("function f(a,b){ a; function f(){ function g() { b; } } }"))) .containsExactly("b"); } @Test public void testFindModifiedParameters9() { - assertThat(findModifiedParameters(parseFunction("function f(a,b){ (function(){ a;b; }) }"))) + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a,b){ (function(){ a;b; }) }"))) .containsExactly("a", "b"); } @Test public void testFindModifiedParameters10() { - assertThat(findModifiedParameters(parseFunction("function f(a,b){ a; (function (){ b; }) }"))) + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a,b){ a; (function (){ b; }) }"))) .containsExactly("b"); } @Test public void testFindModifiedParameters11() { assertThat( - findModifiedParameters( + functionArgumentInjector.findModifiedParameters( parseFunction("function f(a,b){ a; (function(){ (function () { b; }) }) }"))) .containsExactly("b"); } @@ -146,18 +176,24 @@ public void testFindModifiedParameters11() { @Test public void testFindModifiedParameters12() { assertThat( - findModifiedParameters(parseFunction("function f(a){ { let a = 1; } }"))).isEmpty(); + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a){ { let a = 1; } }"))) + .isEmpty(); } @Test public void testFindModifiedParameters13() { assertThat( - findModifiedParameters(parseFunction("function f(a){ { const a = 1; } }"))).isEmpty(); + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a){ { const a = 1; } }"))) + .isEmpty(); } @Test public void testFindModifiedParameters14() { - assertThat(findModifiedParameters(parseFunction("function f(a){ for (a in []) {} }"))) + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a){ for (a in []) {} }"))) .containsExactly("a"); } @@ -166,25 +202,33 @@ public void testFindModifiedParameters14() { // result in incorrect output. @Test public void testFindModifiedParameters15() { - assertThat(findModifiedParameters(parseFunction("function f(a){ for (const a in []) {} }"))) + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a){ for (const a in []) {} }"))) .containsExactly("a"); } @Test public void testFindModifiedParameters16() { - assertThat(findModifiedParameters(parseFunction("function f(a){ for (a of []) {} }"))) + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a){ for (a of []) {} }"))) .containsExactly("a"); } @Test public void testFindModifiedParameters17() { - assertThat(findModifiedParameters(parseFunction("function f(a){ [a] = [2]; }"))) + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a){ [a] = [2]; }"))) .containsExactly("a"); } @Test public void testFindModifiedParameters18() { - assertThat(findModifiedParameters(parseFunction("function f(a){ var [a] = [2]; }"))) + assertThat( + functionArgumentInjector.findModifiedParameters( + parseFunction("function f(a){ var [a] = [2]; }"))) .containsExactly("a"); } @@ -570,7 +614,8 @@ private void assertArgMapHasKeys(String code, String fnName, ImmutableSet actualMap = getFunctionCallParameterMap(fn, call, getNameSupplier()); + ImmutableMap actualMap = + functionArgumentInjector.getFunctionCallParameterMap(fn, call, getNameSupplier()); assertThat(actualMap.keySet()).isEqualTo(expectedKeys); } @@ -581,11 +626,12 @@ private void testNeededTemps(String code, String fnName, ImmutableSet ex Node call = findCall(n, fnName); assertThat(call).isNotNull(); ImmutableMap args = - ImmutableMap.copyOf(getFunctionCallParameterMap(fn, call, getNameSupplier())); + ImmutableMap.copyOf( + functionArgumentInjector.getFunctionCallParameterMap(fn, call, getNameSupplier())); Set actualTemps = new HashSet<>(); - maybeAddTempsForCallArguments( - getCompiler(), fn, args, actualTemps, new ClosureCodingConvention()); + functionArgumentInjector.maybeAddTempsForCallArguments( + compiler, fn, args, actualTemps, new ClosureCodingConvention()); assertThat(actualTemps).isEqualTo(expectedTemps); } @@ -645,23 +691,14 @@ private static Node findFunction(Node n, String name) { return null; } - private static Node parseFunction(String js) { + private Node parseFunction(String js) { return parse(js).getFirstChild(); } - private static Node parse(String js) { - Compiler compiler = getCompiler(); + private Node parse(String js) { Node n = compiler.parseTestCode(js); assertThat(compiler.getErrors()).isEmpty(); return n; } - private static Compiler getCompiler() { - Compiler compiler = new Compiler(); - CompilerOptions options = new CompilerOptions(); - options.setLanguageIn(ECMASCRIPT_2017); - - compiler.initOptions(options); - return compiler; - } } diff --git a/test/com/google/javascript/jscomp/FunctionInjectorTest.java b/test/com/google/javascript/jscomp/FunctionInjectorTest.java index a224bd9de12..17aad4949c0 100644 --- a/test/com/google/javascript/jscomp/FunctionInjectorTest.java +++ b/test/com/google/javascript/jscomp/FunctionInjectorTest.java @@ -1789,15 +1789,21 @@ public void helperCanInlineReferenceToFunction( final InliningMode mode) { final Compiler compiler = new Compiler(); compiler.initOptions(new CompilerOptions()); - final FunctionInjector injector = new FunctionInjector( - compiler, compiler.getUniqueNameIdSupplier(), allowDecomposition, - assumeStrictThis, - assumeMinimumCapture); + final FunctionArgumentInjector functionArgumentInjector = + new FunctionArgumentInjector(compiler.getAstAnalyzer()); + final FunctionInjector injector = + new FunctionInjector.Builder(compiler) + .allowDecomposition(allowDecomposition) + .allowMethodCallDecomposing(allowDecomposition) + .assumeStrictThis(assumeStrictThis) + .assumeMinimumCapture(assumeMinimumCapture) + .functionArgumentInjector(functionArgumentInjector) + .build(); final Node tree = parse(compiler, code); final Node fnNode = findFunction(tree, fnName); final ImmutableSet unsafe = - ImmutableSet.copyOf(FunctionArgumentInjector.findModifiedParameters(fnNode)); + ImmutableSet.copyOf(functionArgumentInjector.findModifiedParameters(fnNode)); // can-inline tester Method tester = @@ -1846,13 +1852,16 @@ public void helperInlineReferenceToFunction( compiler.init(externsInputs, ImmutableList.of( SourceFile.fromCode("code", code)), options); + final FunctionArgumentInjector functionArgumentInjector = + new FunctionArgumentInjector(compiler.getAstAnalyzer()); final FunctionInjector injector = - new FunctionInjector( - compiler, - compiler.getUniqueNameIdSupplier(), - allowDecomposition, - assumeStrictThis, - assumeMinimumCapture); + new FunctionInjector.Builder(compiler) + .allowDecomposition(allowDecomposition) + .allowMethodCallDecomposing(allowDecomposition) + .assumeStrictThis(assumeStrictThis) + .assumeMinimumCapture(assumeMinimumCapture) + .functionArgumentInjector(functionArgumentInjector) + .build(); Node parseRoot = compiler.parseInputs(); Node externsRoot = parseRoot.getFirstChild(); @@ -1873,7 +1882,7 @@ public void helperInlineReferenceToFunction( final Node fnNode = findFunction(tree, fnName); assertThat(fnNode).isNotNull(); final ImmutableSet unsafe = - ImmutableSet.copyOf(FunctionArgumentInjector.findModifiedParameters(fnNode)); + ImmutableSet.copyOf(functionArgumentInjector.findModifiedParameters(fnNode)); assertThat(fnNode).isNotNull(); // inline tester @@ -1935,12 +1944,12 @@ public boolean doesFunctionMeetMinimumRequirements(final String code, final Stri final Compiler compiler = new Compiler(); compiler.initOptions(new CompilerOptions()); final FunctionInjector injector = - new FunctionInjector( - compiler, - compiler.getUniqueNameIdSupplier(), - allowDecomposition, - assumeStrictThis, - assumeMinimumCapture); + new FunctionInjector.Builder(compiler) + .allowDecomposition(allowDecomposition) + .allowMethodCallDecomposing(allowDecomposition) + .assumeStrictThis(assumeStrictThis) + .assumeMinimumCapture(assumeMinimumCapture) + .build(); final Node tree = parse(compiler, code); final Node fnNode = findFunction(tree, fnName); diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java index efb72672ad9..01f0c3409c3 100644 --- a/test/com/google/javascript/jscomp/NodeUtilTest.java +++ b/test/com/google/javascript/jscomp/NodeUtilTest.java @@ -1908,7 +1908,7 @@ public void testCallSideEffects() { // Parens force interpretation as an expression. Node newXDotMethodCall = helper.parseFirst(CALL, "(new x().method());"); Compiler compiler = helper.getCompiler(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall)).isTrue(); + assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isTrue(); Node newExpr = newXDotMethodCall.getFirstFirstChild(); checkState(newExpr.isNew()); @@ -1921,7 +1921,7 @@ public void testCallSideEffects() { newXDotMethodCall.setSideEffectFlags(flags); assertThat(NodeUtil.evaluatesToLocalValue(newXDotMethodCall)).isTrue(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall)).isFalse(); + assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isFalse(); assertThat(NodeUtil.mayHaveSideEffects(newXDotMethodCall, compiler)).isFalse(); // Modifies this, local result @@ -1932,7 +1932,7 @@ public void testCallSideEffects() { newXDotMethodCall.setSideEffectFlags(flags); assertThat(NodeUtil.evaluatesToLocalValue(newXDotMethodCall)).isTrue(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall)).isFalse(); + assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isFalse(); assertThat(NodeUtil.mayHaveSideEffects(newXDotMethodCall, compiler)).isFalse(); // Modifies this, non-local result @@ -1944,7 +1944,7 @@ public void testCallSideEffects() { newXDotMethodCall.setSideEffectFlags(flags); assertThat(NodeUtil.evaluatesToLocalValue(newXDotMethodCall)).isFalse(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall)).isFalse(); + assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isFalse(); assertThat(NodeUtil.mayHaveSideEffects(newXDotMethodCall, compiler)).isFalse(); // No modifications, non-local result @@ -1955,7 +1955,7 @@ public void testCallSideEffects() { newXDotMethodCall.setSideEffectFlags(flags); assertThat(NodeUtil.evaluatesToLocalValue(newXDotMethodCall)).isFalse(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall)).isFalse(); + assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isFalse(); assertThat(NodeUtil.mayHaveSideEffects(newXDotMethodCall, compiler)).isFalse(); // The new modifies global state, no side-effect call, non-local result @@ -1967,7 +1967,7 @@ public void testCallSideEffects() { newXDotMethodCall.setSideEffectFlags(flags); assertThat(NodeUtil.evaluatesToLocalValue(newXDotMethodCall)).isTrue(); - assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall)).isFalse(); + assertThat(NodeUtil.functionCallHasSideEffects(newXDotMethodCall, compiler)).isFalse(); assertThat(NodeUtil.mayHaveSideEffects(newXDotMethodCall, compiler)).isTrue(); }