Closed ahayzen-kdab closed 1 month ago
Appears that we should probably template the CxxQtLocking
inheritance in CxxQtThreading
so that multiple inheritance can happen. See my comment on the original discussion for more info.
I didn't realize that there might be anything special taking place on the Rust side of the bridge. Note that in the example above, I never once use or mention the base type in the derived type's Rust code, but only reference its name in the #[base ...]
macro. My assumption is that this would only lead to C++ inheritance.
I think your suggestion of templating the "offending" base class might do the trick.
Right, i hope that'll solve it which would be nice.
For more insight into what is happening here with inheritance have a look at this input https://github.com/KDAB/cxx-qt/blob/9f2de71cb12e0a816653e2bdb99f4f369d27c5c4/crates/cxx-qt-gen/test_inputs/inheritance.rs#L10-L14
The ends up generating in C++ https://github.com/KDAB/cxx-qt/blob/9f2de71cb12e0a816653e2bdb99f4f369d27c5c4/crates/cxx-qt-gen/test_outputs/inheritance.h#L11-L17
Note in that example only locking is enabled and not threading.
For an example with threading enabled like https://github.com/KDAB/cxx-qt/blob/9f2de71cb12e0a816653e2bdb99f4f369d27c5c4/crates/cxx-qt-gen/test_inputs/invokables.rs#L58
it ends up like this https://github.com/KDAB/cxx-qt/blob/9f2de71cb12e0a816653e2bdb99f4f369d27c5c4/crates/cxx-qt-gen/test_outputs/invokables.h#L16-L22
I hope that by changing CxxQtLocking
-> CxxQtLocking<T>
should "solve" this.
After chasing down some of those errors, you eventually hit errors like this:
rc/ffi.cxx.cpp:315:83: error: reference to ‘unsafeRust’ is ambiguous
cargo:warning= 315 | ::MyObjectBRust const &(::MyObjectB::*cxx_qt_ffi_rust$)() const = &::MyObjectB::unsafeRust;
cargo:warning= | ^~~~~~~~~~
cargo:warning=In file included from /home/ben/cxx-qt/target/debug/build/qml-minimal-no-cmake-2e43c9623c960347/out/cxx-qt-build/target/crates/qml-minimal-no-cmake/include/qml-minimal-no-cmake/ffi.cxxqt.h:5,
cargo:warning= from /home/ben/cxx-qt/target/debug/build/qml-minimal-no-cmake-2e43c9623c960347/out/cxx-qt-gen/src/ffi.cxx.cpp:3:
cargo:warning=/home/ben/cxx-qt/target/debug/build/qml-minimal-no-cmake-2e43c9623c960347/out/cxx-qt-build/target/crates/qml-minimal-no-cmake/include/cxx-qt/type.h:28:12: note: candidates are: ‘const T& rust::cxxqt1::CxxQtType<T>::unsafeRust() const [with T = MyObjectBRust]’
cargo:warning= 28 | T const& unsafeRust() const { return *m_rustObj; }
cargo:warning= | ^~~~~~~~~~
cargo:warning=/home/ben/cxx-qt/target/debug/build/qml-minimal-no-cmake-2e43c9623c960347/out/cxx-qt-build/target/crates/qml-minimal-no-cmake/include/cxx-qt/type.h:28:12: note: ‘const T& rust::cxxqt1::CxxQtType<T>::unsafeRust() const [with T = MyObjectARust]’
cargo:warning=/home/ben/cxx-qt/target/debug/build/qml-minimal-no-cmake-2e43c9623c960347/out/cxx-qt-gen/src/ffi.cxx.cpp: In function ‘MyObjectBRust* cxxbridge1$MyObjectB$cxx_qt_ffi_rust_mut(MyObjectB&)’:
cargo:warning=/home/ben/cxx-qt/target/debug/build/qml-minimal-no-cmake-2e43c9623c960347/out/cxx-qt-gen/src/ffi.cxx.cpp:320:75: error: reference to ‘unsafeRustMut’ is ambiguous
cargo:warning= 320 | ::MyObjectBRust &(::MyObjectB::*cxx_qt_ffi_rust_mut$)() = &::MyObjectB::unsafeRustMut;
cargo:warning= | ^~~~~~~~~~~~~
cargo:warning=/home/ben/cxx-qt/target/debug/build/qml-minimal-no-cmake-2e43c9623c960347/out/cxx-qt-build/target/crates/qml-minimal-no-cmake/include/cxx-qt/type.h:29:6: note: candidates are: ‘T& rust::cxxqt1::CxxQtType<T>::unsafeRustMut() [with T = MyObjectBRust]’
cargo:warning= 29 | T& unsafeRustMut() { return *m_rustObj; }
cargo:warning= | ^~~~~~~~~~~~~
cargo:warning=/home/ben/cxx-qt/target/debug/build/qml-minimal-no-cmake-2e43c9623c960347/out/cxx-qt-build/target/crates/qml-minimal-no-cmake/include/cxx-qt/type.h:29:6: note: ‘T& rust::cxxqt1::CxxQtType<T>::unsafeRustMut() [with T = MyObjectARust]’
which seems to be a deeper problem
It appears even if the CxxQtLocking part was templated, you then hit issues with the CXX generated code. As both the base and derived class have CxxQtType which have methods like unsafeRust(), the code CXX generates is ambiguous and the C++ compiler can't figure out which call is required.
This needs some more thought of how to compose multiple classes together that are from the Rust side or whether that can be even possible with the current structure. cc @LeonMatthesKDAB
Phew, that's a complicated issue... :thinking:
First thoughts on CxxQtLocking
-> CxxQtLocking<T>
:
This would probably solve the "already inherits from X" problem, but in a bit of a weird way.
CxxQtLocking<T>
would create a new super-class for each subclass down the chain. Which also means each subclass would get it's own mutex, as both CxxQtLocking<Parent>
and CxxQtLocking<Child>
would contain a mutex to lock.
This would probably work if disambiguated correctly, but would mean we'd add multiple mutexes, which could maybe deadlock? and in any case definitely waste memory.
My hunch is that the "correct" way to go about this is to not inherit from CxxQtLocking in the first place if the base class already does.
We could of course do this manually with some kind of flag on the #[qobject]
. But with the state of C++ meta-programming, my gut feeling is that this should be possible to do with some kind of meta-programming magic.
The C++ compiler already knows what superclasses are included in the chain, and we have std::is_base_of
, which should make this possible somehow :thinking:
Does anyone have an example repo somewhere with this error so that I can reproduce this easily? :)
Hi @LeonMatthesKDAB, I'll create one since I requested this. Will report it back.
@LeonMatthesKDAB: https://github.com/SanderVocke/cxx-qt-rust-inheritance
Hm, as I'm taking a look at this right now, the larger issue actually seems to be CxxQtType
.
As I mentioned I'm pretty sure we can use meta-programming to only inherit from CxxQtLocking
a single time.
The CXX bindings to the unsafeRust
and unsafeRustMut
methods from CxxQtType<...>
are however ambigious in the generated CXX code.
We could probably work around this by generating a free method with a specific name that CXX can bind to, but that would make the CxxQtType class somewhat pointless.
I'll experiment with this a bit further...
I think I should be able to get this to work with some templating trickery. Will take a bit to figure out the details. Will open a PR when ready :)
So I've got a first version of this going in #1049
For now I've simply disabled inheriting from CxxQtLocking, if it's already inherited by the base class.
However, I've noticed that CxxQtThreading also inherits from Locking, which means this still creates duplicate Locking implementations.
So we need to disable it conditionally there as well.
The alternative I've found is to virtually inherit from CxxQtLocking
, which would solve this issue via a v-table:
See also: https://stackoverflow.com/questions/27545888/is-multiple-inheritance-from-the-same-base-class-via-different-parent-classes-re
This would actually be the easiest solution for our generation, but would require a frequent vtable lookup, so it's probably not ideal. I'll try conditionally disabling first.
Hm, yet another issue with this: Apparently QML_SINGLETON doesn't work with virtual subclassing and clang :roll_eyes: (see the CI failures here: https://github.com/KDAB/cxx-qt/actions/runs/10573868714/job/29294258974?pr=1049 )
I had to resort to virtual subclassing because CxxQtThreading also inherits from CxxQtLocking, which makes it very difficult to conditionally disable the subclassing there.
This kind of calls into question whether we can get away without CxxQtLocking, which we've discussed from time to time. I've opened a Zulip discussion on this topic so we can find a solution once Andrew is back next week: https://cxx-qt.zulipchat.com/#narrow/stream/426346-dev/topic/Deprecate.20CxxQtLocking.3F/near/465360573
Discussed in https://github.com/KDAB/cxx-qt/discussions/1031