Remove special member collection in TypedScopeCreator.LocalScopeBuilder

As noted in the TODO, this was papering over an issue where we skipped declaring properties on instance objects.  The correct fix is to also declare the property if the owner is THIS.

This clears the way for unifying LocalScopeBuilder and GlobalScopeBuilder into a single NormalScopeBuilder.  We can then pull out FunctionScopeBuilder and ClassScopeBuilder with significantly less logic in them.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=198132934

src/com/google/javascript/jscomp/TypedScopeCreator.java

@@ -1846,11 +1846,21 @@ void maybeDeclareQualifiedName(NodeTraversal t, JSDocInfo info,
         if (ownerType != null) {
           // Only declare this as an official property if it has not been
           // declared yet.
-          boolean isExtern = t.getInput() != null && t.getInput().isExtern();
-          if ((!ownerType.hasOwnProperty(propName) || ownerType.isPropertyTypeInferred(propName))
-              && ((isExtern && !ownerType.isNativeObjectType()) || !ownerType.isInstanceType())) {
-            // If the property is undeclared or inferred, declare it now.
-            ownerType.defineDeclaredProperty(propName, valueType, n);
+          if (!ownerType.hasOwnProperty(propName) || ownerType.isPropertyTypeInferred(propName)) {
+            // Define the property if any of the following are true:
+            //   (1) it's a non-native extern type. Native types are excluded here because we don't
+            //       want externs of the form "/** @type {!Object} */ var api = {}; api.foo;" to
+            //       cause a property "foo" to be declared on Object.
+            //   (2) it's a non-instance type. This primarily covers static properties on
+            //       constructors (which are FunctionTypes, not InstanceTypes).
+            //   (3) it's an assignment to 'this', which covers instance properties assigned in
+            //       constructors or other methods.
+            boolean isNonNativeExtern =
+                t.getInput() != null && t.getInput().isExtern() && !ownerType.isNativeObjectType();
+            if (isNonNativeExtern || !ownerType.isInstanceType() || ownerNode.isThis()) {
+              // If the property is undeclared or inferred, declare it now.
+              ownerType.defineDeclaredProperty(propName, valueType, n);
+            }
           }
         }
 
@@ -2098,14 +2108,11 @@ private final class GlobalScopeBuilder extends AbstractScopeBuilder {
    * local variables.
    */
   private final class LocalScopeBuilder extends AbstractScopeBuilder {
-    private final ObjectType thisTypeForProperties;
-
     /**
      * @param scope The scope that we're building.
      */
     private LocalScopeBuilder(TypedScope scope) {
       super(scope);
-      thisTypeForProperties = getThisTypeForCollectingProperties();
     }
 
     /**
@@ -2126,59 +2133,9 @@ private LocalScopeBuilder(TypedScope scope) {
         return;
       }
 
-      // Gather the properties declared in the function,
-      // if that function has a @this type that we can
-      // build properties on.
-      // TODO(nick): It's not clear to me why this is necessary;
-      // it appears to be papering over bugs in the main analyzer.
-      if (thisTypeForProperties != null && n.getParent().isExprResult()) {
-        if (n.isAssign()) {
-          maybeCollectMember(n.getFirstChild(), n, n.getLastChild());
-        } else if (n.isGetProp()) {
-          maybeCollectMember(n, n, null);
-        }
-      }
-
       super.visit(t, n, parent);
     }
 
-    private ObjectType getThisTypeForCollectingProperties() {
-      Node rootNode = currentHoistScope.getClosestContainerScope().getRootNode();
-      if (rootNode.isFromExterns()) {
-        return null;
-      }
-
-      JSType type = rootNode.getJSType();
-      if (type == null || !type.isFunctionType()) {
-        return null;
-      }
-
-      FunctionType fnType = type.toMaybeFunctionType();
-      JSType fnThisType = fnType.getTypeOfThis();
-      return fnThisType == null ? null : fnThisType.toObjectType();
-    }
-
-    private void maybeCollectMember(Node member,
-        Node nodeWithJsDocInfo, @Nullable Node value) {
-      JSDocInfo info = nodeWithJsDocInfo.getJSDocInfo();
-
-      // Do nothing if there is no JSDoc type info, or
-      // if the node is not a member expression, or
-      // if the member expression is not of the form: this.someProperty.
-      if (info == null || !member.isGetProp() || !member.getFirstChild().isThis()) {
-        return;
-      }
-
-      JSType jsType = getDeclaredType(info, member, value);
-      Node name = member.getLastChild();
-      if (jsType != null) {
-        thisTypeForProperties.defineDeclaredProperty(
-            name.getString(),
-            jsType,
-            member);
-      }
-    }
-
     /** Handle bleeding functions and function parameters. */
     private void handleFunctionInputs(Node fnNode) {
       // Handle bleeding functions.

test/com/google/javascript/jscomp/TypedScopeCreatorTest.java

@@ -1143,35 +1143,55 @@ public void testStubsInExterns4() {
   }
 
   public void testPropertyInExterns1() {
+    // Declaring a property on a non-native extern type (e.g. 'Extern') declares it as a property
+    // on the instance type, but only in externs.
     testSame(
         externs(
-            "/** @constructor */ function Extern() {}"
-                + "/** @type {Extern} */ var extern;"
-                + "/** @return {number} */ extern.one;"),
+            lines(
+                "/** @constructor */ function Extern() {}",
+                "/** @type {Extern} */ var extern;",
+                "/** @return {number} */ extern.one;")),
         srcs(
-            "/** @constructor */ function Normal() {}"
-                + "/** @type {Normal} */ var normal;"
-                + "/** @return {number} */ normal.one;"));
+            lines(
+                "/** @constructor */ function Normal() {}",
+                "/** @type {Normal} */ var normal;",
+                "/** @return {number} */ normal.one;",
+                "var result = new Extern().one();")));
 
     JSType e = globalScope.getVar("Extern").getType();
     ObjectType externInstance = ((FunctionType) e).getInstanceType();
     assertTrue(externInstance.hasOwnProperty("one"));
     assertTrue(externInstance.isPropertyTypeDeclared("one"));
     assertEquals("function(): number",
         externInstance.getPropertyType("one").toString());
+    assertEquals("number", globalScope.getVar("result").getType().toString());
 
     JSType n = globalScope.getVar("Normal").getType();
     ObjectType normalInstance = ((FunctionType) n).getInstanceType();
     assertFalse(normalInstance.hasOwnProperty("one"));
   }
 
   public void testPropertyInExterns2() {
+    // Native extern types (such as Object) do not get stray properties declared, since this would
+    // cause problems with bad externs (such as `/** @type {!Object} */ var api = {}; api.foo;`,
+    // where we don't want to declare that all Objects have a "foo" property.  Nevertheless, the
+    // specific qualified name (i.e. extern.one, in the example below) is still declared on the
+    // global scope, so referring to the "one" property specifically on "extern" is still checked
+    // as one would expect.
     testSame(
-        externs("/** @type {Object} */ var extern;" + "/** @return {number} */ extern.one;"),
-        srcs("/** @type {Object} */ var normal;" + "/** @return {number} */ normal.one;"));
+        externs(
+            lines(
+                "/** @type {Object} */ var extern;", //
+                "/** @return {number} */ extern.one;")),
+        srcs(
+            lines(
+                "/** @type {Object} */ var normal;", //
+                "/** @return {number} */ normal.one;",
+                "var result = extern.one();")));
 
     JSType e = globalScope.getVar("extern").getType();
     assertFalse(e.dereference().hasOwnProperty("one"));
+    assertEquals("number", globalScope.getVar("result").getType().toString());
 
     JSType normal = globalScope.getVar("normal").getType();
     assertFalse(normal.dereference().hasOwnProperty("one"));