From e5eabe8506883872ff9c0f6d1854f292f988cfa7 Mon Sep 17 00:00:00 2001 From: johnlenz Date: Wed, 16 May 2018 08:11:47 -0700 Subject: [PATCH] Add missing inference for 'assign op's such as *= so that: x *= 2; gets the same inferrence as: x = x * 2; ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=196829619 --- .../javascript/jscomp/TypeInference.java | 92 ++++++++++++------- .../jscomp/TypeCheckNoTranspileTest.java | 8 +- .../javascript/jscomp/TypeCheckTest.java | 14 +++ 3 files changed, 76 insertions(+), 38 deletions(-) diff --git a/src/com/google/javascript/jscomp/TypeInference.java b/src/com/google/javascript/jscomp/TypeInference.java index 4ec54196023..2f26dff436b 100644 --- a/src/com/google/javascript/jscomp/TypeInference.java +++ b/src/com/google/javascript/jscomp/TypeInference.java @@ -407,9 +407,8 @@ private FlowScope traverse(Node n, FlowScope scope) { case ASSIGN_MUL: case ASSIGN_SUB: case ASSIGN_EXPONENT: - // TODO(b/79437694): update the scope for the lhs - scope = traverseChildren(n, scope); - n.setJSType(getNativeType(NUMBER_TYPE)); + scope = traverseAssignOp(n, scope, getNativeType(NUMBER_TYPE)); + break; case LSH: @@ -592,15 +591,27 @@ private FlowScope traverseCatch(Node catchNode, FlowScope scope) { } private FlowScope traverseAssign(Node n, FlowScope scope) { + Node target = n.getFirstChild(); + Node value = n.getLastChild(); + scope = traverseChildren(n, scope); + + JSType targetType = target.getJSType(); + JSType valueType = getJSType(value); + n.setJSType(valueType); + + updateScopeForAssignment(scope, target, targetType, valueType); + return scope; + } + + private FlowScope traverseAssignOp(Node n, FlowScope scope, JSType resultType) { Node left = n.getFirstChild(); - Node right = n.getLastChild(); scope = traverseChildren(n, scope); JSType leftType = left.getJSType(); - JSType rightType = getJSType(right); - n.setJSType(rightType); + n.setJSType(resultType); - updateScopeForTypeChange(scope, left, leftType, rightType); + // The lhs is both an input and an output, so don't update the input type here. + updateScopeForAssignment(scope, left, leftType, resultType, null); return scope; } @@ -640,26 +651,29 @@ private static void addMissingInterfaceProperties(JSType constructor) { } } - /** - * Updates the scope according to the result of a type change, like - * an assignment or a type cast. - */ - private void updateScopeForTypeChange( - FlowScope scope, Node left, JSType leftType, JSType resultType) { + private void updateScopeForAssignment( + FlowScope scope, Node target, JSType targetType, JSType resultType) { + updateScopeForAssignment(scope, target, targetType, resultType, target); + } + + /** Updates the scope according to the result of an assignment. */ + private void updateScopeForAssignment( + FlowScope scope, Node target, JSType targetType, JSType resultType, Node updateNode) { checkNotNull(resultType); + checkState(updateNode == null || updateNode == target); - Node right = NodeUtil.getRValueOfLValue(left); - if (isPossibleMixinApplication(left, right)) { - addMissingInterfaceProperties(leftType); + Node right = NodeUtil.getRValueOfLValue(target); + if (isPossibleMixinApplication(target, right)) { + addMissingInterfaceProperties(targetType); } - switch (left.getToken()) { + switch (target.getToken()) { case NAME: - String varName = left.getString(); + String varName = target.getString(); TypedVar var = getDeclaredVar(scope, varName); JSType varType = var == null ? null : var.getType(); boolean isVarDeclaration = - left.hasChildren() + target.hasChildren() && varType != null && !var.isTypeInferred() && var.getNameNode() != null; @@ -704,18 +718,21 @@ private void updateScopeForTypeChange( // || !resultType.isSubtype(varType)); if (isVarTypeBetter) { - redeclareSimpleVar(scope, left, varType); + redeclareSimpleVar(scope, target, varType); } else { - redeclareSimpleVar(scope, left, resultType); + redeclareSimpleVar(scope, target, resultType); + } + + if (updateNode != null) { + updateNode.setJSType(resultType); } - left.setJSType(resultType); if (var != null && var.isTypeInferred() // Don't change the typed scope to include "undefined" upon seeing "let foo;", because // this is incompatible with how we currently handle VARs and breaks existing code. // TODO(sdh): remove this condition after cleaning up code depending on it. - && !(left.getParent().isLet() && !left.hasChildren())) { + && !(target.getParent().isLet() && !target.hasChildren())) { JSType oldType = var.getType(); var.setType(oldType == null ? resultType : oldType.getLeastSupertype(resultType)); } else if (isTypelessConstDecl) { @@ -726,24 +743,27 @@ private void updateScopeForTypeChange( } break; case GETPROP: - if (left.isQualifiedName()) { - String qualifiedName = left.getQualifiedName(); + if (target.isQualifiedName()) { + String qualifiedName = target.getQualifiedName(); boolean declaredSlotType = false; - JSType rawObjType = left.getFirstChild().getJSType(); + JSType rawObjType = target.getFirstChild().getJSType(); if (rawObjType != null) { ObjectType objType = ObjectType.cast( rawObjType.restrictByNotNullOrUndefined()); if (objType != null) { - String propName = left.getLastChild().getString(); + String propName = target.getLastChild().getString(); declaredSlotType = objType.isPropertyTypeDeclared(propName); } } - JSType safeLeftType = leftType == null ? unknownType : leftType; - scope.inferQualifiedSlot(left, qualifiedName, safeLeftType, resultType, declaredSlotType); + JSType safeLeftType = targetType == null ? unknownType : targetType; + scope.inferQualifiedSlot( + target, qualifiedName, safeLeftType, resultType, declaredSlotType); } - left.setJSType(resultType); - ensurePropertyDefined(left, resultType, scope); + if (updateNode != null) { + updateNode.setJSType(resultType); + } + ensurePropertyDefined(target, resultType, scope); break; default: break; @@ -852,8 +872,7 @@ private FlowScope traverseName(Node n, FlowScope scope) { JSType type = n.getJSType(); if (value != null) { scope = traverse(value, scope); - updateScopeForTypeChange( - scope, n, n.getJSType() /* could be null */, getJSType(value)); + updateScopeForAssignment(scope, n, n.getJSType() /* could be null */, getJSType(value)); return scope; } else if (n.getParent().isLet()) { // Whenever we see a LET, we're guaranteed it's not yet in the scope, and we don't need to @@ -863,7 +882,7 @@ private FlowScope traverseName(Node n, FlowScope scope) { // TODO(sdh): I would have thought that #updateScopeForTypeChange would handle using the // declared type correctly, but for some reason it doesn't so we handle it here. JSType resultType = type != null ? type : getNativeType(VOID_TYPE); - updateScopeForTypeChange(scope, n, type, resultType); + updateScopeForAssignment(scope, n, type, resultType); type = resultType; } else { StaticTypedSlot var = scope.getSlot(varName); @@ -999,7 +1018,10 @@ private FlowScope traverseAdd(Node n, FlowScope scope) { n.setJSType(type); if (n.isAssignAdd()) { - updateScopeForTypeChange(scope, left, leftType, type); + // TODO(johnlenz): this should not update the type of the lhs as that is use as a + // input and need to be preserved for type checking. + // Instead call this overload `updateScopeForAssignment(scope, left, leftType, type, null);` + updateScopeForAssignment(scope, left, leftType, type); } return scope; diff --git a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java index ac495d0b3dc..36c95b8ee47 100644 --- a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java @@ -382,15 +382,17 @@ public void testExponent1() { } public void testExponent2() { - // TODO(b/79437694): this should produce a type error but assign ops are not properly - // tightened. testTypes( lines( "function fn(someUnknown) {", " var x = someUnknown;", " x **= 2;", // infer the result " var /** null */ y = x;", - "}")); + "}"), + lines( + "initializing variable", + "found : number", + "required: null")); } public void testExponent3() { diff --git a/test/com/google/javascript/jscomp/TypeCheckTest.java b/test/com/google/javascript/jscomp/TypeCheckTest.java index cfedda6ec64..77afdbe2433 100644 --- a/test/com/google/javascript/jscomp/TypeCheckTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckTest.java @@ -19770,6 +19770,20 @@ public void testRefinedTypeInNestedShortCircuitingAndOr() { "required: null")); } + public void testAssignOp() { + testTypes( + lines( + "function fn(someUnknown) {", + " var x = someUnknown;", + " x *= 2;", // infer 'x' to now be 'number' + " var /** null */ y = x;", + "}"), + lines( + "initializing variable", + "found : number", + "required: null")); + } + private void testClosureTypes(String js, String description) { testClosureTypesMultipleWarnings(js, description == null ? null : ImmutableList.of(description));