Skip to content

Commit

Permalink
Fix some bugs in TypedScopeCreator around destructuring
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lauraharker authored and blickly committed Aug 31, 2018
1 parent 9ae499f commit 4aa1db6
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 16 deletions.
66 changes: 52 additions & 14 deletions src/com/google/javascript/jscomp/TypedScopeCreator.java
Expand Up @@ -866,20 +866,42 @@ void defineVarChild(JSDocInfo declarationInfo, Node child, TypedScope scope) {
}
}

@Nullable
private JSType inferTypeForDestructuredTarget(
DestructuredTarget target, Supplier<JSType> 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<JSType> 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<JSType> typeSupplier =
(target.hasStringKey() && !target.hasDefaultValue())
? target.getInferredTypeSupplier()
: () -> null;
() -> inferTypeForDestructuredTarget(target, patternTypeSupplier);

if (target.getNode().isDestructuringPattern()) {
defineDestructuringPatternInVarDeclaration(target.getNode(), scope, typeSupplier);
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -1652,6 +1668,28 @@ void defineSlot() {
}
}

/**
* Validates that anything typed as an enum is initialized to either a qualified name or another
* enum.
*
* <p>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
Expand Down
23 changes: 23 additions & 0 deletions test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java
Expand Up @@ -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'(){} };");
}
Expand Down
35 changes: 33 additions & 2 deletions test/com/google/javascript/jscomp/TypedScopeCreatorTest.java
Expand Up @@ -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());
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit 4aa1db6

Please sign in to comment.