[ffigen] Clone methods when copying them between classes

pkgs/ffigen/CHANGELOG.md

@@ -14,6 +14,8 @@
     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.
+- Fix [a bug](https://github.com/dart-lang/native/issues/2419) where methods
+  copied from super types might not be renamed correctly.
 
 ## 19.1.0
 

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

@@ -234,17 +234,30 @@ class Parameter extends AstNode {
   String name;
   Type type;
   final bool objCConsumed;
-  bool isCovariant = false;
+  bool isCovariant;
+
+  Parameter._({
+    required this.originalName,
+    required this.name,
+    required this.type,
+    required this.objCConsumed,
+    required this.isCovariant,
+  });
 
   Parameter({
     String? originalName,
-    this.name = '',
+    String name = '',
     required Type type,
-    required this.objCConsumed,
-  }) : originalName = originalName ?? name,
-       // A [NativeFunc] is wrapped with a pointer because this is a shorthand
-       // used in C for Pointer to function.
-       type = type.typealiasType is NativeFunc ? PointerType(type) : type;
+    required bool objCConsumed,
+  }) : this._(
+         originalName: originalName ?? name,
+         name: name,
+         // A [NativeFunc] is wrapped with a pointer because this is a shorthand
+         // used in C for Pointer to function.
+         type: type.typealiasType is NativeFunc ? PointerType(type) : type,
+         objCConsumed: objCConsumed,
+         isCovariant: false,
+       );
 
   String getNativeType({String varName = ''}) =>
       '${type.getNativeType(varName: varName)}'
@@ -260,4 +273,12 @@ class Parameter extends AstNode {
   }
 
   bool get isNullable => type.typealiasType is ObjCNullable;
+
+  Parameter clone() => Parameter._(
+    originalName: originalName,
+    name: name,
+    type: type,
+    objCConsumed: objCConsumed,
+    isCovariant: isCovariant,
+  );
 }

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

