diff --git a/src/com/google/javascript/jscomp/DestructuredTarget.java b/src/com/google/javascript/jscomp/DestructuredTarget.java index 403bf2149a9..73767a2249b 100644 --- a/src/com/google/javascript/jscomp/DestructuredTarget.java +++ b/src/com/google/javascript/jscomp/DestructuredTarget.java @@ -89,6 +89,12 @@ boolean hasStringKey() { return objectPatternKey != null && objectPatternKey.isStringKey(); } + /** Returns a STRING_KEY node or null */ + @Nullable + Node getStringKey() { + return hasStringKey() ? objectPatternKey : null; + } + @Nullable Node getDefaultValue() { return defaultValue; diff --git a/src/com/google/javascript/jscomp/TypeCheck.java b/src/com/google/javascript/jscomp/TypeCheck.java index 3edc841edbc..78a40eb14fa 100644 --- a/src/com/google/javascript/jscomp/TypeCheck.java +++ b/src/com/google/javascript/jscomp/TypeCheck.java @@ -902,6 +902,11 @@ public void visit(NodeTraversal t, Node n, Node parent) { validator.expectIndexMatch( t, computedProperty, patternType, getJSType(computedProperty.getFirstChild())); } + + if (target.hasStringKey()) { + checkPropertyAccessForDestructuring( + t, n, getJSType(n), target.getStringKey(), getJSType(target.getNode())); + } } break; @@ -1214,7 +1219,8 @@ private void checkPropCreation(NodeTraversal t, Node lvalue) { return; } - reportMissingProperty(objType, propName, kind, t, lvalue, true); + reportMissingProperty( + lvalue.getFirstChild(), objType, lvalue.getSecondChild(), kind, t, true); } } } @@ -1756,24 +1762,64 @@ private void visitGetProp(NodeTraversal t, Node n) { report(t, property, TypeValidator.ILLEGAL_PROPERTY_ACCESS, "'.'", "dict"); } else if (validator.expectNotNullOrUndefined(t, n, childType, "No properties on this expression", getNativeType(OBJECT_TYPE))) { - checkPropertyAccess(childType, property.getString(), t, n); + checkPropertyAccessForGetProp(t, n); } ensureTyped(n); } + private void checkPropertyAccessForGetProp(NodeTraversal t, Node getProp) { + checkArgument(getProp.isGetProp(), getProp); + Node objNode = getProp.getFirstChild(); + JSType objType = getJSType(objNode); + Node propNode = getProp.getSecondChild(); + JSType propType = getJSType(getProp); + + checkPropertyAccess(objType, t, getProp, propType, propNode, objNode); + } + + private void checkPropertyAccessForDestructuring( + NodeTraversal t, Node pattern, JSType objectType, Node stringKey, JSType inferredPropType) { + checkArgument(pattern.isDestructuringPattern(), pattern); + checkArgument(stringKey.isStringKey(), stringKey); + + // Get the object node being destructured when it exists. These cases have an actual node `obj`: + // const {a} = obj; + // ({a} = obj); + // while these do not: + // for (const {a} of arrayOfObjects) { + // const {{a}} = obj; + Node objNode = null; + Node patternParent = pattern.getParent(); + if ((patternParent.isAssign() || patternParent.isDestructuringLhs()) + && pattern.getNext() != null) { + objNode = pattern.getNext(); + } + checkPropertyAccess(objectType, t, /* getProp= */ null, inferredPropType, stringKey, objNode); + } + /** - * Emit a warning if we can prove that a property cannot possibly be - * defined on an object. Note the difference between JS and a strictly - * statically typed language: we're checking if the property - * *cannot be defined*, whereas a java compiler would check if the - * property *can be undefined*. + * Emits a warning if we can prove that a property cannot possibly be defined on an object. Note + * the difference between JS and a strictly statically typed language: we're checking if the + * property *cannot be defined*, whereas a java compiler would check if the property *can be + * undefined. + * + *

This method handles property access in both GETPROPs and object destructuring. + * Consequentially some of its arguments are optional - the actual object node and the getprop - + * while others are required. + * + * @param objNode the actual node representing the object we're accessing. optional because + * destructuring accesses MAY not have an actual object node + * @param getProp the GETPROP node representing this property access. optional because + * destructuring accesses don't have actual getprops. */ - private void checkPropertyAccess(JSType childType, String propName, NodeTraversal t, Node n) { - // If the property type is unknown, check the object type to see if it - // can ever be defined. We explicitly exclude CHECKED_UNKNOWN (for - // properties where we've checked that it exists, or for properties on - // objects that aren't in this binary). - JSType propType = getJSType(n); + private void checkPropertyAccess( + JSType childType, + NodeTraversal t, + @Nullable Node getProp, + JSType propType, + Node propNode, + @Nullable Node objNode) { + String propName = propNode.getString(); if (propType.isEquivalentTo(typeRegistry.getNativeType(UNKNOWN_TYPE))) { childType = childType.autobox(); ObjectType objectType = ObjectType.cast(childType); @@ -1784,39 +1830,45 @@ private void checkPropertyAccess(JSType childType, String propName, NodeTraversa if (!objectType.hasProperty(propName) || objectType.isEquivalentTo(typeRegistry.getNativeType(UNKNOWN_TYPE))) { if (objectType instanceof EnumType) { - report(t, n, INEXISTENT_ENUM_ELEMENT, propName); + report(t, getProp != null ? getProp : propNode, INEXISTENT_ENUM_ELEMENT, propName); } else { - checkPropertyAccessHelper(objectType, propName, t, n, false); + checkPropertyAccessHelper(objectType, propNode, objNode, t, getProp, false); } } } else { - checkPropertyAccessHelper(childType, propName, t, n, false); + checkPropertyAccessHelper(childType, propNode, objNode, t, getProp, false); } - } else if (childType.isUnionType() && !isLValueGetProp(n)) { + } else if (childType.isUnionType() && getProp != null && !isLValueGetProp(getProp)) { // NOTE: strict property assignment checks are done on assignment. - checkPropertyAccessHelper(childType, propName, t, n, true); + checkPropertyAccessHelper(childType, propNode, objNode, t, getProp, true); } } boolean isLValueGetProp(Node n) { Node parent = n.getParent(); + // TODO(b/77597706): this won't work for destructured lvalues return (NodeUtil.isUpdateOperator(parent) || NodeUtil.isAssignmentOp(parent)) && parent.getFirstChild() == n; } /** * @param strictCheck Whether this is a check that is only performed when "strict missing - * properties" cheks are enabled. + * properties" checks are enabled. */ private void checkPropertyAccessHelper( - JSType objectType, String propName, NodeTraversal t, Node n, boolean strictCheck) { - boolean isStruct = objectType.isStruct(); - boolean maybePropExistenceCheck = !isStruct && allowLoosePropertyAccessOnNode(n); + JSType objectType, + Node propNode, + @Nullable Node objNode, + NodeTraversal t, + Node getProp, + boolean strictCheck) { if (!reportMissingProperties || objectType.isEmptyType() - || allowStrictPropertyAccessOnNode(n)) { + || (getProp != null && allowStrictPropertyAccessOnNode(getProp))) { return; } + + String propName = propNode.getString(); PropDefinitionKind kind = typeRegistry.canPropertyBeDefined(objectType, propName); if (kind.equals(PropDefinitionKind.KNOWN)) { return; @@ -1830,27 +1882,34 @@ private void checkPropertyAccessHelper( // We still don't want to report this. return; } - boolean loosePropertyDeclaration = isQNameAssignmentTarget(n) && !isStruct; + + boolean isStruct = objectType.isStruct(); + boolean loosePropertyDeclaration = + getProp != null && isQNameAssignmentTarget(getProp) && !isStruct; + // always false for destructuring + boolean maybePropExistenceCheck = + !isStruct && getProp != null && allowLoosePropertyAccessOnNode(getProp); // Traditionally, we would not report a warning for "loose" properties, but we want to be // able to be more strict, so introduce an optional warning. - boolean strictReport = strictCheck || isLooselyAssociated || loosePropertyDeclaration - || maybePropExistenceCheck; + boolean strictReport = + strictCheck || isLooselyAssociated || loosePropertyDeclaration || maybePropExistenceCheck; - reportMissingProperty(objectType, propName, kind, t, n, strictReport); + reportMissingProperty(objNode, objectType, propNode, kind, t, strictReport); } private void reportMissingProperty( - JSType objectType, String propName, PropDefinitionKind kind, NodeTraversal t, Node n, + @Nullable Node objNode, + JSType objectType, + Node propNode, + PropDefinitionKind kind, + NodeTraversal t, boolean strictReport) { - checkState(n.isGetProp()); + String propName = propNode.getString(); boolean isObjectType = objectType.isEquivalentTo(getNativeType(OBJECT_TYPE)); boolean lowConfidence = objectType.isUnknownType() || objectType.isAllType() || isObjectType; boolean isKnownToUnionMember = kind.equals(PropDefinitionKind.LOOSE_UNION); - // boolean loosePropertyDeclaration = isQNameAssignmentTarget(n) && !isStruct; - // Traditionally, we would not report a warning for "loose" properties, but we want to be - // able to be more strict, so introduce an optional warning. SuggestionPair pair = null; if (!lowConfidence && !isKnownToUnionMember) { pair = getClosestPropertySuggestion(objectType, propName, (propName.length() - 1) / 4); @@ -1864,10 +1923,10 @@ private void reportMissingProperty( } report( t, - n.getLastChild(), + propNode, reportType, propName, - typeRegistry.getReadableTypeName(n.getFirstChild()), + objNode != null ? typeRegistry.getReadableTypeName(objNode) : objectType.toString(), pair.suggestion); } else { DiagnosticType reportType; @@ -1884,10 +1943,10 @@ private void reportMissingProperty( } report( t, - n.getLastChild(), + propNode, reportType, propName, - typeRegistry.getReadableTypeName(n.getFirstChild())); + objNode != null ? typeRegistry.getReadableTypeName(objNode) : objectType.toString()); } } diff --git a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java index ed3341282a5..5fdec523677 100644 --- a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java @@ -4001,10 +4001,14 @@ public void testObjectDestructuringNullCausesWarning() { "required: Object")); } - public void testObjectDestructuringNullableDoesntCausesWarning() { - // This matches the behavior of typechecking transpiled destructuring patterns, although - // we certainly could be more strict in the future and forbid dereferencing undefined/null. - testTypes(lines("function f(/** ?Object */ nullableObj) {", "const {x} = nullableObj;", "}")); + public void testObjectDestructuringNullableDoesntCauseWarning() { + // Test that we don't get a "cannot destructure 'null' or 'undefined'" warning, which matches + // the legacy behavior when typechecking transpiled destructuring patterns. + testTypes( + lines( + "function f(/** ?{x: number} */ nullableObj) {", // + "const {x} = nullableObj;", + "}")); } public void testArrayDestructuringNonIterableCausesWarning() { @@ -4040,9 +4044,58 @@ public void testArrayDestructuringParameterWithElision() { testTypes("/** @param {!Array} numbers */ function f([, x, , y]) {}"); } + public void testObjectPatternDeclarationWithMissingPropertyWarning() { + testTypes( + lines( + "function f(/** {a: number} */ obj) {", // + " const {a, b} = obj;", + "}"), + "Property b never defined on obj"); + } + + public void testObjectPatternAssignWithMissingPropertyWarning() { + testTypes( + lines( + "function f(/** {a: number} */ obj) {", // + " let a, b;", + " ({a, b} = obj);", + "}"), + "Property b never defined on obj"); + } + + public void testObjectPatternDeclarationWithMissingPropertyWarningInForOf() { + testTypes( + lines( + "function f(/** !Iterable<{a: number}> */ aNumberObj) {", + " for (const {a, b} of aNumberObj) {}", + "}"), + "Property b never defined on {a: number}"); + } + + public void testObjectPatternWithMissingPropertyWarningInParameters() { + testTypes( + lines( + "/** @param {{a: number}} obj */", // + "function f(/** {a: number} */ {b}) {}"), + "Property b never defined on {a: number}"); + } + + public void testArrayPatternAssignWithIllegalPropCreationInStruct() { + testTypes( + lines( + "class C {", // + " f(/** !Iterable */ ) {", + " [this.x] = arr;", + " }", + "}"), + new String[] { + "Cannot add a property to a struct instance after it is constructed. " + + "(If you already declared the property, make sure to give it a type.)", + "Property x never defined on C" + }); + } + public void testDictClass1() { testTypes("/** @dict */ var C = class { constructor() {} 'x'(){} };"); } - - // TODO(b/77597706): add tests for missing property warnings on object destructuring string keys }