Use getDartType rather than getFfiDartType in ObjC block codegen #632

liamappelbe · 2023-10-25T01:40:03Z

Improves the codegen for blocks with arguments or return types that are objects, nullables, or blocks. Users can now pass or receive the Dart wrapper object, rather than dealing with pointers.

Ref counting notes:

Arguments to a Dart block might outlive the scope of the block, so must be retained when they're wrapped
Returns from a Dart block might outlive the wrapper object, so must be retained as the raw pointer is being returned
To balance those 2 points, the call method of the Block wrapper shouldn't retain arguments it passes to the block, or the result it gets back.
Due to Delete Dart Functions associated with ObjC Blocks dart-lang/native#204, some of the ref counting tests have leaks, because a wrapper object is being captured in a lambda context that is stored in the block trampoline table.

Fixes dart-lang/native#473

liamappelbe · 2023-10-25T04:14:15Z

Marking not ready for review. I can't repro the mac CI failure locally. Need to investigate more.

liamappelbe · 2023-10-26T04:25:49Z

The failures were to do with ref counting mismatches. They're all fixed now.

dcharkes

lgtm w. comments

Thanks @liamappelbe!

(If you want to release a version, don't forget changelog entry and pubspec version bump.)

dcharkes · 2023-10-26T07:56:19Z

test/native_objc_test/block_test.m

@@ -34,6 +70,10 @@ + (NSThread*)callOnNewThread:(VoidBlock)block;
 + (float)callFloatBlock:(FloatBlock)block;
 + (double)callDoubleBlock:(DoubleBlock)block;
 + (Vec4)callVec4Block:(Vec4Block)block;
+ (DummyObject*)callObjectBlock:(ObjectBlock)block NS_RETURNS_RETAINED;
+ (DummyObject* _Nullable)callNullableBlock:(NullableBlock)block;
+ (IntBlock)newBlock:(BlockBlock)block withMult:(int)mult;


Suggested change

+ (IntBlock)newBlock:(BlockBlock)block withMult:(int)mult;

+ (nullable DummyObject*)callNullableBlock:(NullableBlock)block;

Ditto in other places.

Also, does Objective-C default to nonnull if nullable is omitted? Or is that Objective-C version specific?
It might be worth adding at least one test with nonnull to make sure we parse it correctly.

Also, does Objective-C default to nonnull if nullable is omitted?

Yep. From the parser's perspective, we don't even see the annotations. We just see the clang_Type_getNullability flag, which combines both these annotations. I've added a test case to nullable_test anyway.

dcharkes · 2023-10-26T08:01:18Z

test/native_objc_test/block_test.m

 typedef int32_t (^IntBlock)(int32_t);
 typedef float (^FloatBlock)(float);
 typedef double (^DoubleBlock)(double);
 typedef Vec4 (^Vec4Block)(Vec4);
 typedef void (^VoidBlock)();
+typedef DummyObject* (^ObjectBlock)(DummyObject*);
+typedef DummyObject* _Nullable (^NullableBlock)(DummyObject* _Nullable);


Nit: The block itself isn't nullable, but the argument and return type are. I don't have a good suggestion for a better name though. If we split the test in two blocks, one with nullable argument and one with nullable return it could be NullableReturnBlock and NullableArgumentBlock or something.

How about NullableObjectBlock?

dcharkes · 2023-10-26T09:49:49Z

test/native_objc_test/block_test.m

@@ -12,11 +12,47 @@
  double w;
 } Vec4;



nit for block_config.yaml

description: 'Tests calling Objective-C blocks.'

dcharkes · 2023-10-26T09:51:23Z

test/native_objc_test/block_test.dart

+      });
+
+      final obj = DummyObject.new1(lib);
+      final result1 = block(obj);


Add test which pass null?

dcharkes · 2023-10-26T09:52:21Z

test/native_objc_test/block_test.dart

