Improve Dart plugin registration handling (#122046)

Improve Dart plugin registration handling

packages/flutter_tools/lib/src/flutter_plugins.dart

@@ -1191,13 +1191,13 @@ bool hasPlugins(FlutterProject project) {
 
 /// Resolves the platform implementation for Dart-only plugins.
 ///
-///   * If there are multiple direct pub dependencies on packages that implement the
-///     frontend plugin for the current platform, fail.
+///   * If there is only one dependency on a package that implements the
+///     frontend plugin for the current platform, use that.
 ///   * If there is a single direct dependency on a package that implements the
-///     frontend plugin for the target platform, this package is the selected implementation.
-///   * If there is no direct dependency on a package that implements the frontend
-///     plugin for the target platform, and the frontend plugin has a default implementation
-///     for the target platform the default implementation is selected.
+///     frontend plugin for the current platform, use that.
+///   * If there is no direct dependency on a package that implements the
+///     frontend plugin, but there is a default for the current platform,
+///     use that.
 ///   * Else fail.
 ///
 ///  For more details, https://flutter.dev/go/federated-plugins.
@@ -1214,11 +1214,15 @@ List<PluginInterfaceResolution> resolvePlatformImplementation(
     MacOSPlugin.kConfigKey,
     WindowsPlugin.kConfigKey,
   ];
-  final Map<String, PluginInterfaceResolution> directDependencyResolutions
-      = <String, PluginInterfaceResolution>{};
+  final Map<String, List<PluginInterfaceResolution>> possibleResolutions
+      = <String, List<PluginInterfaceResolution>>{};
   final Map<String, String> defaultImplementations = <String, String>{};
-  bool didFindError = false;
+  // Generates a key for the maps above.
+  String getResolutionKey({required String platform, required String packageName}) {
+    return '$packageName:$platform';
+  }
 
+  bool hasPubspecError = false;
   for (final Plugin plugin in plugins) {
     for (final String platform in platforms) {
       if (plugin.platforms[platform] == null &&
@@ -1257,11 +1261,12 @@ List<PluginInterfaceResolution> resolvePlatformImplementation(
               '\n'
             );
           }
-          didFindError = true;
+          hasPubspecError = true;
           continue;
         }
+        final String defaultImplementationKey = getResolutionKey(platform: platform, packageName: plugin.name);
         if (defaultImplementation != null) {
-          defaultImplementations['$platform/${plugin.name}'] = defaultImplementation;
+          defaultImplementations[defaultImplementationKey] = defaultImplementation;
           continue;
         } else {
           // An app-facing package (i.e., one with no 'implements') with an
@@ -1281,52 +1286,87 @@ List<PluginInterfaceResolution> resolvePlatformImplementation(
             minFlutterVersion.compareTo(semver.Version(2, 11, 0)) >= 0;
           if (!isDesktop || hasMinVersionForImplementsRequirement) {
             implementsPackage = plugin.name;
-            defaultImplementations['$platform/${plugin.name}'] = plugin.name;
+            defaultImplementations[defaultImplementationKey] = plugin.name;
+          } else {
+            // If it doesn't meet any of the conditions, it isn't eligible for
+            // auto-registration.
+            continue;
           }
         }
       }
+      // If there's no Dart implementation, there's nothing to register.
       if (plugin.pluginDartClassPlatforms[platform] == null ||
           plugin.pluginDartClassPlatforms[platform] == 'none') {
         continue;
       }
-      final String resolutionKey = '$platform/$implementsPackage';
-      if (directDependencyResolutions.containsKey(resolutionKey)) {
-        final PluginInterfaceResolution? currResolution = directDependencyResolutions[resolutionKey];
-        if (currResolution != null && currResolution.plugin.isDirectDependency) {
-          if (plugin.isDirectDependency) {
-            if (throwOnPluginPubspecError) {
-              globals.printError(
-                'Plugin `${plugin.name}` implements an interface for `$platform`, which was already '
-                'implemented by plugin `${currResolution.plugin.name}`.\n'
-                'To fix this issue, remove either dependency from pubspec.yaml.'
-                '\n\n'
-              );
-            }
-            didFindError = true;
-          }
-          // Use the plugin implementation added by the user as a direct dependency.
-          continue;
-        }
+
+      // If it hasn't been skipped, it's a candidate for auto-registration, so
+      // add it as a possible resolution.
+      final String resolutionKey = getResolutionKey(platform: platform, packageName: implementsPackage);
+      if (!possibleResolutions.containsKey(resolutionKey)) {
+        possibleResolutions[resolutionKey] = <PluginInterfaceResolution>[];
       }
-      directDependencyResolutions[resolutionKey] = PluginInterfaceResolution(
+      possibleResolutions[resolutionKey]!.add(PluginInterfaceResolution(
         plugin: plugin,
         platform: platform,
-      );
+      ));
     }
   }
-  if (didFindError && throwOnPluginPubspecError) {
+  if (hasPubspecError && throwOnPluginPubspecError) {
     throwToolExit('Please resolve the errors');
   }
+
+  // Now resolve all the possible resolutions to a single option for each
+  // plugin, or throw if that's not possible.
+  bool hasResolutionError = false;
   final List<PluginInterfaceResolution> finalResolution = <PluginInterfaceResolution>[];
-  for (final MapEntry<String, PluginInterfaceResolution> resolution in directDependencyResolutions.entries) {
-    if (resolution.value.plugin.isDirectDependency) {
-      finalResolution.add(resolution.value);
-    } else if (defaultImplementations.containsKey(resolution.key)) {
-      // Pick the default implementation.
-      if (defaultImplementations[resolution.key] == resolution.value.plugin.name) {
-        finalResolution.add(resolution.value);
+  for (final MapEntry<String, List<PluginInterfaceResolution>> entry in possibleResolutions.entries) {
+    final List<PluginInterfaceResolution> candidates = entry.value;
+    // If there's only one candidate, use it.
+    if (candidates.length == 1) {
+      finalResolution.add(candidates.first);
+      continue;
+    }
+    // Next, try direct dependencies of the resolving application.
+    final Iterable<PluginInterfaceResolution> directDependencies = candidates.where((PluginInterfaceResolution r) {
+      return r.plugin.isDirectDependency;
+    });
+    if (directDependencies.isNotEmpty) {
+      if (directDependencies.length > 1) {
+        globals.printError(
+          'Plugin ${entry.key} has conflicting direct dependency implementations:\n'
+          '${directDependencies.map((PluginInterfaceResolution r) => '  ${r.plugin.name}\n').join()}'
+          'To fix this issue, remove all but one of these dependencies from pubspec.yaml.\n'
+        );
+        hasResolutionError = true;
+      } else {
+        finalResolution.add(directDependencies.first);
+      }
+      continue;
+    }
+    // Next, defer to the default implementation if there is one.
+    final String? defaultPackageName = defaultImplementations[entry.key];
+    if (defaultPackageName != null) {
+      final int defaultIndex = candidates
+          .indexWhere((PluginInterfaceResolution r) => r.plugin.name == defaultPackageName);
+      if (defaultIndex != -1) {
+        finalResolution.add(candidates[defaultIndex]);
+        continue;
       }
     }
+    // Otherwise, require an explicit choice.
+    if (candidates.length > 1) {
+      globals.printError(
+        'Plugin ${entry.key} has multiple possible implementations:\n'
+        '${candidates.map((PluginInterfaceResolution r) => '  ${r.plugin.name}\n').join()}'
+        'To fix this issue, add one of these dependencies to pubspec.yaml.\n'
+      );
+      hasResolutionError = true;
+      continue;
+    }
+  }
+  if (hasResolutionError) {
+    throwToolExit('Please resolve the errors');
   }
   return finalResolution;
 }

packages/flutter_tools/lib/src/plugins.dart

@@ -418,4 +418,9 @@ class PluginInterfaceResolution {
       'dartClass': plugin.pluginDartClassPlatforms[platform] ?? '',
     };
   }
+
+  @override
+  String toString() {
+    return '<PluginInterfaceResolution ${plugin.name} for $platform>';
+  }
 }

packages/flutter_tools/test/general.shard/dart_plugin_test.dart

@@ -38,7 +38,7 @@ void main() {
     });
 
     group('resolvePlatformImplementation', () {
-      testWithoutContext('selects implementation from direct dependency', () async {
+      testWithoutContext('selects uncontested implementation from direct dependency', () async {
         final Set<String> directDependencies = <String>{
           'url_launcher_linux',
           'url_launcher_macos',
@@ -76,14 +76,38 @@ void main() {
             fileSystem: fs,
             appDependencies: directDependencies,
           ),
+        ]);
+
+        expect(resolutions.length, equals(2));
+        expect(resolutions[0].toMap(), equals(
+          <String, String>{
+            'pluginName': 'url_launcher_linux',
+            'dartClass': 'UrlLauncherPluginLinux',
+            'platform': 'linux',
+          })
+        );
+        expect(resolutions[1].toMap(), equals(
+          <String, String>{
+            'pluginName': 'url_launcher_macos',
+            'dartClass': 'UrlLauncherPluginMacOS',
+            'platform': 'macos',
+          })
+        );
+      });
+
+      testWithoutContext('selects uncontested implementation from transitive dependency', () async {
+        final Set<String> directDependencies = <String>{
+          'url_launcher_macos',
+        };
+        final List<PluginInterfaceResolution> resolutions = resolvePlatformImplementation(<Plugin>[
           Plugin.fromYaml(
-            'undirect_dependency_plugin',
+            'url_launcher_macos',
             '',
             YamlMap.wrap(<String, dynamic>{
               'implements': 'url_launcher',
               'platforms': <String, dynamic>{
-                'windows': <String, dynamic>{
-                  'dartPluginClass': 'UrlLauncherPluginWindows',
+                'macos': <String, dynamic>{
+                  'dartPluginClass': 'UrlLauncherPluginMacOS',
                 },
               },
             }),
@@ -92,17 +116,14 @@ void main() {
             fileSystem: fs,
             appDependencies: directDependencies,
           ),
-        ]);
-
-        resolvePlatformImplementation(<Plugin>[
           Plugin.fromYaml(
-            'url_launcher_macos',
+            'transitive_dependency_plugin',
             '',
             YamlMap.wrap(<String, dynamic>{
               'implements': 'url_launcher',
               'platforms': <String, dynamic>{
-                'macos': <String, dynamic>{
-                  'dartPluginClass': 'UrlLauncherPluginMacOS',
+                'windows': <String, dynamic>{
+                  'dartPluginClass': 'UrlLauncherPluginWindows',
                 },
               },
             }),
@@ -116,16 +137,16 @@ void main() {
         expect(resolutions.length, equals(2));
         expect(resolutions[0].toMap(), equals(
           <String, String>{
-            'pluginName': 'url_launcher_linux',
-            'dartClass': 'UrlLauncherPluginLinux',
-            'platform': 'linux',
+            'pluginName': 'url_launcher_macos',
+            'dartClass': 'UrlLauncherPluginMacOS',
+            'platform': 'macos',
           })
         );
         expect(resolutions[1].toMap(), equals(
           <String, String>{
-            'pluginName': 'url_launcher_macos',
-            'dartClass': 'UrlLauncherPluginMacOS',
-            'platform': 'macos',
+            'pluginName': 'transitive_dependency_plugin',
+            'dartClass': 'UrlLauncherPluginWindows',
+            'platform': 'windows',
           })
         );
       });
@@ -301,14 +322,17 @@ void main() {
             fileSystem: fs,
             appDependencies: directDependencies,
           ),
+          // Include three possible implementations, one before and one after
+          // to ensure that the selection is working as intended, not just by
+          // coincidence of order.
           Plugin.fromYaml(
-            'url_launcher_linux',
+            'another_url_launcher_linux',
             '',
             YamlMap.wrap(<String, dynamic>{
               'implements': 'url_launcher',
               'platforms': <String, dynamic>{
                 'linux': <String, dynamic>{
-                  'dartPluginClass': 'UrlLauncherPluginLinux',
+                  'dartPluginClass': 'UnofficialUrlLauncherPluginLinux',
                 },
               },
             }),
@@ -317,28 +341,14 @@ void main() {
             fileSystem: fs,
             appDependencies: directDependencies,
           ),
-        ]);
-        expect(resolutions.length, equals(1));
-        expect(resolutions[0].toMap(), equals(
-          <String, String>{
-            'pluginName': 'url_launcher_linux',
-            'dartClass': 'UrlLauncherPluginLinux',
-            'platform': 'linux',
-          })
-        );
-      });
-
-      testWithoutContext('selects default implementation if interface is direct dependency', () async {
-        final Set<String> directDependencies = <String>{'url_launcher'};
-
-        final List<PluginInterfaceResolution> resolutions = resolvePlatformImplementation(<Plugin>[
           Plugin.fromYaml(
-            'url_launcher',
+            'url_launcher_linux',
             '',
             YamlMap.wrap(<String, dynamic>{
+              'implements': 'url_launcher',
               'platforms': <String, dynamic>{
                 'linux': <String, dynamic>{
-                  'default_package': 'url_launcher_linux',
+                  'dartPluginClass': 'UrlLauncherPluginLinux',
                 },
               },
             }),
@@ -348,13 +358,13 @@ void main() {
             appDependencies: directDependencies,
           ),
           Plugin.fromYaml(
-            'url_launcher_linux',
+            'yet_another_url_launcher_linux',
             '',
             YamlMap.wrap(<String, dynamic>{
               'implements': 'url_launcher',
               'platforms': <String, dynamic>{
                 'linux': <String, dynamic>{
-                  'dartPluginClass': 'UrlLauncherPluginLinux',
+                  'dartPluginClass': 'UnofficialUrlLauncherPluginLinux2',
                 },
               },
             }),
@@ -374,11 +384,8 @@ void main() {
         );
       });
 
-      testWithoutContext('selects user selected implementation despites default implementation', () async {
-        final Set<String> directDependencies = <String>{
-          'user_selected_url_launcher_implementation',
-          'url_launcher',
-        };
+      testWithoutContext('selects default implementation if interface is direct dependency', () async {
+        final Set<String> directDependencies = <String>{'url_launcher'};
 
         final List<PluginInterfaceResolution> resolutions = resolvePlatformImplementation(<Plugin>[
           Plugin.fromYaml(
@@ -412,27 +419,11 @@ void main() {
             fileSystem: fs,
             appDependencies: directDependencies,
           ),
-          Plugin.fromYaml(
-            'user_selected_url_launcher_implementation',
-            '',
-            YamlMap.wrap(<String, dynamic>{
-              'implements': 'url_launcher',
-              'platforms': <String, dynamic>{
-                'linux': <String, dynamic>{
-                  'dartPluginClass': 'UrlLauncherPluginLinux',
-                },
-              },
-            }),
-            null,
-            <String>[],
-            fileSystem: fs,
-            appDependencies: directDependencies,
-          ),
         ]);
         expect(resolutions.length, equals(1));
         expect(resolutions[0].toMap(), equals(
           <String, String>{
-            'pluginName': 'user_selected_url_launcher_implementation',
+            'pluginName': 'url_launcher_linux',
             'dartClass': 'UrlLauncherPluginLinux',
             'platform': 'linux',
           })
@@ -545,22 +536,27 @@ void main() {
             ),
           ]);
 
-          expect(
-            testLogger.errorText,
-            'Plugin `url_launcher_linux_2` implements an interface for `linux`, which was already implemented by plugin `url_launcher_linux_1`.\n'
-            'To fix this issue, remove either dependency from pubspec.yaml.'
-            '\n\n'
-          );
         },
         throwsToolExit(
           message: 'Please resolve the errors',
         ));
+
+        expect(
+          testLogger.errorText,
+          'Plugin url_launcher:linux has conflicting direct dependency implementations:\n'
+          '  url_launcher_linux_1\n'
+          '  url_launcher_linux_2\n'
+          'To fix this issue, remove all but one of these dependencies from pubspec.yaml.'
+          '\n\n'
+        );
       });
 
       testUsingContext('provides all errors when user selected multiple implementations', () async {
         final Set<String> directDependencies = <String>{
           'url_launcher_linux_1',
           'url_launcher_linux_2',
+          'url_launcher_windows_1',
+          'url_launcher_windows_2',
         };
         expect(() {
           resolvePlatformImplementation(<Plugin>[
@@ -596,18 +592,109 @@ void main() {
               fileSystem: fs,
               appDependencies: directDependencies,
             ),
+            Plugin.fromYaml(
+              'url_launcher_windows_1',
+              '',
+              YamlMap.wrap(<String, dynamic>{
+                'implements': 'url_launcher',
+                'platforms': <String, dynamic>{
+                  'windows': <String, dynamic>{
+                    'dartPluginClass': 'UrlLauncherPluginWindows1',
+                  },
+                },
+              }),
+              null,
+              <String>[],
+              fileSystem: fs,
+              appDependencies: directDependencies,
+            ),
+            Plugin.fromYaml(
+              'url_launcher_windows_2',
+              '',
+              YamlMap.wrap(<String, dynamic>{
+                'implements': 'url_launcher',
+                'platforms': <String, dynamic>{
+                  'windows': <String, dynamic>{
+                    'dartPluginClass': 'UrlLauncherPluginWindows2',
+                  },
+                },
+              }),
+              null,
+              <String>[],
+              fileSystem: fs,
+              appDependencies: directDependencies,
+            ),
           ]);
+        },
+        throwsToolExit(
+          message: 'Please resolve the errors',
+        ));
 
-          expect(
-            testLogger.errorText,
-            'Plugin `url_launcher_linux_2` implements an interface for `linux`, which was already implemented by plugin `url_launcher_linux_1`.\n'
-            'To fix this issue, remove either dependency from pubspec.yaml.'
-            '\n\n'
-          );
+        expect(
+          testLogger.errorText,
+          'Plugin url_launcher:linux has conflicting direct dependency implementations:\n'
+          '  url_launcher_linux_1\n'
+          '  url_launcher_linux_2\n'
+          'To fix this issue, remove all but one of these dependencies from pubspec.yaml.'
+          '\n\n'
+          'Plugin url_launcher:windows has conflicting direct dependency implementations:\n'
+          '  url_launcher_windows_1\n'
+          '  url_launcher_windows_2\n'
+          'To fix this issue, remove all but one of these dependencies from pubspec.yaml.'
+          '\n\n'
+        );
+      });
+
+      testUsingContext('provides error when user needs to select among multiple implementations', () async {
+        final Set<String> directDependencies = <String>{};
+        expect(() {
+          resolvePlatformImplementation(<Plugin>[
+            Plugin.fromYaml(
+              'url_launcher_linux_1',
+              '',
+              YamlMap.wrap(<String, dynamic>{
+                'implements': 'url_launcher',
+                'platforms': <String, dynamic>{
+                  'linux': <String, dynamic>{
+                    'dartPluginClass': 'UrlLauncherPluginLinux1',
+                  },
+                },
+              }),
+              null,
+              <String>[],
+              fileSystem: fs,
+              appDependencies: directDependencies,
+            ),
+            Plugin.fromYaml(
+              'url_launcher_linux_2',
+              '',
+              YamlMap.wrap(<String, dynamic>{
+                'implements': 'url_launcher',
+                'platforms': <String, dynamic>{
+                  'linux': <String, dynamic>{
+                    'dartPluginClass': 'UrlLauncherPluginLinux2',
+                  },
+                },
+              }),
+              null,
+              <String>[],
+              fileSystem: fs,
+              appDependencies: directDependencies,
+            ),
+          ]);
         },
         throwsToolExit(
           message: 'Please resolve the errors',
         ));
+
+        expect(
+          testLogger.errorText,
+          'Plugin url_launcher:linux has multiple possible implementations:\n'
+          '  url_launcher_linux_1\n'
+          '  url_launcher_linux_2\n'
+          'To fix this issue, add one of these dependencies to pubspec.yaml.'
+          '\n\n'
+        );
       });
     });
 
@@ -994,8 +1081,11 @@ void createFakeDartPlugins(
 ) {
   final Directory fakePubCache = fs.systemTempDirectory.childDirectory('cache');
   final File packagesFile = flutterProject.directory
-    .childFile('.packages')
-    ..createSync(recursive: true);
+    .childFile('.packages');
+  if (packagesFile.existsSync()) {
+    packagesFile.deleteSync();
+  }
+  packagesFile.createSync(recursive: true);
 
   for (final MapEntry<String, String> entry in plugins.entries) {
     final String name = fs.path.basename(entry.key);