diff --git a/src/com/google/javascript/jscomp/DestructuredTarget.java b/src/com/google/javascript/jscomp/DestructuredTarget.java new file mode 100644 index 00000000000..4eae537c8ac --- /dev/null +++ b/src/com/google/javascript/jscomp/DestructuredTarget.java @@ -0,0 +1,139 @@ +/* + * Copyright 2018 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.google.javascript.jscomp; + +import static com.google.common.base.Preconditions.checkArgument; + +import com.google.common.base.Supplier; +import com.google.javascript.rhino.Node; +import com.google.javascript.rhino.jstype.JSType; +import com.google.javascript.rhino.jstype.JSTypeNative; +import com.google.javascript.rhino.jstype.JSTypeRegistry; +import javax.annotation.Nullable; + +/** + * Represents a single target inside a destructuring pattern, whether another pattern or a lhs + * expression. + * + *

Call {@code inferType} to do type inference on the target lazily. This class is designed so + * that a caller can get information about a target, use that information to do additional type + * inference, and finally call {@code inferType} if desired. + */ +final class DestructuredTarget { + private final JSTypeRegistry registry; + /** + * Holds the STRING_KEY or COMPUTED_PROPERTY for a target in an object pattern. Null for targets + * in array patterns. + */ + @Nullable private final Node objectPatternKey; + /** + * The target being assigned to. Can be a destructuring pattern or name, if from a declaration, or + * an arbitrary lhs expression in an assign. + */ + private final Node node; + /** + * A supplier to get the type of the pattern containing this target. e.g. for `a` in `const {a} = + * {a: 3}`, the supplier provides the record type `{a: number}` + * + *

This is only called by {@inferType}, not when calling {@code createTarget}. + */ + private final Supplier patternTypeSupplier; + + private DestructuredTarget( + Node node, + @Nullable Node objectPatternKey, + JSTypeRegistry registry, + Supplier patternTypeSupplier) { + this.node = node; + this.objectPatternKey = objectPatternKey; + this.registry = registry; + this.patternTypeSupplier = patternTypeSupplier; + } + + public Node getNode() { + return node; + } + + static DestructuredTarget createTarget( + JSTypeRegistry registry, Supplier destructuringPatternType, Node destructuringChild) { + checkArgument(destructuringChild.getParent().isDestructuringPattern(), destructuringChild); + final Node node; + Node objectLiteralKey = null; + switch (destructuringChild.getToken()) { + case STRING_KEY: + // const {objectLiteralKey: x} = ... + objectLiteralKey = destructuringChild; + Node value = destructuringChild.getFirstChild(); + node = value.isDefaultValue() ? value.getFirstChild() : value; + break; + + case COMPUTED_PROP: + // const {['objectLiteralKey']: x} = ... + objectLiteralKey = destructuringChild; + value = destructuringChild.getSecondChild(); + node = value.isDefaultValue() ? value.getFirstChild() : value; + break; + + case OBJECT_PATTERN: // const [{x}] = ... + case ARRAY_PATTERN: // const [[x]] = ... + case NAME: // const [x] = ... + node = destructuringChild; + break; + + case DEFAULT_VALUE: // const [x = 3] = ... + case REST: // const [...x] = ... + node = destructuringChild.getFirstChild(); + break; + + default: + throw new IllegalStateException("Unexpected parameter node " + destructuringChild); + } + return new DestructuredTarget(node, objectLiteralKey, registry, destructuringPatternType); + } + + Supplier getInferredTypeSupplier() { + return () -> inferType(); + } + + JSType inferType() { + // TODO(b/203401365): handle array patterns + return objectPatternKey != null ? inferObjectPatternKeyType() : null; + } + + private JSType inferObjectPatternKeyType() { + JSType patternType = patternTypeSupplier.get(); + switch (objectPatternKey.getToken()) { + case STRING_KEY: + JSType propertyType = patternType.findPropertyType(objectPatternKey.getString()); + return propertyType != null + ? propertyType + : registry.getNativeType(JSTypeNative.UNKNOWN_TYPE); + + case COMPUTED_PROP: + return patternType != null + ? patternType + .getTemplateTypeMap() + .getResolvedTemplateType(registry.getObjectElementKey()) + : registry.getNativeType(JSTypeNative.UNKNOWN_TYPE); + + default: + // TODO(b/203401365): handle object rest + throw new IllegalStateException("Unexpected key " + objectPatternKey); + } + + // TODO(b/77597706): handle default values + } +} diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index cd6c46c0b39..e2ad6386949 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -4814,6 +4814,10 @@ static boolean isConstantDeclaration( // TODO(b/77597706): Update this method to handle destructured declarations. if (node.isName() && node.getParent().isConst()) { return true; + } else if (node.isName() + && isLhsByDestructuring(node) + && getRootTarget(node).getGrandparent().isConst()) { + return true; } if (info != null && info.isConstant()) { diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java index 4d6880e9556..e02904dbbf2 100644 --- a/src/com/google/javascript/jscomp/TypedScopeCreator.java +++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java @@ -46,6 +46,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.base.Supplier; import com.google.common.collect.HashBasedTable; import com.google.common.collect.HashMultimap; import com.google.common.collect.HashMultiset; @@ -737,7 +738,7 @@ void processObjectLitProperties( Node value = keyNode.getFirstChild(); String memberName = NodeUtil.getObjectLitKeyName(keyNode); JSDocInfo info = keyNode.getJSDocInfo(); - JSType valueType = getDeclaredType(info, keyNode, value); + JSType valueType = getDeclaredType(info, keyNode, value, null); JSType keyType = objLitType.isEnumType() ? objLitType.toMaybeEnumType().getElementsType() @@ -813,7 +814,7 @@ void defineCatch(Node n) { // the catch declaration. // e.g. `} catch ({message, errno}) {` for (Node catchName : NodeUtil.findLhsNodesInNode(n)) { - JSType type = getDeclaredType(catchName.getJSDocInfo(), catchName, null); + JSType type = getDeclaredType(catchName.getJSDocInfo(), catchName, null, null); new SlotDefiner() .forDeclarationNode(catchName) .forVariableName(catchName.getString()) @@ -831,38 +832,61 @@ void defineVars(Node n) { JSDocInfo info = n.getJSDocInfo(); // `var` declarations are hoisted, but `let` and `const` are not. TypedScope scope = n.isVar() ? currentHoistScope : currentScope; - // Get a list of all of the name nodes for all the variables being declared. - // This handles even complex destructuring cases. - // e.g. - // let x = 3, {y, someProp: [ a1, a2 ]} = someObject; - // varNames will contain x, y, a1, and a2 - List varNames = NodeUtil.findLhsNodesInNode(n); - if (varNames.size() == 1) { - // e.g. - // /** @type {string} */ - // let x = 'hi'; - Node singleVarName = varNames.get(0); - if (info == null) { - // There might be inline JSDoc on the variable name. - // e.g. - // let /** string */ x = 'hi'; - info = singleVarName.getJSDocInfo(); + + if (n.hasMoreThanOneChild() && info != null) { + report(JSError.make(n, MULTIPLE_VAR_DEF)); + } + + for (Node child : n.children()) { + defineVarChild(info, child, scope); + } + } + + /** Defines a variable declared with `var`, `let`, or `const`. */ + void defineVarChild(JSDocInfo declarationInfo, Node child, TypedScope scope) { + if (child.isName()) { + if (declarationInfo == null) { + declarationInfo = child.getJSDocInfo(); // TODO(bradfordcsmith): Report an error if both the declaration node and the name itself // have JSDoc. } - defineName(singleVarName, scope, info); + defineName(child, scope, declarationInfo); } else { - if (info != null) { - // We do not allow a single JSDoc to apply to multiple variables in a single - // declaration. - report(JSError.make(n, MULTIPLE_VAR_DEF)); - } - // TODO(bradfordcsmith): Should we report an error if there are actually 0 variable names? - // This can happen with destructuring patterns. - // e.g. - // let {} = foo(); - for (Node nameNode : varNames) { - defineName(nameNode, scope, nameNode.getJSDocInfo()); + checkState(child.isDestructuringLhs(), child); + Node pattern = child.getFirstChild(); + Node value = child.getSecondChild(); + defineDestructuringPatternInVarDeclaration( + pattern, scope, () -> getDeclaredRValueType(/* lValue= */ null, value)); + } + } + + void defineDestructuringPatternInVarDeclaration( + Node pattern, TypedScope scope, Supplier patternTypeSupplier) { + for (Node child : pattern.children()) { + DestructuredTarget target = + DestructuredTarget.createTarget(typeRegistry, patternTypeSupplier, child); + if (target.getNode().isDestructuringPattern()) { + defineDestructuringPatternInVarDeclaration( + target.getNode(), scope, target.getInferredTypeSupplier()); + } else { + Node name = target.getNode(); + checkState(name.isName(), "This method is only for declaring variables: %s", name); + + // variable's type + JSType type = + getDeclaredType( + name.getJSDocInfo(), name, /* rValue= */ null, target.getInferredTypeSupplier()); + if (type == null) { + // The variable's type will be inferred. + type = name.isFromExterns() ? unknownType : null; + } + new SlotDefiner() + .forDeclarationNode(name) + .forVariableName(name.getString()) + .inScope(scope) + .withType(type) + .allowLaterTypeInference(type == null) + .defineSlot(); } } } @@ -946,7 +970,7 @@ private void defineName(Node name, TypedScope scope, JSDocInfo info) { Node value = name.getFirstChild(); // variable's type - JSType type = getDeclaredType(info, name, value); + JSType type = getDeclaredType(info, name, value, /* declaredRValueTypeSupplier= */ null); if (type == null) { // The variable's type will be inferred. type = name.isFromExterns() ? unknownType : null; @@ -1668,14 +1692,20 @@ private TypedScope getLValueRootScope(Node n) { } /** - * Look for a type declaration on a property assignment - * (in an ASSIGN or an object literal key). + * Look for a type declaration on a property assignment (in an ASSIGN or an object literal key). + * * @param info The doc info for this property. * @param lValue The l-value node. - * @param rValue The node that {@code n} is being initialized to, - * or {@code null} if this is a stub declaration. + * @param rValue The node that {@code n} is being initialized to, or {@code null} if this is a + * stub declaration. + * @param declaredRValueTypeSupplier A supplier for the declared type of the rvalue, used for + * destructuring declarations where we have to do additional work on the rvalue. */ - JSType getDeclaredType(JSDocInfo info, Node lValue, @Nullable Node rValue) { + JSType getDeclaredType( + JSDocInfo info, + Node lValue, + @Nullable Node rValue, + @Nullable Supplier declaredRValueTypeSupplier) { if (info != null && info.hasType()) { return getDeclaredTypeInAnnotation(lValue, info); } else if (rValue != null @@ -1698,14 +1728,20 @@ && shouldUseFunctionLiteralType( } // Check if this is constant, and if it has a known type. - if (NodeUtil.isConstantDeclaration( - compiler.getCodingConvention(), info, lValue)) { + if (NodeUtil.isConstantDeclaration(compiler.getCodingConvention(), info, lValue)) { if (rValue != null) { JSType rValueType = getDeclaredRValueType(lValue, rValue); maybeDeclareAliasType(lValue, rValue, rValueType); if (rValueType != null) { return rValueType; } + } else if (declaredRValueTypeSupplier != null) { + JSType rValueType = declaredRValueTypeSupplier.get(); + // TODO(b/77597706): make this work for destructuring? + // maybeDeclareAliasType(lValue, rValue, rValueType); + if (rValueType != null) { + return rValueType; + } } } @@ -1722,6 +1758,7 @@ && shouldUseFunctionLiteralType( * as a type name, depending on what other.name is defined to be. */ private void maybeDeclareAliasType(Node lValue, Node rValue, JSType rValueType) { + // TODO(b/77597706): handle destructuring here if (!lValue.isQualifiedName() || !rValue.isQualifiedName()) { return; } @@ -1980,7 +2017,7 @@ void maybeDeclareQualifiedName(NodeTraversal t, JSDocInfo info, // about getting as much type information as possible for them. // Determining type for #1 + #2 + #3 + #4 - JSType valueType = getDeclaredType(info, n, rhsValue); + JSType valueType = getDeclaredType(info, n, rhsValue, null); if (valueType == null && rhsValue != null) { // Determining type for #5 valueType = rhsValue.getJSType(); diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java index c9e40f3da14..b1d6d518bbc 100644 --- a/test/com/google/javascript/jscomp/NodeUtilTest.java +++ b/test/com/google/javascript/jscomp/NodeUtilTest.java @@ -2560,8 +2560,16 @@ public void testIsConstantDeclaration() { assertIsConstantDeclaration(true, parse("var /** @const */ x = 1;").getFirstFirstChild()); assertIsConstantDeclaration(false, parse("var x, /** @const */ y = 1;").getFirstFirstChild()); - // TODO(b/77597706): Update this NodeUtil.isConstantDeclaration() to handle destructured - // declarations. + assertIsConstantDeclaration(true, getNameNodeFrom("const [a] = [];", "a")); + assertIsConstantDeclaration(true, getNameNodeFrom("const [[[a]]] = [];", "a")); + assertIsConstantDeclaration(true, getNameNodeFrom("const [a = 1] = [];", "a")); + assertIsConstantDeclaration(true, getNameNodeFrom("const [...a] = [];", "a")); + + assertIsConstantDeclaration(true, getNameNodeFrom("const {a} = {};", "a")); + assertIsConstantDeclaration(true, getNameNodeFrom("const {a = 1} = {};", "a")); + assertIsConstantDeclaration(true, getNameNodeFrom("const {[3]: a} = {};", "a")); + assertIsConstantDeclaration(true, getNameNodeFrom("const {a: [a]} = {};", "a")); + // TODO(bradfordcsmith): Add test cases for other coding conventions. } diff --git a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java index 73fb54c298b..231ade0aca2 100644 --- a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java @@ -144,15 +144,18 @@ public void process(Node externs, Node root) { } public void testVarDeclarationWithJSDocForObjPatWithOneVariable() { + // Ignore JSDoc on a destructuring declaration + // TODO(b/111938518): warn for this in CheckJSDoc instead of silently ignoring it. testSame("/** @type {number} */ const {a} = {a: 1};"); TypedVar aVar = checkNotNull(globalScope.getVar("a")); - assertType(aVar.getType()).toStringIsEqualTo("number"); - assertFalse(aVar.isTypeInferred()); + assertType(aVar.getType()).toStringIsEqualTo("?"); } public void testVarDeclarationWithJSDocForObjPatWithMultipleVariables() { - // We don't allow statement-level JSDoc when multiple variables are declared. - testWarning("/** @type {number} */ const {a, b} = {a: 1};", TypeCheck.MULTIPLE_VAR_DEF); + // Ignore JSDoc on a destructuring declaration + testSame("/** @type {number} */ const {a, b} = {a: 1};"); + TypedVar aVar = checkNotNull(globalScope.getVar("a")); + assertType(aVar.getType()).toStringIsEqualTo("?"); } public void testVarDeclarationObjPatShorthandProp() { @@ -237,11 +240,15 @@ public void testConstDeclarationObjectPatternInfersType_forAliasedConstructor() "ns.Foo = function() {}", "", "const {Foo} = ns;", - "// const /** !Foo */ fooInstance = new Foo();")); + "const /** !Foo */ fooInstance = new Foo();")); TypedVar fooVar = checkNotNull(globalScope.getVar("Foo")); - // TODO(b/77597706): treat `Foo` as an alias type for `ns.Foo` - assertNull(fooVar.getType()); // type should be `function(new:ns.Foo): undefined` + assertType(fooVar.getType()).toStringIsEqualTo("function(new:ns.Foo): undefined"); + assertFalse(fooVar.isTypeInferred()); + + TypedVar fooInstanceVar = checkNotNull(globalScope.getVar("fooInstance")); + assertType(fooInstanceVar.getType()).toStringIsEqualTo("ns.Foo"); + assertFalse(fooInstanceVar.isTypeInferred()); } public void testConstDeclarationObjectPatternInfersType() { @@ -251,9 +258,33 @@ public void testConstDeclarationObjectPatternInfersType() { "const /** {a: number} */ obj = {a: 3};", // preserve newline "const {a} = obj;")); - // TODO(b/77597706): Infer `a` to have type number TypedVar aVar = checkNotNull(globalScope.getVar("a")); - assertNull(aVar.getType()); + assertType(aVar.getType()).toStringIsEqualTo("number"); + assertFalse(aVar.isTypeInferred()); + } + + public void testConstDeclarationObjectPatternInfersTypeGivenComputedProperty() { + disableTypeInfoValidation(); + testSame( + lines( + "const /** !IObject */ obj = {a: 3};", // preserve newline + "const {['foobar']: a} = obj;")); + + TypedVar aVar = checkNotNull(globalScope.getVar("a")); + assertType(aVar.getType()).toStringIsEqualTo("number"); + assertFalse(aVar.isTypeInferred()); + } + + public void testConstDeclarationObjectPatternInfersTypeGivenUnknownComputedProperty() { + disableTypeInfoValidation(); + testSame( + lines( + "var obj = {};", // preserve newline + "const {['foobar']: a} = obj;")); + + TypedVar aVar = checkNotNull(globalScope.getVar("a")); + assertType(aVar.getType()).toStringIsEqualTo("?"); + assertFalse(aVar.isTypeInferred()); } public void testConstDeclarationArrayPatternInfersType() {