Skip to content

Commit

Permalink
Start handling destructuring parameters in FunctionTypeBuilder
Browse files Browse the repository at this point in the history 
Our current system for annotating destructuring parameters (transpiling before typechecking) is sort of hacky. When transpiling the nth parameter,assuming it's destructuring, we associate the nth parameter in the AST with the nth parameter in the JSDoc, and rename the parameter to match the JSDoc.

This CL doesn't handle declaring named destructuring parameters inside a function scope or inline JSDoc on destructuring parameters.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=204940576
  • Loading branch information
@lauraharker @tjgq
lauraharker authored and tjgq committed Jul 18, 2018
1 parent 1a1e274 commit e5a6a10
Show file tree
Hide file tree
Showing 4 changed files with 185 additions and 18 deletions.
52 changes: 35 additions & 17 deletions src/com/google/javascript/jscomp/FunctionTypeBuilder.java
View file
Expand Up @@ -18,6 +18,7 @@

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Strings.nullToEmpty;
import static com.google.javascript.jscomp.TypeCheck.BAD_IMPLEMENTED_TYPE;
import static com.google.javascript.rhino.jstype.JSTypeNative.FUNCTION_FUNCTION_TYPE;
Expand Down Expand Up @@ -529,8 +530,8 @@ FunctionTypeBuilder inferParameterTypes(JSDocInfo info) {
}

