[dart2js] Update generic function subtyping.

We now test bounds for mutual subtyping rather than structural equality
up to renaming of bound type variables and equating all top types.

Change-Id: I7dd23a3211a1631e463ea90c3173f3deae46ca23
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134042
Commit-Queue: Mayank Patke <fishythefish@google.com>
Reviewed-by: Sigmund Cherem <sigmund@google.com>

CHANGELOG.md

@@ -46,12 +46,16 @@ additional details see the [announcement].
   enforced consistently in `ddc`. It will now be enforced in both.
 * JS interop classes with an index operator are now static errors.
 
+ [announcement]: https://github.com/dart-lang/sdk/issues/38994
+
 #### Dart2JS
 
 * JS interop classes with an index operator are now static errors instead of
   causing invalid code in dart2js.
-
- [announcement]: https://github.com/dart-lang/sdk/issues/38994
+* **Breaking Change**: The subtyping rule for generic functions is now more
+  forgiving. Corresponding type parameter bounds now only need to be mutual
+  subtypes rather than structurally equal up to renaming of bound type variables
+  and equating all top types.
 
 #### Linter
 

pkg/compiler/lib/src/elements/types.dart

@@ -82,21 +82,6 @@ abstract class DartType {
 
   bool _equals(DartType other, _Assumptions assumptions);
 
-  /// Returns `true` if `this` is structurally equal to [other] up to renaming
-  /// of bound type variables (trivially true) and equating all top types.
-  ///
-  /// This method handles the common cases of identity and top types. Types
-  /// should add any additional logic by implementing
-  /// [_equalsModuloTopInternal].
-  bool _equalsModuloTop(DartType other, bool isLegacy) {
-    other = other.unaliased;
-    if (identical(this, other)) return true;
-    if (_isTop(isLegacy)) return other._isTop(isLegacy);
-    return _equalsModuloTopInternal(other, isLegacy);
-  }
-
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy);
-
   @override
   String toString() => _DartTypeToStringVisitor().run(this);
 }
@@ -194,14 +179,6 @@ class LegacyType extends DartType {
 
   bool _equalsInternal(LegacyType other, _Assumptions assumptions) =>
       baseType._equals(other.baseType, assumptions);
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy) {
-    if (other is LegacyType) {
-      return baseType._equalsModuloTop(other.baseType, isLegacy);
-    }
-    return false;
-  }
 }
 
 class NullableType extends DartType {
@@ -243,14 +220,6 @@ class NullableType extends DartType {
 
   bool _equalsInternal(NullableType other, _Assumptions assumptions) =>
       baseType._equals(other.baseType, assumptions);
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy) {
-    if (other is NullableType) {
-      return baseType._equalsModuloTop(other.baseType, isLegacy);
-    }
-    return false;
-  }
 }
 
 class InterfaceType extends DartType {
@@ -321,15 +290,6 @@ class InterfaceType extends DartType {
     return identical(element, other.element) &&
         _equalTypes(typeArguments, other.typeArguments, assumptions);
   }
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy) {
-    if (other is InterfaceType) {
-      return identical(element, other.element) &&
-          _equalTypesModuloTop(typeArguments, other.typeArguments, isLegacy);
-    }
-    return false;
-  }
 }
 
 class TypedefType extends DartType {
@@ -392,13 +352,6 @@ class TypedefType extends DartType {
     return identical(element, other.element) &&
         _equalTypes(typeArguments, other.typeArguments, assumptions);
   }
-
-  @override
-  bool _equalsModuloTop(DartType other, bool isLegacy) =>
-      unaliased._equalsModuloTop(other, isLegacy);
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy) => false;
 }
 
 class TypeVariableType extends DartType {
@@ -434,14 +387,6 @@ class TypeVariableType extends DartType {
     }
     return false;
   }
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool legacy) {
-    if (other is TypeVariableType) {
-      return identical(other.element, element);
-    }
-    return false;
-  }
 }
 
 /// A type variable declared on a function type.
