Skip to content

Commit

Permalink
Fix some bugs with default and destructuring parameters in FunctionTy…
Browse files Browse the repository at this point in the history
…peBuilder

- Infer overridden method params as optional if they have default values
- Treat destructuring parameters w/o default values as optional as long as they are marked optional in the JSDocInfo (previously they were being marked as required)

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=209863909
  • Loading branch information
lauraharker authored and blickly committed Aug 27, 2018
1 parent 27fc5d7 commit 9b17382
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 34 deletions.
63 changes: 29 additions & 34 deletions src/com/google/javascript/jscomp/FunctionTypeBuilder.java
Expand Up @@ -308,24 +308,29 @@ FunctionTypeBuilder inferFromOverriddenFunction(
Node oldParam = oldParams.next();
Node newParam = paramBuilder.newParameterFromNode(oldParam);

oldParamsListHitOptArgs = oldParamsListHitOptArgs ||
oldParam.isVarArgs() ||
oldParam.isOptionalArg();
oldParamsListHitOptArgs =
oldParamsListHitOptArgs || oldParam.isVarArgs() || oldParam.isOptionalArg();

// The subclass method might write its var_args as individual
// arguments.
// The subclass method might write its var_args as individual arguments.
if (currentParam.getNext() != null && newParam.isVarArgs()) {
newParam.setVarArgs(false);
newParam.setOptionalArg(true);
}
// The subclass method might also make a required parameter into an optional parameter
// with a default value
if (currentParam.isDefaultValue()) {
newParam.setOptionalArg(true);
}
} else {
warnedAboutArgList |= addParameter(
paramBuilder,
typeRegistry.getNativeType(UNKNOWN_TYPE),
warnedAboutArgList,
codingConvention.isOptionalParameter(currentParam) ||
oldParamsListHitOptArgs,
codingConvention.isVarArgsParameter(currentParam));
warnedAboutArgList |=
addParameter(
paramBuilder,
typeRegistry.getNativeType(UNKNOWN_TYPE),
warnedAboutArgList,
codingConvention.isOptionalParameter(currentParam)
|| oldParamsListHitOptArgs
|| currentParam.isDefaultValue(),
codingConvention.isVarArgsParameter(currentParam));
}
}

Expand Down Expand Up @@ -562,8 +567,8 @@ FunctionTypeBuilder inferParameterTypes(@Nullable Node paramsParent, @Nullable J
param = checkNotNull(param.getFirstChild(), param);
isOptionalParam = true;
} else {
isVarArgs = isVarArgsParameter(param, info);
isOptionalParam = isOptionalParameter(param, info);
isVarArgs = isVarArgsParameterByConvention(param);
isOptionalParam = isOptionalParameterByConvention(param);
}

String paramName = null;
Expand All @@ -583,7 +588,10 @@ FunctionTypeBuilder inferParameterTypes(@Nullable Node paramsParent, @Nullable J
// type from JSDocInfo
JSType parameterType = null;
if (info != null && info.hasParameterType(paramName)) {
parameterType = info.getParameterType(paramName).evaluate(templateScope, typeRegistry);
JSTypeExpression parameterTypeExpression = info.getParameterType(paramName);
parameterType = parameterTypeExpression.evaluate(templateScope, typeRegistry);
isOptionalParam = isOptionalParam || parameterTypeExpression.isOptionalArg();
isVarArgs = isVarArgs || parameterTypeExpression.isVarArgs();
} else if (param.getJSDocInfo() != null && param.getJSDocInfo().hasType()) {
JSTypeExpression parameterTypeExpression = param.getJSDocInfo().getType();
parameterType = parameterTypeExpression.evaluate(templateScope, typeRegistry);
Expand Down Expand Up @@ -678,38 +686,25 @@ private void setConstructorTemplateTypeNames(List<TemplateType> templates, @Null
}
}

/**
* @return Whether the given param is an optional param.
*/
private boolean isOptionalParameter(Node param, @Nullable JSDocInfo info) {
/** @return Whether the given param is an optional param. */
private boolean isOptionalParameterByConvention(Node param) {
if (param.isDestructuringPattern()) {
return false;
}
if (codingConvention.isOptionalParameter(param)) {
return true;
}

String paramName = param.getString();
return info != null && info.hasParameterType(paramName) &&
info.getParameterType(paramName).isOptionalArg();
return codingConvention.isOptionalParameter(param);
}

/**
* Determine whether this is a var args parameter.
*
* @return Whether the given param is a var args param.
*/
private boolean isVarArgsParameter(
Node param, @Nullable JSDocInfo info) {
private boolean isVarArgsParameterByConvention(Node param) {
if (param.isDestructuringPattern()) {
return false;
}
if (codingConvention.isVarArgsParameter(param)) {
return true;
}

String paramName = param.getString();
return info != null && info.hasParameterType(paramName) &&
info.getParameterType(paramName).isVarArgs();
return codingConvention.isVarArgsParameter(param);
}

private ImmutableList<TemplateType> buildTemplateTypesFromJSDocInfo(
Expand Down
35 changes: 35 additions & 0 deletions test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java
Expand Up @@ -3944,6 +3944,41 @@ public void testDefaultParameterWithNoJSDocTreatedAsOptional() {
testTypes("function f(a = 3) {} f();");
}

public void testOverridingMethodHasDefaultParameterInPlaceOfRequiredParam() {
testTypes(
lines(
"class Parent {",
" /** @param {number} num */",
" f(num) {}",
"}",
"class Child extends Parent {",
" /** @override */",
" f(num = undefined) {}",
"}",
"(new Child()).f();",
"(new Child()).f(undefined);"));
}

public void testOverridingMethodAddsOptionalParameterWithDefaultValue() {
testTypes(
lines(
"class Parent {",
" /** @param {number} num */",
" f(num) {}",
"}",
"class Child extends Parent {",
" /** @override */",
" f(num, otherParam = undefined) {}",
"}",
"(new Child()).f(3);",
"(new Child()).f(3, 'str');",
"(new Child()).f(3, undefined);"));
}

public void testOptionalDestructuringParameterWithoutDefaultValue() {
testTypes("/** @param {{x: number}=} opts */ function f({x}) {} f();");
}

public void testBasicArrayPatternDeclaration() {
testTypes(
lines(
Expand Down

0 comments on commit 9b17382

Please sign in to comment.