Separate getDartType and getUserType #623
Conversation
liamappelbe
changed the title
There was a problem hiding this comment.
https://github.com/dart-lang/ffigen/issues/386#issuecomment-1728584216
Nice write up!
Reading your write-up, I think we should make a general solution for where the user-visible-dart-type is not equal to the internally-used-with-ffi-dart-type.
Should we have a separate pass parse -> transform -> generate where in the transform step we recognize unwrapped types and wrap them? The current PR special cases the Pointer Type constructor, but that seems not the right place for the decision point.
cc @mannprerak2 What do you think about this?
That migration should be incremental to keep the PR size reasonable.
I'd prefer not having the repo in a half-migrated state. I'm happy with incremental PRs, but then please commit to doing the follow up PRs soon. (I want to avoid having a half-migrated repo and then contributors (including ourselves) having to deal with that technical debt.) Having a full migration PR is preferable imo.
|
|
||
| PointerType._(this.child); | ||
|
|
||
| factory PointerType(Type child) { |
There was a problem hiding this comment.
Should we maybe have something more general to recognize "internal-dart-types" and return the "user-visible-dart-types"?
For example, if we have something more general we could also recognize Pointer<Char> and make it available as String in signatures?
It should probably be a separate pass. Rather than making a choice point in factory constructors of types.
// Parse the bindings according to config object provided.
final library = parse(config);
final libraryTransformed = transform(library);
// Generate file for the parsed bindings.
final gen = File(config.output);
libraryTransformed.generateFile(gen);Or maybe because we already make choices in parse based on the config, it should a Type wrapDartType(Type type) function being invoked on any type. And for now it only replaces Pointer with ObjCObjectPointer.
There was a problem hiding this comment.
At the moment we can handle all these cases, including your Pointer<Char> idea, using this factory pattern. The type I'm returning is still a Pointer, it's just a more specialized version of it. That's one of the main use cases of a factory constructor, so I think it's appropriate to make the decision here. I don't see the point of adding a entire transformer infrastructure if we don't actually need it. We can add something like that later if we find a case that isn't cleanly handled by this pattern.
| if (_isObject(type)) return 'NSObject'; | ||
| if (_isInstanceType(type)) return enclosingClass; | ||
| return type.getDartType(w); | ||
| if (type is ObjCInstanceType) return enclosingClass; |
There was a problem hiding this comment.
Nice cleanup!
This special case is the instancetype right? The one that returns the type of the object itself even in subclasses.
| /// Objective C's instancetype. | ||
| /// | ||
| /// This is an alias for an NSObject* that is special cased in code generation. | ||
| class ObjCInstanceType extends Typealias { |
There was a problem hiding this comment.
Would it be possible to populate the enclosing class here? So that the code generator doesn't make the decision but we do it here?
There was a problem hiding this comment.
I don't think so. Not with the existing plumbing.
Well, the migration isn't really incremental. This PR fully migrates all existing bits of the repo that made this distinction (only objc_interface.dart). The incremental part is applying that distinction to the rest of the code gen. Each time I change one of those other modules to distinguish |
There was a problem hiding this comment.
lgtm!
I think we should consider refactoring this so that it becomes more of a pipeline with individual steps being responsible for individual aspects. Now it starts to be a spaghetti with lots of responsibilities both in the parse and in the generate step. However, we should not gate this CL on that.
|
Thanks @liamappelbe ! 🚀 |
A bunch of ObjC polish bugs are related to the fact that we don't draw a distinction between the 2 different uses of
Type.getDartType:Type.getCTypemaps to.For C bindings these 2 types are always the same. But for ObjC types they can be different. For example, an
NSString*has a Dart type ofPointer<ObjCObject>and a user type of the Dart wrapper classNSString. I've been distinguishing them in objc_interface.dart with some hard coded special cases for class methods, but this misses top level functions, ObjC blocks, struct fields, and typedefs.More info: dart-lang/native#235
So we need to separate these 2 uses by defining a new
Type.getUserTypemethod. Over time I'll migrate the rest of ffigen to make this distinction (ie find everywhereType.getDartTypeis called and switch the ones that are user facing toType.getUserType), and that will fix all these bugs. But it'll be complicated because we have to do conversions between the Dart type and the user type. So to keep things simple I've just migrated objc_interface.dart for now.To facilitate this I also had to define new specializations of some
Types:ObjCObject*was an ordinaryPointerbut is nowObjCObjectPointerinstancetypewas aTypealiasbut is nowObjCInstanceTypeRelated bugs: dart-lang/native#308 dart-lang/native#235 dart-lang/native#473 dart-lang/native#496