cxx-qt-gen: allow for optional #[qobject] macro

ahayzen-kdab · ahayzen-kdab · commit b0a970085349 · 2024-03-01T16:09:52.000Z
This allows for using CXX-Qt helpers with normal C++ classes, eg #[inherit] and #[cxx_override] etc. Closes #824
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `cxx-qt-gen` now does not generate code requiring `cxx-qt-lib`, this allows for `cxx-qt-lib` to be optional
 - `cxx-qt-lib` headers must be given to `cxx-qt-build` with `.with_opts(cxx_qt_lib_headers::build_opts())`
 - File name is used for CXX bridges rather than module name to match upstream
+- `#[qobject]` attribute is now optional on types in `extern "RustQt"`
 
 ### Fixed
 
diff --git a/crates/cxx-qt-gen/src/generator/cpp/constructor.rs b/crates/cxx-qt-gen/src/generator/cpp/constructor.rs
@@ -17,8 +17,8 @@ fn default_constructor(
     base_class: String,
     initializers: String,
 ) -> GeneratedCppQObjectBlocks {
-    GeneratedCppQObjectBlocks {
-        methods: vec![CppFragment::Pair {
+    let constructor = if qobject.has_qobject_macro {
+        CppFragment::Pair {
             header: format!(
                 "explicit {class_name}(QObject* parent = nullptr);",
                 class_name = qobject.ident
@@ -34,7 +34,33 @@ fn default_constructor(
                 namespace_internals = qobject.namespace_internals,
                 rust_obj = qobject.rust_ident,
             ),
-        }],
+        }
+    } else {
+        CppFragment::Pair {
+            header: format!("explicit {class_name}();", class_name = qobject.ident),
+            source: formatdoc!(
+                r#"
+            {class_name}::{class_name}()
+              {base_class_line}
+              , ::rust::cxxqt1::CxxQtType<{rust_obj}>(::{namespace_internals}::createRs()){initializers}
+            {{ }}
+            "#,
+                base_class_line = if base_class.is_empty() {
+                    unreachable!(
+                        "Cannot have an empty #[base] attribute  with no #[qobject] attribute"
+                    );
+                } else {
+                    format!(": {base_class}()")
+                },
+                class_name = qobject.ident,
+                namespace_internals = qobject.namespace_internals,
+                rust_obj = qobject.rust_ident,
+            ),
+        }
+    };
+
+    GeneratedCppQObjectBlocks {
+        methods: vec![constructor],
         ..Default::default()
     }
 }
@@ -143,6 +169,7 @@ mod tests {
             namespace: "".to_string(),
             namespace_internals: "rust".to_string(),
             blocks: GeneratedCppQObjectBlocks::default(),
+            has_qobject_macro: true,
         }
     }
 
@@ -222,6 +249,37 @@ mod tests {
         );
     }
 
