Skip to content

Commit

Permalink
Remove FlowScope#findUniqueRefinedSlot.
Browse files Browse the repository at this point in the history 
This method exists solely to support short-circuiting AND and OR.  As an alternative, I've added a separate wrapper class around FlowScope, used by just the method that care about this, that adds names of inferred variables to a set, allowing us to remove the brittle implementation-dependent behavior from FlowScope.  This should open the way for massively simplifying the internals of LinkedFlowScope.  See github.com//issues/2904.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=194608273
  • Loading branch information
@shicks @lauraharker
shicks authored and lauraharker committed Apr 30, 2018
1 parent 40fe067 commit 39dc51b
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 80 deletions.
35 changes: 0 additions & 35 deletions src/com/google/javascript/jscomp/LinkedFlowScope.java
View file
Expand Up @@ -234,44 +234,9 @@ public FlowScope createChildFlowScope(StaticTypedScope<JSType> scope) {
return new LinkedFlowScope(this, typedScope);
}

/**
* Iterate through all the linked flow scopes before this one.
* If there's one and only one slot defined between this scope
* and the blind scope, return it.
*/
@Override
public StaticTypedSlot<JSType> findUniqueRefinedSlot(FlowScope blindScope) {
LinkedFlowSlot result = null;

for (LinkedFlowScope currentScope = this;
currentScope != blindScope;
currentScope = currentScope.parent) {
LinkedFlowSlot parentSlot = currentScope.parent != null ? currentScope.parent.lastSlot : null;
for (LinkedFlowSlot currentSlot = currentScope.lastSlot;
currentSlot != null && currentSlot != parentSlot;
currentSlot = currentSlot.parent) {
if (result == null) {
result = currentSlot;
} else if (!currentSlot.var.equals(result.var)) {
return null;
}
}
}

return result;
}

/**
* Remove flow scopes that add nothing to the flow.
*/
// NOTE(nicksantos): This function breaks findUniqueRefinedSlot, because
// findUniqueRefinedSlot assumes that this scope is a direct descendant
// of blindScope. This is not necessarily true if this scope has been
// optimize()d and blindScope has not. This should be fixed. For now,
// we only use optimize() where we know that we won't have to do
// a findUniqueRefinedSlot on it (i.e. between CFG nodes, while the
// latter is only used within a single node to backwards-infer the LHS
// of short circuiting AND and OR operators).
@Override
public LinkedFlowScope optimize() {
LinkedFlowScope current = this;
Expand Down
10 changes: 0 additions & 10 deletions src/com/google/javascript/jscomp/type/FlowScope.java
View file
Expand Up @@ -20,7 +20,6 @@
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.jstype.JSType;
import com.google.javascript.rhino.jstype.StaticTypedScope;
import com.google.javascript.rhino.jstype.StaticTypedSlot;

/**
* A symbol table for inferring types during data flow analysis.
Expand Down Expand Up @@ -70,15 +69,6 @@ void inferQualifiedSlot(Node node, String symbol, JSType bottomType,
*/
FlowScope optimize();

/**
* Tries to find a unique refined variable in the refined scope, up to the
* the blind scope.
* @param blindScope The scope before the refinement, i.e. some parent of the
* this scope or itself.
* @return The unique refined variable if found or null.
*/
StaticTypedSlot<JSType> findUniqueRefinedSlot(FlowScope blindScope);

/** Returns the underlying TypedScope. */
StaticTypedScope<JSType> getDeclarationScope();
}
117 changes: 103 additions & 14 deletions src/com/google/javascript/jscomp/type/SemanticReverseAbstractInterpreter.java
View file
Expand Up @@ -27,9 +27,12 @@
import com.google.javascript.rhino.jstype.JSTypeNative;
import com.google.javascript.rhino.jstype.JSTypeRegistry;
import com.google.javascript.rhino.jstype.ObjectType;
import com.google.javascript.rhino.jstype.StaticTypedScope;
import com.google.javascript.rhino.jstype.StaticTypedSlot;
import com.google.javascript.rhino.jstype.UnionType;
import com.google.javascript.rhino.jstype.Visitor;
import java.util.HashSet;
import java.util.Set;

