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

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

Closed
HosseinYousefi opened this issue Nov 7, 2022 · 18 comments

Comments

@HosseinYousefi
Copy link
Member

HosseinYousefi commented Nov 7, 2022

Current state of things

Assuming that dart-archive/jnigen#118 gets merged, arrays are instantiated this way (See #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 #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 #722).

Some ways to do this:

  • Add context to the TypeClass as a parameter (maybe defaulted to the singleton context)
  • Get context as the first argument of the newInstance methods
  • newInstance methods returning a function (Obj Function(JniContext)) instead of the object directly
  • ...

Drawbacks?

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

@dcharkes
Copy link
Collaborator

dcharkes commented Nov 7, 2022

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 #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
Copy link
Member Author

HosseinYousefi commented Nov 7, 2022

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
Copy link
Collaborator

dcharkes commented Nov 8, 2022

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
Copy link
Member Author

HosseinYousefi commented Nov 8, 2022

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
Copy link
Collaborator

dcharkes commented Nov 8, 2022

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
Copy link
Member Author

HosseinYousefi commented Nov 8, 2022

I don't like this because

  • 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.
  • A lot of generated classes already have multiple constructors and we rarely use the "first" one MyFoo.ctorN(...).
  • We could have two classes, one special and one non-special that have the same looking constructor with very different meaning:
// 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.klass(), B.klass()); // foo is a non-null object that has null properties

edit:

  • If we have builds for some classes then we would want to have builds for all classes even the "non-special" ones which increases the code we generate.
  • As mentioned in the first post, we might want to add additional context to the object (like choosing the JVM) before constructing it, this can be done rather nicely by chaining commands or using the constructor before calling build:
final foo = MyFoo()..setContext(...)..build(...);

@dcharkes
Copy link
Collaborator

dcharkes commented Nov 8, 2022

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
Copy link
Collaborator

dcharkes commented Nov 8, 2022

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
Copy link
Member Author

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
Copy link
Collaborator

dcharkes commented Nov 8, 2022

Ah, that's even more confusing indeed.

@HosseinYousefi
Copy link
Member Author

Another important thing to consider in our design is supporting subtyping (I believe you already created the issue #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
Copy link
Collaborator

dcharkes commented Nov 8, 2022

Gist from discussion:

  • If we have need of the type hierarchy in other JNI features, we should make it front and center.
  • If we only need it for arrays (we don't need it for generics), it should be in the background.

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

@mahesh-hegde
Copy link
Contributor

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

  • pass klass to generic constructors and skip explicit type annotations. If there are other arguments, append them after.
var documents = JniArray(PDDocument.klass)
var documentList = List.of2(PDDocument.klass, myDocument, myOtherDocument);
  • Ctors and statics remain as before. But can take optional context argument which is always a named argument, like allocator in FFI. This can be done when (or if) we implement multi-jvm, local refs etc..

@mahesh-hegde
Copy link
Contributor

mahesh-hegde commented Nov 9, 2022

// 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
Copy link
Member Author

// 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
Copy link
Member Author

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:

  • We need to have the same fields inside the type class and also the actual case.
  • fromRef also needs to exist both in the actual class and the type class.

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
Copy link
Collaborator

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
Copy link
Member Author

Closing in favor of #705. Since the named argument approach allows us to have type inference.

@HosseinYousefi HosseinYousefi transferred this issue from dart-archive/jnigen Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants