Skip to content

Commit

Permalink
Add support for type checking for/of.
Browse files Browse the repository at this point in the history 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=159473475
  • Loading branch information
@EatingW @blickly
EatingW authored and blickly committed Jun 19, 2017
1 parent 8208fe3 commit 90b5259
Show file tree
Hide file tree
Showing 4 changed files with 324 additions and 122 deletions.
218 changes: 138 additions & 80 deletions src/com/google/javascript/jscomp/NewTypeInference.java
View file
Expand Up @@ -150,6 +150,16 @@ final class NewTypeInference implements CompilerPass {
"JSC_NTI_FORIN_EXPECTS_STRING_KEY", "JSC_NTI_FORIN_EXPECTS_STRING_KEY",
"For/in creates string keys, but variable has declared type {1}."); "For/in creates string keys, but variable has declared type {1}.");


static final DiagnosticType FOROF_EXPECTS_ITERABLE =
DiagnosticType.warning(
"JSC_NTI_FOROF_EXPECTS_ITERABLE", "For/of expects an iterable, found type {0}.");

static final DiagnosticType MISTYPED_FOROF_ELEMENT_TYPE =
DiagnosticType.warning(
"JSC_NTI_MISTYPED_FOROF_ELEMENT_TYPE",
"Invalid type for for/of element.\n"
+ "{0}");