+    #[test]
+    fn default_constructor_no_qobject_macro() {
+        let mut qobject = qobject_for_testing();
+        qobject.has_qobject_macro = false;
+        let blocks = generate(
+            &qobject,
+            &[],
+            "BaseClass".to_owned(),
+            &[],
+            &TypeNames::default(),
+        )
+        .unwrap();
+
+        assert_empty_blocks(&blocks);
+        assert!(blocks.private_methods.is_empty());
+        assert_eq!(
+            blocks.methods,
+            vec![CppFragment::Pair {
+                header: "explicit MyObject();".to_string(),
+                source: formatdoc!(
+                    "
+                    MyObject::MyObject()
+                      : BaseClass()
+                      , ::rust::cxxqt1::CxxQtType<MyObjectRust>(::rust::createRs())
+                    {{ }}
+                    "
+                ),
+            }]
+        );
+    }
+
     #[test]
     fn constructor_without_base_arguments() {
         let blocks = generate(
diff --git a/crates/cxx-qt-gen/src/generator/cpp/inherit.rs b/crates/cxx-qt-gen/src/generator/cpp/inherit.rs
@@ -22,6 +22,9 @@ pub fn generate(
 
     for method in inherited_methods {
         let return_type = syn_type_to_cpp_return_type(&method.method.sig.output, type_names)?;
+        // Note that no qobject macro with no base class is an error
+        //
+        // So a default of QObject is fine here
         let base_class = base_class.as_deref().unwrap_or("QObject");
 
         result.methods.push(CppFragment::Header(formatdoc! {
diff --git a/crates/cxx-qt-gen/src/generator/cpp/qobject.rs b/crates/cxx-qt-gen/src/generator/cpp/qobject.rs
@@ -92,6 +92,8 @@ pub struct GeneratedCppQObject {
     pub namespace_internals: String,
     /// The blocks of the QObject
     pub blocks: GeneratedCppQObjectBlocks,
+    /// Whether this type has a #[qobject] / Q_OBJECT macro
+    pub has_qobject_macro: bool,
 }
 
 impl GeneratedCppQObject {
@@ -106,6 +108,7 @@ impl GeneratedCppQObject {
             namespace: qobject.namespace.clone(),
             namespace_internals: namespace_idents.internal,
             blocks: GeneratedCppQObjectBlocks::from(qobject),
+            has_qobject_macro: qobject.has_qobject_macro,
         };
 
         // Ensure that we include MaybeLockGuard<T> that is used in multiple places
@@ -115,10 +118,16 @@ impl GeneratedCppQObject {
             .insert("#include <cxx-qt/maybelockguard.h>".to_owned());
 
         // Build the base class
-        let base_class = qobject
-            .base_class
-            .clone()
-            .unwrap_or_else(|| "QObject".to_string());
+        let base_class = qobject.base_class.clone().unwrap_or_else(|| {
+            // If there is a QObject macro then assume the base class is QObject
+            if qobject.has_qobject_macro {
+                "QObject".to_string()
+            } else {
+                unreachable!(
+                    "Cannot have an empty #[base] attribute  with no #[qobject] attribute"
+                );
+            }
+        });
         generated.blocks.base_classes.push(base_class.clone());
 
         // Add the CxxQtType rust and rust_mut methods
diff --git a/crates/cxx-qt-gen/src/parser/cxxqtdata.rs b/crates/cxx-qt-gen/src/parser/cxxqtdata.rs
@@ -80,27 +80,44 @@ impl ParsedCxxQtData {
                                     syn::parse2(tokens.clone())?;
 
                                 // Check this type is tagged with a #[qobject]
-                                if attribute_take_path(&mut foreign_alias.attrs, &["qobject"])
-                                    .is_some()
+                                let has_qobject_macro =
+                                    attribute_take_path(&mut foreign_alias.attrs, &["qobject"])
+                                        .is_some();
+
+                                // Load the QObject
+                                let mut qobject = ParsedQObject::try_from(&foreign_alias)?;
+                                qobject.has_qobject_macro = has_qobject_macro;
+
+                                // Ensure that the base class attribute is not empty, as this is not valid in both cases
+                                // - when there is a qobject macro it is not valid
+                                // - when there is not a qobject macro it is not valid
+                                if qobject
+                                    .base_class
+                                    .as_ref()
+                                    .is_some_and(|base| base.is_empty())
                                 {
-                                    // Load the QObject
-                                    let mut qobject = ParsedQObject::try_from(&foreign_alias)?;
-
-                                    // Inject the bridge namespace if the qobject one is empty
-                                    if qobject.namespace.is_empty() && namespace.is_some() {
-                                        qobject.namespace = namespace.clone().unwrap();
-                                    }
-
-                                    // Note that we assume a compiler error will occur later
-                                    // if you had two structs with the same name
-                                    self.qobjects
-                                        .insert(foreign_alias.ident_left.clone(), qobject);
-                                } else {
                                     return Err(Error::new(
                                         foreign_item.span(),
-                                        "type A = super::B must be tagged with #[qobject]",
+                                        "The #[base] attribute cannot be empty",
                                     ));
                                 }
+
+                                // Ensure that if there is no qobject macro that a base class is specificed
+                                //
+                                // Note this assumes the check above
+                                if !qobject.has_qobject_macro && qobject.base_class.is_none() {
+                                    return Err(Error::new(foreign_item.span(), "A type without a #[qobject] attribute must specify a #[base] attribute"));
+                                }
+
+                                // Inject the bridge namespace if the qobject one is empty
+                                if qobject.namespace.is_empty() && namespace.is_some() {
+                                    qobject.namespace = namespace.clone().unwrap();
+                                }
+
+                                // Note that we assume a compiler error will occur later
+                                // if you had two structs with the same name
+                                self.qobjects
+                                    .insert(foreign_alias.ident_left.clone(), qobject);
                             }
                             // Const Macro, Type are unsupported in extern "RustQt" for now
                             _others => {
@@ -316,6 +333,13 @@ mod tests {
         assert!(result.is_ok());
         assert_eq!(cxx_qt_data.qobjects.len(), 1);
         assert!(cxx_qt_data.qobjects.contains_key(&qobject_ident()));
+        assert!(
+            cxx_qt_data
+                .qobjects
+                .get(&qobject_ident())
+                .unwrap()
+                .has_qobject_macro
+        );
     }
 
     #[test]
@@ -380,7 +404,7 @@ mod tests {
     }
 
     #[test]
-    fn test_find_qobjects_no_qobject() {
+    fn test_find_qobjects_no_qobject_no_base() {
         let mut cxx_qt_data = ParsedCxxQtData::new(format_ident!("ffi"), None);
 
         let module: ItemMod = parse_quote! {
@@ -395,6 +419,39 @@ mod tests {
         assert!(result.is_err());
     }
 
+    #[test]
+    fn test_find_qobjects_no_qobject_with_base() {
+        let mut cxx_qt_data = ParsedCxxQtData::new(format_ident!("ffi"), None);
+
+        let module: ItemMod = parse_quote! {
+            mod module {
+                extern "RustQt" {
+                    #[base = "OtherBase"]
+                    type Other = super::OtherRust;
+                    #[base = "MyObjectBase"]
+                    type MyObject = super::MyObjectRust;
+                }
+            }
+        };
+        let result = cxx_qt_data.find_qobject_types(&module.content.unwrap().1);
+        assert!(result.is_ok());
+        assert_eq!(cxx_qt_data.qobjects.len(), 2);
+        assert!(
+            !cxx_qt_data
+                .qobjects
+                .get(&format_ident!("Other"))
+                .unwrap()
+                .has_qobject_macro
+        );
+        assert!(
+            !cxx_qt_data
+                .qobjects
+                .get(&format_ident!("MyObject"))
+                .unwrap()
+                .has_qobject_macro
+        );
+    }
+
     #[test]
     fn test_find_and_merge_cxx_qt_item_struct_qobject_passthrough() {
         let mut cxx_qt_data = create_parsed_cxx_qt_data();
diff --git a/crates/cxx-qt-gen/src/parser/qobject.rs b/crates/cxx-qt-gen/src/parser/qobject.rs
@@ -57,6 +57,8 @@ pub struct ParsedQObject {
     pub locking: bool,
     /// Whether threading has been enabled for this QObject
     pub threading: bool,
+    /// Whether this type has a #[qobject] / Q_OBJECT macro
+    pub has_qobject_macro: bool,
 }
 
 impl TryFrom<&ForeignTypeIdentAlias> for ParsedQObject {
@@ -97,6 +99,7 @@ impl TryFrom<&ForeignTypeIdentAlias> for ParsedQObject {
             qml_metadata,
             locking: true,
             threading: false,
+            has_qobject_macro: false,
         })
     }
 }
diff --git a/crates/cxx-qt-gen/src/writer/cpp/header.rs b/crates/cxx-qt-gen/src/writer/cpp/header.rs
@@ -76,12 +76,23 @@ fn forward_declare(generated: &GeneratedCppBlocks) -> Vec<String> {
 /// For a given GeneratedCppBlocks write the classes
 fn qobjects_header(generated: &GeneratedCppBlocks) -> Vec<String> {
     generated.qobjects.iter().map(|qobject| {
+        let ident = &qobject.ident;
+        let qobject_macro = if qobject.has_qobject_macro {
+            "Q_OBJECT"
+        } else {
+            ""
+        };
+        let qobject_assert = if qobject.has_qobject_macro {
+            format!("static_assert(::std::is_base_of<QObject, {ident}>::value, \"{ident} must inherit from QObject\");")
+        } else {
+            "".to_owned()
+        };
         let class_definition = namespaced(
             &qobject.namespace,
             &formatdoc! { r#"
                 class {ident} : {base_classes}
                 {{
-                  Q_OBJECT
+                  {qobject_macro}
                 public:
                   {metaobjects}
 
@@ -91,8 +102,8 @@ fn qobjects_header(generated: &GeneratedCppBlocks) -> Vec<String> {
                 {private_methods}
                 }};
 
-                static_assert(::std::is_base_of<QObject, {ident}>::value, "{ident} must inherit from QObject");"#,
-            ident = qobject.ident,
+                {qobject_assert}"#,
+            // Note that there is always a base class as we always have CxxQtType
             base_classes = qobject.blocks.base_classes.iter().map(|base| format!("public {}", base)).collect::<Vec<String>>().join(", "),
             metaobjects = qobject.blocks.metaobjects.join("\n  "),
             public_methods = create_block("public", &qobject.blocks.methods.iter().filter_map(pair_as_header).collect::<Vec<String>>()),
@@ -107,17 +118,24 @@ fn qobjects_header(generated: &GeneratedCppBlocks) -> Vec<String> {
             .collect::<Vec<String>>()
             .join("\n");
 
+        let declare_metatype = if qobject.has_qobject_macro {
+            let ty = if qobject.namespace.is_empty() {
+                qobject.ident.clone()
+            } else {
+                format!("{namespace}::{ident}", namespace = qobject.namespace, ident = qobject.ident)
+            };
+            format!("Q_DECLARE_METATYPE({ty}*)")
+        } else {
+            "".to_owned()
+        };
+
         formatdoc! {r#"
             {fragments}
             {class_definition}
 
-            Q_DECLARE_METATYPE({metatype}*)
-            "#,
-        metatype = if qobject.namespace.is_empty() {
-            qobject.ident.clone()
-        } else {
-            format!("{namespace}::{ident}", namespace = qobject.namespace, ident = qobject.ident)
-        },}
+            {declare_metatype}
+            "#
+        }
     }).collect::<Vec<String>>()
 }
 
diff --git a/crates/cxx-qt-gen/src/writer/cpp/mod.rs b/crates/cxx-qt-gen/src/writer/cpp/mod.rs
@@ -62,6 +62,7 @@ mod tests {
                     rust_ident: "MyObjectRust".to_owned(),
                     namespace: "cxx_qt::my_object".to_owned(),
                     namespace_internals: "cxx_qt::my_object::cxx_qt_my_object".to_owned(),
+                    has_qobject_macro: true,
                     blocks: GeneratedCppQObjectBlocks {
                         base_classes: vec!["QStringListModel".to_owned()],
                         includes: {
@@ -185,6 +186,7 @@ mod tests {
                     rust_ident: "FirstObjectRust".to_owned(),
                     namespace: "cxx_qt".to_owned(),
                     namespace_internals: "cxx_qt::cxx_qt_first_object".to_owned(),
+                    has_qobject_macro: true,
                     blocks: GeneratedCppQObjectBlocks {
                         base_classes: vec!["QStringListModel".to_owned()],
                         includes: {
@@ -228,6 +230,7 @@ mod tests {
                     rust_ident: "SecondObjectRust".to_owned(),
                     namespace: "cxx_qt".to_owned(),
                     namespace_internals: "cxx_qt::cxx_qt_second_object".to_owned(),
+                    has_qobject_macro: true,
                     blocks: GeneratedCppQObjectBlocks {
                         base_classes: vec!["QStringListModel".to_owned()],
                         includes: {

Original file line number	Diff line number	Diff line change
`@@ -57,6 +57,8 @@ pub struct ParsedQObject {`
`57`	`57`	`pub locking: bool,`
`58`	`58`	`/// Whether threading has been enabled for this QObject`
`59`	`59`	`pub threading: bool,`
	`60`	`+ /// Whether this type has a #[qobject] / Q_OBJECT macro`
	`61`	`+ pub has_qobject_macro: bool,`
`60`	`62`	`}`
`61`	`63`
`62`	`64`	`impl TryFrom<&ForeignTypeIdentAlias> for ParsedQObject {`
`@@ -97,6 +99,7 @@ impl TryFrom<&ForeignTypeIdentAlias> for ParsedQObject {`
`97`	`99`	`qml_metadata,`
`98`	`100`	`locking: true,`
`99`	`101`	`threading: false,`
	`102`	`+ has_qobject_macro: false,`
`100`	`103`	`})`
`101`	`104`	`}`
`102`	`105`	`}`