/**
* A reverse abstract interpreter using the semantics of the JavaScript
Expand Down Expand Up @@ -344,27 +347,35 @@ private FlowScope caseAndOrNotShortCircuiting(Node left, Node right,

private FlowScope caseAndOrMaybeShortCircuiting(Node left, Node right,
FlowScope blindScope, boolean outcome) {
FlowScope leftScope = firstPreciserScopeKnowingConditionOutcome(
left, blindScope, !outcome);
StaticTypedSlot<JSType> leftVar = leftScope.findUniqueRefinedSlot(blindScope);
// Perform two separate refinements, one for if short-circuiting occurred, and one for if it did
// not. Because it's not clear whether short-circuiting occurred, we actually have to ignore
// both separate result flow scopes individually, but if they both refined the same slot, we
// can join the two refinements. TODO(sdh): look into simplifying this. If joining were
// more efficient, we should just be able to join the scopes unconditionally?
Set<String> refinements = new HashSet<>();
blindScope = new RefinementTrackingFlowScope(blindScope, refinements);
FlowScope leftScope = firstPreciserScopeKnowingConditionOutcome(left, blindScope, !outcome);
StaticTypedSlot<JSType> leftVar =
refinements.size() == 1 ? leftScope.getSlot(refinements.iterator().next()) : null;
if (leftVar == null) {
// If we did create a more precise scope, blindScope has a child and
// it is frozen. We can't just throw it away to return it. So we
// must create a child instead.
return blindScope == leftScope ?
blindScope : blindScope.createChildFlowScope();
}
FlowScope rightScope = firstPreciserScopeKnowingConditionOutcome(
left, blindScope, outcome);
rightScope = firstPreciserScopeKnowingConditionOutcome(
right, rightScope, !outcome);
StaticTypedSlot<JSType> rightVar = rightScope.findUniqueRefinedSlot(blindScope);
return unwrap(blindScope == leftScope ? blindScope : blindScope.createChildFlowScope());
}
refinements.clear();
// Note: re-wrap the scope, in case it was unwrapped by a nested call to this method.
FlowScope rightScope =
new RefinementTrackingFlowScope(
firstPreciserScopeKnowingConditionOutcome(left, blindScope, outcome), refinements);
rightScope = firstPreciserScopeKnowingConditionOutcome(right, rightScope, !outcome);
StaticTypedSlot<JSType> rightVar =
refinements.size() == 1 ? rightScope.getSlot(refinements.iterator().next()) : null;
if (rightVar == null || !leftVar.getName().equals(rightVar.getName())) {
return blindScope == rightScope ?
blindScope : blindScope.createChildFlowScope();
return unwrap(blindScope == rightScope ? blindScope : blindScope.createChildFlowScope());
}
JSType type = leftVar.getType().getLeastSupertype(rightVar.getType());
FlowScope informed = blindScope.createChildFlowScope();
FlowScope informed = unwrap(blindScope).createChildFlowScope();
informed.inferSlotType(leftVar.getName(), type);
return informed;
}
Expand Down Expand Up @@ -595,4 +606,82 @@ public JSType caseFunctionType(FunctionType type) {
return caseObjectType(type);
}
}

/** Unwraps any RefinementTrackingFlowScopes. */
private static FlowScope unwrap(FlowScope scope) {
while (scope instanceof RefinementTrackingFlowScope) {
scope = ((RefinementTrackingFlowScope) scope).delegate;
}
return scope;
}

