Skip to content
This repository has been archived by the owner on Jan 28, 2024. It is now read-only.

Separate getDartType and getUserType #623

Merged
merged 5 commits into from
Sep 24, 2023
Merged

Separate getDartType and getUserType #623

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
51e4bc7
Separate getDartType and getUserType
liamappelbe Sep 21, 2023
876fa36
Fix analysis
liamappelbe Sep 21, 2023
0dc2797
Document instancetype special casing
liamappelbe Sep 22, 2023
724085d
s/getDartType/getFfiDartType
liamappelbe Sep 22, 2023
276f30c
s/getUserType/getDartType
liamappelbe Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/src/code_generator/native_type.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ class NativeType extends Type {
}

class BooleanType extends NativeType {
// Booleans are treated as uint8.
const BooleanType._() : super._('Bool', 'bool', 'false');
static const _boolean = BooleanType._();
factory BooleanType() => _boolean;
Expand Down
3 changes: 3 additions & 0 deletions lib/src/code_generator/objc_block.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,9 @@ class $name extends _ObjCBlockBase {
String getCType(Writer w) =>
PointerType(builtInFunctions.blockStruct).getCType(w);

@override
String getUserType(Writer w) => name;

@override
String toString() => '($returnType (^)(${argTypes.join(', ')}))';
}
32 changes: 11 additions & 21 deletions lib/src/code_generator/objc_interface.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import 'package:ffigen/src/code_generator.dart';
import 'package:logging/logging.dart';

import '../strings.dart' as strings;
import 'binding_string.dart';
import 'utils.dart';
import 'writer.dart';
Expand Down Expand Up @@ -281,7 +280,7 @@ class $name extends ${superType?.name ?? '_ObjCWrapper'} {
if (m.isClass &&
!_excludedNSObjectClassMethods.contains(m.originalName)) {
addMethod(m);
} else if (_isInstanceType(m.returnType)) {
} else if (m.returnType is ObjCInstanceType) {
addMethod(m);
}
}
Expand Down Expand Up @@ -355,13 +354,8 @@ class $name extends ${superType?.name ?? '_ObjCWrapper'} {
@override
String getCType(Writer w) => PointerType(objCObjectType).getCType(w);

bool _isObject(Type type) =>
type is PointerType && type.child == objCObjectType;

bool _isInstanceType(Type type) =>
type is Typealias &&
type.originalName == strings.objcInstanceType &&
_isObject(type.type);
@override
String getUserType(Writer w) => name;

// Utils for converting between the internal types passed to native code, and
// the external types visible to the user. For example, ObjCInterfaces are
Expand All @@ -370,16 +364,12 @@ class $name extends ${superType?.name ?? '_ObjCWrapper'} {
bool _needsConverting(Type type) =>
type is ObjCInterface ||
type is ObjCBlock ||
_isObject(type) ||
_isInstanceType(type);
type is ObjCObjectPointer ||
type is ObjCInstanceType;

String _getConvertedType(Type type, Writer w, String enclosingClass) {
if (type is BooleanType) return 'bool';
if (type is ObjCInterface) return type.name;
if (type is ObjCBlock) return type.name;
if (_isObject(type)) return 'NSObject';
if (_isInstanceType(type)) return enclosingClass;
return type.getDartType(w);
if (type is ObjCInstanceType) return enclosingClass;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup!

This special case is the instancetype right? The one that returns the type of the object itself even in subclasses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

return type.getUserType(w);
}

String _getConvertedReturnType(
Expand All @@ -393,8 +383,8 @@ class $name extends ${superType?.name ?? '_ObjCWrapper'} {

String _doArgConversion(ObjCMethodParam arg) {
if (arg.type is ObjCInterface ||
_isObject(arg.type) ||
_isInstanceType(arg.type) ||
arg.type is ObjCObjectPointer ||
arg.type is ObjCInstanceType ||
arg.type is ObjCBlock) {
if (arg.isNullable) {
return '${arg.name}?._id ?? ffi.nullptr';
Expand All @@ -415,10 +405,10 @@ class $name extends ${superType?.name ?? '_ObjCWrapper'} {
if (type is ObjCBlock) {
return '$prefix${type.name}._($value, $library)';
}
if (_isObject(type)) {
if (type is ObjCObjectPointer) {
return '${prefix}NSObject._($value, $library, $ownerFlags)';
}
if (_isInstanceType(type)) {
if (type is ObjCInstanceType) {
return '$prefix$enclosingClass._($value, $library, $ownerFlags)';
}
return prefix + value;
Expand Down
25 changes: 22 additions & 3 deletions lib/src/code_generator/pointer.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,15 @@ import 'writer.dart';
/// Represents a pointer.
class PointerType extends Type {
final Type child;
PointerType(this.child);

PointerType._(this.child);

factory PointerType(Type child) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (child == objCObjectType) {
return ObjCObjectPointer();
}
return PointerType._(child);
}

@override
void addDependencies(Set<Binding> dependencies) {
Expand All @@ -33,7 +41,7 @@ class PointerType extends Type {
/// Represents a constant array, which has a fixed size.
class ConstantArray extends PointerType {
final int length;
ConstantArray(this.length, Type child) : super(child);
ConstantArray(this.length, Type child) : super._(child);

@override
Type get baseArrayType => child.baseArrayType;
Expand All @@ -50,7 +58,7 @@ class ConstantArray extends PointerType {

/// Represents an incomplete array, which has an unknown size.
class IncompleteArray extends PointerType {
IncompleteArray(Type child) : super(child);
IncompleteArray(Type child) : super._(child);

@override
Type get baseArrayType => child.baseArrayType;
Expand All @@ -61,3 +69,14 @@ class IncompleteArray extends PointerType {
@override
String cacheKey() => '${child.cacheKey()}[]';
}

/// A pointer to an NSObject.
class ObjCObjectPointer extends PointerType {
factory ObjCObjectPointer() => _inst;

static final _inst = ObjCObjectPointer._();
ObjCObjectPointer._() : super._(objCObjectType);

@override
String getUserType(Writer w) => 'NSObject';
}
12 changes: 10 additions & 2 deletions lib/src/code_generator/type.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,15 @@ abstract class Type {
/// passed to native code.
String getCType(Writer w) => throw 'No mapping for type: $this';

/// Returns the Dart type of the Type. This is the user visible type that is
/// passed to Dart code.
/// Returns the Dart type of the Type. This is the type that is passed from
/// FFI to Dart code.
String getDartType(Writer w) => getCType(w);
liamappelbe marked this conversation as resolved.
Show resolved Hide resolved

/// Returns the user type of the Type. This is the type that is presented to
/// users by the ffigened API to users. For C bindings this is always the same
/// as getDartType. For ObjC bindings this refers to the wrapper object.
String getUserType(Writer w) => getDartType(w);

/// Returns the string representation of the Type, for debugging purposes
/// only. This string should not be printed as generated code.
@override
Expand Down Expand Up @@ -99,6 +104,9 @@ abstract class BindingType extends NoLookUpBinding implements Type {
@override
String getDartType(Writer w) => getCType(w);

@override
String getUserType(Writer w) => getDartType(w);

@override
String toString() => originalName;

Expand Down
40 changes: 40 additions & 0 deletions lib/src/code_generator/typealias.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'package:ffigen/src/code_generator.dart';

import '../strings.dart' as strings;
import 'binding_string.dart';
import 'utils.dart';
import 'writer.dart';
Expand Down Expand Up @@ -40,6 +41,18 @@ class Typealias extends BindingType {
isInternal: isInternal,
)));
}
if ((originalName ?? name) == strings.objcInstanceType &&
type is ObjCObjectPointer) {
return ObjCInstanceType._(
usr: usr,
originalName: originalName,
dartDoc: dartDoc,
name: name,
type: type,
useDartType: useDartType,
isInternal: isInternal,
);
}
return Typealias._(
usr: usr,
originalName: originalName,
Expand Down Expand Up @@ -122,3 +135,30 @@ class Typealias extends BindingType {
String? getDefaultValue(Writer w, String nativeLib) =>
type.getDefaultValue(w, nativeLib);
}

/// Objective C's instancetype.
///
/// This is an alias for an NSObject* that is special cased in code generation.
liamappelbe marked this conversation as resolved.
Show resolved Hide resolved
class ObjCInstanceType extends Typealias {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Not with the existing plumbing.

ObjCInstanceType._({
String? usr,
String? originalName,
String? dartDoc,
required String name,
required Type type,

/// If true, the binding string uses Dart type instead of C type.
///
/// E.g if C type is ffi.Void func(ffi.Int32), Dart type is void func(int).
bool useDartType = false,
bool isInternal = false,
}) : super._(
usr: usr,
originalName: originalName,
dartDoc: dartDoc,
name: name,
type: type,
useDartType: useDartType,
isInternal: isInternal,
);
}