From 9b17382220087cee05e86da98b407b1edbb39e10 Mon Sep 17 00:00:00 2001 From: lharker Date: Wed, 22 Aug 2018 18:13:16 -0700 Subject: [PATCH] Fix some bugs with default and destructuring parameters in FunctionTypeBuilder - 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 --- .../jscomp/FunctionTypeBuilder.java | 63 +++++++++---------- .../jscomp/TypeCheckNoTranspileTest.java | 35 +++++++++++ 2 files changed, 64 insertions(+), 34 deletions(-) diff --git a/src/com/google/javascript/jscomp/FunctionTypeBuilder.java b/src/com/google/javascript/jscomp/FunctionTypeBuilder.java index a362f18f981..d768e478269 100644 --- a/src/com/google/javascript/jscomp/FunctionTypeBuilder.java +++ b/src/com/google/javascript/jscomp/FunctionTypeBuilder.java @@ -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)); } } @@ -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; @@ -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); @@ -678,38 +686,25 @@ private void setConstructorTemplateTypeNames(List 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 buildTemplateTypesFromJSDocInfo( diff --git a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java index ef964861d50..8cc5a0d7f90 100644 --- a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java @@ -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(