Skip to content

Commit

Permalink
Adds support for TAGGED_TEMPLATELIT, SPREAD, REST, and FOR_AWAIT_OF t…
Browse files Browse the repository at this point in the history 
…o `PureFunctionIdentifier.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=239084640
  • Loading branch information
@nreid260 @tjgq
nreid260 authored and tjgq committed Mar 19, 2019
1 parent fbedb5f commit 592de23
Show file tree
Hide file tree
Showing 4 changed files with 219 additions and 25 deletions.
11 changes: 7 additions & 4 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -1158,7 +1158,7 @@ private static boolean checkForStateChangeHelper(
case LET:
case CONST:
case EXPORT:
// Variable declarations are side-effects in practically all contexts.
// Variable declarations are side-effects.
return true;

case OBJECTLIT:
Expand Down Expand Up @@ -1606,10 +1606,11 @@ static boolean nodeTypeMayHaveSideEffects(Node n, AbstractCompiler compiler) {
case THROW:
case AWAIT:
case FOR_IN: // assigns to a loop LHS
case FOR_OF: // assigns to a loop LHS
case FOR_AWAIT_OF: // assigns to a loop LHS
case FOR_OF: // assigns to a loop LHS, runs an iterator
case FOR_AWAIT_OF: // assigns to a loop LHS, runs an iterator, async operations.
return true;
case CALL:
case TAGGED_TEMPLATELIT:
return NodeUtil.functionCallHasSideEffects(n, compiler);
case NEW:
return NodeUtil.constructorCallHasSideEffects(n);
Expand All @@ -1620,8 +1621,10 @@ static boolean nodeTypeMayHaveSideEffects(Node n, AbstractCompiler compiler) {
case SPREAD:
return NodeUtil.iteratesImpureIterable(n);
default:
return false;
break;
}

return false;
}

static boolean allArgsUnescapedLocal(Node callOrNew) {
Expand Down
79 changes: 60 additions & 19 deletions src/com/google/javascript/jscomp/PureFunctionIdentifier.java
View file
Expand Up @@ -170,8 +170,7 @@ public void process(Node externsAst, Node srcAst) {
private static Iterable<Node> unwrapCallableExpression(Node exp) {
switch (exp.getToken()) {
case GETPROP:
String propName = exp.getLastChild().getString();
if (propName.equals("apply") || propName.equals("call")) {
if (isCallOrApply(exp.getParent())) {
return unwrapCallableExpression(exp.getFirstChild());
}
return ImmutableList.of(exp);
Expand Down Expand Up @@ -224,15 +223,15 @@ private Iterable<Node> getGoogCacheCallableExpression(Cache cacheCall) {
}

@Nullable
private List<AmbiguatedFunctionSummary> getSummariesForCallee(Node call) {
checkArgument(call.isCall() || call.isNew(), call);
private List<AmbiguatedFunctionSummary> getSummariesForCallee(Node invocation) {
checkArgument(NodeUtil.isInvocation(invocation), invocation);

Iterable<Node> expanded;
Cache cacheCall = compiler.getCodingConvention().describeCachingCall(call);
Cache cacheCall = compiler.getCodingConvention().describeCachingCall(invocation);
if (cacheCall != null) {
expanded = getGoogCacheCallableExpression(cacheCall);
} else {
expanded = unwrapCallableExpression(call.getFirstChild());
expanded = unwrapCallableExpression(invocation.getFirstChild());
}
if (expanded == null) {
return null;
Expand All @@ -243,7 +242,7 @@ private List<AmbiguatedFunctionSummary> getSummariesForCallee(Node call) {
// isExtern is false in the call to the constructor for the
// FunctionExpressionDefinition below because we know that
// getFunctionDefinitions() will only be called on the first
// child of a call and thus the function expression
// child of an invocation and thus the function expression
// definition will never be an extern.
results.addAll(summariesForAllNamesOfFunctionByNode.get(expression));
continue;
Expand Down Expand Up @@ -374,11 +373,11 @@ private void markPureFunctionCalls() {
flags.setThrows();
}

if (callNode.isCall()) {
if (isCallOrTaggedTemplateLit(callNode)) {
if (calleeSummary.mutatesThis()) {
// A FunctionInfo for "f" maps to both "f()" and "f.call()" nodes.
if (isCallOrApply(callNode)) {
flags.setMutatesArguments();
flags.setMutatesArguments(); // `this` is actually an argument.
} else {
flags.setMutatesThis();
}
Expand All @@ -392,7 +391,7 @@ private void markPureFunctionCalls() {
}

// Handle special cases (Math, RegExp)
if (callNode.isCall()) {
if (isCallOrTaggedTemplateLit(callNode)) {
if (!NodeUtil.functionCallHasSideEffects(callNode, compiler)) {
flags.clearSideEffectFlags();
}
Expand Down Expand Up @@ -458,7 +457,7 @@ public void visit(NodeTraversal traversal, Node node, Node parent) {
return;
}

if (NodeUtil.isCallOrNew(node)) {
if (NodeUtil.isInvocation(node)) {
allFunctionCalls.add(node);
}

Expand All @@ -481,6 +480,7 @@ public void updateSideEffectsForNode(
NodeTraversal traversal,
Node node,
Node enclosingFunction) {

switch (node.getToken()) {
case ASSIGN:
// e.g.
Expand All @@ -506,6 +506,9 @@ public void updateSideEffectsForNode(
RHS_IS_ALWAYS_LOCAL);
break;

case FOR_AWAIT_OF:
setSideEffectsForControlLoss(encloserSummary); // Control is lost while awaiting.
// Fall through.
case FOR_OF:
// e.g.
// for (const {prop1, prop2} of iterable) {...}
Expand All @@ -521,6 +524,7 @@ public void updateSideEffectsForNode(
// The RHS of a for-of must always be an iterable, making it a container, so we can't
// consider its contents to be local
RHS_IS_NEVER_LOCAL);
checkIteratesImpureIterable(node, encloserSummary);
break;

case FOR_IN:
Expand All @@ -539,6 +543,7 @@ public void updateSideEffectsForNode(

case CALL:
case NEW:
case TAGGED_TEMPLATELIT:
visitCall(encloserSummary, node);
break;

Expand Down Expand Up @@ -567,15 +572,21 @@ public void updateSideEffectsForNode(
}
break;

case YIELD: // 'yield' throws if the caller calls `.throw` on the generator object.
case AWAIT: // 'await' throws if the promise it's waiting on is rejected.
encloserSummary.setFunctionThrows();
case YIELD:
checkIteratesImpureIterable(node, encloserSummary); // `yield*` triggers iteration.
// 'yield' throws if the caller calls `.throw` on the generator object.
setSideEffectsForControlLoss(encloserSummary);
break;

case AWAIT:
// 'await' throws if the promise it's waiting on is rejected.
setSideEffectsForControlLoss(encloserSummary);
break;

case FOR_AWAIT_OF:
case REST:
case SPREAD:
break; // TODO(b/123649765): Actually check for related side-effects.
checkIteratesImpureIterable(node, encloserSummary);
break;

default:
if (NodeUtil.isCompoundAssignmentOp(node)) {
Expand All @@ -596,6 +607,32 @@ public void updateSideEffectsForNode(
}
}

/**
* Inspect {@code node} for impure iteration and assign the appropriate side-effects to {@code
* encloserSummary} if so.
*/
private void checkIteratesImpureIterable(Node node, AmbiguatedFunctionSummary encloserSummary) {
if (!NodeUtil.iteratesImpureIterable(node)) {
return;
}

// Treat the (possibly implicit) call to `iterator.next()` as having the same effects as any
// other unknown function call.
encloserSummary.setFunctionThrows();
encloserSummary.setMutatesGlobalState();

// The iterable may be stateful and a param.
encloserSummary.setMutatesArguments();
}

/**
* Assigns the set of side-effects associated with an arbitrary loss of control flow to {@code
* encloserSummary}.
*/
private void setSideEffectsForControlLoss(AmbiguatedFunctionSummary encloserSummary) {
encloserSummary.setFunctionThrows();
}

@Override
public void enterScope(NodeTraversal t) {
// Nothing to do.
Expand Down Expand Up @@ -744,6 +781,10 @@ private static boolean isCallOrApply(Node callSite) {
return NodeUtil.isFunctionObjectCall(callSite) || NodeUtil.isFunctionObjectApply(callSite);
}

private static boolean isCallOrTaggedTemplateLit(Node invocation) {
return invocation.isCall() || invocation.isTaggedTemplateLit();
}

/**
* This class stores all the information about a call site needed to propagate side effects from
* one instance of {@link AmbiguatedFunctionSummary} to another.
Expand All @@ -753,7 +794,7 @@ private static class CallSitePropagationInfo {

private CallSitePropagationInfo(
boolean allArgsUnescapedLocal, boolean calleeThisEqualsCallerThis, Token callType) {
checkArgument(callType == Token.CALL || callType == Token.NEW);
checkArgument(NodeUtil.isInvocation(new Node(callType)), callType);
this.allArgsUnescapedLocal = allArgsUnescapedLocal;
this.calleeThisEqualsCallerThis = calleeThisEqualsCallerThis;
this.callType = callType;
Expand Down Expand Up @@ -814,10 +855,10 @@ boolean propagate(AmbiguatedFunctionSummary callee, AmbiguatedFunctionSummary ca
}

static CallSitePropagationInfo computePropagationType(Node callSite) {
checkArgument(callSite.isCall() || callSite.isNew());
checkArgument(NodeUtil.isInvocation(callSite), callSite);

boolean thisIsOuterThis = false;
if (callSite.isCall()) {
if (isCallOrTaggedTemplateLit(callSite)) {
// Side effects only propagate via regular calls.
// Calling a constructor that modifies "this" has no side effects.
// Notice that we're using "mutatesThis" from the callee
Expand Down
1 change: 0 additions & 1 deletion test/com/google/javascript/jscomp/NodeUtilTest.java
View file
Expand Up @@ -741,7 +741,6 @@ public void testMayHaveSideEffects_undifferentiatedCases() {
assertSideEffect(true, "Math.random(seed);");
assertSideEffect(false, "[1, 1].foo;");


assertSideEffect(true, "export var x = 0;");
assertSideEffect(true, "export let x = 0;");
assertSideEffect(true, "export const x = 0;");
Expand Down

0 comments on commit 592de23

Please sign in to comment.