[dart2js] Support required named parameters in weak NNBD function types.

We cannot simply discard the `required` modifier in weak mode function types since function types that differ in the placement of `required` cannot compare equal. Instead, we do that during subtype checks. We continue to ignore `required` in the actual calling convention in weak mode. Change-Id: I7dbb28550095c635f65592f78e495e8e4e8d7026 Fixes: #42608 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/153386 Reviewed-by: Mark Zhou <markzipan@google.com> Reviewed-by: Stephen Adams <sra@google.com> Commit-Queue: Mayank Patke <fishythefish@google.com>
dart-lang · Jul 7, 2020 · 5c99205 · 5c99205
1 parent a1f5100
commit 5c99205
Show file tree

Hide file tree

Showing 7 changed files with 27 additions and 21 deletions.
diff --git a/pkg/compiler/lib/src/elements/types.dart b/pkg/compiler/lib/src/elements/types.dart
@@ -2114,21 +2114,25 @@ abstract class DartTypes {
                 String sName = sNamed[sIndex++];
                 int comparison = sName.compareTo(tName);
                 if (comparison > 0) return false;
-                bool sIsRequired = sRequiredNamed.contains(sName);
+                bool sIsRequired =
+                    !useLegacySubtyping && sRequiredNamed.contains(sName);
                 if (comparison < 0) {
                   if (sIsRequired) return false;
                   continue;
                 }
-                bool tIsRequired = tRequiredNamed.contains(tName);
+                bool tIsRequired =
+                    !useLegacySubtyping && tRequiredNamed.contains(tName);
                 if (sIsRequired && !tIsRequired) return false;
                 if (!_isSubtype(
                     tNamedTypes[tIndex], sNamedTypes[sIndex - 1], env))
                   return false;
                 break;
               }
             }
-            while (sIndex < sNamedLength) {
-              if (sRequiredNamed.contains(sNamed[sIndex++])) return false;
+            if (!useLegacySubtyping) {
+              while (sIndex < sNamedLength) {
+                if (sRequiredNamed.contains(sNamed[sIndex++])) return false;
+              }
             }
             return true;
           } finally {

diff --git a/pkg/compiler/lib/src/ir/visitors.dart b/pkg/compiler/lib/src/ir/visitors.dart
@@ -130,12 +130,10 @@ class DartTypeConverter extends ir.DartTypeVisitor<DartType> {
             .skip(node.requiredParameterCount)
             .toList()),
         node.namedParameters.map((n) => n.name).toList(),
-        _options.useLegacySubtyping
-            ? const <String>{}
-            : node.namedParameters
-                .where((n) => n.isRequired)
-                .map((n) => n.name)
-                .toSet(),
+        node.namedParameters
+            .where((n) => n.isRequired)
+            .map((n) => n.name)
+            .toSet(),
         node.namedParameters.map((n) => visitType(n.type)).toList(),
         typeVariables ?? const <FunctionTypeVariable>[]);
     DartType type = _convertNullability(functionType, node.nullability);