/** A wrapper around FlowScope that keeps track of which vars were refined. */
private static class RefinementTrackingFlowScope implements FlowScope {
final FlowScope delegate;
final Set<String> refinements;

RefinementTrackingFlowScope(FlowScope delegate, Set<String> refinements) {
this.delegate = delegate;
this.refinements = refinements;
}

@Override
public FlowScope createChildFlowScope() {
return new RefinementTrackingFlowScope(delegate.createChildFlowScope(), refinements);
}

@Override
public FlowScope createChildFlowScope(StaticTypedScope<JSType> scope) {
return new RefinementTrackingFlowScope(delegate.createChildFlowScope(scope), refinements);
}

@Override
public void inferSlotType(String symbol, JSType type) {
refinements.add(symbol);
delegate.inferSlotType(symbol, type);
}

@Override
public void inferQualifiedSlot(Node node, String symbol, JSType bottomType,
JSType inferredType, boolean declare) {
refinements.add(symbol);
delegate.inferQualifiedSlot(node, symbol, bottomType, inferredType, declare);
}

@Override
public FlowScope optimize() {
FlowScope optimized = delegate.optimize();
return optimized != delegate ? new RefinementTrackingFlowScope(optimized, refinements) : this;
}

@Override
public StaticTypedScope<JSType> getDeclarationScope() {
return delegate.getDeclarationScope();
}

@Override
public Node getRootNode() {
return delegate.getRootNode();
}

@Override
public StaticTypedScope<JSType> getParentScope() {
throw new UnsupportedOperationException();
}

@Override
public StaticTypedSlot<JSType> getSlot(String name) {
return delegate.getSlot(name);
}

@Override
public StaticTypedSlot<JSType> getOwnSlot(String name) {
return delegate.getOwnSlot(name);
}

@Override
public JSType getTypeOfThis() {
return delegate.getTypeOfThis();
}
}
}
21 changes: 0 additions & 21 deletions test/com/google/javascript/jscomp/LinkedFlowScopeTest.java
View file
Expand Up @@ -247,27 +247,6 @@ private void verifyLongChains(FlowScope chainA, FlowScope chainB) {
assertScopesDiffer(chainB, joined);
}

public void testFindUniqueSlot() {
FlowScope childA = localEntry.createChildFlowScope();
childA.inferSlotType("localB", getNativeNumberType());

FlowScope childAB = childA.createChildFlowScope();
childAB.inferSlotType("localB", getNativeStringType());

FlowScope childABC = childAB.createChildFlowScope();
childABC.inferSlotType("localA", getNativeBooleanType());

assertNull(childABC.findUniqueRefinedSlot(childABC));
assertTypeEquals(getNativeBooleanType(), childABC.findUniqueRefinedSlot(childAB).getType());
assertNull(childABC.findUniqueRefinedSlot(childA));
assertNull(childABC.findUniqueRefinedSlot(localEntry));

assertTypeEquals(getNativeStringType(), childAB.findUniqueRefinedSlot(childA).getType());
assertTypeEquals(getNativeStringType(), childAB.findUniqueRefinedSlot(localEntry).getType());

assertTypeEquals(getNativeNumberType(), childA.findUniqueRefinedSlot(localEntry).getType());
}

public void testDiffer1() {
FlowScope childA = localEntry.createChildFlowScope();
childA.inferSlotType("localB", getNativeNumberType());
Expand Down
24 changes: 24 additions & 0 deletions test/com/google/javascript/jscomp/TypeCheckTest.java
View file
Expand Up @@ -19801,6 +19801,30 @@ public void testShadowedForwardReference() {
"required: number"));
}

public void testRefinedTypeInNestedShortCircuitingAndOr() {
// Ensure we don't have a strict property warning, and do get the right type.
// In particular, ensure we don't forget about tracking refinements in between the two.
testTypes(
lines(
"/** @constructor */ function B() {}",
"/** @constructor @extends {B} */ function C() {}",
"/** @return {string} */ C.prototype.foo = function() {};",
"/** @return {boolean} */ C.prototype.bar = function() {};",
"/** @constructor @extends {B} */ function D() {}",
"/** @return {number} */ D.prototype.foo = function() {};",
"/** @param {!B} arg",
" @return {null} */",
"function f(arg) {",
" if ((arg instanceof C && arg.bar()) || arg instanceof D) {",
" return arg.foo();",
" }",
" return null;",
"}"),
lines(
"inconsistent return type", //
"found : (number|string)",
"required: null"));
}

private void testTypes(String js) {
testTypes(js, (String) null);
Expand Down

0 comments on commit 39dc51b

Please sign in to comment.