From 58d2d5814e5ece6090f111fa251a789484569284 Mon Sep 17 00:00:00 2001 From: Andrew Hayzen Date: Wed, 31 May 2023 17:41:24 +0100 Subject: [PATCH] cxx-qt-gen: add cxx_qt::Locking as a trait which can be negated Closes #561 --- CHANGELOG.md | 1 + book/src/concepts/threading.md | 8 +++ book/src/qobject/cxxqtthread.md | 2 + .../cxx-qt-gen/src/generator/cpp/invokable.rs | 33 +++++---- crates/cxx-qt-gen/src/generator/cpp/mod.rs | 3 - .../src/generator/cpp/property/getter.rs | 11 ++- .../src/generator/cpp/property/mod.rs | 50 +++++++++----- .../src/generator/cpp/property/setter.rs | 11 ++- .../cxx-qt-gen/src/generator/cpp/qobject.rs | 11 +++ crates/cxx-qt-gen/src/generator/cpp/signal.rs | 41 ++++++++---- .../cxx-qt-gen/src/generator/rust/qobject.rs | 14 ++++ crates/cxx-qt-gen/src/parser/qobject.rs | 38 ++++++++++- crates/cxx-qt-gen/src/writer/cpp/header.rs | 7 +- crates/cxx-qt-gen/src/writer/cpp/mod.rs | 5 +- crates/cxx-qt-gen/src/writer/cpp/source.rs | 7 +- .../test_inputs/passthrough_and_naming.rs | 2 + crates/cxx-qt-gen/test_outputs/inheritance.rs | 1 + crates/cxx-qt-gen/test_outputs/invokables.rs | 1 + .../test_outputs/passthrough_and_naming.cpp | 12 ++-- .../test_outputs/passthrough_and_naming.h | 1 - .../test_outputs/passthrough_and_naming.rs | 1 + crates/cxx-qt-gen/test_outputs/properties.rs | 1 + crates/cxx-qt-gen/test_outputs/signals.rs | 1 + crates/cxx-qt/src/lib.rs | 19 ++++++ tests/basic_cxx_qt/cpp/main.cpp | 67 +++++++++++++++++++ tests/basic_cxx_qt/rust/build.rs | 1 + tests/basic_cxx_qt/rust/src/lib.rs | 1 + tests/basic_cxx_qt/rust/src/locking.rs | 58 ++++++++++++++++ 28 files changed, 343 insertions(+), 65 deletions(-) create mode 100644 tests/basic_cxx_qt/rust/src/locking.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 36eaf019b..1340bf718 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Allow associated constants, types and macro invocations within `impl qobject::T` blocks - Ensure that generated Rust code works when `#![deny(missing_docs)]` is enabled - Ability to connect and disconnect from signals in Rust triggering a function pointer +- `unsafe impl !cxx_qt::Locking for qobject::T` to disable internal locking ### Changed diff --git a/book/src/concepts/threading.md b/book/src/concepts/threading.md index 46c6907ac..f40376530 100644 --- a/book/src/concepts/threading.md +++ b/book/src/concepts/threading.md @@ -15,6 +15,14 @@ This means that Rust code, such as invokables and properties, which are directly Note that a recursive mutex is used internally, this allows for signals to be emitted and then call slots on the same object without deadlocks. +### Locking + +In certain scenarios it might be useful to disable locking that occurs when the context switches from C++ to Rust. + +To disable the generation of locking use an unsafe negation `unsafe impl !cxx_qt::Locking for qobject::T {}`. + +However if locking is disabled the `cxx_qt::Threading` trait can not be enabled on the object. + ## Multi threading To achieve safe multi-threading on the Rust side we use an [`CxxQtThread`](../qobject/cxxqtthread.md). diff --git a/book/src/qobject/cxxqtthread.md b/book/src/qobject/cxxqtthread.md index c9662e6e8..6b809d6cd 100644 --- a/book/src/qobject/cxxqtthread.md +++ b/book/src/qobject/cxxqtthread.md @@ -21,6 +21,8 @@ First threading needs to be enabled for the [`qobject::T`](./generated-qobject.m {{#include ../../../examples/qml_features/rust/src/threading.rs:book_qt_thread}} ``` +Note that locking must not be disabled for the object (eg `unsafe impl cxx_qt::Locking for qobject::T`) for `cxx_qt::Threading` to be allowed. + Then to access the `CxxQtThread` use the `qt_thread(&self)` method on a [`qobject::T`](./generated-qobject.md). ```rust,ignore,noplayground diff --git a/crates/cxx-qt-gen/src/generator/cpp/invokable.rs b/crates/cxx-qt-gen/src/generator/cpp/invokable.rs index 8d36fa8b2..ba030626f 100644 --- a/crates/cxx-qt-gen/src/generator/cpp/invokable.rs +++ b/crates/cxx-qt-gen/src/generator/cpp/invokable.rs @@ -9,7 +9,6 @@ use crate::{ fragment::{CppFragment, CppNamedType}, qobject::GeneratedCppQObjectBlocks, types::CppType, - RUST_OBJ_MUTEX_LOCK_GUARD, }, naming::{invokable::QInvokableName, qobject::QObjectName}, }, @@ -25,6 +24,7 @@ pub fn generate_cpp_invokables( invokables: &Vec, qobject_idents: &QObjectName, cxx_mappings: &ParsedCxxMappings, + lock_guard: Option<&str>, ) -> Result { let mut generated = GeneratedCppQObjectBlocks::default(); let qobject_ident = qobject_idents.cpp_class.cpp.to_string(); @@ -132,7 +132,7 @@ pub fn generate_cpp_invokables( is_const = is_const, parameter_types = parameter_types, qobject_ident = qobject_ident, - rust_obj_guard = RUST_OBJ_MUTEX_LOCK_GUARD, + rust_obj_guard = lock_guard.unwrap_or_default(), body = if return_cxx_ty.is_some() { format!("return {body}", body = body) } else { @@ -202,9 +202,13 @@ mod tests { ]; let qobject_idents = create_qobjectname(); - let generated = - generate_cpp_invokables(&invokables, &qobject_idents, &ParsedCxxMappings::default()) - .unwrap(); + let generated = generate_cpp_invokables( + &invokables, + &qobject_idents, + &ParsedCxxMappings::default(), + Some("// ::std::lock_guard"), + ) + .unwrap(); // methods assert_eq!(generated.methods.len(), 4); @@ -221,7 +225,7 @@ mod tests { void MyObject::voidInvokable() const { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard m_rustObj->voidInvokableWrapper(*this); } "#} @@ -242,7 +246,7 @@ mod tests { ::std::int32_t MyObject::trivialInvokable(::std::int32_t param) const { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard return m_rustObj->trivialInvokableWrapper(*this, param); } "#} @@ -263,7 +267,7 @@ mod tests { ::std::unique_ptr MyObject::opaqueInvokable(QColor const& param) { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard return m_rustObj->opaqueInvokableWrapper(*this, param); } "#} @@ -284,7 +288,7 @@ mod tests { ::std::int32_t MyObject::specifiersInvokable(::std::int32_t param) const { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard return m_rustObj->specifiersInvokableWrapper(*this, param); } "#} @@ -312,8 +316,13 @@ mod tests { .cxx_names .insert("B".to_owned(), "B2".to_owned()); - let generated = - generate_cpp_invokables(&invokables, &qobject_idents, &cxx_mappings).unwrap(); + let generated = generate_cpp_invokables( + &invokables, + &qobject_idents, + &cxx_mappings, + Some("// ::std::lock_guard"), + ) + .unwrap(); // methods assert_eq!(generated.methods.len(), 1); @@ -330,7 +339,7 @@ mod tests { B2 MyObject::trivialInvokable(A1 param) const { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard return m_rustObj->trivialInvokableWrapper(*this, param); } "#} diff --git a/crates/cxx-qt-gen/src/generator/cpp/mod.rs b/crates/cxx-qt-gen/src/generator/cpp/mod.rs index 106fec761..c2f06c33d 100644 --- a/crates/cxx-qt-gen/src/generator/cpp/mod.rs +++ b/crates/cxx-qt-gen/src/generator/cpp/mod.rs @@ -16,9 +16,6 @@ use crate::parser::Parser; use qobject::GeneratedCppQObject; use syn::Result; -pub const RUST_OBJ_MUTEX_LOCK_GUARD: &str = - "const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex);"; - /// Representation of the generated C++ code for a group of QObjects pub struct GeneratedCppBlocks { /// Stem of the CXX header to include diff --git a/crates/cxx-qt-gen/src/generator/cpp/property/getter.rs b/crates/cxx-qt-gen/src/generator/cpp/property/getter.rs index 5491ed5db..e4d9b1ca7 100644 --- a/crates/cxx-qt-gen/src/generator/cpp/property/getter.rs +++ b/crates/cxx-qt-gen/src/generator/cpp/property/getter.rs @@ -4,12 +4,17 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 use crate::generator::{ - cpp::{fragment::CppFragment, types::CppType, RUST_OBJ_MUTEX_LOCK_GUARD}, + cpp::{fragment::CppFragment, types::CppType}, naming::property::QPropertyName, }; use indoc::formatdoc; -pub fn generate(idents: &QPropertyName, qobject_ident: &str, cxx_ty: &CppType) -> CppFragment { +pub fn generate( + idents: &QPropertyName, + qobject_ident: &str, + cxx_ty: &CppType, + lock_guard: Option<&str>, +) -> CppFragment { CppFragment::Pair { header: format!( "{return_cxx_ty} const& {ident_getter}() const;", @@ -28,7 +33,7 @@ pub fn generate(idents: &QPropertyName, qobject_ident: &str, cxx_ty: &CppType) - return_cxx_ty = cxx_ty.as_cxx_ty(), ident_getter = idents.getter.cpp.to_string(), qobject_ident = qobject_ident, - rust_obj_guard = RUST_OBJ_MUTEX_LOCK_GUARD, + rust_obj_guard = lock_guard.unwrap_or_default(), ), } } diff --git a/crates/cxx-qt-gen/src/generator/cpp/property/mod.rs b/crates/cxx-qt-gen/src/generator/cpp/property/mod.rs index a2db670ea..1b96598c1 100644 --- a/crates/cxx-qt-gen/src/generator/cpp/property/mod.rs +++ b/crates/cxx-qt-gen/src/generator/cpp/property/mod.rs @@ -19,6 +19,7 @@ pub fn generate_cpp_properties( properties: &Vec, qobject_idents: &QObjectName, cxx_mappings: &ParsedCxxMappings, + lock_guard: Option<&str>, ) -> Result { let mut generated = GeneratedCppQObjectBlocks::default(); let qobject_ident = qobject_idents.cpp_class.cpp.to_string(); @@ -28,12 +29,18 @@ pub fn generate_cpp_properties( let cxx_ty = CppType::from(&property.ty, cxx_mappings)?; generated.metaobjects.push(meta::generate(&idents, &cxx_ty)); - generated - .methods - .push(getter::generate(&idents, &qobject_ident, &cxx_ty)); - generated - .methods - .push(setter::generate(&idents, &qobject_ident, &cxx_ty)); + generated.methods.push(getter::generate( + &idents, + &qobject_ident, + &cxx_ty, + lock_guard, + )); + generated.methods.push(setter::generate( + &idents, + &qobject_ident, + &cxx_ty, + lock_guard, + )); generated.methods.push(signal::generate(&idents)); } @@ -67,9 +74,13 @@ mod tests { ]; let qobject_idents = create_qobjectname(); - let generated = - generate_cpp_properties(&properties, &qobject_idents, &ParsedCxxMappings::default()) - .unwrap(); + let generated = generate_cpp_properties( + &properties, + &qobject_idents, + &ParsedCxxMappings::default(), + Some("// ::std::lock_guard"), + ) + .unwrap(); // metaobjects assert_eq!(generated.metaobjects.len(), 2); @@ -90,7 +101,7 @@ mod tests { ::std::int32_t const& MyObject::getTrivialProperty() const { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard return m_rustObj->getTrivialProperty(*this); } "#} @@ -111,7 +122,7 @@ mod tests { void MyObject::setTrivialProperty(::std::int32_t const& value) { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard m_rustObj->setTrivialProperty(*this, value); } "#} @@ -137,7 +148,7 @@ mod tests { ::std::unique_ptr const& MyObject::getOpaqueProperty() const { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard return m_rustObj->getOpaqueProperty(*this); } "#} @@ -158,7 +169,7 @@ mod tests { void MyObject::setOpaqueProperty(::std::unique_ptr const& value) { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard m_rustObj->setOpaqueProperty(*this, value); } "#} @@ -186,8 +197,13 @@ mod tests { .cxx_names .insert("A".to_owned(), "A1".to_owned()); - let generated = - generate_cpp_properties(&properties, &qobject_idents, &cxx_mapping).unwrap(); + let generated = generate_cpp_properties( + &properties, + &qobject_idents, + &cxx_mapping, + Some("// ::std::lock_guard"), + ) + .unwrap(); // metaobjects assert_eq!(generated.metaobjects.len(), 1); @@ -207,7 +223,7 @@ mod tests { A1 const& MyObject::getMappedProperty() const { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard return m_rustObj->getMappedProperty(*this); } "#} @@ -225,7 +241,7 @@ mod tests { void MyObject::setMappedProperty(A1 const& value) { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard m_rustObj->setMappedProperty(*this, value); } "#} diff --git a/crates/cxx-qt-gen/src/generator/cpp/property/setter.rs b/crates/cxx-qt-gen/src/generator/cpp/property/setter.rs index 8d761ae0a..68812806a 100644 --- a/crates/cxx-qt-gen/src/generator/cpp/property/setter.rs +++ b/crates/cxx-qt-gen/src/generator/cpp/property/setter.rs @@ -4,12 +4,17 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 use crate::generator::{ - cpp::{fragment::CppFragment, types::CppType, RUST_OBJ_MUTEX_LOCK_GUARD}, + cpp::{fragment::CppFragment, types::CppType}, naming::property::QPropertyName, }; use indoc::formatdoc; -pub fn generate(idents: &QPropertyName, qobject_ident: &str, cxx_ty: &CppType) -> CppFragment { +pub fn generate( + idents: &QPropertyName, + qobject_ident: &str, + cxx_ty: &CppType, + lock_guard: Option<&str>, +) -> CppFragment { CppFragment::Pair { header: format!( "Q_SLOT void {ident_setter}({cxx_ty} const& value);", @@ -28,7 +33,7 @@ pub fn generate(idents: &QPropertyName, qobject_ident: &str, cxx_ty: &CppType) - cxx_ty = cxx_ty.as_cxx_ty(), ident_setter = idents.setter.cpp, qobject_ident = qobject_ident, - rust_obj_guard = RUST_OBJ_MUTEX_LOCK_GUARD, + rust_obj_guard = lock_guard.unwrap_or_default(), }, } } diff --git a/crates/cxx-qt-gen/src/generator/cpp/qobject.rs b/crates/cxx-qt-gen/src/generator/cpp/qobject.rs index c635596b4..d7194fdc0 100644 --- a/crates/cxx-qt-gen/src/generator/cpp/qobject.rs +++ b/crates/cxx-qt-gen/src/generator/cpp/qobject.rs @@ -74,6 +74,8 @@ pub struct GeneratedCppQObject { pub base_class: String, /// The blocks of the QObject pub blocks: GeneratedCppQObjectBlocks, + /// Whether locking is enabled + pub locking: bool, } impl GeneratedCppQObject { @@ -93,6 +95,12 @@ impl GeneratedCppQObject { .clone() .unwrap_or_else(|| "QObject".to_string()), blocks: GeneratedCppQObjectBlocks::from(qobject), + locking: qobject.locking, + }; + let lock_guard = if qobject.locking { + Some("const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex);") + } else { + None }; // Generate methods for the properties, invokables, signals @@ -100,17 +108,20 @@ impl GeneratedCppQObject { &qobject.properties, &qobject_idents, cxx_mappings, + lock_guard, )?); generated.blocks.append(&mut generate_cpp_invokables( &qobject.invokables, &qobject_idents, cxx_mappings, + lock_guard, )?); if let Some(signals_enum) = &qobject.signals { generated.blocks.append(&mut generate_cpp_signals( &signals_enum.signals, &qobject_idents, cxx_mappings, + lock_guard, )?); } generated.blocks.append(&mut inherit::generate( diff --git a/crates/cxx-qt-gen/src/generator/cpp/signal.rs b/crates/cxx-qt-gen/src/generator/cpp/signal.rs index d63b53306..bdda9dd15 100644 --- a/crates/cxx-qt-gen/src/generator/cpp/signal.rs +++ b/crates/cxx-qt-gen/src/generator/cpp/signal.rs @@ -5,10 +5,7 @@ use crate::{ generator::{ - cpp::{ - fragment::CppFragment, qobject::GeneratedCppQObjectBlocks, types::CppType, - RUST_OBJ_MUTEX_LOCK_GUARD, - }, + cpp::{fragment::CppFragment, qobject::GeneratedCppQObjectBlocks, types::CppType}, naming::{qobject::QObjectName, signals::QSignalName}, }, parser::{cxxqtdata::ParsedCxxMappings, signals::ParsedSignal}, @@ -20,6 +17,7 @@ pub fn generate_cpp_signals( signals: &Vec, qobject_idents: &QObjectName, cxx_mappings: &ParsedCxxMappings, + lock_guard: Option<&str>, ) -> Result { let mut generated = GeneratedCppQObjectBlocks::default(); let qobject_ident = qobject_idents.cpp_class.cpp.to_string(); @@ -98,7 +96,7 @@ pub fn generate_cpp_signals( &{qobject_ident}::{signal_ident}, this, [&, func = ::std::move(func)]({parameters_cpp}) {{ - {RUST_OBJ_MUTEX_LOCK_GUARD} + {rust_obj_guard} func({parameter_values}); }}, type); }} @@ -107,6 +105,7 @@ pub fn generate_cpp_signals( parameters_cpp = parameter_types_cpp.join(", "), parameters_rust = parameter_types_rust.join(", "), parameter_values = parameter_values_connection.join(", "), + rust_obj_guard = lock_guard.unwrap_or_default(), }, }); } @@ -144,8 +143,13 @@ mod tests { }]; let qobject_idents = create_qobjectname(); - let generated = - generate_cpp_signals(&signals, &qobject_idents, &ParsedCxxMappings::default()).unwrap(); + let generated = generate_cpp_signals( + &signals, + &qobject_idents, + &ParsedCxxMappings::default(), + Some("// ::std::lock_guard"), + ) + .unwrap(); assert_eq!(generated.methods.len(), 3); let header = if let CppFragment::Header(header) = &generated.methods[0] { @@ -197,7 +201,7 @@ mod tests { &MyObject::dataChanged, this, [&, func = ::std::move(func)](::std::int32_t trivial, ::std::unique_ptr opaque) { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard func(*this, ::std::move(trivial), ::std::move(opaque)); }, type); } @@ -223,7 +227,13 @@ mod tests { .cxx_names .insert("A".to_owned(), "A1".to_owned()); - let generated = generate_cpp_signals(&signals, &qobject_idents, &cxx_mappings).unwrap(); + let generated = generate_cpp_signals( + &signals, + &qobject_idents, + &cxx_mappings, + Some("// ::std::lock_guard"), + ) + .unwrap(); assert_eq!(generated.methods.len(), 3); let header = if let CppFragment::Header(header) = &generated.methods[0] { @@ -269,7 +279,7 @@ mod tests { &MyObject::dataChanged, this, [&, func = ::std::move(func)](A1 mapped) { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard func(*this, ::std::move(mapped)); }, type); } @@ -287,8 +297,13 @@ mod tests { }]; let qobject_idents = create_qobjectname(); - let generated = - generate_cpp_signals(&signals, &qobject_idents, &ParsedCxxMappings::default()).unwrap(); + let generated = generate_cpp_signals( + &signals, + &qobject_idents, + &ParsedCxxMappings::default(), + Some("// ::std::lock_guard"), + ) + .unwrap(); assert_eq!(generated.methods.len(), 2); let (header, source) = if let CppFragment::Pair { header, source } = &generated.methods[0] { @@ -324,7 +339,7 @@ mod tests { &MyObject::baseName, this, [&, func = ::std::move(func)]() { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + // ::std::lock_guard func(*this); }, type); } diff --git a/crates/cxx-qt-gen/src/generator/rust/qobject.rs b/crates/cxx-qt-gen/src/generator/rust/qobject.rs index 416b50791..bd8f87a2a 100644 --- a/crates/cxx-qt-gen/src/generator/rust/qobject.rs +++ b/crates/cxx-qt-gen/src/generator/rust/qobject.rs @@ -122,6 +122,20 @@ impl GeneratedRustQObject { )?); } + // If this type has locking enabling then implement the trait + // + // This could be implemented using an auto trait in the future once stable + // https://doc.rust-lang.org/beta/unstable-book/language-features/auto-traits.html + if qobject.locking { + let cpp_class_name_rust = &qobject_idents.cpp_class.rust; + generated + .blocks + .cxx_qt_mod_contents + .push(syn::parse_quote! { + impl cxx_qt::Locking for #cpp_class_name_rust {} + }); + } + Ok(generated) } } diff --git a/crates/cxx-qt-gen/src/parser/qobject.rs b/crates/cxx-qt-gen/src/parser/qobject.rs index 445df39c2..b5213d2e3 100644 --- a/crates/cxx-qt-gen/src/parser/qobject.rs +++ b/crates/cxx-qt-gen/src/parser/qobject.rs @@ -64,6 +64,8 @@ pub struct ParsedQObject { pub fields: Vec, /// List of specifiers to register with in QML pub qml_metadata: Option, + /// Whether locking is enabled for this QObject + pub locking: bool, /// Whether threading has been enabled for this QObject pub threading: bool, /// Items that we don't need to generate anything for CXX or C++ @@ -119,6 +121,7 @@ impl ParsedQObject { properties, fields, qml_metadata, + locking: true, threading: false, others: vec![], }) @@ -234,7 +237,32 @@ impl ParsedQObject { )); } - if path_compare_str(trait_path, &["cxx_qt", "Threading"]) { + if path_compare_str(trait_path, &["cxx_qt", "Locking"]) { + if imp.unsafety.is_none() { + return Err(Error::new_spanned( + trait_path, + "cxx_qt::Locking must be an unsafe impl", + )); + } + + if not.is_none() { + return Err(Error::new_spanned( + trait_path, + "cxx_qt::Locking is enabled by default, it can only be negated.", + )); + } + + // Check that cxx_qt::Threading is not enabled + if self.threading { + return Err(Error::new_spanned( + trait_path, + "cxx_qt::Locking must be enabled if cxx_qt::Threading is enabled", + )); + } + + self.locking = false; + Ok(()) + } else if path_compare_str(trait_path, &["cxx_qt", "Threading"]) { if not.is_some() { return Err(Error::new_spanned( trait_path, @@ -242,6 +270,14 @@ impl ParsedQObject { )); } + // Check that cxx_qt::Locking is not disabled + if !self.locking { + return Err(Error::new_spanned( + trait_path, + "cxx_qt::Locking must be enabled if cxx_qt::Threading is enabled", + )); + } + self.threading = true; Ok(()) } else { diff --git a/crates/cxx-qt-gen/src/writer/cpp/header.rs b/crates/cxx-qt-gen/src/writer/cpp/header.rs index 450583e08..930753c26 100644 --- a/crates/cxx-qt-gen/src/writer/cpp/header.rs +++ b/crates/cxx-qt-gen/src/writer/cpp/header.rs @@ -104,9 +104,14 @@ fn qobjects_header(generated: &GeneratedCppBlocks) -> Vec { members = { let mut members = vec![ format!("::rust::Box<{rust_ident}> m_rustObj;", rust_ident = qobject.rust_ident), - "::std::shared_ptr<::std::recursive_mutex> m_rustObjMutex;".to_string(), ]; + if qobject.locking { + members.extend(vec![ + "::std::shared_ptr<::std::recursive_mutex> m_rustObjMutex;".to_string(), + ]); + } + members.extend(qobject.blocks.members.iter().filter_map(pair_as_header).collect::>()); members.join("\n ") }, diff --git a/crates/cxx-qt-gen/src/writer/cpp/mod.rs b/crates/cxx-qt-gen/src/writer/cpp/mod.rs index c124eca3b..9969dfdb1 100644 --- a/crates/cxx-qt-gen/src/writer/cpp/mod.rs +++ b/crates/cxx-qt-gen/src/writer/cpp/mod.rs @@ -59,6 +59,7 @@ mod tests { rust_ident: "MyObjectRust".to_owned(), namespace_internals: "cxx_qt::my_object::cxx_qt_my_object".to_owned(), base_class: "QStringListModel".to_owned(), + locking: true, blocks: GeneratedCppQObjectBlocks { deconstructors: vec![], forward_declares: vec![], @@ -159,6 +160,7 @@ mod tests { rust_ident: "FirstObjectRust".to_owned(), namespace_internals: "cxx_qt::cxx_qt_first_object".to_owned(), base_class: "QStringListModel".to_owned(), + locking: true, blocks: GeneratedCppQObjectBlocks { deconstructors: vec![], forward_declares: vec![], @@ -198,6 +200,7 @@ mod tests { rust_ident: "SecondObjectRust".to_owned(), namespace_internals: "cxx_qt::cxx_qt_second_object".to_owned(), base_class: "QStringListModel".to_owned(), + locking: false, blocks: GeneratedCppQObjectBlocks { deconstructors: vec![], forward_declares: vec![], @@ -381,7 +384,6 @@ mod tests { private: ::rust::Box m_rustObj; - ::std::shared_ptr<::std::recursive_mutex> m_rustObjMutex; }; static_assert(::std::is_base_of::value, "SecondObject must inherit from QObject"); @@ -596,7 +598,6 @@ mod tests { SecondObject::SecondObject(QObject* parent) : QStringListModel(parent) , m_rustObj(cxx_qt::cxx_qt_second_object::createRs()) - , m_rustObjMutex(::std::make_shared<::std::recursive_mutex>()) { } diff --git a/crates/cxx-qt-gen/src/writer/cpp/source.rs b/crates/cxx-qt-gen/src/writer/cpp/source.rs index 323654c54..21cc815fb 100644 --- a/crates/cxx-qt-gen/src/writer/cpp/source.rs +++ b/crates/cxx-qt-gen/src/writer/cpp/source.rs @@ -68,9 +68,14 @@ fn qobjects_source(generated: &GeneratedCppBlocks) -> Vec { members = { let mut members = vec![ format!(", m_rustObj({namespace_internals}::createRs())", namespace_internals = qobject.namespace_internals), - ", m_rustObjMutex(::std::make_shared<::std::recursive_mutex>())".to_string(), ]; + if qobject.locking { + members.extend(vec![ + ", m_rustObjMutex(::std::make_shared<::std::recursive_mutex>())".to_string(), + ]); + } + members.extend(qobject.blocks.members.iter().filter_map(pair_as_source).collect::>()); members.join("\n ") }, diff --git a/crates/cxx-qt-gen/test_inputs/passthrough_and_naming.rs b/crates/cxx-qt-gen/test_inputs/passthrough_and_naming.rs index c00b84e17..ed0eb7086 100644 --- a/crates/cxx-qt-gen/test_inputs/passthrough_and_naming.rs +++ b/crates/cxx-qt-gen/test_inputs/passthrough_and_naming.rs @@ -132,6 +132,8 @@ pub mod ffi { property_name: i32, } + unsafe impl !cxx_qt::Locking for qobject::SecondObject {} + impl Default for SecondObject { fn default() -> Self { Self { property_name: 32 } diff --git a/crates/cxx-qt-gen/test_outputs/inheritance.rs b/crates/cxx-qt-gen/test_outputs/inheritance.rs index c52d8573d..f23d5c59a 100644 --- a/crates/cxx-qt-gen/test_outputs/inheritance.rs +++ b/crates/cxx-qt-gen/test_outputs/inheritance.rs @@ -144,6 +144,7 @@ mod cxx_qt_inheritance { false } } + impl cxx_qt::Locking for MyObjectQt {} impl cxx_qt::CxxQtType for MyObjectQt { type Rust = MyObject; fn rust(&self) -> &Self::Rust { diff --git a/crates/cxx-qt-gen/test_outputs/invokables.rs b/crates/cxx-qt-gen/test_outputs/invokables.rs index 2333bcbe2..28ae7222a 100644 --- a/crates/cxx-qt-gen/test_outputs/invokables.rs +++ b/crates/cxx-qt-gen/test_outputs/invokables.rs @@ -289,6 +289,7 @@ mod cxx_qt_ffi { pub struct MyObjectCxxQtThreadQueuedFn { inner: std::boxed::Box) + Send>, } + impl cxx_qt::Locking for MyObjectQt {} impl cxx_qt::CxxQtType for MyObjectQt { type Rust = MyObject; fn rust(&self) -> &Self::Rust { diff --git a/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.cpp b/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.cpp index b85f43884..89b0b1340 100644 --- a/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.cpp +++ b/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.cpp @@ -80,7 +80,6 @@ namespace cxx_qt::multi_object { SecondObject::SecondObject(QObject* parent) : QObject(parent) , m_rustObj(cxx_qt::multi_object::cxx_qt_second_object::createRs()) - , m_rustObjMutex(::std::make_shared<::std::recursive_mutex>()) { } @@ -101,21 +100,21 @@ SecondObject::unsafeRustMut() ::std::int32_t const& SecondObject::getPropertyName() const { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + return m_rustObj->getPropertyName(*this); } void SecondObject::setPropertyName(::std::int32_t const& value) { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + m_rustObj->setPropertyName(*this, value); } void SecondObject::invokableName() { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); + m_rustObj->invokableNameWrapper(*this); } @@ -133,10 +132,7 @@ SecondObject::readyConnect(::rust::Fn func, this, &SecondObject::ready, this, - [&, func = ::std::move(func)]() { - const ::std::lock_guard<::std::recursive_mutex> guard(*m_rustObjMutex); - func(*this); - }, + [&, func = ::std::move(func)]() { func(*this); }, type); } diff --git a/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.h b/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.h index 8eb177b0b..894c61d35 100644 --- a/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.h +++ b/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.h @@ -84,7 +84,6 @@ class SecondObject : public QObject private: ::rust::Box m_rustObj; - ::std::shared_ptr<::std::recursive_mutex> m_rustObjMutex; }; static_assert(::std::is_base_of::value, diff --git a/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.rs b/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.rs index fac65de9d..dea2d8ae6 100644 --- a/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.rs +++ b/crates/cxx-qt-gen/test_outputs/passthrough_and_naming.rs @@ -331,6 +331,7 @@ mod cxx_qt_ffi { } } } + impl cxx_qt::Locking for MyObjectQt {} impl cxx_qt::CxxQtType for MyObjectQt { type Rust = MyObject; fn rust(&self) -> &Self::Rust { diff --git a/crates/cxx-qt-gen/test_outputs/properties.rs b/crates/cxx-qt-gen/test_outputs/properties.rs index 9be7881c8..9dd8c6731 100644 --- a/crates/cxx-qt-gen/test_outputs/properties.rs +++ b/crates/cxx-qt-gen/test_outputs/properties.rs @@ -247,6 +247,7 @@ mod cxx_qt_ffi { } } } + impl cxx_qt::Locking for MyObjectQt {} impl cxx_qt::CxxQtType for MyObjectQt { type Rust = MyObject; fn rust(&self) -> &Self::Rust { diff --git a/crates/cxx-qt-gen/test_outputs/signals.rs b/crates/cxx-qt-gen/test_outputs/signals.rs index a6ce9bc09..d45c4653f 100644 --- a/crates/cxx-qt-gen/test_outputs/signals.rs +++ b/crates/cxx-qt-gen/test_outputs/signals.rs @@ -259,6 +259,7 @@ mod cxx_qt_ffi { } } } + impl cxx_qt::Locking for MyObjectQt {} impl cxx_qt::CxxQtType for MyObjectQt { type Rust = MyObject; fn rust(&self) -> &Self::Rust { diff --git a/crates/cxx-qt/src/lib.rs b/crates/cxx-qt/src/lib.rs index 99d316379..f1bc2bfe5 100644 --- a/crates/cxx-qt/src/lib.rs +++ b/crates/cxx-qt/src/lib.rs @@ -31,3 +31,22 @@ pub trait CxxQtType { /// The property changed signal must be emitted manually. unsafe fn rust_mut(self: core::pin::Pin<&mut Self>) -> core::pin::Pin<&mut Self::Rust>; } + +/// Types which implement the `Locking` trait are guarded from concurrent access in C++ (the default in CxxQt). +/// +/// # Safety +/// +/// This is a marker trait used to disable locking. +/// +/// By default, CxxQt will guard all access to the generated QObject with a recursive mutex. +/// For performance reasons it may be desirable to disable this behavior for certain QObjects. +/// You can do so by negative implementing this trait `unsafe impl !cxx_qt::Locking for qobject::T {}`. +/// +/// However, this is unsafe, as it may lead to concurrent mutable access to the QObject from C++. +/// You are responsible for ensuring this does not happen! +// +// This could be implemented using an auto trait in the future once stable +// https://doc.rust-lang.org/beta/unstable-book/language-features/auto-traits.html +pub trait Locking { + // empty +} diff --git a/tests/basic_cxx_qt/cpp/main.cpp b/tests/basic_cxx_qt/cpp/main.cpp index 04b8b5bfc..6fb4eb2c3 100644 --- a/tests/basic_cxx_qt/cpp/main.cpp +++ b/tests/basic_cxx_qt/cpp/main.cpp @@ -5,15 +5,34 @@ // SPDX-FileContributor: Gerhard de Clercq // // SPDX-License-Identifier: MIT OR Apache-2.0 +#include #include #include #include #include "cxx-qt-gen/empty.cxxqt.h" +#include "cxx-qt-gen/locking.cxxqt.h" #include "cxx-qt-gen/my_data.cxxqt.h" #include "cxx-qt-gen/my_object.cxxqt.h" #include "cxx-qt-gen/my_types.cxxqt.h" +class LockingWorkerThread : public QThread +{ + Q_OBJECT + +public: + LockingWorkerThread(std::function lambda, QObject* parent = nullptr) + : QThread(parent) + , m_lambda(lambda) + { + } + + void run() override { m_lambda(); } + +private: + std::function m_lambda; +}; + class CxxQtTest : public QObject { Q_OBJECT @@ -90,6 +109,54 @@ private Q_SLOTS: QStringLiteral("{\"number\":16,\"string\":\"Hello\"}")); } + // Ensure that locking can be disabled + void test_locking_disabled() + { + RustLockingDisabled lockingDisabled; + QCOMPARE(lockingDisabled.getCounter(), 0); + + QVector threads; + for (int i = 0; i < 10; i++) { + threads.push_back(new LockingWorkerThread( + [&lockingDisabled]() { lockingDisabled.increment(); }, this)); + } + + for (auto& thread : threads) { + thread->start(); + } + + for (auto& thread : threads) { + thread->wait(); + } + + // We should expect some increments but not all + QVERIFY(lockingDisabled.getCounter() > 0); + QVERIFY(lockingDisabled.getCounter() < 10); + } + + // Ensure that locking works when enabled + void test_locking_enabled() + { + RustLockingEnabled lockingEnabled; + QCOMPARE(lockingEnabled.getCounter(), 0); + + QVector threads; + for (int i = 0; i < 10; i++) { + threads.push_back(new LockingWorkerThread( + [&lockingEnabled]() { lockingEnabled.increment(); }, this)); + } + + for (auto& thread : threads) { + thread->start(); + } + + for (auto& thread : threads) { + thread->wait(); + } + + QCOMPARE(lockingEnabled.getCounter(), 10); + } + // CXX-Qt allows Rust code to queue a request void test_queue_request() { diff --git a/tests/basic_cxx_qt/rust/build.rs b/tests/basic_cxx_qt/rust/build.rs index 5fd2bad05..74764048e 100644 --- a/tests/basic_cxx_qt/rust/build.rs +++ b/tests/basic_cxx_qt/rust/build.rs @@ -10,6 +10,7 @@ fn main() { .file("src/empty.rs") .file("src/data.rs") .file("src/lib.rs") + .file("src/locking.rs") .file("src/types.rs") .build(); } diff --git a/tests/basic_cxx_qt/rust/src/lib.rs b/tests/basic_cxx_qt/rust/src/lib.rs index b18b6f78a..22afbb90b 100644 --- a/tests/basic_cxx_qt/rust/src/lib.rs +++ b/tests/basic_cxx_qt/rust/src/lib.rs @@ -6,6 +6,7 @@ mod data; mod empty; +mod locking; mod types; #[cxx_qt::bridge(cxx_file_stem = "my_object", namespace = "cxx_qt::my_object")] diff --git a/tests/basic_cxx_qt/rust/src/locking.rs b/tests/basic_cxx_qt/rust/src/locking.rs new file mode 100644 index 000000000..2ffa72bc8 --- /dev/null +++ b/tests/basic_cxx_qt/rust/src/locking.rs @@ -0,0 +1,58 @@ +// SPDX-FileCopyrightText: 2023 Klarälvdalens Datakonsult AB, a KDAB Group company +// SPDX-FileContributor: Andrew Hayzen +// +// SPDX-License-Identifier: MIT OR Apache-2.0 + +/// Two QObject that allow for testing that locking works +#[cxx_qt::bridge(cxx_file_stem = "locking")] +pub mod ffi { + use std::{ + sync::atomic::{AtomicU32, Ordering}, + thread, + time::Duration, + }; + + /// A QObject which has cxx_qt::Locking + #[cxx_qt::qobject] + #[derive(Default)] + pub struct RustLockingEnabled { + counter: AtomicU32, + } + + impl qobject::RustLockingEnabled { + #[qinvokable] + pub fn get_counter(&self) -> u32 { + self.counter().load(Ordering::Acquire) + } + + #[qinvokable] + pub fn increment(self: Pin<&mut Self>) { + let counter = self.as_ref().get_counter(); + thread::sleep(Duration::from_millis(100)); + self.counter().store(counter + 1, Ordering::Release); + } + } + + /// A QObject which has !cxx_qt::Locking + #[cxx_qt::qobject] + #[derive(Default)] + pub struct RustLockingDisabled { + counter: AtomicU32, + } + + unsafe impl !cxx_qt::Locking for qobject::RustLockingDisabled {} + + impl qobject::RustLockingDisabled { + #[qinvokable] + pub fn get_counter(&self) -> u32 { + self.counter().load(Ordering::Acquire) + } + + #[qinvokable] + pub fn increment(self: Pin<&mut Self>) { + let counter = self.as_ref().get_counter(); + thread::sleep(Duration::from_millis(100)); + self.counter().store(counter + 1, Ordering::Release); + } + } +}