From 21aa01fc05189dab0513b5f8be5836eb12c5a09a Mon Sep 17 00:00:00 2001 From: lharker Date: Tue, 13 Nov 2018 16:30:39 -0800 Subject: [PATCH] Treat destructured parameters as inferred if their type is unknown in TypedScopeCreator This gets rid of some unknown typed parameters that were coming from looking up properties on unresolved NamedTypes. It's not ideal: we'd still prefer that these parameters have a known declared type, not known inferred type. This is meant to be a temporary solution ; in the long term, we want to switch to annotating types for individual destructuring parameters and ban giving a single type for the entire parameter. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=221355865 --- .../javascript/jscomp/TypedScopeCreator.java | 50 ++++-- .../jscomp/TypedScopeCreatorTest.java | 159 +++++++++++++++++- 2 files changed, 192 insertions(+), 17 deletions(-) diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java index ed8fbcc8284..c49c349d65d 100644 --- a/src/com/google/javascript/jscomp/TypedScopeCreator.java +++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java @@ -1900,8 +1900,6 @@ && 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. - * - * @param rvalueName the rvalue's qualified name if it exists, null otherwise */ private void maybeDeclareAliasType( Node lValue, @Nullable QualifiedName rValue, @Nullable JSType rValueType) { @@ -2795,33 +2793,53 @@ private void declareNamesInPositionalParameter( } } - /** Declares all names inside a destructuring pattern in a parameter list in the scope */ + /** + * Declares all names inside a destructuring pattern in a parameter list in the scope if we can + * find a non-unknown type for them. + * + *

Unknown typed parameters are always treated as inferred, not declared. TypeInference may + * later give them a better inferred type than unknown, but they will never become declared. + * + *

NOTE: currently, there are some less-than-ideal aspects to how we do this. If the pattern + * type is an unresolved NamedType, then we can't lookup properties on it (to find the + * individual parameter types) until after name resolution. The current state is to defer to + * TypeInference to type those parameters, with the drawback that they are 'inferred', not + * 'declared', and so any type can be assigned to them. In the future we will just enforce + * typing each parameter individually: relevant issue + */ private void declareDestructuringParameter( boolean isInferred, Node pattern, JSType patternType) { for (DestructuredTarget target : DestructuredTarget.createAllNonEmptyTargetsInPattern( typeRegistry, patternType, pattern)) { - JSType inferredType = target.inferTypeWithoutUsingDefaultValue(); + JSType parameterType = target.inferTypeWithoutUsingDefaultValue(); if (target.getNode().isDestructuringPattern()) { - declareDestructuringParameter(isInferred, target.getNode(), inferredType); + declareDestructuringParameter(isInferred, target.getNode(), parameterType); } else { 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; + if (parameterType == null || parameterType.isUnknownType()) { + JSDocInfo paramJSDoc = paramName.getJSDocInfo(); + if (paramJSDoc != null && paramJSDoc.hasType()) { + // see if the parameter has its own inline JSDoc, and use that unless we already have + // a type from @param JSDoc. + // TODO(b/112651122): this should happen inside FunctionTypeBuilder, so that we can + // check that calls to the function match the inline JSDoc. + // TODO(b/111523967): we should also report a + // warning if the inline and non-inline JSDoc conflict. + parameterType = + typeRegistry.evaluateTypeExpression(paramJSDoc.getType(), currentScope); + isInferred = false; + } else { + // note - these parameters may get better types during TypeInference + isInferred = true; + } } - declareSingleParameterName(isInferred, paramName, inferredType); + declareSingleParameterName(isInferred, paramName, parameterType); } } } diff --git a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java index 7b2a208c002..fa24e59a056 100644 --- a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java @@ -33,6 +33,7 @@ import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; +import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; import com.google.javascript.rhino.InputId; import com.google.javascript.rhino.Node; @@ -836,7 +837,163 @@ public void testObjectPatternParameterWithUnknownPropertyWithFullJSDoc() { TypedVar bVar = checkNotNull(lastFunctionScope.getVar("b")); assertType(bVar.getType()).toStringIsEqualTo("?"); - assertThat(bVar.isTypeInferred()).isFalse(); + assertThat(bVar.isTypeInferred()).isTrue(); + } + + /** + * Note: these tests expect the parameters to have inferred, not declared, types because of the + * order in which scope creation & named type resolution happens. + * + *

