Skip to content

Commit

Permalink
FunctionArgumentInjector should AstAnalyzer.functionCallHasSideEffects
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@brad4d @lauraharker
brad4d authored and lauraharker committed May 16, 2019
1 parent 3626911 commit 08f5740
Show file tree
Hide file tree
Showing 10 changed files with 278 additions and 185 deletions.
82 changes: 42 additions & 40 deletions src/com/google/javascript/jscomp/FunctionArgumentInjector.java
View file
Expand Up @@ -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;
}

/**
Expand All @@ -62,12 +64,11 @@ private FunctionArgumentInjector() {
* Nodes.
* @return The root node or its replacement.
*/
static Node inject(
AbstractCompiler compiler, Node node, Node parent, Map<String, Node> replacements) {
Node inject(AbstractCompiler compiler, Node node, Node parent, Map<String, Node> replacements) {
return inject(compiler, node, parent, replacements, /* replaceThis */ true);
}

private static Node inject(
private Node inject(
AbstractCompiler compiler,
Node node,
Node parent,
Expand Down Expand Up @@ -117,10 +118,8 @@ private static Node inject(
return node;
}

/**
* Get a mapping for function parameter names to call arguments.
*/
static ImmutableMap<String, Node> getFunctionCallParameterMap(
/** Get a mapping for function parameter names to call arguments. */
ImmutableMap<String, Node> getFunctionCallParameterMap(
final Node fnNode, Node callNode, Supplier<String> safeNameIdSupplier) {
checkNotNull(fnNode);
// Create an argName -> expression map
Expand Down Expand Up @@ -189,14 +188,18 @@ private static String getUniqueAnonymousParameterName(

/**
* Retrieve a set of names that can not be safely substituted in place.
* Example:
*
* <p>Example: <code><pre>
*
* function(a) {
* a = 0;
* }
* Inlining this without taking precautions would cause the call site value
* to be modified (bad).
* </pre></code>
*
* <p>Inlining this without taking precautions would cause the call site value to be modified
* (bad).
*/
static Set<String> findModifiedParameters(Node fnNode) {
Set<String> findModifiedParameters(Node fnNode) {
ImmutableSet<String> names = getFunctionParameterSet(fnNode);
Set<String> unsafeNames = new HashSet<>();
return findModifiedParameters(fnNode.getLastChild(), names, unsafeNames, false);
Expand Down Expand Up @@ -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<String, Node> argMap,
Expand Down Expand Up @@ -359,21 +362,20 @@ 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;
}
return mayHaveConditionalCode(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:
Expand All @@ -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.
* <p>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<String> findParametersReferencedAfterSideEffect(
private ImmutableSet<String> findParametersReferencedAfterSideEffect(
ImmutableSet<String> parameters, Node root) {

// TODO(johnlenz): Consider using scope for this.
Expand All @@ -419,24 +421,24 @@ private static ImmutableSet<String> 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).
* <p>Assumptions:
*
* <ul>
* <li>We assume parameters are not modified in the function body (that is checked separately).
* <li>There are no inner functions (also checked separately).
* </ul>
*
* 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.
* <p>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.
* <p>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<Node> {
private class ReferencedAfterSideEffect implements Visitor, Predicate<Node> {
private final ImmutableSet<String> parameters;
private final ImmutableSet<String> locals;
private boolean sideEffectSeen = false;
Expand Down Expand Up @@ -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) {
Expand Down
142 changes: 93 additions & 49 deletions src/com/google/javascript/jscomp/FunctionInjector.java
View file
Expand Up @@ -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<String> 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<String> safeNameIdSupplier,
boolean allowDecomposition,
boolean assumeStrictThis,
boolean assumeMinimumCapture) {
this(
compiler,
safeNameIdSupplier,
allowDecomposition ? Decomposition.ENABLED : Decomposition.DISABLED,
assumeStrictThis,
assumeMinimumCapture);
private final AbstractCompiler compiler;
private Supplier<String> 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.
*
* <p>If this method is not called, {@code compiler.getUniqueNameIdSupplier()} will be used.
*/
Builder safeNameIdSupplier(Supplier<String> safeNameIdSupplier) {
this.safeNameIdSupplier = checkNotNull(safeNameIdSupplier);
return this;
}

/**
* Allow decomposition of expressions.
*
* <p>Default is {@code true}.
*/
Builder allowDecomposition(boolean allowDecomposition) {
this.allowDecomposition = allowDecomposition;
return this;
}

/**
* Allow decomposition of method calls.
*
* <p>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.
*
* <p>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. */
Expand Down Expand Up @@ -336,7 +381,7 @@ private Node inlineReturnValue(Reference ref, Node fnNode) {

// Create an argName -> expression map, checking for side effects.
Map<String, Node> argMap =
FunctionArgumentInjector.getFunctionCallParameterMap(
functionArgumentInjector.getFunctionCallParameterMap(
fnNode, callNode, this.safeNameIdSupplier);

Node newExpression;
Expand All @@ -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);
Expand Down Expand Up @@ -757,13 +801,13 @@ public boolean apply(Node n) {
// additional VAR declarations because aliasing is needed.
if (forbidTemps) {
ImmutableMap<String, Node> args =
FunctionArgumentInjector.getFunctionCallParameterMap(
functionArgumentInjector.getFunctionCallParameterMap(
fnNode, ref.callNode, this.safeNameIdSupplier);
boolean hasArgs = !args.isEmpty();
if (hasArgs) {
// Limit the inlining
Set<String> allNamesToAlias = new HashSet<>(namesToAlias);
FunctionArgumentInjector.maybeAddTempsForCallArguments(
functionArgumentInjector.maybeAddTempsForCallArguments(
compiler, fnNode, args, allNamesToAlias, compiler.getCodingConvention());
if (!allNamesToAlias.isEmpty()) {
return false;
Expand Down Expand Up @@ -813,13 +857,13 @@ private CanInlineResult canInlineReferenceDirectly(
}

ImmutableMap<String, Node> args =
FunctionArgumentInjector.getFunctionCallParameterMap(
functionArgumentInjector.getFunctionCallParameterMap(
fnNode, callNode, this.throwawayNameSupplier);
boolean hasArgs = !args.isEmpty();
if (hasArgs) {
// Limit the inlining
Set<String> allNamesToAlias = new HashSet<>(namesToAlias);
FunctionArgumentInjector.maybeAddTempsForCallArguments(
functionArgumentInjector.maybeAddTempsForCallArguments(
compiler, fnNode, args, allNamesToAlias, compiler.getCodingConvention());
if (!allNamesToAlias.isEmpty()) {
return CanInlineResult.NO;
Expand Down

0 comments on commit 08f5740

Please sign in to comment.