[ffigen] Fix method vs getter conflicts (#1707)

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

@@ -19,6 +19,7 @@ class ObjCInterface extends BindingType with ObjCMethods {
   late final ObjCMsgSendFunc _isKindOfClassMsgSend;
   final protocols = <ObjCProtocol>[];
   final categories = <ObjCCategory>[];
+  final subtypes = <ObjCInterface>[];
 
   @override
   final ObjCBuiltInFunctions builtInFunctions;
@@ -194,5 +195,9 @@ ${generateAsStub ? '' : _generateMethods(w)}
     visitor.visitAll(protocols);
     visitor.visitAll(categories);
     visitMethods(visitor);
+
+    // Note: Don't visit subtypes here, because they shouldn't affect transitive
+    // inclusion. Including an interface shouldn't auto-include all its
+    // subtypes, even as stubs.
   }
 }

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

@@ -188,7 +188,7 @@ class ObjCMethod extends AstNode {
   final ObjCProperty? property;
   Type returnType;
   final List<Parameter> params;
-  final ObjCMethodKind kind;
+  ObjCMethodKind kind;
   final bool isClassMethod;
   final bool isOptional;
   ObjCMethodOwnership? ownershipAttribute;

pkgs/ffigen/lib/src/header_parser/sub_parsers/objcinterfacedecl_parser.dart

@@ -101,6 +101,7 @@ void _parseSuperType(clang_types.CXCursor cursor, ObjCInterface itf) {
       '$superType ${cursor.completeStringRepr()}');
   if (superType is ObjCInterface) {
     itf.superType = superType;
+    superType.subtypes.add(itf);
   } else {
     _logger.severe(
         'Super type of $itf is $superType, which is not a valid interface.');

pkgs/ffigen/lib/src/visitor/fix_overridden_methods.dart

@@ -10,7 +10,11 @@ class FixOverriddenMethodsVisitation extends Visitation {
   @override
   void visitObjCInterface(ObjCInterface node) {
     node.visitChildren(visitor);
+    _fixNullability(node);
+    _fixMethodsVsProperties(node);
+  }
 
+  void _fixNullability(ObjCInterface node) {
     // 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
     // it's return type from non-null to nullable, or its arg type from nullable
@@ -41,4 +45,43 @@ class FixOverriddenMethodsVisitation extends Visitation {
       }
     }
   }
+
+  void _fixMethodsVsProperties(ObjCInterface node) {
+    // In ObjC, supertypes and subtypes can have a method that's an ordinary
+    // method in some classes of the heirarchy, and a property in others. This
+    // isn't allowed in Dart, so we change all such conflicts to properties.
+    // This change could cause more conflicts in the heirarchy, so first we walk
+    // up to find the root of the subtree that has this method, then we walk
+    // down the subtreee to change all conflicting methods to properties.
+    for (final method in node.methods) {
+      final (root, rootMethod) = _findRootWithMethod(node, method);
+      if (method.isProperty == rootMethod.isProperty) continue;
+      _convertAllSubtreeMethodsToProperties(root, rootMethod);
+    }
+  }
+
+  (ObjCInterface, ObjCMethod) _findRootWithMethod(
+      ObjCInterface node, ObjCMethod method) {
+    var root = node;
+    var rootMethod = method;
+    for (ObjCInterface? t = node; t != null; t = t.superType) {
+      final tMethod = t.getMethod(method.originalName);
+      if (tMethod != null) {
+        root = t;
+        rootMethod = tMethod;
+      }
+    }
+    return (root, rootMethod);
+  }
+
+  void _convertAllSubtreeMethodsToProperties(
+      ObjCInterface node, ObjCMethod rootMethod) {
+    final method = node.getMethod(rootMethod.originalName);
+    if (method != null && method.kind == ObjCMethodKind.method) {
+      method.kind = ObjCMethodKind.propertyGetter;
+    }
+    for (final t in node.subtypes) {
+      _convertAllSubtreeMethodsToProperties(t, rootMethod);
+    }
+  }
 }

pkgs/ffigen/test/native_objc_test/bad_override_config.yaml

@@ -0,0 +1,19 @@
+name: BadOverrideTestObjCLibrary
+description: 'Tests various overrides that are valid in ObjC but invalid in Dart'
+language: objc
+output: 'bad_override_bindings.dart'
+exclude-all-by-default: true
+objc-interfaces:
+  include:
+    - BadOverrideGrandparent
+    - BadOverrideParent
+    - BadOverrideUncle
+    - BadOverrideAunt
+    - BadOverrideChild
+    - BadOverrideSibbling
+    - BadOverrideGrandchild
+headers:
+  entry-points:
+    - 'bad_override_test.m'
+preamble: |
+  // ignore_for_file: camel_case_types, non_constant_identifier_names, unnecessary_non_null_assertion, unused_element, unused_field

pkgs/ffigen/test/native_objc_test/bad_override_test.dart

@@ -0,0 +1,51 @@
+// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
+// 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.
+
+// Objective C support is only available on mac.
+@TestOn('mac-os')
+
+import 'dart:ffi';
+import 'dart:io';
+
+import 'package:ffi/ffi.dart';
+import 'package:test/test.dart';
+import '../test_utils.dart';
+import 'bad_override_bindings.dart';
+import 'util.dart';
+
+void main() {
+  group('bad overrides', () {
+    setUpAll(() {
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
+      final dylib = File('test/native_objc_test/objc_test.dylib');
+      verifySetupFile(dylib);
+      DynamicLibrary.open(dylib.absolute.path);
+      generateBindingsForCoverage('bad_override');
+    });
+
+    test('Method vs getter', () {
+      // In ObjC, supertypes and subtypes can have a method that's an ordinary
+      // method in some classes of the heirarchy, and a property in others. This
+      // isn't allowed in Dart, so we change all such conflicts to properties.
+      // https://github.com/dart-lang/native/issues/1220
+      expect(BadOverrideParent.new1().methodVsGetter, 1);
+      expect(BadOverrideChild.new1().methodVsGetter, 11);
+      expect(BadOverrideSibbling.new1().methodVsGetter, 12);
+      expect(BadOverrideGrandchild.new1().methodVsGetter, 111);
+
+      var inst = BadOverrideParent.new1();
+      expect(inst.methodVsGetter, 1);
+      inst = BadOverrideChild.new1();
+      expect(inst.methodVsGetter, 11);
+      inst = BadOverrideSibbling.new1();
+      expect(inst.methodVsGetter, 12);
+      inst = BadOverrideGrandchild.new1();
+      expect(inst.methodVsGetter, 111);
+
+      // Uncle isn't affected by the transform, so has an ordinary method.
+      expect(BadOverrideUncle.new1().methodVsGetter(), 2);
+    });
+  });
+}

pkgs/ffigen/test/native_objc_test/bad_override_test.m

@@ -0,0 +1,51 @@
+// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
+// 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 <Foundation/NSObject.h>
+
+@interface BadOverrideGrandparent : NSObject {}
+@end
+@implementation BadOverrideGrandparent
+@end
+
+@interface BadOverrideParent : BadOverrideGrandparent {}
+-(int32_t)methodVsGetter;
+@property (readonly) int32_t methodVsGetter;
+@end
+@implementation BadOverrideParent
+-(int32_t)methodVsGetter { return 1; }
+@end
+
+@interface BadOverrideUncle : BadOverrideGrandparent {}
+-(int32_t)methodVsGetter;
+@end
+@implementation BadOverrideUncle
+-(int32_t)methodVsGetter { return 2; }
+@end
+
+@interface BadOverrideAunt : BadOverrideGrandparent {}
+@end
+@implementation BadOverrideAunt
+@end
+
+@interface BadOverrideChild : BadOverrideParent {}
+@property (readonly) int32_t methodVsGetter;
+@end
+@implementation BadOverrideChild
+-(int32_t)methodVsGetter { return 11; }
+@end
+
+@interface BadOverrideSibbling : BadOverrideParent {}
+-(int32_t)methodVsGetter;
+@end
+@implementation BadOverrideSibbling
+-(int32_t)methodVsGetter { return 12; }
+@end
+
+@interface BadOverrideGrandchild : BadOverrideParent {}
+-(int32_t)methodVsGetter;
+@end
+@implementation BadOverrideGrandchild
+-(int32_t)methodVsGetter { return 111; }
+@end