[ffigen] Fix method variance issues

pkgs/ffigen/CHANGELOG.md

@@ -25,6 +25,8 @@
   generating them as stubs. `include-transitive-objc-protocols` defaults to
   false, and `include-transitive-objc-categories` defaults to true, but these
   both replicate the existing behavior.
+- Fix [bugs](https://github.com/dart-lang/native/issues/1220) caused by
+  mismatches between ObjC and Dart's inheritance rules.
 
 ## 15.0.0
 

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

@@ -234,6 +234,7 @@ class Parameter extends AstNode {
   String name;
   Type type;
   final bool objCConsumed;
+  bool isCovariant = false;
 
   Parameter({
     String? originalName,

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

@@ -119,6 +119,21 @@ class FunctionType extends Type {
     visitor.visitAll(parameters);
     visitor.visitAll(varArgParameters);
   }
+
+  @override
+  bool isSupertypeOf(Type other) {
+    other = other.typealiasType;
+    if (other is FunctionType) {
+      return Type.isSupertypeOfVariance(
+        covariantLeft: [returnType],
+        covariantRight: [other.returnType],
+        contravariantLeft: dartTypeParameters.map((p) => p.type).toList(),
+        contravariantRight:
+            other.dartTypeParameters.map((p) => p.type).toList(),
+      );
+    }
+    return false;
+  }
 }
 
 /// Represents a NativeFunction<Function>.
@@ -165,4 +180,11 @@ class NativeFunc extends Type {
     super.visitChildren(visitor);
     visitor.visit(_type);
   }
+
+  @override
+  bool isSupertypeOf(Type other) {
+    other = other.typealiasType;
+    if (other is NativeFunc) return type.isSupertypeOf(other.type);
+    return false;
+  }
 }

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

@@ -389,4 +389,18 @@ $blockName $fnName($blockName block) NS_RETURNS_RETAINED {
     visitor.visitAll(params);
     visitor.visit(_wrapListenerBlock);
   }
+
+  @override
+  bool isSupertypeOf(Type other) {
+    other = other.typealiasType;
+    if (other is ObjCBlock) {
+      return Type.isSupertypeOfVariance(
+        covariantLeft: [returnType],
+        covariantRight: [other.returnType],
+        contravariantLeft: params.map((p) => p.type).toList(),
+        contravariantRight: other.params.map((p) => p.type).toList(),
+      );
+    }
+    return false;
+  }
 }

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

@@ -200,4 +200,15 @@ ${generateAsStub ? '' : _generateMethods(w)}
     // inclusion. Including an interface shouldn't auto-include all its
     // subtypes, even as stubs.
   }
+
+  @override
+  bool isSupertypeOf(Type other) {
+    other = other.typealiasType;
+    if (other is ObjCInterface) {
+      for (ObjCInterface? t = other; t != null; t = t.superType) {
+        if (t == this) return true;
+      }
+    }
+    return false;
+  }
 }

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

@@ -333,7 +333,9 @@ class ObjCMethod extends AstNode {
     final targetType = target.getDartType(w);
     final returnTypeStr = _getConvertedReturnType(w, targetType);
     final paramStr = <String>[
-      for (final p in params) '${p.type.getDartType(w)} ${p.name}',
+      for (final p in params)
+        '${p.isCovariant ? 'covariant ' : ''}'
+            '${p.type.getDartType(w)} ${p.name}',
     ].join(', ');
 
     // The method declaration.

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

@@ -98,4 +98,17 @@ class ObjCNullable extends Type {
     super.visitChildren(visitor);
     visitor.visit(child);
   }
+
+  @override
+  bool isSupertypeOf(Type other) {
+    other = other.typealiasType;
+
+    if (other is ObjCNullable) {
+      // T? :> S? if T :> S
+      return child.isSupertypeOf(other.child);
+    } else {
+      // T? :> S if T :> S
+      return child.isSupertypeOf(other);
+    }
+  }
 }

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

@@ -51,6 +51,15 @@ class PointerType extends Type {
 
   @override
   void visit(Visitation visitation) => visitation.visitPointerType(this);
+
+  @override
+  bool isSupertypeOf(Type other) {
+    other = other.typealiasType;
+    if (other is PointerType) {
+      return child.isSupertypeOf(other.child);
+    }
+    return false;
+  }
 }
 
 /// Represents a constant array, which has a fixed size.

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

