ObjC: Unnecessarily abstract types generated for some typedefs

[brianquinlan]

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

[liamappelbe]

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]

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]

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

[dcharkes]

    // 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]

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]

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]

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

[mannprerak2]

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]

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]

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.)