@@ -30,6 +30,8 @@ mixin ObjCMethods {
 
   void addMethod(ObjCMethod? method) {
     if (method == null) return;
+    assert(!method._added);
+    method._added = true;
     final oldMethod = getSimilarMethod(method);
     if (oldMethod != null) {
       _methods[method.key] = _maybeReplaceMethod(oldMethod, method);
@@ -39,6 +41,11 @@ mixin ObjCMethods {
     }
   }
 
+  void copyMethod(ObjCMethod method) {
+    assert(method._added);
+    addMethod(method.clone());
+  }
+
   void visitMethods(Visitor visitor) {
     visitor.visitAll(_methods.values);
   }
@@ -194,7 +201,7 @@ class ObjCMethod extends AstNode {
   final String originalName;
   String name;
   String? dartMethodName;
-  late final String protocolMethodName;
+  final String protocolMethodName;
   final ObjCProperty? property;
   Type returnType;
   final List<Parameter> params;
@@ -204,10 +211,11 @@ class ObjCMethod extends AstNode {
   ObjCMethodOwnership? ownershipAttribute;
   final ObjCMethodFamily? family;
   final ApiAvailability apiAvailability;
-  bool consumesSelfAttribute = false;
+  bool consumesSelfAttribute;
   ObjCInternalGlobal selObject;
   ObjCMsgSendFunc? msgSend;
   ObjCBlock? protocolBlock;
+  bool _added = false;
 
   @override
   void visitChildren(Visitor visitor) {
@@ -220,26 +228,65 @@ class ObjCMethod extends AstNode {
     visitor.visit(protocolBlock);
   }
 
-  ObjCMethod({
+  ObjCMethod._({
     required this.builtInFunctions,
+    required this.dartDoc,
     required this.originalName,
     required this.name,
-    this.property,
-    this.dartDoc,
+    required this.dartMethodName,
+    required this.protocolMethodName,
+    required this.property,
+    required this.returnType,
+    required this.params,
     required this.kind,
     required this.isClassMethod,
     required this.isOptional,
-    required this.returnType,
+    required this.ownershipAttribute,
     required this.family,
     required this.apiAvailability,
+    required this.consumesSelfAttribute,
+    required this.selObject,
+    required this.msgSend,
+    required this.protocolBlock,
+  });
+
+  ObjCMethod({
+    required ObjCBuiltInFunctions builtInFunctions,
+    required String originalName,
+    required String name,
+    ObjCProperty? property,
+    String? dartDoc,
+    required ObjCMethodKind kind,
+    required bool isClassMethod,
+    required bool isOptional,
+    required Type returnType,
+    required ObjCMethodFamily? family,
+    required ApiAvailability apiAvailability,
     List<Parameter>? params_,
-  }) : params = params_ ?? [],
-       selObject = builtInFunctions.getSelObject(originalName);
+  }) : this._(
+         builtInFunctions: builtInFunctions,
+         dartDoc: dartDoc,
+         originalName: originalName,
+         name: name,
+         dartMethodName: null,
+         protocolMethodName: name.replaceAll(':', '_'),
+         property: property,
+         returnType: returnType,
+         params: params_ ?? [],
+         kind: kind,
+         isClassMethod: isClassMethod,
+         isOptional: isOptional,
+         ownershipAttribute: null,
+         family: family,
+         apiAvailability: apiAvailability,
+         consumesSelfAttribute: false,
+         selObject: builtInFunctions.getSelObject(originalName),
+         msgSend: null,
+         protocolBlock: null,
+       );
 
   // 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:
@@ -273,6 +320,30 @@ class ObjCMethod extends AstNode {
     }
   }
 
+  // Not a super deep clone. Only clones this object and its params. Other
+  // fields are identitcal. It's not necessary to call [finalizeParams] again.
+  ObjCMethod clone() => ObjCMethod._(
+    builtInFunctions: builtInFunctions,
+    dartDoc: dartDoc,
+    originalName: originalName,
+    name: name,
+    dartMethodName: dartMethodName,
+    protocolMethodName: protocolMethodName,
+    property: property,
+    returnType: returnType,
+    params: [for (final p in params) p.clone()],
+    kind: kind,
+    isClassMethod: isClassMethod,
+    isOptional: isOptional,
+    ownershipAttribute: ownershipAttribute,
+    family: family,
+    apiAvailability: apiAvailability,
+    consumesSelfAttribute: consumesSelfAttribute,
+    selObject: selObject,
+    msgSend: msgSend,
+    protocolBlock: protocolBlock,
+  );
+
   bool get isProperty =>
       kind == ObjCMethodKind.propertyGetter ||
       kind == ObjCMethodKind.propertySetter;

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

@@ -49,18 +49,18 @@ class CopyMethodsFromSuperTypesVisitation extends Visitation {
     if (superType != null) {
       for (final m in superType.methods) {
         if (isNSObject) {
-          node.addMethod(m);
+          node.copyMethod(m);
         } else if (m.isClassMethod &&
             !_excludedNSObjectMethods.contains(m.originalName)) {
-          node.addMethod(m);
+          node.copyMethod(m);
         } else if (ObjCBuiltInFunctions.isInstanceType(m.returnType)) {
-          node.addMethod(m);
+          node.copyMethod(m);
         }
       }
     }
 
     // Copy all methods from all the interface's protocols.
-    _copyMethodFromProtocols(node, node.protocols, node.addMethod);
+    _copyMethodFromProtocols(node, node.protocols, node.copyMethod);
 
     // Copy methods from all the categories that extend this interface, if those
     // methods return instancetype, because the Dart inheritance rules don't
@@ -74,7 +74,7 @@ class CopyMethodsFromSuperTypesVisitation extends Visitation {
     for (final category in node.categories) {
       for (final m in category.methods) {
         if (category.shouldCopyMethodToInterface(m)) {
-          node.addMethod(m);
+          node.copyMethod(m);
         }
       }
     }
@@ -83,16 +83,16 @@ class CopyMethodsFromSuperTypesVisitation extends Visitation {
   void _copyMethodFromProtocols(
     Binding node,
     List<ObjCProtocol> protocols,
-    void Function(ObjCMethod) addMethod,
+    void Function(ObjCMethod) copyMethod,
   ) {
     // Copy all methods from all the protocols.
     final isNSObject = ObjCBuiltInFunctions.isNSObject(node.originalName);
     for (final proto in protocols) {
       for (final m in proto.methods) {
         if (isNSObject) {
-          addMethod(m);
+          copyMethod(m);
         } else if (!_excludedNSObjectMethods.contains(m.originalName)) {
-          addMethod(m);
+          copyMethod(m);
         }
       }
     }
@@ -103,7 +103,7 @@ class CopyMethodsFromSuperTypesVisitation extends Visitation {
     node.visitChildren(visitor);
 
     // Copy all methods from all the category's protocols.
-    _copyMethodFromProtocols(node, node.protocols, node.addMethod);
+    _copyMethodFromProtocols(node, node.protocols, node.copyMethod);
   }
 
   @override
@@ -124,7 +124,7 @@ class CopyMethodsFromSuperTypesVisitation extends Visitation {
       // So copy across all the methods explicitly, rather than trying to use
       // Dart inheritance to get them implicitly.
       for (final method in superProtocol.methods) {
-        node.addMethod(method);
+        node.copyMethod(method);
       }
     }
   }

pkgs/ffigen/test/native_objc_test/inherited_instancetype_test.m

@@ -7,9 +7,14 @@
 @interface BaseClass : NSObject {}
 + (instancetype)create;
 - (instancetype)getSelf;
+
+// Regression test for https://github.com/dart-lang/native/issues/2419.
+- (instancetype) initRegress2419:(int32_t) i;
 @end
 
 @interface ChildClass : BaseClass {}
+- (instancetype) initRegress2419:(int32_t) i floatValue:(float) f;
+
 @property int32_t field;
 @end