Skip to content

Commit

Permalink
Fix bugs relating to destructuring in TypedScopeCreator
Browse files Browse the repository at this point in the history 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=214668304
  • Loading branch information
@lauraharker @brad4d
lauraharker authored and brad4d committed Sep 27, 2018
1 parent c58eb9a commit c0967e0
Show file tree
Hide file tree
Showing 2 changed files with 173 additions and 42 deletions.
114 changes: 75 additions & 39 deletions src/com/google/javascript/jscomp/TypedScopeCreator.java
View file
Expand Up @@ -224,6 +224,21 @@ void resolve() {
}
}

/** Stores the type and qualified name for a destructuring rvalue, which has no AST node */
private static class RValueInfo {
@Nullable final JSType type;
@Nullable final String qualifiedName;

private RValueInfo(JSType type, String qualifiedName) {
this.type = type;
this.qualifiedName = qualifiedName;
}

private static RValueInfo empty() {
return new RValueInfo(null, null);
}
}

TypedScopeCreator(AbstractCompiler compiler) {
this(compiler, compiler.getCodingConvention());
}
Expand Down Expand Up @@ -860,15 +875,24 @@ void defineVarChild(JSDocInfo declarationInfo, Node child, TypedScope scope) {
defineDestructuringPatternInVarDeclaration(
pattern,
scope,
// Note that value will be null if we are in an enhanced for loop
// for (const {x, y} of data) {
() -> value != null ? getDeclaredRValueType(/* lValue= */ null, value) : unknownType);
() ->
// Note that value will be null if we are in an enhanced for loop
// for (const {x, y} of data) {
value != null
? new RValueInfo(
getDeclaredRValueType(/* lValue= */ null, value), value.getQualifiedName())
: new RValueInfo(unknownType, /* qualifiedName= */ null));
}
}

