From ed42f9d33bac9f73d631f631f00b3c77d70823b4 Mon Sep 17 00:00:00 2001 From: lharker Date: Wed, 8 Aug 2018 16:42:04 -0700 Subject: [PATCH] Rollforward of "Start typechecking destructuring patterns in TypeCheck". Right now this recalculates what values are being assigned to a lvalue in a destructuring pattern from the rhs value. We should consider modifying this in the future to avoid repeating work done in TypeInference. Still doesn't handle computed properties or default values NEW: allow casts in the lhs of an assignment *** Reason for rollback *** Roll forward after fixing issue in the original CL. *** Original change description *** Automated g4 rollback of changelist 207912024. *** Reason for rollback *** Broke project using pattern (/** @type {?} */) (this.prop) = ... ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=207967623 --- .../google/javascript/jscomp/TypeCheck.java | 194 ++++++++++-------- .../javascript/jscomp/IntegrationTest.java | 13 ++ .../jscomp/TypeCheckNoTranspileTest.java | 103 ++++++++++ .../javascript/jscomp/TypeCheckTest.java | 4 + 4 files changed, 228 insertions(+), 86 deletions(-) diff --git a/src/com/google/javascript/jscomp/TypeCheck.java b/src/com/google/javascript/jscomp/TypeCheck.java index e29fe8db3da..14a1aed2056 100644 --- a/src/com/google/javascript/jscomp/TypeCheck.java +++ b/src/com/google/javascript/jscomp/TypeCheck.java @@ -883,12 +883,20 @@ public void visit(NodeTraversal t, Node n, Node parent) { case FOR: case TEMPLATELIT_SUB: case REST: + case DESTRUCTURING_LHS: + case ARRAY_PATTERN: + case OBJECT_PATTERN: typeable = false; break; case DEFAULT_VALUE: checkCanAssignToWithScope( - t, n, n.getFirstChild(), getJSType(n.getSecondChild()), "default value has wrong type"); + t, + n, + n.getFirstChild(), + getJSType(n.getSecondChild()), + /* info= */ null, + "default value has wrong type"); typeable = false; break; @@ -1025,7 +1033,53 @@ private void visitAssign(NodeTraversal t, Node assign) { Node lvalue = assign.getFirstChild(); Node rvalue = assign.getLastChild(); - // Check property sets to 'object.property' when 'object' is known. + JSType rightType = getJSType(rvalue); + checkCanAssignToWithScope(t, assign, lvalue, rightType, info, "assignment"); + ensureTyped(assign, rightType); + } + + /** + * Checks that we can assign the given right type to the given lvalue or destructuring pattern. + * + *