@@ -491,14 +436,6 @@ class FunctionTypeVariable extends DartType {
     return false;
   }
 
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool legacy) {
-    if (other is FunctionTypeVariable) {
-      return index == other.index;
-    }
-    return false;
-  }
-
   @override
   R accept<R, A>(DartTypeVisitor<R, A> visitor, A argument) =>
       visitor.visitFunctionTypeVariable(this, argument);
@@ -519,9 +456,6 @@ class NeverType extends DartType {
   @override
   bool _equals(DartType other, _Assumptions assumptions) =>
       identical(this, other);
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy) => false;
 }
 
 class VoidType extends DartType {
@@ -543,9 +477,6 @@ class VoidType extends DartType {
   bool _equals(DartType other, _Assumptions assumptions) {
     return identical(this, other);
   }
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy) => false;
 }
 
 class DynamicType extends DartType {
@@ -567,9 +498,6 @@ class DynamicType extends DartType {
   bool _equals(DartType other, _Assumptions assumptions) {
     return identical(this, other);
   }
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy) => false;
 }
 
 class ErasedType extends DartType {
@@ -590,9 +518,6 @@ class ErasedType extends DartType {
   @override
   bool _equals(DartType other, _Assumptions assumptions) =>
       identical(this, other);
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy) => false;
 }
 
 /// Represents a type which is simultaneously top and bottom.