static final DiagnosticType CONST_REASSIGNED = static final DiagnosticType CONST_REASSIGNED =
DiagnosticType.warning( DiagnosticType.warning(
"JSC_NTI_CONST_REASSIGNED", "JSC_NTI_CONST_REASSIGNED",
Expand Down Expand Up @@ -349,20 +359,23 @@ final class NewTypeInference implements CompilerPass {
// TODO(dimvar): Check for which of these warnings it makes sense to keep // TODO(dimvar): Check for which of these warnings it makes sense to keep
// going after warning, eg, for NOT_UNIQUE_INSTANTIATION, we must instantiate // going after warning, eg, for NOT_UNIQUE_INSTANTIATION, we must instantiate
// to the join of the types. // to the join of the types.
static final DiagnosticGroup NEW_DIAGNOSTICS = new DiagnosticGroup( static final DiagnosticGroup NEW_DIAGNOSTICS =
ADDING_PROPERTY_TO_NON_OBJECT, new DiagnosticGroup(
ASSERT_FALSE, ADDING_PROPERTY_TO_NON_OBJECT,
BOTTOM_INDEX_TYPE, ASSERT_FALSE,
BOTTOM_PROP, BOTTOM_INDEX_TYPE,
CROSS_SCOPE_GOTCHA, BOTTOM_PROP,
FORIN_EXPECTS_OBJECT, CROSS_SCOPE_GOTCHA,
INCOMPATIBLE_STRICT_COMPARISON, FORIN_EXPECTS_OBJECT,
INVALID_INFERRED_RETURN_TYPE, FOROF_EXPECTS_ITERABLE,
INVALID_OPERAND_TYPE, MISTYPED_FOROF_ELEMENT_TYPE,
INVALID_THIS_TYPE_IN_BIND, INCOMPATIBLE_STRICT_COMPARISON,
NOT_UNIQUE_INSTANTIATION, INVALID_INFERRED_RETURN_TYPE,
PROPERTY_ACCESS_ON_NONOBJECT, INVALID_OPERAND_TYPE,
UNKNOWN_NAMESPACE_PROPERTY); INVALID_THIS_TYPE_IN_BIND,
NOT_UNIQUE_INSTANTIATION,
PROPERTY_ACCESS_ON_NONOBJECT,
UNKNOWN_NAMESPACE_PROPERTY);


public static class WarningReporter { public static class WarningReporter {
AbstractCompiler compiler; AbstractCompiler compiler;
Expand Down Expand Up @@ -745,6 +758,7 @@ private void buildWorksetHelper(
case WHILE: case WHILE:
case FOR: case FOR:
case FOR_IN: case FOR_IN:
case FOR_OF:
// Do the loop body first, then the loop follow. // Do the loop body first, then the loop follow.
// For DO loops, we do BODY-CONDT-CONDF-FOLLOW // For DO loops, we do BODY-CONDT-CONDF-FOLLOW
// Since CONDT is currently unused, this could be optimized. // Since CONDT is currently unused, this could be optimized.
Expand Down Expand Up @@ -928,9 +942,7 @@ private void analyzeFunctionBwd(
} else { } else {
// TODO(dimvar): look if the meet is needed // TODO(dimvar): look if the meet is needed
requiredType = JSType.meet(declType, inferredType); requiredType = JSType.meet(declType, inferredType);
if (requiredType.isBottom()) { requiredType = firstNonBottom(requiredType, UNKNOWN);
requiredType = UNKNOWN;
}
} }
inEnv = analyzeExprBwd(rhs, inEnv, requiredType).env; inEnv = analyzeExprBwd(rhs, inEnv, requiredType).env;
} }
Expand All @@ -952,9 +964,11 @@ private void analyzeFunctionBwd(
case DO: case DO:
case FOR: case FOR:
case FOR_IN: case FOR_IN:
case FOR_OF:
case IF: case IF:
case WHILE: case WHILE:
Node expr = n.isForIn() ? n.getFirstChild() : NodeUtil.getConditionExpression(n); Node expr =
(n.isForIn() || n.isForOf()) ? n.getFirstChild() : NodeUtil.getConditionExpression(n);
inEnv = analyzeExprBwd(expr, outEnv).env; inEnv = analyzeExprBwd(expr, outEnv).env;
break; break;
case THROW: case THROW:
Expand Down Expand Up @@ -1048,31 +1062,16 @@ n, RETURN_NONDECLARED_TYPE, errorMsgWithTypeDiff(declRetType, actualRetType)),
case DO: case DO:
case IF: case IF:
case FOR: case FOR:
case FOR_IN:
case WHILE: case WHILE:
if (n.isForIn()) {
Node obj = n.getSecondChild();
EnvTypePair pair = analyzeExprFwd(obj, inEnv, pickReqObjType(n));
pair = mayWarnAboutNullableReferenceAndTighten(n, pair.type, null, inEnv);
JSType objType = pair.type;
if (!objType.isSubtypeOf(TOP_OBJECT)) {
warnings.add(JSError.make(obj, FORIN_EXPECTS_OBJECT, objType.toString()));
} else if (objType.isStruct()) {
warnings.add(JSError.make(obj, IN_USED_WITH_STRUCT));
}
Node lhs = n.getFirstChild();
LValueResultFwd lval = analyzeLValueFwd(lhs, inEnv, STRING);
if (lval.declType != null && !commonTypes.isStringScalarOrObj(lval.declType)) {
warnings.add(JSError.make(lhs, FORIN_EXPECTS_STRING_KEY, lval.declType.toString()));
outEnv = lval.env;
} else {
outEnv = updateLvalueTypeInEnv(lval.env, lhs, lval.ptr, STRING);
}
break;
}
conditional = true; conditional = true;
analyzeConditionalStmFwd(dn, NodeUtil.getConditionExpression(n), inEnv); analyzeConditionalStmFwd(dn, NodeUtil.getConditionExpression(n), inEnv);
break; break;
case FOR_IN:
outEnv = processForIn(n, inEnv);
break;
case FOR_OF:
outEnv = processForOf(n, inEnv);
break;
case CASE: { case CASE: {
conditional = true; conditional = true;
// See analyzeExprFwd#Token.CASE for how to handle this precisely // See analyzeExprFwd#Token.CASE for how to handle this precisely
Expand Down Expand Up @@ -1133,6 +1132,58 @@ private void analyzeConditionalStmFwd(
} }
} }


private TypeEnv processForIn(Node n, TypeEnv inEnv) {
Node obj = n.getSecondChild();
EnvTypePair pair = analyzeExprFwd(obj, inEnv, pickReqObjType(n));
pair = mayWarnAboutNullableReferenceAndTighten(n, pair.type, null, inEnv);
JSType objType = pair.type;
if (!objType.isSubtypeOf(TOP_OBJECT)) {
warnings.add(JSError.make(obj, FORIN_EXPECTS_OBJECT, objType.toString()));
} else if (objType.isStruct()) {
warnings.add(JSError.make(obj, IN_USED_WITH_STRUCT));
}
Node lhs = n.getFirstChild();
LValueResultFwd lval = analyzeLValueFwd(lhs, inEnv, STRING);
TypeEnv outEnv;
if (lval.declType != null && !commonTypes.isStringScalarOrObj(lval.declType)) {
warnings.add(JSError.make(lhs, FORIN_EXPECTS_STRING_KEY, lval.declType.toString()));
outEnv = lval.env;
} else {
outEnv = updateLvalueTypeInEnv(lval.env, lhs, lval.ptr, STRING);
}
return outEnv;
}

private TypeEnv processForOf(Node n, TypeEnv inEnv) {
Node rhs = n.getSecondChild();
EnvTypePair rhsPair = analyzeExprFwd(rhs, inEnv, pickReqObjType(n));
rhsPair = mayWarnAboutNullableReferenceAndTighten(n, rhsPair.type, null, inEnv);
JSType rhsObjType = rhsPair.type;
JSType boxedType = rhsObjType.autobox();
JSType lhsExpectedType;
if (boxedType.isSubtypeOf(this.commonTypes.getIterableInstance(UNKNOWN))) {
lhsExpectedType = boxedType.getInstantiatedTypeOfIterable();
} else {
warnings.add(JSError.make(rhs, FOROF_EXPECTS_ITERABLE, rhsObjType.toString()));
lhsExpectedType = UNKNOWN;
}
Node lhsNode = n.getFirstChild();
LValueResultFwd lhsLval = analyzeLValueFwd(lhsNode, inEnv, lhsExpectedType);
TypeEnv outEnv;
if (lhsLval.declType == null || lhsExpectedType.isSubtypeOf(lhsLval.declType)) {
outEnv = updateLvalueTypeInEnv(lhsLval.env, lhsNode, lhsLval.ptr, lhsExpectedType);
} else {
registerMismatchAndWarn(
JSError.make(
n, MISTYPED_FOROF_ELEMENT_TYPE,
errorMsgWithTypeDiff(lhsLval.declType, lhsExpectedType)),
lhsExpectedType,
lhsLval.declType);
outEnv = updateLvalueTypeInEnv(lhsLval.env, lhsNode, lhsLval.ptr, lhsLval.declType);
}
return outEnv;
}

private void createSummary(NTIScope fn) { private void createSummary(NTIScope fn) {
Node fnRoot = fn.getRoot(); Node fnRoot = fn.getRoot();
checkArgument(!fnRoot.isFromExterns()); checkArgument(!fnRoot.isFromExterns());
Expand Down Expand Up @@ -1207,7 +1258,7 @@ && hasPathWithNoReturn(this.cfg)) {
} }
} else if (declType.getNominalType() == null) { } else if (declType.getNominalType() == null) {
// If someone uses the result of a function that doesn't return, they get a warning. // If someone uses the result of a function that doesn't return, they get a warning.
builder.addRetType(actualRetType.isBottom() ? TOP : actualRetType); builder.addRetType(firstNonBottom(actualRetType, TOP));
} else { } else {
// Don't infer a return type for constructors. We want to warn for // Don't infer a return type for constructors. We want to warn for
// constructors called without new who don't explicitly declare @return. // constructors called without new who don't explicitly declare @return.
Expand Down Expand Up @@ -2287,7 +2338,7 @@ private EnvTypePair analyzeGetElemFwd(
JSType indexType = recvType.getIndexType(); JSType indexType = recvType.getIndexType();
if (indexType != null) { if (indexType != null) {
pair = analyzeExprFwd( pair = analyzeExprFwd(
index, pair.env, indexType.isBottom() ? UNKNOWN : indexType); index, pair.env, firstNonBottom(indexType, UNKNOWN));
mayWarnAboutBadIObjectIndex(index, recvType, pair.type, indexType); mayWarnAboutBadIObjectIndex(index, recvType, pair.type, indexType);
pair.type = getIndexedTypeOrUnknown(recvType); pair.type = getIndexedTypeOrUnknown(recvType);
return pair; return pair;
Expand Down Expand Up @@ -2356,9 +2407,7 @@ private EnvTypePair analyzeArrayLitFwd(Node expr, TypeEnv inEnv) {
env = pair.env; env = pair.env;
elementType = JSType.join(elementType, pair.type); elementType = JSType.join(elementType, pair.type);
} }
if (elementType.isBottom()) { elementType = firstNonBottom(elementType, UNKNOWN);
elementType = UNKNOWN;
}
return new EnvTypePair(env, commonTypes.getArrayInstance(elementType)); return new EnvTypePair(env, commonTypes.getArrayInstance(elementType));
} }


Expand Down Expand Up @@ -2669,7 +2718,7 @@ private ImmutableMap<String, JSType> calcTypeInstantiation(
} }
} else if (types.size() == 1) { } else if (types.size() == 1) {
JSType t = Iterables.getOnlyElement(types); JSType t = Iterables.getOnlyElement(types);
builder.put(typeParam, t.isBottom() ? UNKNOWN : t); builder.put(typeParam, firstNonBottom(t, UNKNOWN));
} else { } else {
// Put ? for any uninstantiated type variables // Put ? for any uninstantiated type variables
builder.put(typeParam, UNKNOWN); builder.put(typeParam, UNKNOWN);
Expand Down Expand Up @@ -3366,8 +3415,8 @@ private TypeEnv updateLvalueTypeInEnv(
// Don't specialize THIS in functions where it is unknown. // Don't specialize THIS in functions where it is unknown.
return t == null ? env : envPutType(env, THIS_ID, type); return t == null ? env : envPutType(env, THIS_ID, type);
} }
case VAR: // Can happen iff its parent is a for/in. case VAR: // Can happen iff its parent is a for/in or for/of.
Preconditions.checkState(lvalue.getParent().isForIn()); Preconditions.checkState(lvalue.getParent().isForIn() || lvalue.getParent().isForOf());
return envPutType(env, lvalue.getFirstChild().getString(), type); return envPutType(env, lvalue.getFirstChild().getString(), type);
case GETPROP: case GETPROP:
case GETELEM: { case GETELEM: {
Expand Down Expand Up @@ -3620,11 +3669,10 @@ private EnvTypePair analyzeExprBwd(
pair.type = BOOLEAN; pair.type = BOOLEAN;
return pair; return pair;
} }
case VAR: case VAR: { // Can happen iff its parent is a for/in or for/of.
{ // Can happen iff its parent is a for/in.
Node vdecl = expr.getFirstChild(); Node vdecl = expr.getFirstChild();
String name = vdecl.getString(); String name = vdecl.getString();
// For/in can never have rhs of its VAR // For/in and for/of can never have rhs of its VAR
checkState(!vdecl.hasChildren()); checkState(!vdecl.hasChildren());
return new EnvTypePair(envPutType(outEnv, name, UNKNOWN), UNKNOWN); return new EnvTypePair(envPutType(outEnv, name, UNKNOWN), UNKNOWN);
} }
Expand Down Expand Up @@ -3831,9 +3879,7 @@ private EnvTypePair analyzeCallNewBwd(
JSType formalType = funType.getFormalType(i); JSType formalType = funType.getFormalType(i);
// The type of a formal can be BOTTOM as the result of a join. // The type of a formal can be BOTTOM as the result of a join.
// Don't use this as a requiredType. // Don't use this as a requiredType.
if (formalType.isBottom()) { formalType = firstNonBottom(formalType, UNKNOWN);
formalType = UNKNOWN;
}
tmpEnv = analyzeExprBwd(arg, tmpEnv, formalType).env; tmpEnv = analyzeExprBwd(arg, tmpEnv, formalType).env;
// We don't need deferred checks for args in BWD // We don't need deferred checks for args in BWD
} }
Expand All @@ -3851,9 +3897,7 @@ private EnvTypePair analyzeGetElemBwd(
JSType recvType = pair.type; JSType recvType = pair.type;
JSType indexType = recvType.getIndexType(); JSType indexType = recvType.getIndexType();
if (indexType != null) { if (indexType != null) {
if (indexType.isBottom()) { indexType = firstNonBottom(indexType, UNKNOWN);
indexType = UNKNOWN;
}
pair = analyzeExprBwd(index, pair.env, indexType); pair = analyzeExprBwd(index, pair.env, indexType);
pair.type = getIndexedTypeOrUnknown(recvType); pair.type = getIndexedTypeOrUnknown(recvType);
return pair; return pair;
Expand Down Expand Up @@ -3884,9 +3928,7 @@ private EnvTypePair analyzeArrayLitBwd(Node expr, TypeEnv outEnv) {
env = pair.env; env = pair.env;
elementType = JSType.join(elementType, pair.type); elementType = JSType.join(elementType, pair.type);
} }
if (elementType.isBottom()) { elementType = firstNonBottom(elementType, UNKNOWN);
elementType = UNKNOWN;
}
return new EnvTypePair(env, commonTypes.getArrayInstance(elementType)); return new EnvTypePair(env, commonTypes.getArrayInstance(elementType));
} }


