From e5a6a1000d81ad11f2a25d87c0b56be4c0844b7d Mon Sep 17 00:00:00 2001 From: lharker Date: Tue, 17 Jul 2018 11:11:06 -0700 Subject: [PATCH] Start handling destructuring parameters in FunctionTypeBuilder 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 --- .../jscomp/FunctionTypeBuilder.java | 52 ++++--- .../javascript/jscomp/TypeInference.java | 5 +- .../javascript/jscomp/TypedScopeCreator.java | 5 + .../jscomp/TypedScopeCreatorTest.java | 141 ++++++++++++++++++ 4 files changed, 185 insertions(+), 18 deletions(-) diff --git a/src/com/google/javascript/jscomp/FunctionTypeBuilder.java b/src/com/google/javascript/jscomp/FunctionTypeBuilder.java index 1ce8e66f110..51182fd47b6 100644 --- a/src/com/google/javascript/jscomp/FunctionTypeBuilder.java +++ b/src/com/google/javascript/jscomp/FunctionTypeBuilder.java @@ -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; @@ -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 { @@ -549,30 +550,41 @@ FunctionTypeBuilder inferParameterTypes(@Nullable Node argsParent, @Nullable JSD Set 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(); @@ -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) { @@ -669,6 +681,9 @@ private void setConstructorTemplateTypeNames(List 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; } @@ -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; } diff --git a/src/com/google/javascript/jscomp/TypeInference.java b/src/com/google/javascript/jscomp/TypeInference.java index 7039d9b401f..befccab9b45 100644 --- a/src/com/google/javascript/jscomp/TypeInference.java +++ b/src/com/google/javascript/jscomp/TypeInference.java @@ -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) { diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java index 981c2252cc6..ab6aed9c783 100644 --- a/src/com/google/javascript/jscomp/TypedScopeCreator.java +++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java @@ -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) { diff --git a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java index be37d4c8d9e..04644f88771 100644 --- a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java @@ -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} arr", + " */", + "function f(x, [y]) {}")); + + TypedVar fVar = checkNotNull(globalScope.getVar("f")); + assertType(fVar.getType()).toStringIsEqualTo("function(string, Iterable): 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} arr */ function f([x, ...y]) {}"); + + TypedVar fVar = checkNotNull(globalScope.getVar("f")); + assertType(fVar.getType()).toStringIsEqualTo("function(Iterable): 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}} 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}, 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} 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} 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 + 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(); @@ -3032,3 +3172,4 @@ private ObjectType getNativeObjectType(JSTypeNative type) { return (ObjectType) registry.getNativeType(type); } } +