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

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.
+     *
+     * <p>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.
+     *
+     * <p>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: <a
+     * href="https://github.com/google/closure-compiler/issues/1781">relevant issue</a>
+     */
     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);
         }
       }
     }

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.
+   *
+   * <p>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<!SomeName>} 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<T>}",
+            " * @template T",
+            " */",
+            "function TemplatizedClass() {",
+            "}",
+            "/** @interface @extends {TemplatizedClass<number>} */",
+            "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