Refactor some typechecking logic for getprops

This is so we can reuse it in a future CL when checking property access in for-of loop initializers.

The only non-refactoring change is that we back off some checks when assigning to ".prototype" that we didn't before, which resolves an old TODO in TypeCheckTest.

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

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

@@ -64,6 +64,7 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import javax.annotation.Nullable;
 
 /**
  * <p>Checks the types of JS expressions against any declared type
@@ -84,10 +85,6 @@ public final class TypeCheck implements NodeTraversal.Callback, CompilerPass {
   //
   // User warnings
   //
-
-  protected static final String OVERRIDING_PROTOTYPE_WITH_NON_OBJECT =
-      "overriding prototype with non-object";
-
   static final DiagnosticType DETERMINISTIC_TEST =
       DiagnosticType.warning(
           "JSC_DETERMINISTIC_TEST",
@@ -1012,7 +1009,7 @@ private void visitAssign(NodeTraversal t, Node assign) {
       if (object.isGetProp()) {
         JSType jsType = getJSType(object.getFirstChild());
         if (jsType.isInterface() && object.getLastChild().getString().equals("prototype")) {
-          visitInterfaceGetprop(t, assign, object, rvalue);
+          visitInterfacePropertyAssignment(t, object, rvalue);
         }
       }
 
@@ -1024,42 +1021,29 @@ private void visitAssign(NodeTraversal t, Node assign) {
       // during TypedScopeCreator, and we only look for the "dumb" cases here.
       // object.prototype = ...;
       if (pname.equals("prototype")) {
-        if (objectJsType != null && objectJsType.isFunctionType()) {
-          FunctionType functionType = objectJsType.toMaybeFunctionType();
-          if (functionType.isConstructor()) {
-            JSType rvalueType = rvalue.getJSType();
-            validator.expectObject(t, rvalue, rvalueType,
-                OVERRIDING_PROTOTYPE_WITH_NON_OBJECT);
-            return;
-          }
-        }
+        validator.expectCanAssignToPrototype(t, objectJsType, rvalue, getJSType(rvalue));
+        return;
       }
 
       // The generic checks for 'object.property' when 'object' is known,
       // and 'property' is declared on it.
       // object.property = ...;
-      ObjectType type = ObjectType.cast(
-          objectJsType.restrictByNotNullOrUndefined());
-      if (type != null) {
-        if (type.hasProperty(pname) && !type.isPropertyTypeInferred(pname)) {
-          JSType expectedType = type.getPropertyType(pname);
-          if (!expectedType.isUnknownType()) {
-            if (!propertyIsImplicitCast(type, pname)) {
-              validator.expectCanAssignToPropertyOf(
-                  t, assign, getJSType(rvalue),
-                  expectedType, object, pname);
-              checkPropertyInheritanceOnGetpropAssign(
-                  t, assign, object, pname, info, expectedType);
-            }
-            return;
-          }
+      ObjectType objectCastType = ObjectType.cast(objectJsType.restrictByNotNullOrUndefined());
+      JSType expectedPropertyType = getPropertyTypeIfDeclared(objectCastType, pname);
+
+      checkPropertyInheritanceOnGetpropAssign(t, assign, object, pname, info, expectedPropertyType);
+
+      // If we successfully found a non-unknown declared type, validate the assignment and don't do
+      // any further checks.
+      if (!expectedPropertyType.isUnknownType()) {
+        // Note: if the property has @implicitCast at its declaration, we don't check any
+        // assignments to it.
+        if (!propertyIsImplicitCast(objectCastType, pname)) {
+          validator.expectCanAssignToPropertyOf(
+              t, assign, getJSType(rvalue), expectedPropertyType, object, pname);
         }
+        return;
       }
-
-      // If we couldn't get the property type with normal object property
-      // lookups, then check inheritance anyway with the unknown type.
-      checkPropertyInheritanceOnGetpropAssign(
-          t, assign, object, pname, info, getNativeType(UNKNOWN_TYPE));
     }
 
     checkCanAssignToWithScope(t, assign, lvalue, getJSType(rvalue), "assignment");
@@ -1502,39 +1486,36 @@ static JSType getObjectLitKeyTypeFromValueType(Node key, JSType valueType) {
    * Visits an ASSIGN node for cases such as
    *
    * <pre>
-   * interface.property2.property = ...;
+   * interface.prototype.property = ...;
    * </pre>
    */