See also b/118710352 + */ + @Test + public void testObjectPatternParameter_withUnresolvedForwardDeclaredType_isInferred() { + // Note that this only emits one UNRECOGNIZED_TYPE_ERROR for SomeUnknownName. We don't emit + // an error for SomeUnknownName#a, as that would be redundant. + test( + srcs( + lines( + "goog.forwardDeclare('SomeUnknownName');", + "/** @param {!SomeUnknownName} obj */ function f({a}) {}")), + warning(RhinoErrorReporter.UNRECOGNIZED_TYPE_ERROR)); + + TypedVar aVar = checkNotNull(lastFunctionScope.getVar("a")); + assertType(aVar.getType()).toStringIsEqualTo("?"); + assertThat(aVar.isTypeInferred()).isTrue(); + } + + @Test + public void testObjectPatternParameterWithFullJSDoc_withMissingProperty_isInferred() { + testSame( + lines( + "class SomeName {}", // + "/** @param {!SomeName} obj */ function f({a}) {}")); + + TypedVar aVar = checkNotNull(lastFunctionScope.getVar("a")); + assertType(aVar.getType()).toStringIsEqualTo("?"); + // we still consider this a declared type + assertThat(aVar.isTypeInferred()).isTrue(); + } + + @Test + public void testNestedObjectPatternParameter_withNamedType() { + testSame( + lines( + "class SomeOtherName {", + " constructor() {", + " /** @type {number} */", + " this.a;", + " }", + "}", + "class SomeName {", + " constructor() {", + " /** @type {!SomeOtherName} */", + " this.data;", + " }", + "}", + "/** @param {!SomeName} obj */ function f({data: {a}}) { A: a; }", + "")); + + // This variable becomes inferred, even though SomeName is declared before it is referenced, + // just because the SomeName in the @param is still unresolved at type scope creation time. + TypedVar aVar = checkNotNull(getLabeledStatement("A").enclosingScope.getVar("a")); + assertType(aVar.getType()).toStringIsEqualTo("number"); + assertThat(aVar.isTypeInferred()).isTrue(); + } + + @Test + public void testTypecheckingTemplatizedTypesForObjectParameter() { + testSame( + lines( + "/**", + " * @constructor ", + " * @template T", + " */", + "function TemplatizedClass() {", + " /** @const {T} */", + " this.data;", + "}", + "/** @param {!TemplatizedClass} obj */", + "function f({data}) { A: data.a; }", + "", + "class SomeName {", + " constructor() {", + " /** @type {number} */", + " this.a;", + " }", + "}")); + + LabeledStatement aStatement = getLabeledStatement("A"); + TypedVar dataVar = checkNotNull(aStatement.enclosingScope.getVar("data")); + + assertType(dataVar.getType()).toStringIsEqualTo("SomeName"); + assertType(aStatement.statementNode.getOnlyChild().getJSType()).toStringIsEqualTo("number"); + } + + @Test + public void testTypecheckingAliasOfTypedefType_forObjectParameter() { + testSame( + lines( + "class Foo {", + " constructor() {", + " /** @type {number} */", + " this.a;", + " }", + "}", + "/** @typedef {!Foo} */", + "var TypedefOfFoo;", + "", + "const ns = {};", + "/** @const */", + "ns.TypedefOfFoo = TypedefOfFoo;", + "", + "/** @param {!ns.TypedefOfFoo} obj */", + "function f({a}) { A: a; }")); + + TypedVar aVar = checkNotNull(getLabeledStatement("A").enclosingScope.getVar("a")); + assertType(aVar.getType()).toStringIsEqualTo("number"); + assertThat(aVar.isTypeInferred()).isTrue(); + } + + @Test + public void testArrayPatternParameter_withNamedType() { + testSame( + lines( + "/**", + " * @interface", + " * @extends {Iterable}", + " * @template T", + " */", + "function TemplatizedClass() {", + "}", + "/** @interface @extends {TemplatizedClass} */", + "class ASubclass {}", + "/** @param {!ASubclass} obj */ function f([a]) { A: a; }", + "")); + + // Verify we get the correct type for 'a' after name resolution is complete + TypedVar aVar = checkNotNull(getLabeledStatement("A").enclosingScope.getVar("a")); + assertType(aVar.getType()).toStringIsEqualTo("number"); + assertThat(aVar.isTypeInferred()).isTrue(); + } + + @Test + public void testObjectPatternParameter_withNamedType_andRest() { + setAcceptedLanguage(LanguageMode.ECMASCRIPT_2018); + testSame( + lines( + "class SomeUnknownName {", + " constructor() {", + " /** @type {number} */", + " this.data;", + " }", + "}", + "/** @param {!SomeUnknownName} obj */ function f({...rest}) { REST: rest; }", + "")); + + TypedVar aVar = checkNotNull(getLabeledStatement("REST").enclosingScope.getVar("rest")); + assertType(aVar.getType()).toStringIsEqualTo("Object"); + assertThat(aVar.isTypeInferred()).isFalse(); } @Test