Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ObjC: Unnecessarily abstract types generated for some typedefs #235

Closed
brianquinlan opened this issue May 25, 2022 · 10 comments · Fixed by dart-archive/ffigen#625
Closed
Assignees
Labels
lang-objective_c Related to Objective C support need-for-http package:ffigen type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@brianquinlan
Copy link
Contributor

For example, this Objective-C code:

typedef NSString *NSErrorDomain;

...
@interface NSError : NSObject <NSCopying, NSSecureCoding>
...
@property (readonly, copy) NSErrorDomain domain;
...
@end

Generates this code:

typedef dispatch_queue_t1 = ffi.Pointer<ObjCObject>;
typedef NSErrorDomain = ffi.Pointer<ObjCObject>;
typedef NSErrorUserInfoKey = ffi.Pointer<ObjCObject>;

class NSError extends NSObject {
  ...
  /// These define the error. Domains are described by names that are arbitrary strings used to differentiate groups of codes; for custom domain using reverse-DNS naming will help avoid conflicts. Codes are domain-specific.
  NSErrorDomain get domain {
    return _lib._objc_msgSend_1(_id, _lib._sel_domain1);
  }
  ...

A more natural typedef would be:

typedef NSErrorDomain = NSString
@brianquinlan brianquinlan added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) lang-objective_c Related to Objective C support need-for-http labels May 25, 2022
@liamappelbe
Copy link
Contributor

Looking into this, and it's a bit of a rabbit hole. I'm going to have to do a series of related PRs that fixes this, #308, #473, and #496. All these problems boil down to the difference between the Dart type passed through FFI (Pointer<ObjCObject>) and the wrapper object (NSString).

As an example of why it's more complicated than just changing the typedef:

typedef NSString *NSErrorDomain;

struct SomeStruct {
  NSErrorDomain error_domain;
};

Would generate this Dart:

// This is the typedef we want.
typedef NSErrorDomain = NSString;

class SomeStruct extends Struct {
  // But that typedef is invalid here, because we're trying to use
  // the Dart wrapper object as a struct field.
  external NSErrorDomain error_domain;
}

Instead we have to generate getters and setters:

typedef NSErrorDomain = NSString;

class SomeStruct extends Struct {
  // Need a private field with the correct type.
  external Pointer<ObjCObject> _error_domain;

  // And getters and setters that handle conversion. Due to the
  // lib arg, the getter can't be a real getter. Probably should
  // make the setter not real too for symmetry.
  NSErrorDomain get_error_domain(NativeLibrary lib) =>
      NSErrorDomain._(_error_domain, lib, retain: true, release: true);
  void set_error_domain(NSErrorDomain value) => _error_domain = value._id;
}

Currently ffigen's Type class has a getDartType and a getCType method. getCType is the ffi native type used in function signatures (eg Int64 or Pointer<ObjCObject>). getDartType is both the Dart type used in ffi function signatures (eg int or Pointer<ObjCObject>), and the user visible type we pass out of the generated API (eg int or NSString).

The fact that these 2 different concepts are represented by the same getDartType method never mattered before, because in C bindings these types are always the same, and for ObjC bindings I have some special casing in objc_interface.dart that handles the mapping. But that special casing is only for methods on ObjC objects, so it misses top level functions, ObjC blocks, struct fields, and typedefs.

So the way to fix this is to split these 2 concepts. As well as getDartType and getCType, we need getUserType. Once we've made that split, we can migrate each of those cases to use getUserType anywhere they're using getDartType to refer to a type that the user sees. That migration should be incremental to keep the PR size reasonable.

@dcharkes
Copy link
Collaborator

dcharkes commented Sep 21, 2023

Thanks for the write-up @liamappelbe!

It makes total sense to me that we need two Dart types. In JNIgen this has been the case from the very start. A Dart class representing a Java class which has a Pointer to a handle inside.

Rather than bolting it on, lets make it a more central concept. I've left some of my ideas on how to do that on the PR.

@liamappelbe
Copy link
Contributor

@dcharkes @mannprerak2 What's Typealias._useDartType for? Shouldn't we always use the dart type when generating the typealias?

@dcharkes
Copy link
Collaborator

    // Get function name with first letter in upper case.
    final upperCaseName = name[0].toUpperCase() + name.substring(1);
    if (exposeFunctionTypedefs) {
      _exposedCFunctionTypealias = Typealias(
        name: 'Native$upperCaseName',
        type: functionType,
        isInternal: true,
      );
      _exposedDartFunctionTypealias = Typealias(
        name: 'Dart$upperCaseName',
        type: functionType,
        useDartType: true,
        isInternal: true,
      );

It's a bool that determines whether we generate the C or Dart type.
I don't know where the C types are used.
But as you can see it has a different name.

@liamappelbe
Copy link
Contributor

For function types I get it. We want one version that's the Dart function, and one that's the C function. But then there's also the constructor in typedefdecl_parser.dart. It seems that always uses the C type, but I think it should probably always use the Dart type instead.

@dcharkes
Copy link
Collaborator

It would maybe be cleaner to make it an enum now that we have three types (and possibly even make it required).

If I try the change that you suggest we actually get dart types inside NativeFunction, so that can't be right.

              typedef Typedef1 = ffi.Pointer<ffi.NativeFunction<Typedef1_function>>;
              typedef Typedef1_function = void Function(Object);

@dcharkes
Copy link
Collaborator

Side note the doc comment on the param useDartType should be on the constructor, not on the param.

@mannprerak2
Copy link
Contributor

As far as I can remember, TypeAlias would only generate C type definitions.
But I added the useDartType flag for reusing this in the _exposedDartFunctionTypealias as pointed by @dcharkes

@liamappelbe
Copy link
Contributor

Ok, it sounds like this bug is working as intended then. Typedefs always define the C type, and the C type of NSString is Pointer<ObjCObject>. Maybe instead we need to define a second typedef in this case, like we do for _exposedDartFunctionTypealias.

Specifically, if we're typedeffing a type that has a different user facing type (getDartType) to its C type (getCType) then we can define a second typedef for the Dart type, called foo_dart. Or we could do a breaking change to rename the C type to foo_native, and define foo using the Dart type?

@dcharkes
Copy link
Collaborator

My understanding is that these C typedefs are useful when dealing with callbacks with just function pointers.

Lets add a foo_dart for now, and revisit the naming later if need be. (We also chose to not break naming on the last PR.)

liamappelbe referenced this issue in dart-archive/ffigen Sep 29, 2023
liamappelbe referenced this issue in dart-archive/ffigen Oct 11, 2023
* WIP fix #386

* Fix tests

* fmt

* Update test expectations

* Update test expectations

* Fix analysis

* fmt

* Fix more tests

* WIP refactor

* Refactor `sameFfiDartAndCType` to not require a `Writer`

* Fix tests

* Fix analysis

* Update test expectations

* Format

* Fix typedef_test.dart

* Fix nits

* Fix tests

* Fmt

* Regenerate more test files

* Regen more tests

* Remove accidental commit
@liamappelbe liamappelbe transferred this issue from dart-archive/ffigen Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-objective_c Related to Objective C support need-for-http package:ffigen type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants