From 4aa1db6dcaab605b356133bf2005a12cada46ff0 Mon Sep 17 00:00:00 2001 From: lharker Date: Thu, 30 Aug 2018 18:54:30 -0700 Subject: [PATCH] Fix some bugs in TypedScopeCreator around destructuring 1. allow people to use object destructuring to alias @enum types, e.g. const {MyEnum} = a.b.c; 2. when doing basic inference of the type of 'const {Foo} = obj', only actually declare the type of Foo if there is a property Foo declared on obj. Otherwise we will treat Foo as a valid type during typed scope creation instead of emitting a warning for using Foo. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=211020584 --- .../javascript/jscomp/TypedScopeCreator.java | 66 +++++++++++++++---- .../jscomp/TypeCheckNoTranspileTest.java | 23 +++++++ .../jscomp/TypedScopeCreatorTest.java | 35 +++++++++- 3 files changed, 108 insertions(+), 16 deletions(-) diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java index 2349eab3c76..cfcdb1168ad 100644 --- a/src/com/google/javascript/jscomp/TypedScopeCreator.java +++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java @@ -866,20 +866,42 @@ void defineVarChild(JSDocInfo declarationInfo, Node child, TypedScope scope) { } } + @Nullable + private JSType inferTypeForDestructuredTarget( + DestructuredTarget target, Supplier patternTypeSupplier) { + // Currently we only do type inference for string key nodes in object patterns here, to + // handle aliasing types. e.g + // const {Foo} = ns; + // TypeInference takes care of the rest. + // Note that although DestructuredTarget includes logic for inferring types, we don't use + // it here because we only do some very limited type inference during TypedScopeCreation, + // and only return a non-null type here if we are accessing a declared property on a known + // type. + if (!target.hasStringKey() || target.hasDefaultValue()) { + return null; + } + JSType patternType = patternTypeSupplier.get(); + if (patternType == null || patternType.isUnknownType() || !patternType.isObjectType()) { + // TypeCheck should emit an error later if this is not an object type. + return null; + } + ObjectType patternObjectType = patternType.toMaybeObjectType(); + String propertyName = target.getStringKey().getString(); + if (patternObjectType.hasProperty(propertyName) + && !patternObjectType.isPropertyTypeInferred(propertyName)) { + return patternObjectType.getPropertyType(propertyName); + } + return null; + } + void defineDestructuringPatternInVarDeclaration( Node pattern, TypedScope scope, Supplier patternTypeSupplier) { for (DestructuredTarget target : DestructuredTarget.createAllNonEmptyTargetsInPattern( typeRegistry, patternTypeSupplier, pattern)) { - // Currently we only do type inference for string key nodes in object patterns here, to - // handle aliasing types. e.g - // const {Foo} = ns; - // TypeInference takes care of the rest. Supplier typeSupplier = - (target.hasStringKey() && !target.hasDefaultValue()) - ? target.getInferredTypeSupplier() - : () -> null; + () -> inferTypeForDestructuredTarget(target, patternTypeSupplier); if (target.getNode().isDestructuringPattern()) { defineDestructuringPatternInVarDeclaration(target.getNode(), scope, typeSupplier); @@ -1598,13 +1620,7 @@ void defineSlot() { allowLaterTypeInference); if (type instanceof EnumType) { - Node initialValue = newVar.getInitialValue(); - boolean isValidValue = - initialValue != null - && (initialValue.isObjectLit() || initialValue.isQualifiedName()); - if (!isValidValue) { - report(JSError.make(declarationNode, ENUM_INITIALIZER)); - } + validateEnumInitializer(newVar, declarationNode); } } @@ -1652,6 +1668,28 @@ void defineSlot() { } } + /** + * Validates that anything typed as an enum is initialized to either a qualified name or another + * enum. + * + *

Explicitly allows any name created through destructuring to be of the enum type, because + * the only way to have a destructuring name typed as an enum if it is aliasing something else. + * You cannot put the @enum JSDoc on destructuring declarations + */ + private void validateEnumInitializer(TypedVar var, Node declarationNode) { + final boolean isValidValue; + if (NodeUtil.isLhsByDestructuring(var.getNameNode())) { + isValidValue = true; + } else { + Node initialValue = var.getInitialValue(); + isValidValue = + initialValue != null && (initialValue.isObjectLit() || initialValue.isQualifiedName()); + } + if (!isValidValue) { + report(JSError.make(declarationNode, ENUM_INITIALIZER)); + } + } + /** * Declares a variable with the given {@code name} and {@code type} on the given {@code scope}, * returning the newly-declared {@link TypedVar}. Additionally checks the {@link diff --git a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java index 35398c723b4..76c2e35aa04 100644 --- a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java @@ -4275,6 +4275,29 @@ public void testArrayPatternAssignWithIllegalPropCreationInStruct() { }); } + public void testEnumAliasedThroughDestructuringPassesTypechecking() { + testTypes( + lines( + "const ns = {};", + "/** @enum {number} */", + "ns.myEnum = {FOO: 1, BAR: 2};", + "", + "const {myEnum} = ns;", + "const /** myEnum */ n = myEnum.FOO;")); + } + + public void testEnumAliasedThroughDestructuringReportsCorrectMissingPropWarning() { + testTypes( + lines( + "const ns = {};", + "/** @enum {number} */", + "ns.myEnum = {FOO: 1, BAR: 2};", + "", + "const {myEnum} = ns;", + "const missing = myEnum.MISSING;"), + "element MISSING does not exist on this enum"); + } + public void testDictClass1() { testTypes("/** @dict */ var C = class { constructor() {} 'x'(){} };"); } diff --git a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java index 2ed65df814c..1d33c73bf8d 100644 --- a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java @@ -160,7 +160,6 @@ public void testVarDeclarationWithJSDocForObjPatWithMultipleVariables() { assertType(aVar.getType()).toStringIsEqualTo("number"); assertTrue(aVar.isTypeInferred()); - // TODO(b/77597706): add a test to TypeCheck verifying we issue a missing property warning for b TypedVar bVar = checkNotNull(globalScope.getVar("b")); assertType(bVar.getType()).toStringIsEqualTo("?"); assertTrue(bVar.isTypeInferred()); @@ -310,7 +309,39 @@ public void testConstDeclarationWithOrInRhs() { TypedVar aVar = checkNotNull(globalScope.getVar("a")); assertType(aVar.getType()).toStringIsEqualTo("?"); - assertFalse(aVar.isTypeInferred()); + assertTrue(aVar.isTypeInferred()); + } + + public void testConstDestructuringDeclarationWithUnknownTypeInExtendsClause() { + disableTypeInfoValidation(); + testWarning( + externs("var someUnknownExtern;"), + srcs("const {Parent} = someUnknownExtern; class Child extends Parent {}"), + warning(RhinoErrorReporter.UNRECOGNIZED_TYPE_ERROR)); + + TypedVar parentVar = checkNotNull(globalScope.getVar("Parent")); + assertType(parentVar.getType()).isUnknown(); + + TypedVar childVar = checkNotNull(globalScope.getVar("Child")); + FunctionType childType = childVar.getType().toMaybeFunctionType(); + JSType superclassCtor = childType.getSuperClassConstructor(); + assertType(superclassCtor).isNull(); + } + + public void testConstDestructuringDeclarationWithUnknownPropertyInExtendsClause() { + disableTypeInfoValidation(); + testWarning( + externs("var /** !Object */ someUnknownExtern;"), + srcs("const {Parent} = someUnknownExtern; class Child extends Parent {}"), + warning(RhinoErrorReporter.UNRECOGNIZED_TYPE_ERROR)); + + TypedVar parentVar = checkNotNull(globalScope.getVar("Parent")); + assertType(parentVar.getType()).isUnknown(); + + TypedVar childVar = checkNotNull(globalScope.getVar("Child")); + FunctionType childType = childVar.getType().toMaybeFunctionType(); + JSType superclassCtor = childType.getSuperClassConstructor(); + assertType(superclassCtor).isNull(); } // TODO(bradfordcsmith): Add Object rest test case.