+
+    test('Block block', () {
+      final blockBlock = BlockBlock.fromFunction(lib, (IntBlock intBlock) {
+        return IntBlock.fromFunction(lib, (int x) {


dcharkes · 2023-10-26T10:02:54Z

lib/src/code_generator/type.dart

-  /// only. This string should not be printed as generated code.
+  /// Returns generated Dart code that converts the given value from its
+  /// DartType to its FfiDartType.
+  String convertDartTypeToFfiDartType(


Please document the params so that if someone not familiar with objC introduces a new implementation of Type doesn't have to reverse engineer.

dcharkes · 2023-10-26T10:03:11Z

lib/src/code_generator/type.dart

+
+  /// Returns generated Dart code that converts the given value from its
+  /// FfiDartType to its DartType.
+  String convertFfiDartTypeToDartType(


Ditto, please document the arguments.

dcharkes · 2023-10-26T10:05:00Z

lib/src/code_generator/type.dart

+  String convertDartTypeToFfiDartType(
+    Writer w,
+    String value, {
+    bool objCShouldRetain = false,


objCRetain (It's not a should, it's guaranteed, we generate a retain: true.)

Also, let's make it non-optional. I think it would be useful that all call sites explicitly reason about memory management in the generated code.

dcharkes · 2023-10-26T10:07:30Z

lib/src/code_generator/type.dart

+    Writer w,
+    String value,
+    String library, {
+    bool objCShouldRetain = true,


Ditto, objCRetain and required arg.

dcharkes · 2023-10-26T10:10:33Z

lib/src/code_generator/typealias.dart

+    String? objCEnclosingClass,
+  }) =>
+      ObjCInterface.generateConstructor(
+          objCEnclosingClass ?? 'NSObject', value, library, objCShouldRetain);


Is NSObject ever happening?
If so, is it a sign we are not generating a class because it's not part of the ffigen.yaml?

If it can happen:

Please add a test case that exercises this.

Leave a comment here to that test case.

Emit a warning.

If it cannot (or should not) happen:

Add an assert or exception.

Shouldn't ever happen (clang should give an error). Added a null assertion instead.

liamappelbe added 4 commits October 24, 2023 14:14

Blocks returning proper user facing types

700d539

More tests and refactors

06d0b6e

fmt

2725cc7

Fix test

be9e841

liamappelbe requested a review from dcharkes October 25, 2023 02:20

liamappelbe marked this pull request as ready for review October 25, 2023 02:20

Fix vararg test

6637967

liamappelbe marked this pull request as draft October 25, 2023 04:13

liamappelbe changed the title ~~Use getDartType rather than getFfiDartType in ObjC block codegen~~ WIP: Use getDartType rather than getFfiDartType in ObjC block codegen Oct 25, 2023

liamappelbe removed the request for review from dcharkes October 25, 2023 04:13

liamappelbe added 3 commits October 26, 2023 15:52

Partial fix for block ref counts

2f41968

More block ref counting fixes

7f1dd06

fmt

214b82b

liamappelbe changed the title ~~WIP: Use getDartType rather than getFfiDartType in ObjC block codegen~~ Use getDartType rather than getFfiDartType in ObjC block codegen Oct 26, 2023

liamappelbe requested a review from dcharkes October 26, 2023 04:25

liamappelbe marked this pull request as ready for review October 26, 2023 04:25

dcharkes approved these changes Oct 26, 2023

View reviewed changes

Daco's comments

8225705

liamappelbe merged commit ba94da9 into main Oct 27, 2023
6 checks passed

liamappelbe deleted the blockusertype branch October 27, 2023 00:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use getDartType rather than getFfiDartType in ObjC block codegen #632

Use getDartType rather than getFfiDartType in ObjC block codegen #632

liamappelbe commented Oct 25, 2023 •

edited

Loading

liamappelbe commented Oct 25, 2023

liamappelbe commented Oct 26, 2023

dcharkes left a comment

dcharkes Oct 26, 2023

liamappelbe Oct 26, 2023

dcharkes Oct 26, 2023

liamappelbe Oct 26, 2023

dcharkes Oct 26, 2023

liamappelbe Oct 26, 2023

dcharkes Oct 26, 2023

liamappelbe Oct 26, 2023

dcharkes Oct 26, 2023

dcharkes Oct 26, 2023

liamappelbe Oct 26, 2023

dcharkes Oct 26, 2023

liamappelbe Oct 26, 2023

dcharkes Oct 26, 2023

liamappelbe Oct 26, 2023

dcharkes Oct 26, 2023

liamappelbe Oct 26, 2023

dcharkes Oct 26, 2023

liamappelbe Oct 26, 2023

	+ (IntBlock)newBlock:(BlockBlock)block withMult:(int)mult;
	+ (nullable DummyObject*)callNullableBlock:(NullableBlock)block;

Use getDartType rather than getFfiDartType in ObjC block codegen #632

Use getDartType rather than getFfiDartType in ObjC block codegen #632

Conversation

liamappelbe commented Oct 25, 2023 • edited Loading

liamappelbe commented Oct 25, 2023

liamappelbe commented Oct 26, 2023

dcharkes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamappelbe commented Oct 25, 2023 •

edited

Loading