[ffigen] Switch to named params for ObjC methods (#2298)

Author: liamappelbe

pkgs/ffigen/CHANGELOG.md

@@ -1,6 +1,14 @@
-## 19.0.0-wip
+## 19.0.0
 
 - Use package:objective_c 8.0.0.
+- __Breaking change__: Major change to the way ObjC methods are code-genned.
+  Methods now use named parameters, making them more readable and closer to how
+  they're written in ObjC. For example, the `NSData` method
+  `dataWithBytes:length:` used to be generated as
+  `dataWithBytes_length_(Pointer<Void> bytes, int length)`, but is now generated
+  as `dataWithBytes(Pointer<Void> bytes, {required int length})`. Protocol
+  methods are not affected.
+  - Migration tip: A quick way to find affected methods is to search for `_(`.
 - Make it easier for a downstream clone to change behavior of certain utils.
 - Fix [a bug](https://github.com/dart-lang/native/issues/1268) where types could
   occasionally show up as a generic ObjCObjectBase, when they were supposed to

pkgs/ffigen/example/objective_c/avf_audio_bindings.dart

@@ -7,12 +7,55 @@
 #error "This file must be compiled with ARC enabled"
 #endif
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wundeclared-selector"
+
+typedef struct {
+  int64_t version;
+  void* (*newWaiter)(void);
+  void (*awaitWaiter)(void*);
+  void* (*currentIsolate)(void);
+  void (*enterIsolate)(void*);
+  void (*exitIsolate)(void);
+  int64_t (*getMainPortId)(void);
+  bool (*getCurrentThreadOwnsIsolate)(int64_t);
+} DOBJC_Context;
+
 id objc_retainBlock(id);
 
+#define BLOCKING_BLOCK_IMPL(ctx, BLOCK_SIG, INVOKE_DIRECT, INVOKE_LISTENER)    \
+  assert(ctx->version >= 1);                                                   \
+  void* targetIsolate = ctx->currentIsolate();                                 \
+  int64_t targetPort = ctx->getMainPortId == NULL ? 0 : ctx->getMainPortId();  \
+  return BLOCK_SIG {                                                           \
+    void* currentIsolate = ctx->currentIsolate();                              \
+    bool mayEnterIsolate =                                                     \
+        currentIsolate == NULL &&                                              \
+        ctx->getCurrentThreadOwnsIsolate != NULL &&                            \
+        ctx->getCurrentThreadOwnsIsolate(targetPort);                          \
+    if (currentIsolate == targetIsolate || mayEnterIsolate) {                  \
+      if (mayEnterIsolate) {                                                   \
+        ctx->enterIsolate(targetIsolate);                                      \
+      }                                                                        \
+      INVOKE_DIRECT;                                                           \
+      if (mayEnterIsolate) {                                                   \
+        ctx->exitIsolate();                                                    \
+      }                                                                        \
+    } else {                                                                   \
+      void* waiter = ctx->newWaiter();                                         \
+      INVOKE_LISTENER;                                                         \
+      ctx->awaitWaiter(waiter);                                                \
+    }                                                                          \
+  };
+
+
 Protocol* _AVFAudio_AVAudioPlayerDelegate(void) { return @protocol(AVAudioPlayerDelegate); }
 
 typedef id  (^ProtocolTrampoline)(void * sel);
 __attribute__((visibility("default"))) __attribute__((used))
 id  _AVFAudio_protocolTrampoline_1mbt9g9(id target, void * sel) {
   return ((ProtocolTrampoline)((id (*)(id, SEL, SEL))objc_msgSend)(target, @selector(getDOBJCDartProtocolMethodForSelector:), sel))(sel);
 }
+#undef BLOCKING_BLOCK_IMPL
+
+#pragma clang diagnostic pop

pkgs/ffigen/example/objective_c/avf_audio_bindings.dart.m

@@ -18,9 +18,9 @@ void main(List<String> args) async {
   for (final file in args) {
     final fileStr = NSString(file);
     print('Loading $file');
-    final fileUrl = NSURL.fileURLWithPath_(fileStr);
+    final fileUrl = NSURL.fileURLWithPath(fileStr);
     final player =
-        AVAudioPlayer.alloc().initWithContentsOfURL_error_(fileUrl, nullptr);
+        AVAudioPlayer.alloc().initWithContentsOfURL(fileUrl, error: nullptr);
     if (player == null) {
       print('Failed to load audio');
       continue;

pkgs/ffigen/example/objective_c/play_audio.dart

@@ -14,7 +14,7 @@ void main() {
 
   // TODO(https://github.com/dart-lang/ffigen/issues/443): Add a test for this.
   DynamicLibrary.open('libswiftapi.dylib');
-  final object = SwiftClass.new1();
+  final object = SwiftClass();
   print(object.sayHello().toDartString());
   print('field = ${object.someField}');
   object.someField = 456;

pkgs/ffigen/example/swift/example.dart

@@ -219,7 +219,7 @@ class SwiftClass extends objc.NSObject {
 
   /// init
   SwiftClass init() {
-    objc.checkOsVersion('SwiftClass.init',
+    objc.checkOsVersionInternal('SwiftClass.init',
         iOS: (false, (2, 0, 0)), macOS: (false, (10, 0, 0)));
     final _ret =
         _objc_msgSend_151sglz(this.ref.retainAndReturnPointer(), _sel_init);
@@ -233,7 +233,7 @@ class SwiftClass extends objc.NSObject {
   }
 
   /// allocWithZone:
-  static SwiftClass allocWithZone_(ffi.Pointer<objc.NSZone> zone) {
+  static SwiftClass allocWithZone(ffi.Pointer<objc.NSZone> zone) {
     final _ret =
         _objc_msgSend_1cwp428(_class_SwiftClass, _sel_allocWithZone_, zone);
     return SwiftClass.castFromPointer(_ret, retain: false, release: true);

pkgs/ffigen/example/swift/swift_api_bindings.dart

@@ -7,10 +7,53 @@
 #error "This file must be compiled with ARC enabled"
 #endif
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wundeclared-selector"
+
+typedef struct {
+  int64_t version;
+  void* (*newWaiter)(void);
+  void (*awaitWaiter)(void*);
+  void* (*currentIsolate)(void);
+  void (*enterIsolate)(void*);
+  void (*exitIsolate)(void);
+  int64_t (*getMainPortId)(void);
+  bool (*getCurrentThreadOwnsIsolate)(int64_t);
+} DOBJC_Context;
+
 id objc_retainBlock(id);
 
+#define BLOCKING_BLOCK_IMPL(ctx, BLOCK_SIG, INVOKE_DIRECT, INVOKE_LISTENER)    \
+  assert(ctx->version >= 1);                                                   \
+  void* targetIsolate = ctx->currentIsolate();                                 \
+  int64_t targetPort = ctx->getMainPortId == NULL ? 0 : ctx->getMainPortId();  \
+  return BLOCK_SIG {                                                           \
+    void* currentIsolate = ctx->currentIsolate();                              \
+    bool mayEnterIsolate =                                                     \
+        currentIsolate == NULL &&                                              \
+        ctx->getCurrentThreadOwnsIsolate != NULL &&                            \
+        ctx->getCurrentThreadOwnsIsolate(targetPort);                          \
+    if (currentIsolate == targetIsolate || mayEnterIsolate) {                  \
+      if (mayEnterIsolate) {                                                   \
+        ctx->enterIsolate(targetIsolate);                                      \
+      }                                                                        \
+      INVOKE_DIRECT;                                                           \
+      if (mayEnterIsolate) {                                                   \
+        ctx->exitIsolate();                                                    \
+      }                                                                        \
+    } else {                                                                   \
+      void* waiter = ctx->newWaiter();                                         \
+      INVOKE_LISTENER;                                                         \
+      ctx->awaitWaiter(waiter);                                                \
+    }                                                                          \
+  };
+
+
 typedef id  (^ProtocolTrampoline)(void * sel);
 __attribute__((visibility("default"))) __attribute__((used))
 id  _SwiftLibrary_protocolTrampoline_1mbt9g9(id target, void * sel) {
   return ((ProtocolTrampoline)((id (*)(id, SEL, SEL))objc_msgSend)(target, @selector(getDOBJCDartProtocolMethodForSelector:), sel))(sel);
 }
+#undef BLOCKING_BLOCK_IMPL
+
+#pragma clang diagnostic pop

pkgs/ffigen/example/swift/swift_api_bindings.dart.m

@@ -259,4 +259,6 @@ class Parameter extends AstNode {
     super.visitChildren(visitor);
     visitor.visit(type);
   }
+
+  bool get isNullable => type.typealiasType is ObjCNullable;
 }

pkgs/ffigen/lib/src/code_generator/func.dart

@@ -201,8 +201,9 @@ class ObjCMethod extends AstNode {
   final ObjCBuiltInFunctions builtInFunctions;
   final String? dartDoc;
   final String originalName;
-  final String name;
+  String name;
   String? dartMethodName;
+  late final String protocolMethodName;
   final ObjCProperty? property;
   Type returnType;
   final List<Parameter> params;
@@ -244,6 +245,43 @@ class ObjCMethod extends AstNode {
   })  : params = params_ ?? [],
         selObject = builtInFunctions.getSelObject(originalName);
 
+  // Must be called after all params are added to the method.
+  void finalizeParams() {
+    protocolMethodName = name.replaceAll(':', '_');
+
+    // Split the name at the ':'. The first chunk is the name of the method, and
+    // the rest of the chunks are named parameters. Eg NSString's
+    //   - compare:options:range:
+    // method becomes
+    //   NSComparisonResult compare(NSString string,
+    //       {required NSStringCompareOptions options, required NSRange range})
+    final chunks = name.split(':');
+
+    // Details:
+    //  - The first chunk is always the new Dart method name.
+    //  - The rest of the chunks correspond to the params, so there's always
+    //    one more chunk than the number of params.
+    //  - The correspondence between the chunks and the params is non-trivial:
+    //    - The ObjC name always ends with a ':' unless there are no ':' at all.
+    //    - The first param is an ordinary param, not a named param.
+    final correctNumParams = chunks.length == params.length + 1;
+    final lastChunkIsEmpty = chunks.length == 1 || chunks.last.isEmpty;
+    if (correctNumParams && lastChunkIsEmpty) {
+      // Take the first chunk as the name, ignore the last chunk, and map the
+      // rest to each of the params after the first.
+      name = chunks[0];
+      for (var i = 1; i < params.length; ++i) {
+        params[i].name = chunks[i];
+      }
+    } else {
+      // There are a few methods that don't obey these rules, eg due to variadic
+      // parameters. Most of these are omitted from the bindings as they're not
+      // supported yet. But as a fallback, just replace all the ':' in the name
+      // with '_', like we do for protocol methods.
+      name = protocolMethodName;
+    }
+  }
+
   bool get isProperty =>
       kind == ObjCMethodKind.propertyGetter ||
       kind == ObjCMethodKind.propertySetter;
@@ -271,9 +309,11 @@ class ObjCMethod extends AstNode {
     )..fillProtocolTrampoline();
   }
 
-  String getDartMethodName(UniqueNamer uniqueNamer,
-      {bool usePropertyNaming = true}) {
-    if (property != null && usePropertyNaming) {
+  String getDartProtocolMethodName(UniqueNamer uniqueNamer) =>
+      uniqueNamer.makeUnique(protocolMethodName);
+
+  String getDartMethodName(UniqueNamer uniqueNamer) {
+    if (property != null) {
       // A getter and a setter are allowed to have the same name, so we can't
       // just run the name through uniqueNamer. Instead they need to share
       // the dartName, which is run through uniqueNamer.
@@ -282,12 +322,8 @@ class ObjCMethod extends AstNode {
       }
       return property!.dartName!;
     }
-    // Objective C methods can look like:
-    // foo
-    // foo:
-    // foo:someArgName:
-    // So replace all ':' with '_'.
-    return uniqueNamer.makeUnique(name.replaceAll(':', '_'));
+
+    return uniqueNamer.makeUnique(name);
   }
 
   bool sameAs(ObjCMethod other) {
@@ -345,20 +381,35 @@ class ObjCMethod extends AstNode {
     return returnType.getDartType(w);
   }
 
+  static String _paramToStr(Writer w, Parameter p) =>
+      '${p.isCovariant ? 'covariant ' : ''}${p.type.getDartType(w)} ${p.name}';
+
+  static String _paramToNamed(Writer w, Parameter p) =>
+      '${p.isNullable ? '' : 'required '}${_paramToStr(w, p)}';
+
+  static String _joinParamStr(Writer w, List<Parameter> params) {
+    if (params.isEmpty) return '';
+    if (params.length == 1) return _paramToStr(w, params.first);
+    final named = params.sublist(1).map((p) => _paramToNamed(w, p)).join(',');
+    return '${_paramToStr(w, params.first)}, {$named}';
+  }
+
   String generateBindings(
       Writer w, ObjCInterface target, UniqueNamer methodNamer) {
-    dartMethodName ??= getDartMethodName(methodNamer);
+    if (dartMethodName == null) {
+      dartMethodName = getDartMethodName(methodNamer);
+      final paramNamer = UniqueNamer(parent: methodNamer);
+      for (final p in params) {
+        p.name = paramNamer.makeUnique(p.name);
+      }
+    }
     final methodName = dartMethodName!;
     final upperName = methodName[0].toUpperCase() + methodName.substring(1);
     final s = StringBuffer();
 
     final targetType = target.getDartType(w);
     final returnTypeStr = _getConvertedReturnType(w, targetType);
-    final paramStr = <String>[
-      for (final p in params)
-        '${p.isCovariant ? 'covariant ' : ''}'
-            '${p.type.getDartType(w)} ${p.name}',
-    ].join(', ');
+    final paramStr = _joinParamStr(w, params);
 
     // The method declaration.
     s.write('\n  ${makeDartDoc(dartDoc)}  ');

pkgs/ffigen/lib/src/code_generator/objc_methods.dart

@@ -112,8 +112,7 @@ interface class $name extends $protocolBase $impls{
 
       var anyListeners = false;
       for (final method in methods) {
-        final methodName =
-            method.getDartMethodName(methodNamer, usePropertyNaming: false);
+        final methodName = method.getDartProtocolMethodName(methodNamer);
         final fieldName = methodName;
         final argName = methodName;
         final block = method.protocolBlock!;