Fixes some violations of the equals-hashCode contract in NamedType.

`NamedType` instances used to only be compared based on their string references. Now, if they have been successfully resolved they will be compared based on their underlying (proxied) type instances, else fall back to comparing strings.

This change is not a complete fix because:
  - `NamedType` equality should also consider scopes
  - There is some ambiguity between forward declared types and `NoResolvedType` I haven't been able to unravel.
Fixing these has been punted in favour of an incremental improvement.

As a confirmation that this change is positive and to help prevent the contract from slipping further, `assertTypeEquals` in the "jstype" unit tests is being updated to compare `hashCode()` in addition to `equals()`.

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

src/com/google/javascript/rhino/jstype/JSType.java

@@ -690,7 +690,8 @@ boolean checkEquivalenceHelper(
     if (this.isNoResolvedType() && that.isNoResolvedType()) {
       if (this.isNamedType() && that.isNamedType()) {
         return Objects.equals(
-            ((NamedType) this).getReferenceName(), ((NamedType) that).getReferenceName());
+            this.toMaybeNamedType().getReferenceName(), //
+            that.toMaybeNamedType().getReferenceName());
       } else {
         return true;
       }
@@ -738,8 +739,14 @@ boolean checkEquivalenceHelper(
 
     if (isNominalType() && that.isNominalType()) {
       // TODO(johnlenz): is this valid across scopes?
-      return getConcreteNominalTypeName(this.toObjectType())
-          .equals(getConcreteNominalTypeName(that.toObjectType()));
+      @Nullable String nameOfThis = deepestResolvedTypeNameOf(this.toObjectType());
+      @Nullable String nameOfThat = deepestResolvedTypeNameOf(that.toObjectType());
+
+      if ((nameOfThis == null) && (nameOfThat == null)) {
+        // These are two anonymous types that were masquerading as nominal, so don't compare names.
+      } else {
+        return Objects.equals(nameOfThis, nameOfThat);
+      }
     }
 
     if (isTemplateType() && that.isTemplateType()) {
@@ -769,15 +776,16 @@ boolean checkEquivalenceHelper(
   }
 
   // Named types may be proxies of concrete types.
-  private String getConcreteNominalTypeName(ObjectType objType) {
-    if (objType instanceof ProxyObjectType) {
-      ObjectType internal = ((ProxyObjectType) objType)
-          .getReferencedObjTypeInternal();
-      if (internal != null && internal.isNominalType()) {
-        return getConcreteNominalTypeName(internal);
-      }
+  @Nullable
+  private String deepestResolvedTypeNameOf(ObjectType objType) {
+    if (!objType.isResolved() || !(objType instanceof ProxyObjectType)) {
+      return objType.getReferenceName();
     }
-    return objType.getReferenceName();
+
+    ObjectType internal = ((ProxyObjectType) objType).getReferencedObjTypeInternal();
+    return (internal != null && internal.isNominalType())
+        ? deepestResolvedTypeNameOf(internal)
+        : null;
   }
 
   /**
@@ -1661,11 +1669,26 @@ void setResolvedTypeInternal(JSType type) {
     resolved = true;
   }
 
-  /** Whether the type has been resolved. */
+  /**
+   * Returns whether the type has undergone resolution.
+   *
+   * <p>A value of {@code true} <em>does not</em> indicate that resolution was successful, only that
+   * it was attempted and has finished.
+   */
   public final boolean isResolved() {
     return resolved;
   }
 
+  /** Returns whether the type has undergone resolution and resolved to a "useful" type. */
+  public final boolean isSuccessfullyResolved() {
+    return isResolved() && !isNoResolvedType();
+  }
+
+  /** Returns whether the type has undergone resolution and resolved to a "useless" type. */
+  public final boolean isUnsuccessfullyResolved() {
+    return isResolved() && isNoResolvedType();
+  }
+
   /**
    * A null-safe resolve.
    * @see #resolve

src/com/google/javascript/rhino/jstype/NamedType.java

@@ -217,7 +217,9 @@ public boolean isNominalType() {
 
   @Override
   int recursionUnsafeHashCode() {
-    return nominalHashCode(this);
+    // Recall that equality on `NamedType` uses only the name until successful resolution, then
+    // delegates to the resolved type.
+    return isSuccessfullyResolved() ? super.recursionUnsafeHashCode() : nominalHashCode(this);
   }
 
   /**
@@ -253,7 +255,7 @@ JSType resolveInternal(ErrorReporter reporter) {
 
     JSType result = getReferencedType();
 
-    if (isResolved() && !result.isNoResolvedType()) {
+    if (isSuccessfullyResolved()) {
       int numKeys = result.getTemplateTypeMap().numUnfilledTemplateKeys();
       if (result.isObjectType()
           && (templateTypes != null && !templateTypes.isEmpty())

src/com/google/javascript/rhino/testing/Asserts.java

@@ -42,6 +42,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.common.base.Joiner;
 import com.google.common.collect.Iterables;
 import com.google.javascript.rhino.ErrorReporter;
 import com.google.javascript.rhino.jstype.JSType;
@@ -73,36 +74,74 @@ public static void assertTypeNotEquals(JSType a, JSType b) {
   }
 
   public static void assertTypeNotEquals(String message, JSType a, JSType b) {
+    if (!message.isEmpty()) {
+      message += "\n";
+    }
+    String aDebug = debugStringOf(a);
+    String bDebug = debugStringOf(b);
+
     Assert.assertFalse(
-        message + (message.isEmpty() ? "" : "\n")
-            + " Equals is not symmetric.\n"
-            + "Type: " + b + "\n",
+        lines(
+            message,
+            "Type should not be equal.",
+            "First type              : " + aDebug,
+            "was equal to second type: " + bDebug),
         a.isEquivalentTo(b));
     Assert.assertFalse(
-        message + (message.isEmpty() ? "" : "\n")
-            + " Equals is not symmetric.\n"
-            + "Type: " + b + "\n",
+        lines(
+            message,
+            "Inequality was found to be asymmetric.",
+            "Type should not be equal.",
+            "First type              : " + aDebug,
+            "was equal to second type: " + bDebug),
         b.isEquivalentTo(a));
   }
 
   public static void assertTypeEquals(JSType a, JSType b) {
     assertTypeEquals("", a, b);
   }
 
-  public static void assertTypeEquals(String message, JSType a, JSType b) {
-    checkNotNull(a);
-    checkNotNull(b);
+  public static void assertTypeEquals(String message, JSType expected, JSType actual) {
+    checkNotNull(expected);
+    checkNotNull(actual);
+
+    if (!message.isEmpty()) {
+      message += "\n";
+    }
+    String expectedDebug = debugStringOf(expected);
+    String actualDebug = debugStringOf(actual);
+
     Assert.assertTrue(
-        message + (message.isEmpty() ? "" : "\n")
-            + "Expected: " + a + "\n"
-            + "Actual  : " + b,
-        a.isEquivalentTo(b, true));
+        lines(
+            message, //
+            "Types should be equal.",
+            "Expected: " + expectedDebug,
+            "Actual  : " + actualDebug),
+        expected.isEquivalentTo(actual, true));
     Assert.assertTrue(
-        message
-            + " Equals is not symmetric.\n"
-            + "Expected: " + b + "\n"
-            + "Actual  : " + a,
-        b.isEquivalentTo(a, true));
+        lines(
+            message,
+            "Equality was found to be asymmetric.",
+            "Types should be equal.",
+            "Expected: " + expectedDebug,
+            "Actual  : " + actualDebug),
+        actual.isEquivalentTo(expected, true));
+
+    // Recall the `equals-hashCode` contract: if two objects report being equal, their hashcodes
+    // must also be equal. Breaking this contract breaks structures that depend on it (e.g.
+    // `HashMap`).
+    //
+    // The implementations of `hashCode()` and `equals()` are a bit tricky in some of the `JSType`
+    // classes, so we want to check specifically that this contract is fulfilled in all the unit
+    // tests involving them.
+    Assert.assertEquals(
+        lines(
+            message,
+            "Types violate the `equals-hashCode`: types report e `hashCode()`s do not match.",
+            "Expected: " + expected.hashCode() + " [on " + expectedDebug + "]",
+            "Actual  : " + actual.hashCode() + " [on " + actualDebug + "]"),
+        expected.hashCode(),
+        actual.hashCode());
   }
 
   public static <T extends JSType, S extends JSType> void
@@ -120,10 +159,9 @@ public static void assertTypeEquals(String message, JSType a, JSType b) {
    * should have trivial solutions (getGreatestSubtype, isEquivalentTo, etc)
    */
   public static void assertEquivalenceOperations(JSType a, JSType b) {
-    Assert.assertTrue(a.isEquivalentTo(b));
-    Assert.assertTrue(a.isEquivalentTo(a));
-    Assert.assertTrue(b.isEquivalentTo(b));
-    Assert.assertTrue(b.isEquivalentTo(a));
+    assertTypeEquals(a, a);
+    assertTypeEquals(a, b);
+    assertTypeEquals(b, b);
 
     Assert.assertTrue(a.isSubtypeOf(b));
     Assert.assertTrue(a.isSubtypeOf(a));
@@ -145,4 +183,14 @@ public static void assertEquivalenceOperations(JSType a, JSType b) {
     Assert.assertTrue(b.canCastTo(b));
     Assert.assertTrue(b.canCastTo(a));
   }
+
+  private static String debugStringOf(JSType type) {
+    return (type == null)
+        ? "<Java null>"
+        : type.toString() + " [instanceof " + type.getClass().getName() + "]";
+  }
+
+  private static final String lines(String... lines) {
+    return Joiner.on("\n").join(lines);
+  }
 }

src/com/google/javascript/rhino/testing/BaseJSTypeTestCase.java

@@ -55,6 +55,8 @@
 import junit.framework.TestCase;
 
 public abstract class BaseJSTypeTestCase extends TestCase {
+  protected static final String FORWARD_DECLARED_TYPE_NAME = "forwardDeclared";
+
   protected static final Joiner LINE_JOINER = Joiner.on('\n');
 
   protected JSTypeRegistry registry;
@@ -107,7 +109,7 @@ public abstract class BaseJSTypeTestCase extends TestCase {
   protected void setUp() throws Exception {
     super.setUp();
     errorReporter = new TestErrorReporter(null, null);
-    registry = new JSTypeRegistry(errorReporter, ImmutableSet.of("forwardDeclared"));
+    registry = new JSTypeRegistry(errorReporter, ImmutableSet.of(FORWARD_DECLARED_TYPE_NAME));
     initTypes();
   }
 

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

@@ -20137,23 +20137,52 @@ public void testShadowedForwardReferenceHoisted() {
             "}"));
   }
 
-  public void testShadowedForwardReference() {
-    // TODO(sdh): fix this.
+  public void testTypeDeclarationsShadowOneAnotherWithFunctionScoping() {
+    // TODO(b/110538992): Accuracy of shadowing is unclear here. b/110741413 may be confusing the
+    // issue.
+
+    // `C` at (2) should refer to `C` at (3) and not `C` at (1). Otherwise the assignment at (4)
+    // would be invalid.
     testTypes(
         lines(
-            "/** @constructor */ var C = function() { /** @type {string} */ this.prop = 's'; };",
+            "/** @constructor */",
+            "var A = function() { };",
+            "",
+            "/**",
+            " * @constructor",
+            " * @extends {A}",
+            " */",
+            "var C = function() { };", // (1)
+            "",
             "var fn = function() {",
-            "/** @type {C} */ var x",
-            "/** @constructor */ var C = function() { /** @type {number} */ this.prop = 1; };",
-            "/** @return {C} */ function fn() { return new C(); };",
-            "/** @type {number} */ var n1 = x.prop;",
-            "/** @type {number} */ var n2 = new C().prop;",
+            "  /** @type {?C} */ var innerC;", // (2)
             "",
-            "}"),
+            "  /** @constructor */",
+            "  var C = function() { };", // (3)
+            "",
+            "  innerC = new C();", // (4)
+            "}"));
+  }
+
+  public void testTypeDeclarationsShadowOneAnotherWithFunctionScopingConsideringHoisting() {
+    // TODO(b/110538992): Accuracy of shadowing is unclear here. b/110741413 may be confusing the
+    // issue.
+
+    // `C` at (3) should refer to `C` at (2) and not `C` at (1). Otherwise return the value at (4)
+    // would be invalid.
+    testTypes(
         lines(
-            "initializing variable", //
-            "found   : string", // should be "number"
-            "required: number"));
+            "/** @constructor */",
+            "var C = function() { };", // (1)
+            "",
+            "var fn = function() {",
+            "  /** @constructor */",
+            "  var C = function() { };", // (2)
+            "",
+            // This function will be hoisted above, and therefore type-analysed before, (2).
+            "  /** @return {!C} */", // (3)
+            "  function hoisted() { return new C(); };", // (4)
+            "}"));
   }
 
   public void testRefinedTypeInNestedShortCircuitingAndOr() {