/** Infer the parameter types from the list of parameter names and the JSDoc info. */
FunctionTypeBuilder inferParameterTypes(@Nullable Node argsParent, @Nullable JSDocInfo info) {
if (argsParent == null) {
FunctionTypeBuilder inferParameterTypes(@Nullable Node paramsParent, @Nullable JSDocInfo info) {
if (paramsParent == null) {
if (info == null) {
return this;
} else {
Expand All @@ -549,30 +550,41 @@ FunctionTypeBuilder inferParameterTypes(@Nullable Node argsParent, @Nullable JSD
Set<String> allJsDocParams =
(info == null) ? new HashSet<>() : new HashSet<>(info.getParameterNames());
boolean isVarArgs = false;
for (Node arg : argsParent.children()) {
int paramIndex = 0;
for (Node param : paramsParent.children()) {
boolean isOptionalParam = false;
if (arg.isDefaultValue()) {
if (param.isDefaultValue()) {
// The first child is the actual positional parameter
arg = checkNotNull(arg.getFirstChild(), arg);
param = checkNotNull(param.getFirstChild(), param);
}
if (arg.isRest()) {
if (param.isRest()) {
isVarArgs = true;
arg = arg.getOnlyChild();
param = param.getOnlyChild();
} else {
isVarArgs = isVarArgsParameter(arg, info);
isOptionalParam = isOptionalParameter(arg, info);
isVarArgs = isVarArgsParameter(param, info);
isOptionalParam = isOptionalParameter(param, info);
}

String argumentName = arg.getString();
allJsDocParams.remove(argumentName);
String paramName = null;
if (param.isName()) {
paramName = param.getString();
} else {
checkState(param.isDestructuringPattern());
// Right now, the only way to match a JSDoc param to a destructuring parameter is through
// ordering the JSDoc parameters. So the third formal parameter will correspond to the
// third JSDoc parameter.
if (info != null) {
paramName = info.getParameterNameAt(paramIndex);
}
}
allJsDocParams.remove(paramName);

// type from JSDocInfo
JSType parameterType = null;
if (info != null && info.hasParameterType(argumentName)) {
parameterType =
info.getParameterType(argumentName).evaluate(templateScope, typeRegistry);
} else if (arg.getJSDocInfo() != null && arg.getJSDocInfo().hasType()) {
JSTypeExpression parameterTypeExpression = arg.getJSDocInfo().getType();
if (info != null && info.hasParameterType(paramName)) {
parameterType = info.getParameterType(paramName).evaluate(templateScope, typeRegistry);
} else if (param.getJSDocInfo() != null && param.getJSDocInfo().hasType()) {
JSTypeExpression parameterTypeExpression = param.getJSDocInfo().getType();
parameterType = parameterTypeExpression.evaluate(templateScope, typeRegistry);
isOptionalParam = parameterTypeExpression.isOptionalArg();
isVarArgs = parameterTypeExpression.isVarArgs();
Expand All @@ -593,8 +605,8 @@ FunctionTypeBuilder inferParameterTypes(@Nullable Node argsParent, @Nullable JSD
if (oldParameterType != null) {
oldParameterType = oldParameterType.getNext();
}
paramIndex++;
}

// Copy over any old parameters that aren't in the param list.
if (!isVarArgs) {
while (oldParameterType != null && !isVarArgs) {
Expand Down Expand Up @@ -669,6 +681,9 @@ private void setConstructorTemplateTypeNames(List<TemplateType> templates, @Null
* @return Whether the given param is an optional param.
*/
private boolean isOptionalParameter(Node param, @Nullable JSDocInfo info) {
if (param.isDestructuringPattern()) {
return false;
}
if (codingConvention.isOptionalParameter(param)) {
return true;
}
Expand All @@ -684,6 +699,9 @@ private boolean isOptionalParameter(Node param, @Nullable JSDocInfo info) {
*/
private boolean isVarArgsParameter(
Node param, @Nullable JSDocInfo info) {
if (param.isDestructuringPattern()) {
return false;
}
if (codingConvention.isVarArgsParameter(param)) {
return true;
}
Expand Down
5 changes: 4 additions & 1 deletion src/com/google/javascript/jscomp/TypeInference.java
View file
Expand Up @@ -167,7 +167,10 @@ private void inferParameters(TypedScope functionScope) {
// set astParameter = restParamName
astParameter = astParameter.getOnlyChild();
}
checkState(astParameter.isName(), astParameter);
if (!astParameter.isName()) {
// TODO(lharker): support destructuring parameters
continue;
}
TypedVar var = functionScope.getVar(astParameter.getString());
checkNotNull(var);
if (var.isTypeInferred() && var.getType() == unknownType) {
Expand Down
5 changes: 5 additions & 0 deletions src/com/google/javascript/jscomp/TypedScopeCreator.java
View file
Expand Up @@ -2423,6 +2423,11 @@ void declareArguments() {
if (astParameter.isDefaultValue()) {
astParameter = astParameter.getFirstChild();
}

if (!astParameter.isName()) {
// TODO(lharker): support destructuring ast parameters
continue;
}
JSType paramType = jsDocParameter == null ? unknownType : jsDocParameter.getJSType();
boolean inferred = paramType == null || paramType.equals(unknownType);
if (isRestParameter) {
Expand Down
141 changes: 141 additions & 0 deletions test/com/google/javascript/jscomp/TypedScopeCreatorTest.java
View file
Expand Up @@ -408,6 +408,146 @@ public void testDefaultParameterFullJSDoc_setToUndefined() {
.toStringIsEqualTo("(string|undefined)");
}

public void testDestructuringParameterWithNoJSDoc() {
testSame("function f([x, y], {z}) {}");

TypedVar fVar = checkNotNull(globalScope.getVar("f"));
assertType(fVar.getType()).toStringIsEqualTo("function(?, ?): undefined");
assertFalse(fVar.isTypeInferred());
}

public void testArrayPatternParameterWithFullJSDoc() {
testSame(
lines(
"/**",
" * @param {string} x",
" * @param {!Iterable<number>} arr",
" */",
"function f(x, [y]) {}"));

TypedVar fVar = checkNotNull(globalScope.getVar("f"));
assertType(fVar.getType()).toStringIsEqualTo("function(string, Iterable<number>): undefined");
assertFalse(fVar.isTypeInferred());

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

// TODO(b/77597706): declare y in the function scope
TypedVar yVar = lastFunctionScope.getVar("y");
assertNull(yVar);
// assertType(yVar.getType()).toStringIsEqualTo("number");
// assertFalse(yVar.isTypeInferred());
}

public void testArrayPatternParameterWithRestWithFullJSDoc() {
testSame("/** @param {!Iterable<number>} arr */ function f([x, ...y]) {}");

TypedVar fVar = checkNotNull(globalScope.getVar("f"));
assertType(fVar.getType()).toStringIsEqualTo("function(Iterable<number>): undefined");
assertFalse(fVar.isTypeInferred());
}

public void testObjectPatternParameterWithFullJSDoc() {
testSame("/** @param {{a: string, b: number}} arr */ function f({a, b}) {}");

TypedVar fVar = checkNotNull(globalScope.getVar("f"));
assertType(fVar.getType()).toStringIsEqualTo("function({a: string, b: number}): undefined");
assertFalse(fVar.isTypeInferred());
}

public void testObjectPatternParameterWithUnknownPropertyWithFullJSDoc() {
testSame("/** @param {{a: string}} arr */ function f({a, b}) {}");

TypedVar fVar = checkNotNull(globalScope.getVar("f"));
assertType(fVar.getType()).toStringIsEqualTo("function({a: string}): undefined");
assertFalse(fVar.isTypeInferred());
}

public void testNestedPatternParameterWithFullJSDoc() {
testSame(
lines(
"/**",
" * @param {string} x",
" * @param {{a: !Iterable<number>}} obj",
" * @param {!Iterable<{z: null}>} arr",
" */",
"function f(x, {a: [y]}, [{z}]) {}"));

TypedVar fVar = checkNotNull(globalScope.getVar("f"));
assertType(fVar.getType())
.toStringIsEqualTo(
"function(string, {a: Iterable<number>}, Iterable<{z: null}>): undefined");
assertFalse(fVar.isTypeInferred());

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

public void testObjectPatternParameterWithComputedPropertyWithFullJSDoc() {
// TODO(lharker): re-enable type info validation. it's currently failing on the computed
// property because TypeInference doesn't traverse the destructuring pattern.
disableTypeInfoValidation();
testSame(
lines(
"/**",
" * @param {string} x",
" * @param {!Object<string, number>} arr",
" */",
"function f(x, {['foobar' + 3]: a}) {}"));

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

public void testOutOfOrderJSDocForDestructuringParameter() {
// Even though regular JSDoc parameters can be out of order, there's currently no way for
// putting arbitrary orders on destructuring parameters.
testWarning(
lines(
"/**",
" * @param {!Iterable<number>} arr",
" * @param {string} x",
" */",
"function f(x, [y, z]) {}"),
FunctionTypeBuilder.INEXISTENT_PARAM);

TypedVar fVar = checkNotNull(globalScope.getVar("f"));
// TODO(b/77597706): it would make more sense for this to be function(string, ?) instead
assertType(fVar.getType()).toStringIsEqualTo("function(string, string): undefined");
assertFalse(fVar.isTypeInferred());
}

public void testObjectPatternParameterWithInlineJSDoc() {
testSame("function f({/** number */ x}) {}");

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

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

// TODO(lharker): 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());
}

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
// having tuple types.
TypedVar fVar = checkNotNull(globalScope.getVar("f"));
assertType(fVar.getType()).toStringIsEqualTo("function(?): undefined");
assertFalse(fVar.isTypeInferred());
}

public void testStubProperty() {
testSame("function Foo() {}; Foo.bar;");
ObjectType foo = (ObjectType) globalScope.getVar("Foo").getType();
Expand Down Expand Up @@ -3032,3 +3172,4 @@ private ObjectType getNativeObjectType(JSTypeNative type) {
return (ObjectType) registry.getNativeType(type);
}
}

0 comments on commit e5a6a10

Please sign in to comment.