Expand Down Expand Up @@ -4177,7 +4219,7 @@ private LValueResultFwd analyzeLValueFwd(
} }


private LValueResultFwd analyzeLValueFwd( private LValueResultFwd analyzeLValueFwd(
Node expr, TypeEnv inEnv, JSType type, boolean insideQualifiedName) { Node expr, TypeEnv inEnv, JSType requiredType, boolean insideQualifiedName) {
LValueResultFwd lvalResult = null; LValueResultFwd lvalResult = null;
switch (expr.getToken()) { switch (expr.getToken()) {
case THIS: { case THIS: {
Expand Down Expand Up @@ -4205,10 +4247,10 @@ private LValueResultFwd analyzeLValueFwd(
Node prop = expr.getLastChild(); Node prop = expr.getLastChild();
QualifiedName pname = expr.isGetProp() || prop.isString() QualifiedName pname = expr.isGetProp() || prop.isString()
? new QualifiedName(prop.getString()) : null; ? new QualifiedName(prop.getString()) : null;
LValueResultFwd recvLvalue = analyzeReceiverLvalFwd(obj, pname, inEnv, type); LValueResultFwd recvLvalue = analyzeReceiverLvalFwd(obj, pname, inEnv, requiredType);
if (!recvLvalue.type.isSubtypeOf(TOP_OBJECT)) { if (!recvLvalue.type.isSubtypeOf(TOP_OBJECT)) {
EnvTypePair pair = analyzeExprFwd(prop, recvLvalue.env, type); EnvTypePair pair = analyzeExprFwd(prop, recvLvalue.env, requiredType);
lvalResult = new LValueResultFwd(pair.env, type, null, null); lvalResult = new LValueResultFwd(pair.env, requiredType, null, null);
break; break;
} }
JSType indexType = recvLvalue.type.getIndexType(); JSType indexType = recvLvalue.type.getIndexType();
Expand All @@ -4219,34 +4261,38 @@ private LValueResultFwd analyzeLValueFwd(
} }
// (2) A getelem where the prop is a string literal is like a getprop // (2) A getelem where the prop is a string literal is like a getprop
if (expr.isGetProp() || prop.isString()) { if (expr.isGetProp() || prop.isString()) {
lvalResult = analyzePropLValFwd(obj, pname, recvLvalue, type, insideQualifiedName); lvalResult = analyzePropLValFwd(
obj, pname, recvLvalue, requiredType, insideQualifiedName);
break; break;
} }
// (3) All other getelems // (3) All other getelems
// TODO(dimvar): there is some recomputation here; the receiver will be // TODO(dimvar): there is some recomputation here; the receiver will be
// analyzed again. Some more refactoring can fix this. // analyzed again. Some more refactoring can fix this.
EnvTypePair pair = analyzeExprFwd(expr, recvLvalue.env, type); EnvTypePair pair = analyzeExprFwd(expr, recvLvalue.env, requiredType);
lvalResult = new LValueResultFwd(pair.env, pair.type, null, null); lvalResult = new LValueResultFwd(pair.env, pair.type, null, null);
break; break;
} }
case VAR: case VAR: { // Can happen iff its parent is a for/in or for/of.
{ // Can happen iff its parent is a for/in. checkState(expr.getParent().isForIn() || expr.getParent().isForOf());
checkState(expr.getParent().isForIn());
Node vdecl = expr.getFirstChild(); Node vdecl = expr.getFirstChild();
String name = vdecl.getString(); String name = vdecl.getString();
// For/in can never have rhs of its VAR // For/in and for/of can never have rhs of its VAR
checkState(!vdecl.hasChildren()); checkState(!vdecl.hasChildren());
return new LValueResultFwd(inEnv, STRING, null, new QualifiedName(name)); if (expr.getParent().isForIn()) {
} return new LValueResultFwd(inEnv, STRING, null, new QualifiedName(name));
default: } else {
{ JSType declType = this.currentScope.getDeclaredTypeOf(name);
// Expressions that aren't lvalues should be handled because they may return new LValueResultFwd(inEnv, requiredType, declType, new QualifiedName(name));
// be, e.g., the left child of a getprop. }
// We must check that they are not the direct lvalues.
checkState(insideQualifiedName);
EnvTypePair pair = analyzeExprFwd(expr, inEnv, type);
return new LValueResultFwd(pair.env, pair.type, null, null);
} }
default: {
// Expressions that aren't lvalues should be handled because they may
// be, e.g., the left child of a getprop.
// We must check that they are not the direct lvalues.
checkState(insideQualifiedName);
EnvTypePair pair = analyzeExprFwd(expr, inEnv, requiredType);
return new LValueResultFwd(pair.env, pair.type, null, null);
}
} }
maybeSetTypeI(expr, lvalResult.type); maybeSetTypeI(expr, lvalResult.type);
mayWarnAboutUnknownType(expr, lvalResult.type); mayWarnAboutUnknownType(expr, lvalResult.type);
Expand All @@ -4256,7 +4302,7 @@ private LValueResultFwd analyzeLValueFwd(
private LValueResultFwd analyzeIObjectElmLvalFwd( private LValueResultFwd analyzeIObjectElmLvalFwd(
Node prop, LValueResultFwd recvLvalue, JSType indexType) { Node prop, LValueResultFwd recvLvalue, JSType indexType) {
EnvTypePair pair = analyzeExprFwd( EnvTypePair pair = analyzeExprFwd(
prop, recvLvalue.env, indexType.isBottom() ? UNKNOWN : indexType); prop, recvLvalue.env, firstNonBottom(indexType, UNKNOWN));
if (mayWarnAboutBadIObjectIndex(prop, recvLvalue.type, pair.type, indexType)) { if (mayWarnAboutBadIObjectIndex(prop, recvLvalue.type, pair.type, indexType)) {
return new LValueResultFwd(pair.env, UNKNOWN, null, null); return new LValueResultFwd(pair.env, UNKNOWN, null, null);
} }
Expand Down Expand Up @@ -4479,6 +4525,9 @@ private JSType pickReqObjType(Node expr) {
case FOR_IN: case FOR_IN:
Preconditions.checkState(expr.isForIn()); Preconditions.checkState(expr.isForIn());
return TOP_OBJECT; return TOP_OBJECT;
case FOR_OF:
Preconditions.checkState(expr.isForOf());
return this.commonTypes.getIterableInstance(UNKNOWN);
case GETPROP: case GETPROP:
case GETELEM: case GETELEM:
case IN: case IN:
Expand All @@ -4502,6 +4551,14 @@ TypeEnv getEntryTypeEnv() {
return getOutEnv(this.cfg.getEntry()); return getOutEnv(this.cfg.getEntry());
} }


private static JSType firstNonBottom(JSType t1, JSType t2) {
if (t1.isBottom()) {
Preconditions.checkArgument(!t2.isBottom());
return t2;
}
return t1;
}

private class DeferredCheck { private class DeferredCheck {
final Node callSite; final Node callSite;
final NTIScope callerScope; final NTIScope callerScope;
Expand Down Expand Up @@ -4590,3 +4647,4 @@ public int hashCode() {
} }
} }
} }

0 comments on commit 90b5259

Please sign in to comment.