@Nullable
private JSType inferTypeForDestructuredTarget(
DestructuredTarget target, Supplier<JSType> patternTypeSupplier) {
/**
* Returns information about the qualified name and type of the target, if it exists.
*
* <p>Never returns null, but will return an RValueInfo with null `type` and `qualifiedName`
* slots.
*/
private RValueInfo inferTypeForDestructuredTarget(
DestructuredTarget target, Supplier<RValueInfo> patternTypeSupplier) {
// Currently we only do type inference for string key nodes in object patterns here, to
// handle aliasing types. e.g
// const {Foo} = ns;
Expand All @@ -878,29 +902,30 @@ private JSType inferTypeForDestructuredTarget(
// 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;
return RValueInfo.empty();
}
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();
RValueInfo rvalue = patternTypeSupplier.get();
JSType patternType = rvalue.type;
String propertyName = target.getStringKey().getString();
if (patternObjectType.hasProperty(propertyName)
&& !patternObjectType.isPropertyTypeInferred(propertyName)) {
return patternObjectType.getPropertyType(propertyName);
String qualifiedName =
rvalue.qualifiedName != null ? rvalue.qualifiedName + "." + propertyName : null;
if (patternType == null || patternType.isUnknownType()) {
return new RValueInfo(null, qualifiedName);
}
return null;
if (patternType.hasProperty(propertyName)) {
JSType type = patternType.findPropertyType(propertyName);
return new RValueInfo(type, qualifiedName);
}
return new RValueInfo(null, qualifiedName);
}

void defineDestructuringPatternInVarDeclaration(
Node pattern, TypedScope scope, Supplier<JSType> patternTypeSupplier) {
Node pattern, TypedScope scope, Supplier<RValueInfo> patternTypeSupplier) {
for (DestructuredTarget target :
DestructuredTarget.createAllNonEmptyTargetsInPattern(
typeRegistry, patternTypeSupplier, pattern)) {
typeRegistry, () -> patternTypeSupplier.get().type, pattern)) {

Supplier<JSType> typeSupplier =
Supplier<RValueInfo> typeSupplier =
() -> inferTypeForDestructuredTarget(target, patternTypeSupplier);

if (target.getNode().isDestructuringPattern()) {
Expand Down Expand Up @@ -1809,7 +1834,7 @@ JSType getDeclaredType(
JSDocInfo info,
Node lValue,
@Nullable Node rValue,
@Nullable Supplier<JSType> declaredRValueTypeSupplier) {
@Nullable Supplier<RValueInfo> declaredRValueTypeSupplier) {
if (info != null && info.hasType()) {
return getDeclaredTypeInAnnotation(lValue, info);
} else if (rValue != null
Expand All @@ -1835,16 +1860,17 @@ && shouldUseFunctionLiteralType(
if (NodeUtil.isConstantDeclaration(compiler.getCodingConvention(), info, lValue)) {
if (rValue != null) {
JSType rValueType = getDeclaredRValueType(lValue, rValue);
maybeDeclareAliasType(lValue, rValue, rValueType);
maybeDeclareAliasType(lValue, rValue.getQualifiedName(), 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;
RValueInfo rvalueInfo = declaredRValueTypeSupplier.get();
if (rvalueInfo != null) {
maybeDeclareAliasType(lValue, rvalueInfo.qualifiedName, rvalueInfo.type);
if (rvalueInfo.type != null) {
return rvalueInfo.type;
}
}
}
}
Expand All @@ -1858,16 +1884,17 @@ && shouldUseFunctionLiteralType(
}

/**
* For a const alias, like `const alias = other.name`, this may declare `alias`
* as a type name, depending on what other.name is defined to be.
* For a const alias, like `const alias = other.name`, this may declare `alias` as a type name,
* depending on what other.name is defined to be.
*
* @param rvalueName the rvalue's qualified name if it exists, null otherwise
*/
private void maybeDeclareAliasType(Node lValue, Node rValue, JSType rValueType) {
private void maybeDeclareAliasType(Node lValue, String rvalueName, JSType rValueType) {
// NOTE: this allows some strange patterns such allowing instance properties
// to be aliases of constructors, and then creating a local alias of that to be
// used as a type name. Consider restricting this.

// TODO(b/77597706): handle destructuring here
if (!lValue.isQualifiedName() || !rValue.isQualifiedName()) {
if (!lValue.isQualifiedName() || (rvalueName == null)) {
return;
}
// Treat @const-annotated aliases like @constructor/@interface if RHS has instance type
Expand All @@ -1879,7 +1906,7 @@ private void maybeDeclareAliasType(Node lValue, Node rValue, JSType rValueType)
currentScope, lValue.getQualifiedName(), functionType.getInstanceType());
} else {
// Also infer a type name for aliased @typedef
JSType rhsNamedType = typeRegistry.getType(currentScope, rValue.getQualifiedName());
JSType rhsNamedType = typeRegistry.getType(currentScope, rvalueName);
if (rhsNamedType != null) {
typeRegistry.declareType(currentScope, lValue.getQualifiedName(), rhsNamedType);
}
Expand Down Expand Up @@ -2737,12 +2764,21 @@ private void declareDestructuringParameter(
if (target.getNode().isDestructuringPattern()) {
declareDestructuringParameter(isInferred, target.getNode(), inferredType);
} else {
checkState(
target.getNode().isName(),
"Expected all parameters to be names, got %s",
target.getNode());

declareSingleParameterName(isInferred, target.getNode(), inferredType);
Node paramName = target.getNode();
checkState(paramName.isName(), "Expected all parameters to be names, got %s", paramName);

if ((inferredType == null || inferredType.isUnknownType())
&& paramName.getJSDocInfo() != null
&& paramName.getJSDocInfo().hasType()) {
// see if the parameter has its own inline JSDoc
// TODO(b/112651122): this should happen inside FunctionTypeBuilder, so that we can
// check that calls to the function match the inline JSDoc.
inferredType =
typeRegistry.evaluateTypeExpression(
paramName.getJSDocInfo().getType(), currentScope);
isInferred = false;
}
declareSingleParameterName(isInferred, paramName, inferredType);
}
}
}
Expand Down
101 changes: 98 additions & 3 deletions test/com/google/javascript/jscomp/TypedScopeCreatorTest.java
View file
Expand Up @@ -276,6 +276,26 @@ public void testConstDeclarationObjectPatternInfersType_forAliasedConstructor()
assertFalse(fooInstanceVar.isTypeInferred());
}

@Test
public void testConstDeclarationObjectPatternInfersType_forAliasedTypedef() {
testSame(
lines(
"const ns = {};",
"/** @typedef {string} */ ns.Foo;",
"const {Foo} = ns;",
"let /** !Foo */ f = 'bar';"));

TypedVar fooVar = checkNotNull(globalScope.getVar("Foo"));
assertType(fooVar.getType()).toStringIsEqualTo("None");

JSType fooType = registry.getGlobalType("Foo");
assertType(fooType).isEqualTo(getNativeType(JSTypeNative.STRING_TYPE));

TypedVar fooInstanceVar = checkNotNull(globalScope.getVar("f"));
assertType(fooInstanceVar.getType()).toStringIsEqualTo("string");
assertFalse(fooInstanceVar.isTypeInferred());
}

@Test
public void testConstDeclarationObjectPatternInfersTypeAsDeclared() {
testSame(
Expand Down Expand Up @@ -941,31 +961,106 @@ public void testOutOfOrderJSDocForDestructuringParameter() {
public void testObjectPatternParameterWithInlineJSDoc() {
testSame("function f({/** number */ x}) {}");

// TODO(lharker): infer that f takes {x: number}
// TODO(b/112651122): infer that f takes {x: number}
TypedVar fVar = checkNotNull(globalScope.getVar("f"));
assertType(fVar.getType()).toStringIsEqualTo("function(?): undefined");
assertFalse(fVar.isTypeInferred());

TypedVar xVar = checkNotNull(lastFunctionScope.getVar("x"));
assertType(xVar.getType()).toStringIsEqualTo("number");
assertFalse(xVar.isTypeInferred());
}

@Test
public void testArrayPatternParameterWithInlineJSDoc() {
testSame("function f([/** number */ x]) {}");

// TODO(lharker): either forbid this case or infer that f takes an !Iterable<number>
// TODO(b/112651122): either forbid this case or infer that f takes an !Iterable<number>
TypedVar fVar = checkNotNull(globalScope.getVar("f"));
assertType(fVar.getType()).toStringIsEqualTo("function(?): undefined");
assertFalse(fVar.isTypeInferred());
assertFalse(fVar.isTypeInferred());

TypedVar xVar = checkNotNull(lastFunctionScope.getVar("x"));
assertType(xVar.getType()).toStringIsEqualTo("number");
assertFalse(xVar.isTypeInferred());
}

@Test
public void testArrayPatternParametersWithDifferingInlineJSDoc() {
testSame("function f([/** number */ x, /** string */ y]) {}");

// TODO(lharker): forbid this case, as there's not a good way to type the function without
// TODO(b/112651122): forbid this case, as there's not a good way to type the function without
// having tuple types.
TypedVar fVar = checkNotNull(globalScope.getVar("f"));
assertType(fVar.getType()).toStringIsEqualTo("function(?): undefined");
assertFalse(fVar.isTypeInferred());

TypedVar xVar = checkNotNull(lastFunctionScope.getVar("x"));
assertType(xVar.getType()).toStringIsEqualTo("number");
assertFalse(xVar.isTypeInferred());

TypedVar yVar = checkNotNull(lastFunctionScope.getVar("y"));
assertType(yVar.getType()).toStringIsEqualTo("string");
assertFalse(yVar.isTypeInferred());
}

@Test
public void testObjectPatternCanAliasUnionTypeProperty() {
// tests that we can get properties off the union type `{objA|objB}` even though it's not
// represented as an ObjectType. (this used to crash in TypedScopeCreator)
testSame(
lines(
"const ns = {}; ",
"",
"/** @typedef {{propA: number}} */",
"let objA;",
"/** @typedef {{propB: string}} */",
"let objB;",
"",
"/** @type {objA|objB} */",
"ns.ctor;",
"",
"const {propA, propB} = ns.ctor;"));

TypedVar propAVar = checkNotNull(globalScope.getVar("propA"));
assertType(propAVar.getType()).isEqualTo(getNativeType(JSTypeNative.NUMBER_TYPE));
assertFalse(propAVar.isTypeInferred());

TypedVar propBVar = checkNotNull(globalScope.getVar("propB"));
assertType(propBVar.getType()).isEqualTo(getNativeType(JSTypeNative.STRING_TYPE));
assertFalse(propBVar.isTypeInferred());
}

@Test
public void testConstNestedObjectPatternInArrayPattern() {
// regression test for a TypedScopeCreator crash
disableTypeInfoValidation();
testSame(
lines(
"const /** !Array<{b: string}> */ a = [{b: 'bbb'}];", //
"const [{b}] = a;"));

TypedVar b = checkNotNull(globalScope.getVar("b"));
assertType(b.getType()).isUnknown();
assertTrue(b.isTypeInferred()); // b is inferred but not treated as declared
}

@Test
public void testConstNestedObjectPatternWithComputedPropertyIsUnknown() {
// regression test for a TypedScopeCreator crash
disableTypeInfoValidation(); // fails on a['foo'].b = function() {};
testSame(
lines(
"const a = {};",
"/** @const */ a['foo'] = {};",
"/** @const */ a['foo'].b = function() {};",
"",
"const {['foo']: {b}} = a;"));

TypedVar b = checkNotNull(globalScope.getVar("b"));
assertType(b.getType()).isUnknown();
assertTrue(b.isTypeInferred());
}

@Test
Expand Down

0 comments on commit c0967e0

Please sign in to comment.