[objective_c] Verify generated code is up to date (#1597)

.github/workflows/objective_c.yaml

@@ -62,6 +62,10 @@ jobs:
         run: dart test/setup.dart
       - name: Run VM tests and collect coverage
         run: dart run coverage:test_with_coverage --scope-output=ffigen --scope-output=objective_c
+      - name: Verify generated code is up to date
+        # test/generate_code_test.dart runs the code generator, so if there are
+        # any git-diffs at this point, it means the generated code is outdated.
+        run: if [[ -n $(git status --porcelain | tee /dev/stderr) ]]; then echo -e "\nDIFF:"; git diff; false; fi
       - name: Upload coverage
         uses: coverallsapp/github-action@643bc377ffa44ace6394b2b5d0d3950076de9f63
         with:

pkgs/ffigen/CHANGELOG.md

@@ -13,6 +13,7 @@
 - Fix a bug where ObjC listener blocks could be deleted after being invoked by
   ObjC but before the invocation was received by Dart:
   https://github.com/dart-lang/native/issues/1571
+- `sort:` config option now affects ObjC interface/protocol methods.
 
 ## 14.0.1
 

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

@@ -40,6 +40,13 @@ abstract class Binding {
 
   /// Returns the Objective C bindings, if any.
   BindingString? toObjCBindingString(Writer w) => null;
+
+  /// Sort members of this binding, if possible. For example, sort the methods
+  /// of a ObjCInterface.
+  void sort() {}
+
+  /// Whether these bindings should be generated.
+  bool get generateBindings => true;
 }
 
 /// Base class for bindings which look up symbols in dynamic library.

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

@@ -248,4 +248,7 @@ class Parameter {
   String getNativeType({String varName = ''}) =>
       '${type.getNativeType(varName: varName)}'
       '${objCConsumed ? ' __attribute__((ns_consumed))' : ''}';
+
+  @override
+  String toString() => '$type $name';
 }

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

@@ -140,11 +140,16 @@ class NativeFunc extends Type {
   }
 
   @override
-  String getCType(Writer w) =>
-      '${w.ffiLibraryPrefix}.NativeFunction<${_type.getCType(w)}>';
+  String getCType(Writer w, {bool writeArgumentNames = true}) {
+    final funcType = _type is FunctionType
+        ? _type.getCType(w, writeArgumentNames: writeArgumentNames)
+        : _type.getCType(w);
+    return '${w.ffiLibraryPrefix}.NativeFunction<$funcType>';
+  }
 
   @override
-  String getFfiDartType(Writer w) => getCType(w);
+  String getFfiDartType(Writer w, {bool writeArgumentNames = true}) =>
+      getCType(w, writeArgumentNames: writeArgumentNames);
 
   @override
   String getNativeType({String varName = ''}) =>

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

@@ -126,7 +126,7 @@ final wCharType = ImportedType(ffiImport, 'WChar', 'int', 'wchar_t', '0');
 
 final objCObjectType =
     ImportedType(objcPkgImport, 'ObjCObject', 'ObjCObject', 'void');
-final objCSelType =
-    ImportedType(objcPkgImport, 'ObjCSelector', 'ObjCSelector', 'void');
+final objCSelType = ImportedType(
+    objcPkgImport, 'ObjCSelector', 'ObjCSelector', 'objc_selector');
 final objCBlockType =
     ImportedType(objcPkgImport, 'ObjCBlockImpl', 'ObjCBlockImpl', 'id');

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

@@ -40,9 +40,12 @@ class Library {
   }) {
     _findBindings(bindings, sort);
 
+    final codeGenBindings =
+        this.bindings.where((b) => b.generateBindings).toList();
+
     /// Handle any declaration-declaration name conflicts and emit warnings.
     final declConflictHandler = UniqueNamer({});
-    for (final b in this.bindings) {
+    for (final b in codeGenBindings) {
       _warnIfPrivateDeclaration(b);
       _resolveIfNameConflicts(declConflictHandler, b);
     }
@@ -68,7 +71,7 @@ class Library {
     final nativeBindings = <LookUpBinding>[];
     FfiNativeConfig? nativeConfig;
 
-    for (final binding in this.bindings.whereType<LookUpBinding>()) {
+    for (final binding in codeGenBindings.whereType<LookUpBinding>()) {
       final nativeConfigForBinding = switch (binding) {
         Func() => binding.ffiNativeConfig,
         Global() => binding.nativeConfig,
@@ -83,7 +86,7 @@ class Library {
       (usesLookup ? lookupBindings : nativeBindings).add(binding);
     }
     final noLookUpBindings =
-        this.bindings.whereType<NoLookUpBinding>().toList();
+        codeGenBindings.whereType<NoLookUpBinding>().toList();
 
     _writer = Writer(
       lookUpBindings: lookupBindings,
@@ -112,6 +115,9 @@ class Library {
     bindings = dependencies.toList();
     if (sort) {
       bindings.sortBy((b) => b.name);
+      for (final b in bindings) {
+        b.sort();
+      }
     }
   }
 

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

@@ -116,6 +116,8 @@ class ObjCBlock extends BindingType {
         w.topLevelUniqueNamer.makeUnique('_${name}_fnPtrCallable');
     final closureCallable =
         w.topLevelUniqueNamer.makeUnique('_${name}_closureCallable');
+    final listenerTrampoline =
+        w.topLevelUniqueNamer.makeUnique('_${name}_listenerTrampoline');
     final listenerCallable =
         w.topLevelUniqueNamer.makeUnique('_${name}_listenerCallable');
     final callExtension =
@@ -169,11 +171,12 @@ $voidPtr $closureCallable = ${w.ffiLibraryPrefix}.Pointer.fromFunction<
     if (hasListener) {
       // Write the listener trampoline function.
       s.write('''
-$nativeCallableType $listenerCallable = $nativeCallableType.listener(
-    ($blockCType block, $paramsFfiDartType) {
+$returnFfiDartType $listenerTrampoline($blockCType block, $paramsFfiDartType) {
   ($getBlockClosure(block) as $funcFfiDartType)($paramsNameOnly);
   $releaseFn(block.cast());
-} $exceptionalReturn)..keepIsolateAlive = false;
+}
+$nativeCallableType $listenerCallable = $nativeCallableType.listener(
+    $listenerTrampoline $exceptionalReturn)..keepIsolateAlive = false;
 ''');
     }
 

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

@@ -6,6 +6,7 @@ import '../code_generator.dart';
 import '../config_provider/config_types.dart';
 
 import 'binding_string.dart';
+import 'utils.dart';
 import 'writer.dart';
 
 /// Built in functions used by the Objective C bindings.
@@ -122,12 +123,9 @@ class ObjCBuiltInFunctions {
   final _msgSendFuncs = <String, ObjCMsgSendFunc>{};
   ObjCMsgSendFunc getMsgSendFunc(Type returnType, List<Parameter> params) {
     assert(!_depsAdded);
-    var key = returnType.cacheKey();
-    for (final p in params) {
-      key += ' ${p.type.cacheKey()}';
-    }
-    return _msgSendFuncs[key] ??= ObjCMsgSendFunc(
-        '_objc_msgSend_${_msgSendFuncs.length}',
+    final id = _methodSigId(returnType, params);
+    return _msgSendFuncs[id] ??= ObjCMsgSendFunc(
+        '_objc_msgSend_${fnvHash32(id).toRadixString(36)}',
         returnType,
         params,
         useMsgSendVariants);
@@ -142,12 +140,9 @@ class ObjCBuiltInFunctions {
     );
   }
 
-  final _blockTrampolines = <String, ObjCListenerBlockTrampoline>{};
-  ObjCListenerBlockTrampoline? getListenerBlockTrampoline(ObjCBlock block) {
-    assert(!_depsAdded);
-
+  String _methodSigId(Type returnType, List<Parameter> params) {
     final paramIds = <String>[];
-    for (final param in block.params) {
+    for (final param in params) {
       final retainFunc = param.type.generateRetain('');
 
       // The trampoline ID is based on the getNativeType of the param. Objects
@@ -156,10 +151,17 @@ class ObjCBuiltInFunctions {
       // retainFunc (if any) to all the param IDs.
       paramIds.add('${param.getNativeType()}-${retainFunc ?? ''}');
     }
-    final id = paramIds.join(',');
+    final rt = '${returnType.getNativeType()}-${returnType.generateRetain('')}';
+    return '$rt,${paramIds.join(',')}';
+  }
+
+  final _blockTrampolines = <String, ObjCListenerBlockTrampoline>{};
+  ObjCListenerBlockTrampoline? getListenerBlockTrampoline(ObjCBlock block) {
+    assert(!_depsAdded);
+    final id = _methodSigId(block.returnType, block.params);
 
     return _blockTrampolines[id] ??= ObjCListenerBlockTrampoline(Func(
-      name: '_wrapListenerBlock_${id.hashCode.toRadixString(16)}',
+      name: '_wrapListenerBlock_${fnvHash32(id).toRadixString(36)}',
       returnType: PointerType(objCBlockType),
       parameters: [
         Parameter(
@@ -266,7 +268,7 @@ class ObjCMsgSendVariantFunc extends NoLookUpBinding {
 
   @override
   BindingString toBindingString(Writer w) {
-    final cType = NativeFunc(type).getCType(w);
+    final cType = NativeFunc(type).getCType(w, writeArgumentNames: false);
     final dartType = type.getFfiDartType(w, writeArgumentNames: false);
     final pointer = variant.pointer.gen(w);
 

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

@@ -8,9 +8,10 @@ import 'binding_string.dart';
 import 'utils.dart';
 import 'writer.dart';
 
-// Class methods defined on NSObject that we don't want to copy to child objects
-// by default.
-const _excludedNSObjectClassMethods = {
+// Methods defined on NSObject that we don't want to copy to child objects,
+// because they're unlikely to be used, and pollute the bindings. Note: Many of
+// these are still accessible via inheritance from NSObject.
+const _excludedNSObjectMethods = {
   'allocWithZone:',
   'class',
   'conformsToProtocol:',
@@ -28,6 +29,7 @@ const _excludedNSObjectClassMethods = {
   'poseAsClass:',
   'resolveClassMethod:',
   'resolveInstanceMethod:',
+  'respondsToSelector:',
   'setVersion:',
   'superclass',
   'version',
@@ -59,6 +61,9 @@ class ObjCInterface extends BindingType with ObjCMethods {
   void addProtocol(ObjCProtocol proto) => _protocols.add(proto);
   bool get _isBuiltIn => builtInFunctions.isBuiltInInterface(originalName);
 
+  @override
+  void sort() => sortMethods();
+
   @override
   BindingString toBindingString(Writer w) {
     if (_isBuiltIn) {
@@ -267,9 +272,7 @@ class ObjCInterface extends BindingType with ObjCMethods {
 
     for (final proto in _protocols) {
       proto.addDependencies(dependencies);
-      for (final m in proto.methods) {
-        addMethod(m);
-      }
+      _copyMethodsFromProtocol(proto);
     }
 
     // Add dependencies for any methods that were added.
@@ -284,14 +287,22 @@ class ObjCInterface extends BindingType with ObjCMethods {
     //    Note: instancetype is only allowed as a return type, not an arg type.
     for (final m in superType!.methods) {
       if (m.isClassMethod &&
-          !_excludedNSObjectClassMethods.contains(m.originalName)) {
+          !_excludedNSObjectMethods.contains(m.originalName)) {
         addMethod(m);
       } else if (ObjCBuiltInFunctions.isInstanceType(m.returnType)) {
         addMethod(m);
       }
     }
   }
 
+  void _copyMethodsFromProtocol(ObjCProtocol proto) {
+    for (final m in proto.methods) {
+      if (!_excludedNSObjectMethods.contains(m.originalName)) {
+        addMethod(m);
+      }
+    }
+  }
+
   void _fixNullabilityOfOverriddenMethods() {
     // ObjC ignores nullability when deciding if an override for an inherited
     // method is valid. But in Dart it's invalid to override a method and change

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

@@ -12,8 +12,9 @@ final _logger = Logger('ffigen.code_generator.objc_methods');
 
 mixin ObjCMethods {
   final _methods = <String, ObjCMethod>{};
+  final _order = <String>[];
 
-  Iterable<ObjCMethod> get methods => _methods.values;
+  Iterable<ObjCMethod> get methods => _order.map((name) => _methods[name]!);
   ObjCMethod? getMethod(String name) => _methods[name];
 
   String get originalName;
@@ -22,8 +23,13 @@ mixin ObjCMethods {
 
   void addMethod(ObjCMethod method) {
     if (_shouldIncludeMethod(method)) {
-      _methods[method.originalName] =
-          _maybeReplaceMethod(getMethod(method.originalName), method);
+      final oldMethod = getMethod(method.originalName);
+      if (oldMethod != null) {
+        _methods[method.originalName] = _maybeReplaceMethod(oldMethod, method);
+      } else {
+        _methods[method.originalName] = method;
+        _order.add(method.originalName);
+      }
     }
   }
 
@@ -38,9 +44,7 @@ mixin ObjCMethods {
     }
   }
 
-  ObjCMethod _maybeReplaceMethod(ObjCMethod? oldMethod, ObjCMethod newMethod) {
-    if (oldMethod == null) return newMethod;
-
+  ObjCMethod _maybeReplaceMethod(ObjCMethod oldMethod, ObjCMethod newMethod) {
     // Typically we ignore duplicate methods. However, property setters and
     // getters are duplicated in the AST. One copy is marked with
     // ObjCMethodKind.propertyGetter/Setter. The other copy is missing
@@ -80,12 +84,6 @@ mixin ObjCMethods {
       method.childTypes.every((Type t) {
         t = t.typealiasType.baseType;
 
-        // Ignore methods with variadic args.
-        // TODO(https://github.com/dart-lang/native/issues/1192): Remove this.
-        if (t is Struct && t.originalName == '__va_list_tag') {
-          return false;
-        }
-
         // Ignore methods with block args or rets when we're generating in
         // package:objective_c.
         // TODO(https://github.com/dart-lang/native/issues/1180): Remove this.
@@ -99,6 +97,8 @@ mixin ObjCMethods {
   UniqueNamer createMethodRenamer(Writer w) => UniqueNamer(
       {name, 'pointer', 'toString', 'hashCode', 'runtimeType', 'noSuchMethod'},
       parent: w.topLevelUniqueNamer);
+
+  void sortMethods() => _order.sort();
 }
 
 enum ObjCMethodKind {

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

@@ -12,6 +12,8 @@ class ObjCProtocol extends NoLookUpBinding with ObjCMethods {
   final superProtocols = <ObjCProtocol>[];
   final String lookupName;
   late final ObjCInternalGlobal _protocolPointer;
+
+  @override
   final bool generateBindings;
 
   @override
@@ -28,6 +30,9 @@ class ObjCProtocol extends NoLookUpBinding with ObjCMethods {
   })  : lookupName = lookupName ?? originalName,
         super(name: name ?? originalName);
 
+  @override
+  void sort() => sortMethods();
+
   @override
   BindingString toBindingString(Writer w) {
     if (!generateBindings) {

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

@@ -2,6 +2,7 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'dart:convert';
 import 'dart:io';
 
 import 'package:path/path.dart' as p;
@@ -123,6 +124,15 @@ String makeArrayAnnotation(Writer w, ConstantArray arrayType) {
   return '@${w.ffiLibraryPrefix}.Array.multi([${dimensions.join(', ')}])';
 }
 
+/// 32-bit FNV-1a hash function.
+int fnvHash32(String input) {
+  var hash = 0x811c9dc5;
+  for (final byte in utf8.encode(input)) {
+    hash = ((hash ^ byte) * 0x1000193) & 0xFFFFFFFF;
+  }
+  return hash;
+}
+
 /// The path to the Dart executable.
 ///
 /// This is usually just Platform.resolvedExecutable. But when running flutter