dart-lang / native

Dart packages related to FFI and native assets bundling.
BSD 3-Clause "New" or "Revised" License
83 stars 27 forks source link

Proposal: Use "TypeClasses" to instantiate objects / call static methods #723

Closed HosseinYousefi closed 1 year ago

HosseinYousefi commented 1 year ago

Current state of things

Assuming that dart-lang/jnigen#118 gets merged, arrays are instantiated this way (See https://github.com/dart-lang/native/issues/720):

JniIntTypeClass().newArray(3);

jnigen generated classes are instantiated this way:

JniGeneratedClass(arg1, arg2, ...);
JniGeneratedClass.ctor1(arg1, arg2, ...);
// and more ctorNs...

Instantiating an object from a class that is not generated by jnigen is done this way:

// This always returns JniObject, even if we want to create our own class
Jni.newInstance("java/util/Random", "()V", []);

These feel inconsistent.

Using TypeClasses for everything!

Consistency

Creating arrays would be similar:

JniArrayTypeClass(JniIntTypeClass()).newInstance(3);
// Maybe still having this as syntax sugar:
JniIntTypeClass().newArray(3);

jnigen generated classes will be instantiated this way:

JniGeneratedTypeClass().newInstance(arg1, arg2, ...);
JniGeneratedTypeClass().newInstance1(arg1, arg2, ...);
// and so on...

For the classes that have not been generated, we could create a typeclass:

class RandomTypeClass extends JniTypeClass<JniObject> {
  @override
  String get signature => "Ljava/util/Random;";

  JniObject newInstance() {
    // ...
  }
}

Creating custom classes that were not generated

The nice thing is that we could customize the class and use some other class instead of JniObject:

class Random extends JniObject {
  Random.fromRef(super.reference) : super.fromRef();

  int nextInt() {
    // ...
  }
}

class RandomTypeClass extends JniTypeClass<Random> {
  @override
  String get signature => "Ljava/util/Random;";

  Random newInstance() {
    // ...
  }
}

Generics