-  private void visitInterfaceGetprop(NodeTraversal t, Node assign, Node object, Node rvalue) {
-
+  private void visitInterfacePropertyAssignment(NodeTraversal t, Node object, Node rvalue) {
     JSType rvalueType = getJSType(rvalue);
 
-    // Only 2 values are allowed for methods:
+    // Only 2 values are allowed for interface methods:
     //    goog.abstractMethod
     //    function () {};
-    // or for properties, no assignment such as:
-    //    InterfaceFoo.prototype.foobar;
-
-    String abstractMethodName =
-        compiler.getCodingConvention().getAbstractMethodName();
+    // Other (non-method) interface properties must be stub declarations without assignments, e.g.
+    //     someinterface.prototype.nonMethodProperty;
+    // which is why we enforce that `rvalueType.isFunctionType()`.
     if (!rvalueType.isFunctionType()) {
-      // This is bad i18n style but we don't localize our compiler errors.
-      String abstractMethodMessage = (abstractMethodName != null)
-         ? ", or " + abstractMethodName
-         : "";
-      compiler.report(
-          t.makeError(object, INVALID_INTERFACE_MEMBER_DECLARATION,
-              abstractMethodMessage));
+      reportInvalidInterfaceMemberDeclaration(t, object);
     }
 
-    if (assign.getLastChild().isFunction()
-        && !NodeUtil.isEmptyBlock(assign.getLastChild().getLastChild())) {
-      compiler.report(
-          t.makeError(object, INTERFACE_METHOD_NOT_EMPTY,
-              abstractMethodName));
+    if (rvalue.isFunction() && !NodeUtil.isEmptyBlock(NodeUtil.getFunctionBody(rvalue))) {
+      String abstractMethodName = compiler.getCodingConvention().getAbstractMethodName();
+      compiler.report(t.makeError(object, INTERFACE_METHOD_NOT_EMPTY, abstractMethodName));
     }
   }
 
+  private void reportInvalidInterfaceMemberDeclaration(NodeTraversal t, Node interfaceNode) {
+    String abstractMethodName = compiler.getCodingConvention().getAbstractMethodName();
+    // This is bad i18n style but we don't localize our compiler errors.
+    String abstractMethodMessage = (abstractMethodName != null) ? ", or " + abstractMethodName : "";
+    compiler.report(
+        t.makeError(interfaceNode, INVALID_INTERFACE_MEMBER_DECLARATION, abstractMethodMessage));
+  }
+
   /**
    * Visits a NAME node.
    *
@@ -2570,6 +2551,18 @@ private JSType getJSType(Node n) {
     }
   }
 
+  /**
+   * Returns the type of the property with the given name if declared. Otherwise returns unknown.
+   */
+  private JSType getPropertyTypeIfDeclared(@Nullable ObjectType objectType, String propertyName) {
+    if (objectType != null
+        && objectType.hasProperty(propertyName)
+        && !objectType.isPropertyTypeInferred(propertyName)) {
+      return objectType.getPropertyType(propertyName);
+    }
+    return getNativeType(UNKNOWN_TYPE);
+  }
+
   // TODO(nicksantos): TypeCheck should never be attaching types to nodes.
   // All types should be attached by TypeInference. This is not true today
   // for legacy reasons. There are a number of places where TypeInference

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

@@ -666,6 +666,27 @@ void expectSuperType(NodeTraversal t, Node n, ObjectType superObject,
     }
   }
 
+  /**
+   * Expect that it's valid to assign something to a given type's prototype.
+   *
+   * <p>Most of these checks occur during TypedScopeCreator, so we just handle very basic cases here
+   *
+   * <p>For example, assuming `Foo` is a constructor, `Foo.prototype = 3;` will warn because `3`
+   * is not an object.
+   *
+   * @param ownerType The type of the object whose prototype is being changed. (e.g. `Foo` above)
+   * @param node Node to issue warnings on (e.g. `3` above)
+   * @param rightType the rvalue type being assigned to the prototype (e.g. `number` above)
+   */
+  void expectCanAssignToPrototype(NodeTraversal t, JSType ownerType, Node node, JSType rightType) {
+    if (ownerType.isFunctionType()) {
+      FunctionType functionType = ownerType.toMaybeFunctionType();
+      if (functionType.isConstructor()) {
+        expectObject(t, node, rightType, "cannot override prototype with non-object");
+      }
+    }
+  }
+
   /**
    * Expect that the first type can be cast to the second type. The first type
    * must have some relationship with the second.

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

@@ -7794,6 +7794,25 @@ public void testImplicitCast2() {
         "};\n");
   }
 
+  public void testImplicitCast3() {
+    testTypesWithExterns(
+        lines(
+            "/** @constructor */ function Element() {};",
+            "/**",
+            " * @type {string}",
+            " * @implicitCast",
+            " */",
+            "Element.prototype.innerHTML;"),
+        lines(
+            "/** @param {?Element} element",
+            " * @param {string|number} text",
+            " */",
+            "function f(element, text) {",
+            "  element.innerHTML = text;",
+            "}",
+            ""));
+  }
+
   public void testImplicitCastSubclassAccess() {
     testTypesWithExterns("/** @constructor */ function Element() {};\n" +
              "/** @type {string}\n" +
@@ -15587,23 +15606,17 @@ public void testIssue1024a() {
   }
 
   public void testIssue1024b() {
-    /* TODO(blickly): Make this warning go away.
-     * This is old behavior, but it doesn't make sense to warn about since
-     * both assignments are inferred.
-     */
-    testTypes(
-        "/** @param {Object} a */\n"
-            + "function f(a) {\n"
-            + "  a.prototype = {foo:3};\n"
-            + "}\n"
-            + "/** @param {Object} b\n"
-            + " */\n"
-            + "function g(b) {\n"
-            + "  b.prototype = function(){};\n"
-            + "}\n",
-        "assignment to property prototype of Object\n"
-            + "found   : {foo: number}\n"
-            + "required: function(): undefined");
+    testTypes(
+        lines(
+            "/** @param {Object} a */",
+            "function f(a) {",
+            "  a.prototype = {foo:3};",
+            "}",
+            "/** @param {Object} b",
+            " */",
+            "function g(b) {",
+            "  b.prototype = function(){};",
+            "}"));
   }
 
   public void testBug12722936() {