@@ -624,9 +549,6 @@ class AnyType extends DartType {
   @override
   bool _equals(DartType other, _Assumptions assumptions) =>
       identical(this, other);
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy) => false;
 }
 
 class FunctionType extends DartType {
@@ -753,23 +675,6 @@ class FunctionType extends DartType {
     }
     return result;
   }
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy) {
-    if (other is FunctionType) {
-      return returnType._equalsModuloTop(other.returnType, isLegacy) &&
-          _equalTypesModuloTop(
-              parameterTypes, other.parameterTypes, isLegacy) &&
-          _equalTypesModuloTop(
-              optionalParameterTypes, other.optionalParameterTypes, isLegacy) &&
-          equalElements(namedParameters, other.namedParameters) &&
-          _equalTypesModuloTop(
-              namedParameterTypes, other.namedParameterTypes, isLegacy) &&
-          _equalTypesModuloTop(
-              typeVariableBounds, other.typeVariableBounds, isLegacy);
-    }
-    return false;
-  }
 }
 
 class FutureOrType extends DartType {
@@ -812,14 +717,6 @@ class FutureOrType extends DartType {
   bool _equalsInternal(FutureOrType other, _Assumptions assumptions) {
     return typeArgument._equals(other.typeArgument, assumptions);
   }
-
-  @override
-  bool _equalsModuloTopInternal(DartType other, bool isLegacy) {
-    if (other is FutureOrType) {
-      return typeArgument._equalsModuloTop(other.typeArgument, isLegacy);
-    }
-    return false;
-  }
 }
 
 bool _equalTypes(List<DartType> a, List<DartType> b, _Assumptions assumptions) {
@@ -832,14 +729,6 @@ bool _equalTypes(List<DartType> a, List<DartType> b, _Assumptions assumptions) {
   return true;
 }
 
-bool _equalTypesModuloTop(List<DartType> a, List<DartType> b, bool isLegacy) {
-  if (a.length != b.length) return false;
-  for (int i = 0; i < a.length; i++) {
-    if (!a[i]._equalsModuloTop(b[i], isLegacy)) return false;
-  }
-  return true;
-}
-
 abstract class DartTypeVisitor<R, A> {
   const DartTypeVisitor();
 
@@ -1756,10 +1645,18 @@ abstract class DartTypes {
         if (s is FunctionType) {
           if (t.isGeneric) {
             if (!s.isGeneric) return false;
-            if (!_equalTypesModuloTop(s.typeVariableBounds,
-                t.typeVariableBounds, useLegacySubtyping)) {
+            List<DartType> sBounds = s.typeVariableBounds;
+            List<DartType> tBounds = t.typeVariableBounds;
+            int length = sBounds.length;
+            if (length != tBounds.length) {
               return false;
             }
+            for (int i = 0; i < length; i++) {
+              if (!_isSubtype(sBounds[i], sEnv, tBounds[i], tEnv) ||
+                  !_isSubtype(tBounds[i], tEnv, sBounds[i], sEnv)) {
+                return false;
+              }
+            }
             sEnv = sEnv.toSet()..addAll(s.typeVariables);
             tEnv = tEnv.toSet()..addAll(t.typeVariables);
           }

sdk/lib/_internal/js_runtime/lib/rti.dart

@@ -2505,7 +2505,17 @@ bool _isSubtype(universe, Rti s, sEnv, Rti t, tEnv, bool isLegacy) {
 
     var sBounds = Rti._getGenericFunctionBounds(s);
     var tBounds = Rti._getGenericFunctionBounds(t);
-    if (!typesEqual(sBounds, tBounds, isLegacy)) return false;
+    int sLength = _Utils.arrayLength(sBounds);
+    int tLength = _Utils.arrayLength(tBounds);
+    if (sLength != tLength) return false;
+    for (int i = 0; i < sLength; i++) {
+      var sBound = _Utils.arrayAt(sBounds, i);
+      var tBound = _Utils.arrayAt(tBounds, i);
+      if (!_isSubtype(universe, sBound, sEnv, tBound, tEnv, isLegacy) ||
+          !_isSubtype(universe, tBound, tEnv, sBound, sEnv, isLegacy)) {
+        return false;
+      }
+    }
 
     sEnv = sEnv == null ? sBounds : _Utils.arrayConcat(sBounds, sEnv);
     tEnv = tEnv == null ? tBounds : _Utils.arrayConcat(tBounds, tEnv);
@@ -2694,91 +2704,6 @@ bool _isInterfaceSubtype(universe, Rti s, sEnv, Rti t, tEnv, bool isLegacy) {
   return true;
 }
 
-/// Types are equal if they are structurally equal up to renaming of bound type
-/// variables and equating all top types.
-///
-/// We ignore renaming of bound type variables because we operate on de Bruijn
-/// indices, not names.
-bool typeEqual(Rti s, Rti t, bool isLegacy) {
-  if (_Utils.isIdentical(s, t)) return true;
-
-  if (isTopType(s, isLegacy)) return isTopType(t, isLegacy);
-
-  int sKind = Rti._getKind(s);
-  int tKind = Rti._getKind(t);
-  if (sKind != tKind) return false;
-
-  switch (sKind) {
-    case Rti.kindStar:
-    case Rti.kindQuestion:
-    case Rti.kindFutureOr:
-      return typeEqual(_castToRti(Rti._getPrimary(s)),
-          _castToRti(Rti._getPrimary(t)), isLegacy);
-
-    case Rti.kindInterface:
-      if (Rti._getInterfaceName(s) != Rti._getInterfaceName(t)) return false;
-      return typesEqual(Rti._getInterfaceTypeArguments(s),
-          Rti._getInterfaceTypeArguments(t), isLegacy);
-
-    case Rti.kindBinding:
-      return typeEqual(
-              Rti._getBindingBase(s), Rti._getBindingBase(t), isLegacy) &&
-          typesEqual(Rti._getBindingArguments(s), Rti._getBindingArguments(t),
-              isLegacy);
-
-    case Rti.kindFunction:
-      return typeEqual(
-              Rti._getReturnType(s), Rti._getReturnType(t), isLegacy) &&
-          functionParametersEqual(Rti._getFunctionParameters(s),
-              Rti._getFunctionParameters(t), isLegacy);
-
-    case Rti.kindGenericFunction:
-      return typeEqual(Rti._getGenericFunctionBase(s),
-              Rti._getGenericFunctionBase(t), isLegacy) &&
-          typesEqual(Rti._getGenericFunctionBounds(s),
-              Rti._getGenericFunctionBounds(t), isLegacy);
-
-    default:
-      return false;
-  }
-}
-
-bool typesEqual(Object sArray, Object tArray, isLegacy) {
-  int sLength = _Utils.arrayLength(sArray);
-  int tLength = _Utils.arrayLength(tArray);
-  if (sLength != tLength) return false;
-  for (int i = 0; i < sLength; i++) {
-    if (!typeEqual(_castToRti(_Utils.arrayAt(sArray, i)),
-        _castToRti(_Utils.arrayAt(tArray, i)), isLegacy)) return false;
-  }
-  return true;
-}
-
-bool namedTypesEqual(Object sArray, Object tArray, isLegacy) {
-  int sLength = _Utils.arrayLength(sArray);
-  int tLength = _Utils.arrayLength(tArray);
-  assert(sLength.isEven);
-  assert(tLength.isEven);
-  if (sLength != tLength) return false;
-  for (int i = 0; i < sLength; i += 2) {
-    if (_Utils.asString(_Utils.arrayAt(sArray, i)) !=
-        _Utils.asString(_Utils.arrayAt(tArray, i))) return false;
-    if (!typeEqual(_castToRti(_Utils.arrayAt(sArray, i + 1)),
-        _castToRti(_Utils.arrayAt(tArray, i + 1)), isLegacy)) return false;
-  }
-  return true;
-}
-
-// TODO(fishythefish): Support required named parameters.
-bool functionParametersEqual(_FunctionParameters sParameters,
-        _FunctionParameters tParameters, isLegacy) =>
-    typesEqual(_FunctionParameters._getRequiredPositional(sParameters),
-        _FunctionParameters._getRequiredPositional(tParameters), isLegacy) &&
-    typesEqual(_FunctionParameters._getOptionalPositional(sParameters),
-        _FunctionParameters._getOptionalPositional(tParameters), isLegacy) &&
-    namedTypesEqual(_FunctionParameters._getOptionalNamed(sParameters),
-        _FunctionParameters._getOptionalNamed(tParameters), isLegacy);
-
 bool isLegacyTopType(Rti t) => isTopType(t, true);
 bool isNnbdTopType(Rti t) => isTopType(t, false);
 bool isTopType(Rti t, bool isLegacy) {

sdk_nnbd/lib/_internal/js_runtime/lib/rti.dart

@@ -2505,7 +2505,17 @@ bool _isSubtype(universe, Rti s, sEnv, Rti t, tEnv, bool isLegacy) {
 
     var sBounds = Rti._getGenericFunctionBounds(s);
     var tBounds = Rti._getGenericFunctionBounds(t);
-    if (!typesEqual(sBounds, tBounds, isLegacy)) return false;
+    int sLength = _Utils.arrayLength(sBounds);
+    int tLength = _Utils.arrayLength(tBounds);
+    if (sLength != tLength) return false;
+    for (int i = 0; i < sLength; i++) {
+      var sBound = _Utils.arrayAt(sBounds, i);
+      var tBound = _Utils.arrayAt(tBounds, i);
+      if (!_isSubtype(universe, sBound, sEnv, tBound, tEnv, isLegacy) ||
+          !_isSubtype(universe, tBound, tEnv, sBound, sEnv, isLegacy)) {
+        return false;
+      }
+    }
 
     sEnv = sEnv == null ? sBounds : _Utils.arrayConcat(sBounds, sEnv);
     tEnv = tEnv == null ? tBounds : _Utils.arrayConcat(tBounds, tEnv);
@@ -2694,91 +2704,6 @@ bool _isInterfaceSubtype(universe, Rti s, sEnv, Rti t, tEnv, bool isLegacy) {
   return true;
 }
 
-/// Types are equal if they are structurally equal up to renaming of bound type
-/// variables and equating all top types.
-///
-/// We ignore renaming of bound type variables because we operate on de Bruijn
-/// indices, not names.
-bool typeEqual(Rti s, Rti t, bool isLegacy) {
-  if (_Utils.isIdentical(s, t)) return true;
-
-  if (isTopType(s, isLegacy)) return isTopType(t, isLegacy);
-
-  int sKind = Rti._getKind(s);
-  int tKind = Rti._getKind(t);
-  if (sKind != tKind) return false;
-
-  switch (sKind) {
-    case Rti.kindStar:
-    case Rti.kindQuestion:
-    case Rti.kindFutureOr:
-      return typeEqual(_castToRti(Rti._getPrimary(s)),
-          _castToRti(Rti._getPrimary(t)), isLegacy);
-
-    case Rti.kindInterface:
-      if (Rti._getInterfaceName(s) != Rti._getInterfaceName(t)) return false;
-      return typesEqual(Rti._getInterfaceTypeArguments(s),
-          Rti._getInterfaceTypeArguments(t), isLegacy);
-
-    case Rti.kindBinding:
-      return typeEqual(
-              Rti._getBindingBase(s), Rti._getBindingBase(t), isLegacy) &&
-          typesEqual(Rti._getBindingArguments(s), Rti._getBindingArguments(t),
-              isLegacy);
-
-    case Rti.kindFunction:
-      return typeEqual(
-              Rti._getReturnType(s), Rti._getReturnType(t), isLegacy) &&
-          functionParametersEqual(Rti._getFunctionParameters(s),
-              Rti._getFunctionParameters(t), isLegacy);
-
-    case Rti.kindGenericFunction:
-      return typeEqual(Rti._getGenericFunctionBase(s),
-              Rti._getGenericFunctionBase(t), isLegacy) &&
-          typesEqual(Rti._getGenericFunctionBounds(s),
-              Rti._getGenericFunctionBounds(t), isLegacy);
-
-    default:
-      return false;
-  }
-}
-
-bool typesEqual(Object sArray, Object tArray, isLegacy) {
-  int sLength = _Utils.arrayLength(sArray);
-  int tLength = _Utils.arrayLength(tArray);
-  if (sLength != tLength) return false;
-  for (int i = 0; i < sLength; i++) {
-    if (!typeEqual(_castToRti(_Utils.arrayAt(sArray, i)),
-        _castToRti(_Utils.arrayAt(tArray, i)), isLegacy)) return false;
-  }
-  return true;
-}
-
-bool namedTypesEqual(Object sArray, Object tArray, isLegacy) {
-  int sLength = _Utils.arrayLength(sArray);
-  int tLength = _Utils.arrayLength(tArray);
-  assert(sLength.isEven);
-  assert(tLength.isEven);
-  if (sLength != tLength) return false;
-  for (int i = 0; i < sLength; i += 2) {
-    if (_Utils.asString(_Utils.arrayAt(sArray, i)) !=
-        _Utils.asString(_Utils.arrayAt(tArray, i))) return false;
-    if (!typeEqual(_castToRti(_Utils.arrayAt(sArray, i + 1)),
-        _castToRti(_Utils.arrayAt(tArray, i + 1)), isLegacy)) return false;
-  }
-  return true;
-}
-
-// TODO(fishythefish): Support required named parameters.
-bool functionParametersEqual(_FunctionParameters sParameters,
-        _FunctionParameters tParameters, isLegacy) =>
-    typesEqual(_FunctionParameters._getRequiredPositional(sParameters),
-        _FunctionParameters._getRequiredPositional(tParameters), isLegacy) &&
-    typesEqual(_FunctionParameters._getOptionalPositional(sParameters),
-        _FunctionParameters._getOptionalPositional(tParameters), isLegacy) &&
-    namedTypesEqual(_FunctionParameters._getOptionalNamed(sParameters),
-        _FunctionParameters._getOptionalNamed(tParameters), isLegacy);
-
 bool isLegacyTopType(Rti t) => isTopType(t, true);
 bool isNnbdTopType(Rti t) => isTopType(t, false);
 bool isTopType(Rti t, bool isLegacy) {