See {@link #checkCanAssignToNameGetpropOrGetelem(NodeTraversal, Node, Node, JSType, + * JSDocInfo, String)} for more details + * + * @param nodeToWarn A node to report type mismatch warnings on + * @param lvalue The lvalue to which we're assigning or a destructuring pattern + * @param rightType The type we're assigning to the lvalue + * @param msg A message to report along with any type mismatch warnings + */ + private void checkCanAssignToWithScope( + NodeTraversal t, Node nodeToWarn, Node lvalue, JSType rightType, JSDocInfo info, String msg) { + if (lvalue.isDestructuringPattern()) { + for (Node child : lvalue.children()) { + DestructuredTarget target = DestructuredTarget.createTarget(typeRegistry, rightType, child); + // TODO(b/77597706): this is not very efficient because it re-infers the types below, + // which we already did once in TypeInference. don't repeat the work. + checkCanAssignToWithScope( + t, nodeToWarn, target.getNode(), target.inferType(), /* info= */ null, msg); + } + } else { + checkCanAssignToNameGetpropOrGetelem(t, nodeToWarn, lvalue, rightType, info, msg); + } + } + + /** + * Checks that we can assign the given right type to the given lvalue. + * + *

If the lvalue is a qualified name, and has a declared type in the given scope, uses the + * declared type of the qualified name instead of the type on the node. + * + * @param nodeToWarn A node to report type mismatch warnings on + * @param lvalue The lvalue to which we're assigning - a NAME, GETELEM, or GETPROP + * @param rightType The type we're assigning to the lvalue + * @param msg A message to report along with any type mismatch warnings + */ + private void checkCanAssignToNameGetpropOrGetelem( + NodeTraversal t, Node nodeToWarn, Node lvalue, JSType rightType, JSDocInfo info, String msg) { + checkArgument( + lvalue.isName() || lvalue.isGetProp() || lvalue.isGetElem() || lvalue.isCast(), lvalue); + if (lvalue.isGetProp()) { Node object = lvalue.getFirstChild(); JSType objectJsType = getJSType(object); @@ -1037,11 +1091,11 @@ private void visitAssign(NodeTraversal t, Node assign) { if (object.isGetProp()) { JSType jsType = getJSType(object.getFirstChild()); if (jsType.isInterface() && object.getLastChild().getString().equals("prototype")) { - visitInterfacePropertyAssignment(t, object, rvalue); + visitInterfacePropertyAssignment(t, object, lvalue); } } - checkEnumAlias(t, info, rvalue); + checkEnumAlias(t, info, rightType, nodeToWarn); checkPropCreation(t, lvalue); // Prototype assignments are special, because they actually affect @@ -1049,7 +1103,7 @@ private void visitAssign(NodeTraversal t, Node assign) { // during TypedScopeCreator, and we only look for the "dumb" cases here. // object.prototype = ...; if (pname.equals("prototype")) { - validator.expectCanAssignToPrototype(t, objectJsType, rvalue, getJSType(rvalue)); + validator.expectCanAssignToPrototype(t, objectJsType, nodeToWarn, rightType); return; } @@ -1059,7 +1113,8 @@ private void visitAssign(NodeTraversal t, Node assign) { ObjectType objectCastType = ObjectType.cast(objectJsType.restrictByNotNullOrUndefined()); JSType expectedPropertyType = getPropertyTypeIfDeclared(objectCastType, pname); - checkPropertyInheritanceOnGetpropAssign(t, assign, object, pname, info, expectedPropertyType); + checkPropertyInheritanceOnGetpropAssign( + t, nodeToWarn, object, pname, info, expectedPropertyType); // If we successfully found a non-unknown declared type, validate the assignment and don't do // any further checks. @@ -1068,30 +1123,12 @@ private void visitAssign(NodeTraversal t, Node assign) { // assignments to it. if (!propertyIsImplicitCast(objectCastType, pname)) { validator.expectCanAssignToPropertyOf( - t, assign, getJSType(rvalue), expectedPropertyType, object, pname); + t, nodeToWarn, rightType, expectedPropertyType, object, pname); } return; } } - JSType rightType = getJSType(rvalue); - checkCanAssignToWithScope(t, assign, lvalue, rightType, "assignment"); - ensureTyped(assign, rightType); - } - - /** - * Checks that we can assign the given right type to the given lvalue. - * - *

If the lvalue is a qualified name, and has a declared type in the given scope, uses the - * declared type of the qualified name instead of the type on the node. - * - * @param assign A node to report warnings on for this assignment - * @param lvalue The lvalue to which we're assigning - * @param rightType The type we're assigning to the lvalue - * @param msg A message to report along with any type mismatch warnings - */ - private void checkCanAssignToWithScope( - NodeTraversal t, Node assign, Node lvalue, JSType rightType, String msg) { // Check qualified name sets to 'object' and 'object.property'. // This can sometimes handle cases when the type of 'object' is not known. @@ -1119,7 +1156,7 @@ private void checkCanAssignToWithScope( } } // Fall through case for arbitrary LHS and arbitrary RHS. - validator.expectCanAssignTo(t, assign, rightType, leftType, msg); + validator.expectCanAssignTo(t, nodeToWarn, rightType, leftType, msg); } private void checkPropCreation(NodeTraversal t, Node lvalue) { @@ -1535,13 +1572,20 @@ static JSType getObjectLitKeyTypeFromValueType(Node key, JSType valueType) { } /** - * Visits an ASSIGN node for cases such as + * Visits an lvalue node for cases such as * * 

    * interface.prototype.property = ...;
    *
*/ - private void visitInterfacePropertyAssignment(NodeTraversal t, Node object, Node rvalue) { + private void visitInterfacePropertyAssignment(NodeTraversal t, Node object, Node lvalue) { + if (!lvalue.getParent().isAssign()) { + // assignments to interface properties cannot be in destructuring patterns or for-of loops + reportInvalidInterfaceMemberDeclaration(t, object); + return; + } + Node assign = lvalue.getParent(); + Node rvalue = assign.getSecondChild(); JSType rvalueType = getJSType(rvalue); // Only 2 values are allowed for interface methods: @@ -1631,35 +1675,9 @@ private void checkForOfTypes(NodeTraversal t, Node forOf) { // e.g. get "x" given the VAR in "for (var x of arr) {" lhs = lhs.getFirstChild(); } - - // Do some additional checks for property accesses in the initializer - // e.g. check `obj.foo` in `for (obj.foo of someIterable) {` - if (lhs.isGetProp()) { - Node object = lhs.getFirstChild(); - // Don't allow assigning to an interface member - if (object.isGetProp() && object.getLastChild().getString().equals("prototype")) { - JSType jsType = getJSType(object.getFirstChild()); - if (jsType.isInterface()) { - reportInvalidInterfaceMemberDeclaration(t, object); - } - } - checkPropCreation(t, lhs); - - JSType objectJsType = getJSType(object); - Node property = lhs.getLastChild(); - String pname = property.getString(); - - ObjectType objectCastType = ObjectType.cast(objectJsType.restrictByNotNullOrUndefined()); - JSType expectedPropertyType = getPropertyTypeIfDeclared(objectCastType, pname); - if (!expectedPropertyType.isUnknownType()) { - // Note: if the property has @implicitCast at its declaration, we don't check any - // assignments to it. - if (!propertyIsImplicitCast(objectCastType, pname)) { - validator.expectCanAssignToPropertyOf( - t, iterable, actualType, expectedPropertyType, object, pname); - } - return; - } + if (lhs.isDestructuringLhs()) { + // e.g. get `[x, y]` given the VAR in `for (var [x, y] of arr) {` + lhs = lhs.getFirstChild(); } checkCanAssignToWithScope( @@ -1667,6 +1685,7 @@ private void checkForOfTypes(NodeTraversal t, Node forOf) { forOf, lhs, actualType, + lhs.getJSDocInfo(), "declared type of for-of loop variable does not match inferred type"); } @@ -1876,27 +1895,27 @@ private void visitVar(NodeTraversal t, Node n) { // on the NAME node. We probably want to wait for the parser // merge to fix this. JSDocInfo varInfo = n.hasOneChild() ? n.getJSDocInfo() : null; - for (Node name : n.children()) { - Node value = name.getFirstChild(); - // A null var would indicate a bug in the scope creation logic. - TypedVar var = t.getTypedScope().getVar(name.getString()); - - if (value != null) { - JSType valueType = getJSType(value); - JSType nameType = var.getType(); - nameType = (nameType == null) ? getNativeType(UNKNOWN_TYPE) : nameType; - JSDocInfo info = name.getJSDocInfo(); - if (info == null) { - info = varInfo; - } + for (Node child : n.children()) { + if (child.isName()) { + Node value = child.getFirstChild(); + + if (value != null) { + JSType valueType = getJSType(value); + JSDocInfo info = child.getJSDocInfo(); + if (info == null) { + info = varInfo; + } - checkEnumAlias(t, info, value); - if (var.isTypeInferred()) { - ensureTyped(name, valueType); - } else { - validator.expectCanAssignTo( - t, value, valueType, nameType, "initializing variable"); + checkEnumAlias(t, info, valueType, value); + checkCanAssignToWithScope(t, value, child, valueType, info, "initializing variable"); } + } else { + checkState(child.isDestructuringLhs(), child); + Node name = child.getFirstChild(); + Node value = child.getSecondChild(); + JSType valueType = getJSType(value); + checkCanAssignToWithScope( + t, child, name, valueType, /* info= */ null, "initializing variable"); } } } @@ -2605,26 +2624,26 @@ private void visitBinaryOperator(Token op, NodeTraversal t, Node n) { } /** - *

Checks enum aliases. + * Checks enum aliases. + * + *

We verify that the enum element type of the enum used for initialization is a subtype of the + * enum element type of the enum the value is being copied in. * - *

We verify that the enum element type of the enum used - * for initialization is a subtype of the enum element type of - * the enum the value is being copied in.

+ *

Example: * - *

Example:

* 
var myEnum = myOtherEnum;
* - *

Enum aliases are irregular, so we need special code for this :(

+ *

Enum aliases are irregular, so we need special code for this :( * - * @param value the value used for initialization of the enum + * @param valueType the type of the value used for initialization of the enum + * @param nodeToWarn the node on which to issue warnings on */ private void checkEnumAlias( - NodeTraversal t, JSDocInfo declInfo, Node value) { + NodeTraversal t, JSDocInfo declInfo, JSType valueType, Node nodeToWarn) { if (declInfo == null || !declInfo.hasEnumParameterType()) { return; } - JSType valueType = getJSType(value); if (!valueType.isEnumType()) { return; } @@ -2632,7 +2651,10 @@ private void checkEnumAlias( EnumType valueEnumType = valueType.toMaybeEnumType(); JSType valueEnumPrimitiveType = valueEnumType.getElementsType().getPrimitiveType(); - validator.expectCanAssignTo(t, value, valueEnumPrimitiveType, + validator.expectCanAssignTo( + t, + nodeToWarn, + valueEnumPrimitiveType, declInfo.getEnumParameterType().evaluate(t.getTypedScope(), typeRegistry), "incompatible enum element types"); } diff --git a/test/com/google/javascript/jscomp/IntegrationTest.java b/test/com/google/javascript/jscomp/IntegrationTest.java index f1981fac988..4b8a2f6c738 100644 --- a/test/com/google/javascript/jscomp/IntegrationTest.java +++ b/test/com/google/javascript/jscomp/IntegrationTest.java @@ -5538,4 +5538,17 @@ public void testGithubIssue3040() { " JSCompiler_StaticMethods_restart(e)", "}")); } + + public void testCastInLhsOfAssign() { + CompilerOptions options = createCompilerOptions(); + options.setCheckTypes(true); + CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options); + + test( + options, + // it's kind of weird that we allow casts on the lhs, but some existing code uses this + // feature. this is just a regression test so we won't accidentally break this feature. + "var /** string */ str = 'foo'; (/** @type {?} */ (str)) = 3;", + ""); + } } diff --git a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java index d625c12cdef..99e0fdd1bf9 100644 --- a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java @@ -3854,4 +3854,107 @@ public void testDefaultParameterInIifeWithInferredType() { "found : (number|string)", "required: string")); } + + public void testBasicArrayPatternDeclaration() { + testTypes( + lines( + "function f(/** !Iterable */ numbers) {", + " const [/** string */ str] = numbers;", + "}"), + lines( + "initializing variable", // + "found : number", + "required: string")); + } + + public void testNestedDestructuringPatternDeclaration() { + testTypes( + lines( + "function f(/** !Iterable<{x: number}> */ xNumberObjs) {", + " const [{/** string */ x}] = xNumberObjs;", + "}"), + lines( + "initializing variable", // + "found : number", + "required: string")); + } + + public void testBasicArrayPatternAssign() { + testTypes( + lines( + "function f(/** !Iterable */ numbers) {", + " var /** string */ str;", + " [str] = numbers;", + "}"), + lines( + "assignment", // + "found : number", + "required: string")); + } + + public void testNestedDestructuringPatternAssign() { + testTypes( + lines( + "function f(/** !Iterable<{x: number}> */ xNumberObjs) {", + " var /** string */ x;", + " [{x}] = xNumberObjs;", + "}"), + lines( + "assignment", // + "found : number", + "required: string")); + } + + public void testValidArrayPatternInForOfInitializer() { + testTypes( + lines( + "function f(/** !Iterable> */ numberLists) {", + " for (const [/** number */ x, /** number */ y] of numberLists) {}", + "}")); + } + + public void testArrayPatternInForOfInitializerWithTypeMismatch() { + testTypes( + lines( + "function f(/** !Iterable> */ numberLists) {", + " for (const [/** number */ x, /** string */ y] of numberLists) {}", + "}"), + lines( + "declared type of for-of loop variable does not match inferred type", + "found : number", + "required: string")); + } + + public void testCannotAliasEnumThroughDestructuring() { + testTypesWithExterns( + new TestExternsBuilder().addArray().build(), + lines( + "/** @enum {number} */ const THINGS = {THING1: 1, THING2: 2};", + // TODO(lharker): warn for putting @enum here + "/** @enum */ const [OTHERTHINGS] = [THINGS];")); + } + + public void testArrayPatternAssign_badInterfacePropertyCreation() { + testTypes( + "/** @interface */ function Foo() {}; [Foo.prototype.bar] = [];", + "interface members can only be " + + "empty property declarations, empty functions, or goog.abstractMethod"); + } + + public void testArrayPatternAssign_badPropertyAssignment() { + testTypes( + lines( + "/** @param {!Iterable} numbers */", + "function f(numbers) {", + " const /** {a: string} */ obj = {a: 'foo'};", + " [obj.a] = numbers;", + "}"), + lines( + "assignment to property a of obj", // + "found : number", + "required: string")); + } + + // TODO(b/77597706): add tests for default values and computed properties in destructuring, + // and cases where the rvalue is not iterable/an object } diff --git a/test/com/google/javascript/jscomp/TypeCheckTest.java b/test/com/google/javascript/jscomp/TypeCheckTest.java index 669f8b25667..290bdef55a2 100644 --- a/test/com/google/javascript/jscomp/TypeCheckTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckTest.java @@ -20483,6 +20483,10 @@ public void testTypeCheckingDoesntCrashOnDebuggerStatement() { testTypes("var x = 1; debugger; x = 2;"); } + public void testCastOnLhsOfAssignBlocksBadAssignmentWarning() { + testTypes("var /** number */ x = 1; (/** @type {?} */ (x)) = 'foobar';"); + } + private void testClosureTypes(String js, String description) { testClosureTypesMultipleWarnings(js, description == null ? null : ImmutableList.of(description));