diff --git a/pkg/compiler/lib/src/js_model/element_map_impl.dart b/pkg/compiler/lib/src/js_model/element_map_impl.dart
@@ -917,7 +917,7 @@ class JsKernelToElementMap implements JsToElementMap, IrToElementMap {
     for (ir.VariableDeclaration variable in sortedNamedParameters) {
       namedParameters.add(variable.name);
       namedParameterTypes.add(getParameterType(variable));
-      if (variable.isRequired && !options.useLegacySubtyping) {
+      if (variable.isRequired) {
         requiredNamedParameters.add(variable.name);
       }
     }

diff --git a/pkg/compiler/lib/src/kernel/element_map_impl.dart b/pkg/compiler/lib/src/kernel/element_map_impl.dart
@@ -539,7 +539,7 @@ class KernelToElementMapImpl implements KernelToElementMap, IrToElementMap {
     for (ir.VariableDeclaration variable in sortedNamedParameters) {
       namedParameters.add(variable.name);
       namedParameterTypes.add(getParameterType(variable));
-      if (variable.isRequired && !options.useLegacySubtyping) {
+      if (variable.isRequired) {
         requiredNamedParameters.add(variable.name);
       }
     }

diff --git a/sdk/lib/_internal/js_runtime/lib/rti.dart b/sdk/lib/_internal/js_runtime/lib/rti.dart
@@ -2913,22 +2913,26 @@ bool _isFunctionSubtype(
       String sName = _Utils.asString(_Utils.arrayAt(sNamed, sIndex));
       sIndex += 3;
       if (_Utils.stringLessThan(tName, sName)) return false;
-      bool sIsRequired = _Utils.asBool(_Utils.arrayAt(sNamed, sIndex - 2));
+      bool sIsRequired = !JS_GET_FLAG('LEGACY') &&
+          _Utils.asBool(_Utils.arrayAt(sNamed, sIndex - 2));
       if (_Utils.stringLessThan(sName, tName)) {
         if (sIsRequired) return false;
         continue;
       }
-      bool tIsRequired = _Utils.asBool(_Utils.arrayAt(tNamed, tIndex + 1));
+      bool tIsRequired = !JS_GET_FLAG('LEGACY') &&
+          _Utils.asBool(_Utils.arrayAt(tNamed, tIndex + 1));
       if (sIsRequired && !tIsRequired) return false;
       Rti sType = _Utils.asRti(_Utils.arrayAt(sNamed, sIndex - 1));
       Rti tType = _Utils.asRti(_Utils.arrayAt(tNamed, tIndex + 2));
       if (!_isSubtype(universe, tType, tEnv, sType, sEnv)) return false;
       break;
     }
   }
-  while (sIndex < sNamedLength) {
-    if (_Utils.asBool(_Utils.arrayAt(sNamed, sIndex + 1))) return false;
-    sIndex += 3;
+  if (!JS_GET_FLAG('LEGACY')) {
+    while (sIndex < sNamedLength) {
+      if (_Utils.asBool(_Utils.arrayAt(sNamed, sIndex + 1))) return false;
+      sIndex += 3;
+    }
   }
   return true;
 }

diff --git a/tests/dart2js/internal/rti/required_named_parameters_test.dart b/tests/dart2js/internal/rti/required_named_parameters_test.dart
@@ -51,11 +51,11 @@ main() {
 
   // Subtype may not redeclare optional parameters as required
   rti2 = rti.testingUniverseEval(universe, "@(A,{a!A,b!A,c!A})");
-  Expect.isFalse(rti.testingIsSubtype(universe, rti2, rti1));
+  Expect.equals(isWeakMode, rti.testingIsSubtype(universe, rti2, rti1));
 
   // Subtype may not declare new required named parameters
   rti2 = rti.testingUniverseEval(universe, "@(A,{a!A,b:A,c!A,d!A})");
-  Expect.isFalse(rti.testingIsSubtype(universe, rti2, rti1));
+  Expect.equals(isWeakMode, rti.testingIsSubtype(universe, rti2, rti1));
 
   // Rti.toString() appears as expected
   Expect.equals('(B, {required B a, B b, required B c}) => dynamic',

diff --git a/tests/dart2js_2/internal/rti/required_named_parameters_test.dart b/tests/dart2js_2/internal/rti/required_named_parameters_test.dart
@@ -51,11 +51,11 @@ main() {
 
   // Subtype may not redeclare optional parameters as required
   rti2 = rti.testingUniverseEval(universe, "@(A,{a!A,b!A,c!A})");
-  Expect.isFalse(rti.testingIsSubtype(universe, rti2, rti1));
+  Expect.equals(isWeakMode, rti.testingIsSubtype(universe, rti2, rti1));
 
   // Subtype may not declare new required named parameters
   rti2 = rti.testingUniverseEval(universe, "@(A,{a!A,b:A,c!A,d!A})");
-  Expect.isFalse(rti.testingIsSubtype(universe, rti2, rti1));
+  Expect.equals(isWeakMode, rti.testingIsSubtype(universe, rti2, rti1));
 
   // Rti.toString() appears as expected
   Expect.equals('(B, {required B a, B b, required B c}) => dynamic',