Remove flutter api named and optional parameters (#5689)

fixes flutter/flutter#140045 replaces #5663 as no longer needed.
flutter · Dec 16, 2023 · d7dee79 · d7dee79
1 parent baae12e
commit d7dee79
Show file tree

Hide file tree

Showing 8 changed files with 92 additions and 9 deletions.
diff --git a/packages/pigeon/CHANGELOG.md b/packages/pigeon/CHANGELOG.md
@@ -1,3 +1,8 @@
+## 15.0.2
+
+* Prevents optional and non-positional parameters in Flutter APIs.
+* [dart] Fixes named parameters in test output of host API methods.
+
 ## 15.0.1
 
 * [java] Adds @CanIgnoreReturnValue annotation to class builder.

diff --git a/packages/pigeon/lib/ast.dart b/packages/pigeon/lib/ast.dart
@@ -277,7 +277,7 @@ class Parameter extends NamedType {
     bool? isPositional,
     bool? isRequired,
     super.documentationComments,
-  })  : isNamed = isNamed ?? true,
+  })  : isNamed = isNamed ?? false,
         isOptional = isOptional ?? false,
         isPositional = isPositional ?? true,
         isRequired = isRequired ?? true;

diff --git a/packages/pigeon/lib/dart_generator.dart b/packages/pigeon/lib/dart_generator.dart
@@ -418,9 +418,9 @@ $resultAt != null
                     }
                   });
                   final Iterable<String> argNames =
-                      indexMap(func.parameters, (int index, NamedType field) {
+                      indexMap(func.parameters, (int index, Parameter field) {
                     final String name = _getSafeArgumentName(index, field);
-                    return '$name${field.type.isNullable ? '' : '!'}';
+                    return '${field.isNamed ? '${field.name}: ' : ''}$name${field.type.isNullable ? '' : '!'}';
                   });
                   call = 'api.${func.name}(${argNames.join(', ')})';
                 }

diff --git a/packages/pigeon/lib/generator_tools.dart b/packages/pigeon/lib/generator_tools.dart
@@ -13,7 +13,7 @@ import 'ast.dart';
 /// The current version of pigeon.
 ///
 /// This must match the version in pubspec.yaml.
-const String pigeonVersion = '15.0.1';
+const String pigeonVersion = '15.0.2';
 
 /// Read all the content from [stdin] to a String.
 String readStdin() {

diff --git a/packages/pigeon/lib/pigeon_lib.dart b/packages/pigeon/lib/pigeon_lib.dart
@@ -809,6 +809,21 @@ List<Error> _validateAst(Root root, String source) {
             lineNumber: _calculateLineNumberNullable(source, param.offset),
           ));
         }
+        if (api.location == ApiLocation.flutter) {
+          if (!param.isPositional) {
+            result.add(Error(
+              message:
+                  'FlutterApi method parameters must be positional, in method "${method.name}" in API: "${api.name}"',
+              lineNumber: _calculateLineNumberNullable(source, param.offset),
+            ));
+          } else if (param.isOptional) {
+            result.add(Error(
+              message:
+                  'FlutterApi method parameters must not be optional, in method "${method.name}" in API: "${api.name}"',
+              lineNumber: _calculateLineNumberNullable(source, param.offset),
+            ));
+          }
+        }
       }
       if (method.objcSelector.isNotEmpty) {
         if (':'.allMatches(method.objcSelector).length !=
@@ -1132,10 +1147,10 @@ class _RootBuilder extends dart_ast_visitor.RecursiveAstVisitor<Object?> {
         ),
         name: formalParameter.name?.lexeme ?? '',
         offset: formalParameter.offset,
-        isNamed: isNamed,
-        isOptional: isOptional,
-        isPositional: isPositional,
-        isRequired: isRequired,
+        isNamed: isNamed ?? formalParameter.isNamed,
+        isOptional: isOptional ?? formalParameter.isOptional,
+        isPositional: isPositional ?? formalParameter.isPositional,
+        isRequired: isRequired ?? formalParameter.isRequired,
         defaultValue: defaultValue,
       );
     } else if (simpleFormalParameter != null) {

diff --git a/packages/pigeon/pubspec.yaml b/packages/pigeon/pubspec.yaml
@@ -2,7 +2,7 @@ name: pigeon
 description: Code generator tool to make communication between Flutter and the host platform type-safe and easier.
 repository: https://github.com/flutter/packages/tree/main/packages/pigeon
 issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+pigeon%22
-version: 15.0.1 # This must match the version in lib/generator_tools.dart
+version: 15.0.2 # This must match the version in lib/generator_tools.dart
 
 environment:
   sdk: ">=3.0.0 <4.0.0"

diff --git a/packages/pigeon/test/dart_generator_test.dart b/packages/pigeon/test/dart_generator_test.dart
@@ -1417,6 +1417,41 @@ void main() {
     expect(code, contains('void doit(int? foo);'));
   });
 
+  test('named argument flutter', () {
+    final Root root = Root(
+      apis: <Api>[
+        Api(name: 'Api', location: ApiLocation.flutter, methods: <Method>[
+          Method(
+              name: 'doit',
+              returnType: const TypeDeclaration.voidDeclaration(),
+              parameters: <Parameter>[
+                Parameter(
+                    name: 'foo',
+                    type: const TypeDeclaration(
+                      baseName: 'int',
+                      isNullable: false,
+                    ),
+                    isNamed: true,
+                    isPositional: false),
+              ])
+        ])
+      ],
+      classes: <Class>[],
+      enums: <Enum>[],
+    );
+    final StringBuffer sink = StringBuffer();
+    const DartGenerator generator = DartGenerator();
+    generator.generate(
+      const DartOptions(),
+      root,
+      sink,
+      dartPackageName: DEFAULT_PACKAGE_NAME,
+    );
+    final String code = sink.toString();
+    expect(code, contains('void doit({required int foo});'));
+    expect(code, contains('api.doit(foo: arg_foo!)'));
+  });
+
   test('uses output package name for imports', () {
     const String overriddenPackageName = 'custom_name';
     const String outputPackageName = 'some_output_package';

diff --git a/packages/pigeon/test/pigeon_lib_test.dart b/packages/pigeon/test/pigeon_lib_test.dart
@@ -1324,4 +1324,32 @@ abstract class Api {
     });
     await completer.future;
   });
+
+  test('unsupported non-positional parameters on FlutterApi', () {
+    const String code = '''
+@FlutterApi()
+abstract class Api {
+  int? calc({int? anInt});
+}
+''';
+
+    final ParseResults results = parseSource(code);
+    expect(results.errors.length, 1);
+    expect(results.errors[0].message,
+        contains('FlutterApi method parameters must be positional'));
+  });
+
+  test('unsupported optional parameters on FlutterApi', () {
+    const String code = '''
+@FlutterApi()
+abstract class Api {
+  int? calc([int? anInt]);
+}
+''';
+
+    final ParseResults results = parseSource(code);
+    expect(results.errors.length, 1);
+    expect(results.errors[0].message,
+        contains('FlutterApi method parameters must not be optional'));
+  });
 }