rust-qt / ritual

Use C++ libraries from Rust
Apache License 2.0
1.22k stars 49 forks source link

Add an example for Qt 3D use from Rust #64

Closed er-vin closed 5 years ago

er-vin commented 6 years ago

It's not really pleasant to use yet due to the amount of unsafe methods and the into_raw calls which are needed to leave the life time management to QObject's parent/child tree.

Clearly comes from QObject's parent/child tree fighting with Rust own memory management and Qt 3D using heavily composition and not inheritance. My goal is now to change the generator to simplify this code.

Riateche commented 6 years ago

Thanks for the contribution! I can't merge it just yet because this code requires Qt 5.9, and currently published crates use Qt 5.8. I will merge it after I implement multiple Qt version support.

I've realized that automatically wrapping pointers in CppBox was not a good idea, since you almost always want to transfer ownership of an object to Qt. I have another idea which will hopefully make the code more ergonomic, and I will implement it when I have time.

Unsafe is probably not going anywhere. Basically, every method that accepts raw pointers is unsafe. Rust ownership is so alien to Qt that I don't believe a safe wrapper is possible for the most part of Qt, unless you change the exposed API significantly. I think the best solution is to wrap the whole blocks of unsafe code in unsafe { ... }.

er-vin commented 6 years ago

I think I got something in mind which could work with an extended CppBox which would be aware of the QObject parent/child but I need to investigate it.

What do you have in mind for your alternative to automatically wrapping pointers in CppBox?

And yes, I'm aware of the unsafe blocks, but really I wanted to keep all the cases separated for later attempt. I know both memory models are very different but I'd like to see the ergonomics of the generated crates go far enough as to not need unsafe at all or at least very rarely.

Riateche commented 6 years ago

The current implementation creates a Rust struct TypeName for each non-movable type and treats *mut TypeName as C++ pointers. (The struct itself is never instantiated.) The methods are implemented on TypeName, so when you need to call a method, you need to convert *mut TypeName to &mut TypeName and then use it to make a call. Without CppBox, converting the pointer to the reference is quite noisy. Besides being inconvenient, this is actually dangerous because &mut TypeName in Rust implies that you have exclusive access to the object, which is not really the case.

Instead, I want to create struct TypeName(*mut c_void) for each non-movable type and store the C++ pointer in the struct's field. The type's methods will be implemented on TypeName again, but in this case there is no need for extra conversion:

let object: TypeName = TypeName::new();
object.method();

This implies that all impl methods on this type are unsafe, as we're dereferencing a pointer of unknown status for passing as this.

Next, you can manually wrap it in a CppBox<TypeName> if you want it to be automatically deleted when the object goes out of scope in Rust. You can safely get a TypeName out of CppBox<TypeName> (e.g. by automatic deref) and then call any method of TypeName. Come to think of it, in most cases it's enough to use CppBox on the topmost object (e.g. the top level widget in Qt Widgets).

I also want to expose QPointer that is a smart pointer for QObject that automatically becomes null when the object is deleted. It should work like this:

let pointer: Pointer<TypeName> = Pointer::new(TypeName::new());
//...
if let Some(object) = pointer.data() {
  // object is still alive
  object.method();
}

The drawback is that the impl methods on TypeName are always unsafe, even if we somehow manage to guarantee that the pointer is currently valid. I don't think it's a big deal though, because this guarantee is hard to make anyway.

All of the above does not apply to types that are considered movable (like QPoint or QString). Memory of these types is managed by Rust, and safe methods are possible for them.

If you have any suggestions or thoughts, don't hesitate to share them.

er-vin commented 6 years ago

I had something less radical in mind but maybe that'd be considered less clean as far as Rust is concerned. My thinking was to generalize the use of CppBox instead of keeping naked pointers in the generated API for anything inheriting QObject.

Indeed for those we know Qt has a consistent memory model for them in the form of the parent/child relationship and we can assume it being honored properly. Then it'd about marrying this model with the CppBox model in a way that it wouldn't look too foreign on the Rust side.

So I was thinking of modifying the CppDeletable implementation generated by the generator in the case of classes inheriting QObject. Instead of providing a deleter which would straight call into the ffi's delete operator of the type, it'd return a deleter which first check if the object has a parent, if it does then you wouldn't delete, if it doesn't then you would delete like before.

I think it'd make for something more natural where you wouldn't have lots of unsafe calls around while making sure you can't leak memory since CppBox would properly delete all the top most objects which is what you'd want as a Qt user.

Obviously my view is coming from the angle of "Qt developer getting into Rust", not the other way around... it might look like shock and horror to a "Rust developer getting into Qt" with reasons. Depends who we want to optimize that API for I guess.

Note that I like your proposed model a lot for everything which doesn't inherit from QObject. Indeed, there we can't make any assumption on the life time of the objects.

Riateche commented 6 years ago

The idea makes sense. If we have an object with a parent, we often don't want to delete it manually. However, what's the purpose of CppBox in this case? Just to do nothing? Isn't it simpler just to use a pointer? By using CppBox only on the top level objects, we'll make it more clear that we intend to delete these objects instead of relying on Qt to do that.

This idea also doesn't really affect how many unsafe calls we'll get. Even if an object has a parent (I'd say, especially if it has a parent), it can be implicitly deleted at any time. We still need QPointer functionality to catch these cases. Generally, it's very hard to prevent Qt from deleting your objects. It's just how it's designed.

If we aim for minimizing unsafety, we need some kind of "guaranteed to be currently valid" pointer. However, there is no way to get such guarantee in Qt at all. Unless you're using a smart pointer, any object can be deleted at any time. And smart pointers are not used in Qt (although it even provides its own smart pointers). Even QPointer doesn't have any kind of lock to prevent the received pointer from becoming invalid during the time you use it. In fact, I cannot imagine a safe call involving any Qt object (be it QObject, QListWidgetItem or something else). Only value types (like QPoint or QString) can be made safe because pointers to them are almost never stored anywhere. Making the rest of API safe requires a manually constructed and carefully checked higher-level wrapper, and it's currently out of scope of this project because we don't have a good low-level API yet.

er-vin commented 6 years ago

Trying to reply to the various points, I don't think I like the GitHub comments for such discussions, I'm more used to email threads. ;-)

Yes, CppBox would do nothing in the case of a QObject with a parent. And it's definitely an intent vs convenience of use for Qt/C++ user trade off. The value would be in ::new() returning always CppBox<> so that the user can't make a mistake. If we leave to the user to decide when to wrap in a CppBox (the reason being indeed "it's a top level object") I'm sure that it'll lead to undetected leaks in user code during maintenance phase (e.g. a previously non-top level object becoming one during a refactoring).

Of course you are right regarding unsafe-ness, that's something I'm not quite sure how to properly wrap yet, I think we can make some assumptions on what the Qt API does or doesn't do with the passed QObject pointers in most cases but we probably can't come up with an assumption which will work in 100% of the cases like the parent/child relationship and how it can relate to CppBox. It's a tougher one.

On the other hand, what I'm sure of is that for anything I created using a _::new() call, since it's in a CppBox I control the calls wouldn't be unsafe without CppBox I'd end up going through unsafe calls for something I just created myself and am still responsible of, it's a bit unfortunate IMO.

er-vin commented 5 years ago

5.9 is long published now, can this be merged?

Riateche commented 5 years ago

Sorry for the slowness. Merged into master and develop now.

We've been reimplementing various parts of the generator to improve the overall API stability. In addition to CppBox, we're now experimenting with Ptr and ConstPtr in hopes of improving the ergonomics, but that's still work in progress.