[ffigen] Move all instance methods to extension methods (#2434)

Author: liamappelbe

pkgs/ffigen/CHANGELOG.md

@@ -4,6 +4,16 @@
   `Config` classes. Rename `FfiGen.run` to `.generate`, and make it an extension
   method on the `Config` class. So `FfiGen().run(config)` becomes
   `config.generate(logger)`.
+- __Breaking change__: Minor breaking change in the way that ObjC interface
+  methods are generated. Interface methods are now generated as extension
+  methods instead of being part of the class. This shouldn't require any code
+  changes unless you are using `show` or `hide` when importing the interface.
+  - If you are using `show`/`hide` to show or hide a particular interface, eg
+    `Foo`, you'll now also need to show or hide `Foo$Methods`.
+  - In rare cases the runtime type of the Dart wrapper object around the ObjC
+    object may change, but the underlying ObjC object will still be the same.
+    In any case, you should be using `Foo.isInstance(x)` instead of `x is Foo`
+    to check the runtime type of an ObjC object.
 
 ## 19.1.0
 

pkgs/ffigen/dartdoc_options.yaml

@@ -0,0 +1,5 @@
+dartdoc:
+  categories:
+    Errors:
+      markdown: doc/errors.md
+      name: Common errors in ffigen

pkgs/ffigen/doc/errors.md

@@ -24,3 +24,30 @@ compiler-opts:
 As a last resort, you can pass in `--ignore-source-errors` or set `ignore-source-errors: true` in yaml config.
 
 **Warning: This will likely lead to incorrect bindings!**
+
+## Errors in Objective-C bindings
+
+### Method not defined
+
+ObjC's method naming is a bit different to Dart's. For example, `NSString` has
+a method `stringWithCharacters:length:`. To make invocation feel as similar to
+ObjC as possible, we generate this as a method named `stringWithCharacters`,
+with a named required parameter called `length`.
+
+So one reason you might be having trouble invoking the method is that this
+naming convention is different to ObjC. The generated methods have a comment
+giving their full ObjC name, so you can try searching for that string to find
+the corresponding Dart method.
+
+Since Dart doesn't support method overloading, the method may have been renamed
+to avoid collisions. Again the best approach is to search for the ObjC method
+name.
+
+When ffigen generates bindings for an ObjC interface, eg `Foo`, it generates a
+Dart class `Foo`, which contains the constructor and static methods. The
+instance methods are generated in an extension, `Foo$Methods`.
+
+So if you've searched for the generated method and found it, and still can't
+invoke it, it's possible that you've imported the class, `Foo`, but not the
+extension `Foo$Methods`. This can happen if you're using `show`/`hide` when you
+import the bindings. Make sure to import both `Foo` and `Foo$Methods`.

pkgs/ffigen/lib/ffigen.dart

@@ -6,6 +6,8 @@
 ///
 /// For most use cases the YAML based API is simpler. See
 /// https://pub.dev/packages/ffigen for details.
+///
+/// {@category Errors}
 library;
 
 export 'src/code_generator/imports.dart' show ImportedType, LibraryImport;

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

@@ -111,18 +111,28 @@ class $name extends ${superType?.getDartType(w) ?? wrapObjType} $protoImpl{
       {bool retain = false, bool release = false}) :
       this._(other, retain: retain, release: release);
 
-${generateAsStub ? '' : _generateMethods(w)}
+${generateAsStub ? '' : _generateStaticMethods(w)}
 }
 
 ''');
 
+    if (!generateAsStub) {
+      final extName = w.topLevelUniqueNamer.makeUnique('$name\$Methods');
+      s.write('''
+extension $extName on $name {
+${generateInstanceMethodBindings(w, this)}
+}
+
+''');
+    }
+
     return BindingString(
       type: BindingStringType.objcInterface,
       string: s.toString(),
     );
   }
 
-  String _generateMethods(Writer w) {
+  String _generateStaticMethods(Writer w) {
     final wrapObjType = ObjCBuiltInFunctions.objectBase.gen(w);
     final s = StringBuffer();
 
@@ -132,7 +142,8 @@ ${generateAsStub ? '' : _generateMethods(w)}
     return ${_isKindOfClassMsgSend.invoke(w, 'obj.ref.pointer', _isKindOfClass.name, [classObject.name])};
   }
 ''');
-    s.write(generateMethodBindings(w, this));
+
+    s.write(generateStaticMethodBindings(w, this));
 
     final newMethod = methods
         .where(

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

@@ -115,10 +115,21 @@ mixin ObjCMethods {
     _methods = newMethods;
   }
 
-  String generateMethodBindings(Writer w, ObjCInterface target) {
+  String generateMethodBindings(Writer w, ObjCInterface target) =>
+      _generateMethods(w, target, null);
+
+  String generateStaticMethodBindings(Writer w, ObjCInterface target) =>
+      _generateMethods(w, target, true);
+
+  String generateInstanceMethodBindings(Writer w, ObjCInterface target) =>
+      _generateMethods(w, target, false);
+
+  String _generateMethods(Writer w, ObjCInterface target, bool? staticMethods) {
     final methodNamer = createMethodRenamer(w);
     return [
-      for (final m in methods) m.generateBindings(w, target, methodNamer),
+      for (final m in methods)
+        if (staticMethods == null || staticMethods == m.isClassMethod)
+          m.generateBindings(w, target, methodNamer),
     ].join('\n');
   }
 }
@@ -359,7 +370,7 @@ class ObjCMethod extends AstNode {
   }
 
   static String _paramToStr(Writer w, Parameter p) =>
-      '${p.isCovariant ? 'covariant ' : ''}${p.type.getDartType(w)} ${p.name}';
+      '${p.type.getDartType(w)} ${p.name}';
 
   static String _paramToNamed(Writer w, Parameter p) =>
       '${p.isNullable ? '' : 'required '}${_paramToStr(w, p)}';

pkgs/ffigen/test/native_objc_test/bad_override_test.dart

@@ -88,7 +88,10 @@ void main() {
 
       parent = BadOverrideChild();
       expect(parent.covariantArg(square).toDartString(), 'Rectangle: Square');
-      expect(() => parent.covariantArg(triangle), throwsA(isA<TypeError>()));
+      expect(
+        parent.covariantArg(triangle).toDartString(),
+        'Rectangle: Triangle',
+      );
     });
   });
 }

pkgs/ffigen/test/native_objc_test/category_config.yaml

@@ -15,6 +15,7 @@ objc-categories:
     - CatImplementsProto
     - InstanceTypeCategory
     - InterfaceOnBuiltInType
+    - StaticAndInstanceMethodsWithSameNameCategory
 headers:
   entry-points:
     - 'category_test.h'

pkgs/ffigen/test/native_objc_test/category_test.h

@@ -54,3 +54,8 @@
 @interface NSString (NSStringCategory)
 -(int32_t)excludedExtensionMethod;
 @end
+
+@interface Thing (StaticAndInstanceMethodsWithSameNameCategory)
+-(int32_t)sameNameMethod;
++(int32_t)sameNameMethod;
+@end

pkgs/ffigen/test/native_objc_test/inherited_instancetype_test.dart

@@ -60,11 +60,13 @@ void main() {
       final ChildClass child = ChildClass.alloc().init();
       final BaseClass base = child;
 
-      // Calling base.getSelf() should still go through ChildClass.getSelf, so
-      // the result will have a compile time type of BaseClass, but a runtime
-      // type of ChildClass.
+      // Calling base.getSelf() goes through BaseClass.getSelf on the Dart side,
+      // but is dynamically dispatched to the ObjC method ChildClass.getSelf. So
+      // the Dart wrapper object is a BaseClass, but the underlying ObjC object
+      // is a ChildClass.
       final BaseClass sameChild = base.getSelf();
-      expect(sameChild, isA<ChildClass>());
+      expect(sameChild, isA<BaseClass>());
+      expect(ChildClass.isInstance(sameChild), isTrue);
     });
   });
 }