We could also support generics (See dart-lang/native#759):

class MyMapTypeClass<K, V> extends JniTypeClass<MyMap<K, V>> {
  final JniTypeClass<K> keyTypeClass;
  final JniTypeClass<V> valueTypeClass;

  MyMapTypeClass(this.keyTypeClass, this.valueTypeClass);

  @override
  String get signature => "...";

  MyMap<K, V> newInstance() {
    // ...
  }
}

So we can do

// The type of m is inferred to be MyMap<JniString, JniString>
final m = MyMapTypeClass(JniStringTypeClass(), JniStringTypeClass()).newInstance();

Static methods

Static methods could also be called from the instances of TypeClasses instead of the classes themselves. This is very similar to the object creation.

Passing context to support multiple JVMs and more

All this also allows more context to be added to the object creation, for example the JVM on which this object should be created on (See dart-lang/native#722).

Some ways to do this:

Drawbacks?

The main drawback is that the syntax becomes less similar to Dart object creation. As SomeClass(...) becomes SomeTypeClass.newInstance(...).

dcharkes commented 1 year ago

Thanks for the proposal @HosseinYousefi!

There's definitely an interaction between arrays and generics going on here.

I'd like to center the API as most around a single class as possible, and only expose a type class if absolutely necessary.

JniGeneratedTypeClass().newInstance(arg1, arg2, ...);
JniGeneratedTypeClass().newInstance1(arg1, arg2, ...);
// and so on...

Assuming that these belong to MyFoo that is a class that we generate. I'd prefer

> MyFoo.newInstance(arg1, arg2, ...);
> MyFoo.newInstance1(arg1, arg2, ...);
> // and so on...

With JNI's array API we need to pass something in that can produce the right Java signature string. With the current Dart feature set, that means an object (since type arguments won't cut it) Since we have an object representing the type, we need a class for that object and introduce the extra class.

However, I'd like the API to be structured making that class as invisible as possible:

JniArrayTypeClass(MyFoo.type).newInstance(3);

The .type static method on all generate classes returns the object representing the type.

With generics, the type needs to be a method that takes parameters for the nested types.

JniArrayTypeClass(MyBar.type(MyKey.type,MyValue.type)).newInstance(3);

I'd maybe even prefer:

JniArray.newInstance(
  JniArray.type(
    MyBar.type(MyKey.type, MyValue.type),
  ),
  3,
);

Actually, maybe we don't even need the runtime String representation of the type arguments, because those are erased right? But having some type arguments be inferred and others not is also slightly ugly:

JniArray<MyBar<MyKey, MyValue>> myArray = JniArray.newInstance(
  JniArray.type(
    MyBar.typeErased,
  ),
  3,
);

Then we might as well write out the first option.

Do we need the MyBar? Or do object arrays have no reified runtime type arg as well? If we don't even need MyBar.type:

JniArray<MyBar<MyKey, MyValue>> myArray = JniArray.newInstance(
  3,
  // `type` should then be a named optional argument
);

We would only see this stuff on on the arrays, because there we have an API in which we need to pass the type strings.

For instantiating an object with generics, we only need to instantiate Dart type arguments. Java has no runtime reification of the type arguments, so we don't have to pass in the class-strings:

MyBar<MyKey, MyValue> myBar = MyBar.newInstance(arg1, arg2, ...)

I haven't looked at the actual JNI API we're mapping to while writing this reply. So maybe there are more constraints why what I'm proposing wouldn't work.

edit: with my request for .type I'm preferring Slava's design from https://github.com/dart-lang/native/issues/720 over putting emphasis on the 'type class'. Maybe .klass is better than .type. We should look in all contexts where this is used.

HosseinYousefi commented 1 year ago

I'd like to center the API as most around a single class as possible, and only expose a type class if absolutely necessary.

I understand that you want to keep the API simpler, but I feel like we're writing .type (or any other one like .klass) here many times.

What about only exposing the type class and naming it as we currently name the normal classes without the TypeClass suffix?

// MyFoo is actually the TypeClass and newInstance returns MyFooObject
MyFoo().newInstance(...);
MyFoo().invokeStaticMethod(...);
final array1 = JniArrayTypeClass(MyBar.type(MyKey.type,MyValue.type)).newInstance(3);

// turns into:

final array2 = JniArray(MyBar(MyKey(), MyValue())).newInstance(3);

I'm not sure how the tree-shaking works for consts but we could also create const instances of these TypeClasses for the ones that don't require any arguments or if we want to simply have the type erased.

// In the generated file:
const MyKey = MyKeyTypeClass();
const MyValue = MyValueTypeClass();

// Somewhere else:
final foo = MyFoo(MyKey, MyValue).newInstance(...);
dcharkes commented 1 year ago

MyFoo is actually the TypeClass and newInstance returns MyFooObject

Does that mean that if you were to write a Flutter app, where Flutter lints force you to spell out the type your code would look like:

final MyFooObject myFoo = MyFoo().newInstance(...);

I think having two types would make it rather confusing. Using .klass or .type would feel much more natural because users write a single Dart type (twice).

final MyFoo myFoo = MyFoo.klass.newInstance(...);

Same with generics and arrays, they would be forced to be:

final JArray<MyFooObject> jArray = ...

instead of

final JArray<MyFoo> jArray = ...
HosseinYousefi commented 1 year ago

Hmm, what about combining the two classes? We're using _ensureNotDeleted(); before calling every method of JniObject. So not all JniObjects are non-deleted, valid ones, with existing references. Maybe MyFoo() could simply mean that we create a MyFoo with a nullptr reference. then we can have:

// Assuming that build returns `void`, could also return `this` I guess

final MyFoo myFoo = MyFoo()..build(...);
final MyFoo myFoo1 = MyFoo()..build1(...);

final bar = MyBar(MyKey(), MyValue())..build(...);

This assigns some non-null value to the reference pointer.

dcharkes commented 1 year ago

That thought also crossed my mind, that would make the type situation much simpler indeed.

But using the normal constructors for these "special" objects pollutes the constructor space. Especially, if a lot of code might not deal with generics or arrays. I'd prefer having constructors over build methods for object instantiation because it's only the "special" objects we use for passing types. And since we don't have overloading in Dart, we can't just have unnamed constructors for both normal allocation and the "special" type objects. And adding a named constructor causes us to write similar things to having a static getter/method on the class (but constructors cannot have no parenthesis):

// I'd prefer keeping the non-generic use case as simple as possible.
final MyFoo myFoo = MyFoo(...)
final MyFoo myFoo1 = MyFoo(...);

// Using a named constructor for the special objects has the same effect as a static getter/method
final bar = MyBar(MyKey.klass(), MyValue.klass());

Assuming that build returns void, could also return this I guess

Yeah other programming languages do that often, we don't because we have .. operator.

HosseinYousefi commented 1 year ago

I don't like this because

edit:

dcharkes commented 1 year ago

Thinking about it, the trick to infer the type arguments from the type object, only works if we use the "special" object.

final myList = MyList</*MyFoo inferred*/>.newInstance(MyFoo(), 50);

edit: actually that trick can still work with two classes:

final myList = MyList</*MyFoo inferred*/>.newInstance(/*extends jni.JniTypeClass<MyFoo>*/ MyFoo.klass, 50);
dcharkes commented 1 year ago

The API is inconsistent. I might try to do SomeClass( and wait for IDE suggestions and not see what I expect depending on the "specialness" of the class.

Yeah, I don't like that about the "special" objects of a single class either.

We could have two classes, one special and one non-special that have the same looking constructor with very different meaning:

That is very bad indeed.

I believe your snippet should be:

// java world: MyBar<A, B>
final bar = MyBar(A.klass(), B.klass()); // bar is now null
// java world: MyFoo that has a constructor that gets A and B objects
final foo = MyFoo(A(), B()); // foo is a non-null object that has two objects passed to its constructor

Indeed, far from desirable.

Let me do some prototyping in an editor and let's discuss all the options offline and write some notes here afterwards.

HosseinYousefi commented 1 year ago

I believe your snippet should be:

// java world: MyBar<A, B>
final bar = MyBar(A.klass(), B.klass()); // bar is now null
// java world: MyFoo that has a constructor that gets A and B objects
final foo = MyFoo(A(), B()); // foo is a non-null object that has two objects passed to its constructor

No, I actually meant the original snippet. We're not constructing A and B, we're passing nullptrs as A and B using A.klass() and B.klass() (since they create an object with a null reference as we're using the same type for both TypeClasses and Classes)

dcharkes commented 1 year ago

Ah, that's even more confusing indeed.

HosseinYousefi commented 1 year ago

Another important thing to consider in our design is supporting subtyping (I believe you already created the issue dart-lang/native#764).

Currently this happens:

// java world: Foo extends Bar
void f(Bar bar) {}
final foo = Foo();
f(foo); // Error! Expected Bar instead of Foo
dcharkes commented 1 year ago

Gist from discussion:

Design for in the background: https://gist.github.com/dcharkes/31351012ff4c67884bd406235ef9c571

mahesh-hegde commented 1 year ago

I skimmed over previous discussion. So correct me if I got something wrong.


Alternatively, you can keep the current design largely, with the benefit of class constructors and static calls being similar to Java, and incrementally expose the klass part. That's mostly how Java does it.

In Java We use generic type parameter whenever possible. But some APIs need more context which can't be inferred from T, because no concrete object of T is passed. Then we pass T.class special object.

Another thing to consider is static methods, of which there can be many. My opinion is static methods and constructors should be same in interface they provide - although they are coded differently. In Java we don't have factory methods as language feature, so I use static methods as named constructors very often.

There should not be a special status for constructor than static method in the interface we export to user.

The entry points to JNI in generated code are: constructors, array constructors and static methods. So if you want to pass any JVM context, allocator or reference pool, you should have near-uniform API for all of these.

I think having any methods on klass object complicates it. Let's do it the Java way (TM).

var documents = JniArray(PDDocument.klass)
var documentList = List.of2(PDDocument.klass, myDocument, myOtherDocument);

mahesh-hegde commented 1 year ago

// Error! Expected Bar instead of Foo

Does this happen even if both Bar and Foo are in same jnigen config and Foo extends Bar? This should not happen. I implemented some hash map based renaming also, to avoid subtle but frequent override conflicts.

HosseinYousefi commented 1 year ago

// Error! Expected Bar instead of Foo

Does this happen even if both Bar and Foo are in same jnigen config and Foo extends Bar? This should not happen. I implemented some hash map based renaming also, to avoid subtle but frequent override conflicts.

Yes, as far as I remember, when I tried this, I put both Foo and Bar in the same jnigen config.

HosseinYousefi commented 1 year ago

Let’s consider the following Java Class:

public class MyStack<T> {
  private Stack<T> stack;

  public MyStack() {
    stack = new Stack<>();
  }

  public void push(T item) {
    stack.push(item);
  }

  public T pop() {
    return stack.pop();
  }
}

Currently this generates the following Dart class without any of the methods:

class MyStack<T extends jni.JObject> extends jni.JObject {
  MyStack.fromRef(ffi.Pointer<ffi.Void> ref) : super.fromRef(ref);

  /// The type which includes information such as the signature of this class.
  static jni.JObjType<MyStack<T>> type<T extends jni.JObject>(
    jni.JObjType<T> $T,
  ) {
    return _$MyStackType(
      $T,
    );
  }

  static final _ctor =
      jniLookup<ffi.NativeFunction<jni.JniResult Function()>>("MyStack__ctor")
          .asFunction<jni.JniResult Function()>();

  /// from: public void <init>()
  MyStack() : super.fromRef(_ctor().object);
}

class _$MyStackType<T extends jni.JObject> extends jni.JObjType<MyStack<T>> {
  final jni.JObjType<T> $T;

  const _$MyStackType(
    this.$T,
  );

  @override
  String get signature => r"Lcom/github/dart_lang/jnigen/generics/MyStack;";

  @override
  MyStack<T> fromRef(jni.JObjectPtr ref) => MyStack.fromRef(ref);
}

extension $MyStackArray<T extends jni.JObject> on jni.JArray<MyStack<T>> {
  MyStack<T> operator [](int index) {
    return MyStack.fromRef(elementAt(index, jni.JniCallType.objectType).object);
  }

  void operator []=(int index, MyStack<T> value) {
    (this as jni.JArray<jni.JObject>)[index] = value;
  }
}

Let’s try adding the pop method, for simplicity assume _popFromC() returns the reference to the returned value from C and we just need to put it inside the correct T type. As usual we can’t do T.fromRef directly here as T is a type, so we must pass in the JObjType<T> to the class:

final jni.JObjType<T> $T;

/// from: public void <init>()
MyStack(this.$T) : super.fromRef(_ctor().object);

Now MyStack.fromRef would also need to populate $T, maybe like so:

MyStack.fromRef(this.$T, ffi.Pointer<ffi.Void> ref) : super.fromRef(ref);

And with that, this will work:

T pop() {
  return $T.fromRef(_popFromC());
}

But now the arrays will have a problem with their operator []. The following code won't work, as we need to also pass in $T.

MyStack<T> operator [](int index) {
  return MyStack.fromRef(elementAt(index, jni.JniCallType.objectType).object);
}

So now we need to know the elementType inside the array, let's call it $E. And then get the inner $T from that.

MyStack<T> operator [](int index) {
  return MyStack.fromRef(($E as _$MyStackType<T>).$T,
      elementAt(index, jni.JniCallType.objectType).object);
}

Now for some getArr static method that returns an int array, we need to update its code to add the required type:

static jni.JArray<jni.JInt> getArr() =>
    jni.JArray<jni.JInt>.fromRef(jni.JInt.type, _getArr().object);

JArray.filled will no longer work as now $E is required.

Although this code is generated, we have a lot of duplicated logic:

Moving to a solution where class act as the both the class and the type, for example by throwing if we're calling an instance method on an "unconstructed" object, would make both the codegen and the generated code much cleaner. As we don't have to have a copy of every type param in both places.

// using .call() instead of ..build() to show another option
final myStack = MyStack(T())();
dcharkes commented 1 year ago

As we don't have to have a copy of every type param in both places.

That would indeed only work if we conflate the two concepts into a single class. If we don't conflate them but only make the Type objects more central, we would still always copy the type type objects into the normal objects because methods need to access them.

final type = MyStack(T());
final object = type();
// object.push(...)
final object2 = object.pop(); // uses the saved type in in object.

So we have kind of three options then if I understand it correctly:

  1. Object hierarchy, type hierarchy - object hierarchy front and center, every object refers to a type object
  2. Object hierarchy, type hierarchy - type hierarchy front and center, every object refers to a type object
  3. conflated hierarchy - only one hierarchy, every object is also a type object, but might not be a Java object.

Am I understanding you correctly, that you're no longer advocating for option 2 (your first post), but option 3?

And we're trying to solve two related problems here I believe:

A. Having the actual information to be able to invoke the right things in Java and Dart. (We need access to type objects) B. Having an API which makes sense for users. (Do users operate mostly on objects, mostly on type objects, or a conflated concept that has an internal state diagram.)

A is a hard requirement. For B we'd need to write multiple examples to see what the code looks like. (Your post above only writes 2 lines of example, which is too not enough for a decision.) All three API designs can satisfy A.

We need to have the same fields inside the type class and also the actual case.

Shouldn't we only have a field in the type class, and forward the relevant things from the object?

fromRef also needs to exist both in the actual class and the type class.

Side note: I'd hope fromRef never occurs in user code, users should only see JObjects everywhere. And again, the one implementation can forward to the other, the JObject subclasses one should forward to type likely. Can we get rid of fromRef on the objects?

HosseinYousefi commented 1 year ago

Closing in favor of https://github.com/dart-lang/native/issues/705. Since the named argument approach allows us to have type inference.