@@ -33,6 +33,18 @@ abstract class Type extends AstNode {
   /// Returns true if the type is a [Compound] and is incomplete.
   bool get isIncompleteCompound => false;
 
+  /// Returns true if this is a subtype of [other]. That is this <: other.
+  ///
+  /// The behavior of this function should mirror Dart's subtyping logic, not
+  /// Objective-C's. It's used to detect and fix cases where the generated
+  /// bindings would fail `dart analyze` due to Dart's subtyping rules.
+  ///
+  /// Note: Implementers should implement [isSupertypeOf].
+  bool isSubtypeOf(Type other) => other.isSupertypeOf(this);
+
+  /// Returns true if this is a supertype of [other]. That is this :> other.
+  bool isSupertypeOf(Type other) => typealiasType == other.typealiasType;
+
   /// Returns the C type of the Type. This is the FFI compatible type that is
   /// passed to native code.
   String getCType(Writer w) =>
@@ -124,6 +136,28 @@ abstract class Type extends AstNode {
 
   @override
   void visit(Visitation visitation) => visitation.visitType(this);
+
+  // Helper for [isSupertypeOf] that applies variance rules.
+  static bool isSupertypeOfVariance({
+    List<Type> covariantLeft = const [],
+    List<Type> covariantRight = const [],
+    List<Type> contravariantLeft = const [],
+    List<Type> contravariantRight = const [],
+  }) =>
+      isSupertypeOfCovariance(left: covariantLeft, right: covariantRight) &&
+      isSupertypeOfCovariance(
+          left: contravariantRight, right: contravariantLeft);
+
+  static bool isSupertypeOfCovariance({
+    required List<Type> left,
+    required List<Type> right,
+  }) {
+    if (left.length != right.length) return false;
+    for (var i = 0; i < left.length; ++i) {
+      if (!left[i].isSupertypeOf(right[i])) return false;
+    }
+    return true;
+  }
 }
 
 /// Base class for all Type bindings.
@@ -152,6 +186,12 @@ abstract class BindingType extends NoLookUpBinding implements Type {
   @override
   bool get isIncompleteCompound => false;
 
+  @override
+  bool isSubtypeOf(Type other) => other.isSupertypeOf(this);
+
+  @override
+  bool isSupertypeOf(Type other) => typealiasType == other.typealiasType;
+
   @override
   String getFfiDartType(Writer w) => getCType(w);
 

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

@@ -209,6 +209,9 @@ class Typealias extends BindingType {
 
   @override
   void visit(Visitation visitation) => visitation.visitTypealias(this);
+
+  @override
+  bool isSupertypeOf(Type other) => type.isSupertypeOf(other);
 }
 
 /// Objective C's instancetype.

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

@@ -61,6 +61,12 @@ ObjCProtocol? parseObjCProtocolDeclaration(clang_types.CXCursor cursor) {
           protocol.superProtocols.add(superProtocol);
         }
         break;
+      case clang_types.CXCursorKind.CXCursor_ObjCPropertyDecl:
+        final (getter, setter) =
+            parseObjCProperty(child, decl, config.objcProtocols);
+        protocol.addMethod(getter);
+        protocol.addMethod(setter);
+        break;
       case clang_types.CXCursorKind.CXCursor_ObjCInstanceMethodDecl:
       case clang_types.CXCursorKind.CXCursor_ObjCClassMethodDecl:
         protocol.addMethod(parseObjCMethod(child, decl, config.objcProtocols));

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

@@ -87,13 +87,7 @@ class CopyMethodsFromSuperTypesVisitation extends Visitation {
     for (final proto in protocols) {
       for (final m in proto.methods) {
         if (isNSObject) {
-          if (m.originalName == 'description' || m.originalName == 'hash') {
-            // TODO(https://github.com/dart-lang/native/issues/1220): Remove
-            // this special case. These methods only clash because they're
-            // sometimes declared as getters and sometimes as normal methods.
-          } else {
-            addMethod(m);
-          }
+          addMethod(m);
         } else if (!_excludedNSObjectMethods.contains(m.originalName)) {
           addMethod(m);
         }

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

@@ -2,46 +2,118 @@
 // 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 'package:logging/logging.dart';
+
 import '../code_generator.dart';
 
 import 'ast.dart';
 
+final _logger = Logger('ffigen.visitor.FixOverriddenMethodsVisitation');
+
 class FixOverriddenMethodsVisitation extends Visitation {
   @override
   void visitObjCInterface(ObjCInterface node) {
-    node.visitChildren(visitor);
-    _fixNullability(node);
+    // Visit the supertype, then perform all AST mutations, then visit the other
+    // children. That way we can be sure that the supertype's AST mutations have
+    // been completed before this node's mutations (this is important for
+    // _fixContravariantReturns).
+    visitor.visit(node.superType);
+
+    _fixMethodVariance(node);
     _fixMethodsVsProperties(node);
+
+    node.visitChildren(visitor);
   }
 
-  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
-    // to non-null. So in these cases we have to make the non-null type
-    // nullable, to avoid Dart compile errors.
+  (ObjCInterface?, ObjCMethod?) _findNearestWithMethod(
+      ObjCInterface node, ObjCMethod method) {
     for (var t = node.superType; t != null; t = t.superType) {
-      for (final method in node.methods) {
-        final superMethod = t.getSimilarMethod(method);
-        if (superMethod != null &&
-            !superMethod.isClassMethod &&
-            !method.isClassMethod) {
-          if (superMethod.returnType.typealiasType is! ObjCNullable &&
-              method.returnType.typealiasType is ObjCNullable) {
-            superMethod.returnType = ObjCNullable(superMethod.returnType);
-          }
-          final numArgs = method.params.length < superMethod.params.length
-              ? method.params.length
-              : superMethod.params.length;
-          for (var i = 0; i < numArgs; ++i) {
-            final param = method.params[i];
-            final superParam = superMethod.params[i];
-            if (superParam.type.typealiasType is ObjCNullable &&
-                param.type.typealiasType is! ObjCNullable) {
-              param.type = ObjCNullable(param.type);
-            }
-          }
-        }
+      final tMethod = t.getSimilarMethod(method);
+      if (tMethod != null) {
+        return (t, tMethod);
+      }
+    }
+    return (null, null);
+  }
+
+  void _fixContravariantReturns(ObjCInterface node, ObjCMethod method,
+      ObjCInterface superType, ObjCMethod superMethod) {
+    // In Dart, method return types are covariant, but ObjC allows them to be
+    // contravariant. So fix these cases by changing the supertype's return type
+    // to match the subtype's return type.
+
+    if (method.returnType.isSubtypeOf(superMethod.returnType)) {
+      // Covariant return, nothing to fix.
+      return;
+    }
+
+    if (!superMethod.returnType.isSubtypeOf(method.returnType)) {
+      // Types are unrelated, so this can't be sensibly fixed.
+      _logger.severe(
+          '${node.originalName} is a subtype of ${superType.originalName} but '
+          'the return types of their ${method.originalName} methods are '
+          'unrelated');
+      return;
+    }
+
+    superMethod.returnType = method.returnType;
+    _logger.info('Changed the return type of '
+        '${superType.originalName}.${superMethod.originalName} to '
+        '${method.returnType} to match ${node.originalName}');
+
+    final (superSuperType, superSuperMethod) =
+        _findNearestWithMethod(superType, superMethod);
+    if (superSuperType != null && superSuperMethod != null) {
+      _fixContravariantReturns(node, method, superSuperType, superSuperMethod);
+    }
+  }
+
+  void _fixCoavariantArgs(ObjCInterface node, ObjCMethod method,
+      ObjCInterface superType, ObjCMethod superMethod) {
+    // In Dart, method arg types are contravariant, but ObjC allows them to be
+    // covariant. So fix these cases by adding the `covariant` keyword to the
+    // parameter.
+    final n = method.params.length;
+    if (n != superMethod.params.length) {
+      _logger.severe(
+          '${node.originalName} is a subtype of ${superType.originalName} but '
+          'their ${method.originalName} methods have a different number of '
+          'parameters');
+      return;
+    }
+
+    for (var i = 0; i < n; ++i) {
+      final pt = method.params[i].type;
+      final st = superMethod.params[i].type;
+
+      if (st.isSubtypeOf(pt)) {
+        // Contravariant param, nothing to fix.
+        continue;
+      }
+
+      if (!pt.isSubtypeOf(st)) {
+        // Types are unrelated, so this can't be sensibly fixed.
+        _logger.severe(
+            '${node.originalName} is a subtype of ${superType.originalName} '
+            'but their ${method.originalName} methods have a parameter at '
+            'position ${i + 1} with an unrelated type');
+        return;
+      }
+
+      _logger.info('Set the parameter of '
+          '${node.originalName}.${method.originalName} at position ${i + 1} to '
+          'be covariant');
+      method.params[i].isCovariant = true;
+    }
+  }
+
+  void _fixMethodVariance(ObjCInterface node) {
+    for (final method in node.methods) {
+      if (method.isClassMethod) continue;
+      final (superType, superMethod) = _findNearestWithMethod(node, method);
+      if (superType != null && superMethod != null) {
+        _fixContravariantReturns(node, method, superType, superMethod);
+        _fixCoavariantArgs(node, method, superType, superMethod);
       }
     }
   }
@@ -54,6 +126,7 @@ class FixOverriddenMethodsVisitation extends Visitation {
     // up to find the root of the subtree that has this method, then we walk
     // down the subtree to change all conflicting methods to properties.
     for (final method in node.methods) {
+      if (method.isClassMethod) continue;
       final (root, rootMethod) = _findRootWithMethod(node, method);
       if (method.isProperty == rootMethod.isProperty) continue;
       _convertAllSubtreeMethodsToProperties(root, rootMethod);
@@ -79,6 +152,8 @@ class FixOverriddenMethodsVisitation extends Visitation {
     final method = node.getSimilarMethod(rootMethod);
     if (method != null && method.kind == ObjCMethodKind.method) {
       method.kind = ObjCMethodKind.propertyGetter;
+      _logger.info(
+          'Converted ${node.originalName}.${method.originalName} to a getter');
     }
     for (final t in node.subtypes) {
       _convertAllSubtreeMethodsToProperties(t, rootMethod);