The null_mut is not urgent and can be made nice later. A good test that shows how to use the features is required to let he code be maintainable.

I'm going to add comments on the generated file, as they are easier to read this way.

I didn't properly tackle thread safety nor memory leaks yet.

This is a good start to making the nesting of binding objects more flexible.

The code in this patch allows for double frees. Consider this function for the Person class from test_object.json:

void doubleFree()
{
    Person person;
    Person person2(person);
}

And this is also problematic:

void doubleRustFree()
{
    InnerObject inner;
    InnerObject inner2(inner);
}

The default constructor for Person calls new InnerObject(false, this). The copy constructor copies the pointer to create person2. On leaving the context, the destructor of person2 deallocates the innerobject. The destructor for person also wants to destroy it, which leads to a double free and a crash.

How can this be fixed? A possible solution is to use a counter m_instance_copies that is increased when the copy constructor is called and decreased when the destructor is called.

The copy constructor that is causing the double free is not used in the test code. Where is it needed? I've thought about it some more and see that copying the object causes more problems. If you copy an object, what do you do with the signal/slot bindings? Only one of the copies will have the binding, so when you want to remove the binding you'll have to remove it from the same copy.

The Qt documentation says that QObject and derived classes should not have copy constructor or assignment operator. Is it possible to make the same functionality without them?

http://doc.qt.io/qt-5/qobject.html#no-copy-constructor-or-assignment-operator

The copy constructor was used at some point, but with your comment I realized that it's not needed any more.

No more copy constructors saves a lot of code. :-) I'll continue the review with this new version.

I tried to find an easy error in the patch with this code:

Person person;
InnerObject* inner = new InnerObject();
person.setReplaceableObject(inner);
delete inner;
QCOMPARE(person.replaceableObject(), nullptr);

To my astonishment, this test passes. How does the code know that the object is null?

When replaceableObject() is called in the above example, Option<InnerObject> in Person is still Some. This (of course) is incorrect. When inner is deleted, the member of Person should become None.
Nevertheless, it still exists and then the line: *v.emit().qobject.lock().unwrap().deref_mut() as *mut InnerObjectQObject is called. And the qobject there has become null somehow.

So the getter behavior is good, but it's possible to write rust code that interacts with a dead object.

The reason why the above test works is

rust
#[derive(Clone)]
pub struct InnerObjectEmitter {
    qobject: Arc<Mutex<*const InnerObjectQObject>>,
    description_changed: fn(*const InnerObjectQObject),
}

The qobject is in an Arc, so when you clone it, it's not cloned, but another reference is created. When that referenced object is set to null, the reference in Person will also be null.
However any other information that got cloned is probably not wrapped in Rc or Arc. The question is if that is cloning is the intended behavior or if a single reference is desired.
The current patch allows both, but requires the user to be aware of the issue. This could be helped by documenting clearly how this works.

A remaining issue is then how to notify the rust code about the deletion of the object. When delete inner is called, the pointer in the Arc<Mutex<>> is set to null, but the Person will not know this until it tries to access the object. If the object is visible in a GUI, it should be removed from there. The way to do that is to listen to the QObject::destroyed signal. So in Person::setReplaceableObject there should be a connection to that signal. When the object is unset the signal should be disconnected.

Here is test that could be added for the delete functionality:

void TestRustObjects::testObjectSetterDelete()
{
    // GIVEN
    Person person;
    InnerObject* inner = new InnerObject();
    QSignalSpy spy(&person, &Person::replaceableObjectChanged);

    // WHEN
    person.setReplaceableObject(inner);
    delete inner;

    // THEN
    QVERIFY(spy.isValid());
    QCOMPARE(spy.count(), 2);
    QCOMPARE(person.replaceableObject(), nullptr);
}

With this update, the deleted object removes itself from the parent. I also added your test - how should I credit you?

However any other information that got cloned is probably not wrapped in Rc or Arc. The question is if that is cloning is the intended behavior or if a single reference is desired.
The current patch allows both, but requires the user to be aware of the issue. This could be helped by documenting clearly how this works.

What other information is getting cloned with the emitter? It seems to me that both the pointer to the object and the function can be cloned "cleanly".

The one issue I see with cloning is in tests/rust_objects/src/implementation.rs:29, where object-related data can indeed be cloned independently.

Deleting the QObject is now no memory issue. That's good progress.
I'm still wondering about the Rust side of the replaceableObject. The Rust struct should be tied to the replaceableObject.
A good practice might be to use something like

rust
struct InnerObjectData {
    ...
}
type InnerObject = Arc<Mutex<InnerObjectData>>;

Still I'm somewhat uncomfortable with the patch because the added functionality is complicated and I cannot quite oversee the safety and ergonomics. If code like this were used in a project for a while that would help take away some doubts.

Thanks for reviewing. I'm not happy about the Rust side either - the object should have a uniform representation no matter the number of times it's been instantiated. I like your idea, and I will see how it works.

I do agree that the complexity of this is getting high. While I do have a WIP project depending on (a subset of) this functionality, what I necessarily need to actually make the code useful is the ability to create C++ objects (lists of items) from Rust, which will be substantially more complicated, especially on the ergonomics side, if my memory serves me well.

Perhaps this line of improvements could become a separate experimental branch?

This could certainly become a separate experimental branch. You have commit access and also the ability to make a new branch (I think). Note however, that branches are not easily deleted on KDE infrastructure. Creating a branch moveable_objects is fine with me.

Thank you, I will create a new branch based on current master. Since I don't want the moveable_objects branch to become an undertested collection of patches (I can do that myself on github), I will still push changes to it through Phabricator if you don't mind.

Sure, i can give comments via Phabricator.

The implementation language of RQBG has changed from C++ to Rust. This affects the files src/cpp.cpp and src/rust.cpp which are now src/cpp.rs and src/rust.rs.

I promise I'll not change programming language again before this review is done.

In D14634#335527, @vandenoever wrote:

The implementation language of RQBG has changed from C++ to Rust. This affects the files src/cpp.cpp and src/rust.cpp which are now src/cpp.rs and src/rust.rs.

I promise I'll not change programming language again before this review is done.

I'm in favour of that and I actually considered suggesting that myself. Now it may be possible to embed the correct git revision in the dependencies and do away with generated sources in consumer programs (I know you generally do not approve of this, but they generate lots of noise in the repo :)).

The possibility of not having in-tree builds also makes me happy.

To stray even further off-topic, did you consider using a template engine like Handlebars/Rustache? I had some success using it for code generation: https://github.com/rhn/gpx-rust/blob/master/xml_parsergen/src/gpx.rs

If anyone's interested, I've been working on a POC for a change that allows objects to be created in Rust and then passed to C++. It can be viewed approximately here: https://gitlab.com/rhn_mk1/rust-qt-bindings-generator/tree/creatable_objects/tests/rust_objects . Now I'm certain it's possible.

This branch also carries additional goodies that could be beneficially adopted upstream, like Rust side testing for example.