From 86d00e56a431192410a0ba19936c745f8040962a Mon Sep 17 00:00:00 2001 From: BenFordTytherington <110855418+BenFordTytherington@users.noreply.github.com> Date: Thu, 5 Sep 2024 16:24:01 +0100 Subject: [PATCH] cxx-qt-gen: refactor MethodFields * Introduce method_fields to 3 structs * Update mocking functions and clean up imports * Simplify generation for signals using the improved parse function * Move common method field into MethodFields, refactoring parse functions * Add comment to ParsedMethod to indicate why it doesn't include a docs field like the other 2 method types --- .../src/generator/cpp/property/signal.rs | 16 +--- crates/cxx-qt-gen/src/generator/cpp/signal.rs | 10 +- .../src/generator/naming/property.rs | 2 +- .../src/generator/naming/signals.rs | 4 +- .../src/generator/rust/property/signal.rs | 16 +--- .../cxx-qt-gen/src/generator/rust/signals.rs | 20 ++-- crates/cxx-qt-gen/src/parser/inherit.rs | 50 ++++------ crates/cxx-qt-gen/src/parser/method.rs | 81 +++++++---------- crates/cxx-qt-gen/src/parser/signals.rs | 91 +++++-------------- 9 files changed, 102 insertions(+), 188 deletions(-) diff --git a/crates/cxx-qt-gen/src/generator/cpp/property/signal.rs b/crates/cxx-qt-gen/src/generator/cpp/property/signal.rs index c4c134a90..54345aa74 100644 --- a/crates/cxx-qt-gen/src/generator/cpp/property/signal.rs +++ b/crates/cxx-qt-gen/src/generator/cpp/property/signal.rs @@ -6,7 +6,7 @@ use syn::ForeignItemFn; use crate::naming::Name; -use crate::syntax::attribute::attribute_take_path; +use crate::syntax::safety::Safety; use crate::{ generator::naming::property::{NameState, QPropertyNames}, parser::signals::ParsedSignal, @@ -19,23 +19,13 @@ pub fn generate(idents: &QPropertyNames, qobject_name: &Name) -> Option); }; - let mut docs = vec![]; - while let Some(doc) = attribute_take_path(&mut method.attrs, &["doc"]) { - docs.push(doc); - } - - Some(ParsedSignal::from_property_method( - method, - notify.clone(), - qobject_name.rust_unqualified().clone(), - docs, - )) + Some(ParsedSignal::parse(method, Safety::Safe).unwrap()) } else { None } diff --git a/crates/cxx-qt-gen/src/generator/cpp/signal.rs b/crates/cxx-qt-gen/src/generator/cpp/signal.rs index 41db8ae9e..b9bb0c5e6 100644 --- a/crates/cxx-qt-gen/src/generator/cpp/signal.rs +++ b/crates/cxx-qt-gen/src/generator/cpp/signal.rs @@ -224,7 +224,7 @@ mod tests { let method: ForeignItemFn = parse_quote! { fn data_changed(self: Pin<&mut MyObject>, trivial: i32, opaque: UniquePtr); }; - let signal = ParsedSignal::mock_with_method(&method); + let signal = ParsedSignal::mock(&method); let signals = vec![&signal]; let qobject_idents = create_qobjectname(); @@ -304,7 +304,7 @@ mod tests { let method: ForeignItemFn = parse_quote! { fn data_changed(self: Pin<&mut MyObject>, mapped: A); }; - let signal = ParsedSignal::mock_with_method(&method); + let signal = ParsedSignal::mock(&method); let signals = vec![&signal]; let qobject_idents = create_qobjectname(); @@ -384,7 +384,7 @@ mod tests { }; let signal = ParsedSignal { inherit: true, - ..ParsedSignal::mock_with_method(&method) + ..ParsedSignal::mock(&method) }; let signals = vec![&signal]; @@ -459,7 +459,7 @@ mod tests { }; let signal = ParsedSignal { inherit: true, - ..ParsedSignal::mock_with_method(&method) + ..ParsedSignal::mock(&method) }; let mut type_names = TypeNames::default(); @@ -537,7 +537,7 @@ mod tests { }; let signal = ParsedSignal { inherit: true, - ..ParsedSignal::mock_with_method(&method) + ..ParsedSignal::mock(&method) }; let mut type_names = TypeNames::default(); diff --git a/crates/cxx-qt-gen/src/generator/naming/property.rs b/crates/cxx-qt-gen/src/generator/naming/property.rs index 9a7b3b402..e1074951f 100644 --- a/crates/cxx-qt-gen/src/generator/naming/property.rs +++ b/crates/cxx-qt-gen/src/generator/naming/property.rs @@ -11,7 +11,7 @@ use quote::format_ident; use syn::{Ident, Result}; use crate::generator::structuring::StructuredQObject; -use std::ops::Deref; +use core::ops::Deref; #[derive(Debug)] pub enum NameState { diff --git a/crates/cxx-qt-gen/src/generator/naming/signals.rs b/crates/cxx-qt-gen/src/generator/naming/signals.rs index 0b76affbd..e0c9686d9 100644 --- a/crates/cxx-qt-gen/src/generator/naming/signals.rs +++ b/crates/cxx-qt-gen/src/generator/naming/signals.rs @@ -110,7 +110,7 @@ mod tests { let method = parse_quote! { fn data_changed(self: Pin<&mut MyObject>); }; - let qsignal = ParsedSignal::mock_with_method(&method); + let qsignal = ParsedSignal::mock(&method); let names = QSignalNames::from(&qsignal); assert_eq!(names.name.cxx_unqualified(), "dataChanged"); @@ -132,7 +132,7 @@ mod tests { #[cxx_name = "baseName"] fn existing_signal(self: Pin<&mut MyObject>); }; - let qsignal = ParsedSignal::mock_with_method(&method); + let qsignal = ParsedSignal::mock(&method); let names = QSignalNames::from(&qsignal); assert_eq!(names.name.cxx_unqualified(), "baseName"); diff --git a/crates/cxx-qt-gen/src/generator/rust/property/signal.rs b/crates/cxx-qt-gen/src/generator/rust/property/signal.rs index b72f6f74d..3bb538012 100644 --- a/crates/cxx-qt-gen/src/generator/rust/property/signal.rs +++ b/crates/cxx-qt-gen/src/generator/rust/property/signal.rs @@ -5,7 +5,7 @@ use syn::ForeignItemFn; -use crate::syntax::attribute::attribute_take_path; +use crate::syntax::safety::Safety; use crate::{ generator::naming::{ property::{NameState, QPropertyNames}, @@ -22,23 +22,13 @@ pub fn generate(idents: &QPropertyNames, qobject_names: &QObjectNames) -> Option let notify_rust = notify.rust_unqualified(); let notify_cpp_str = notify.cxx_unqualified(); - let mut method: ForeignItemFn = syn::parse_quote! { + let method: ForeignItemFn = syn::parse_quote! { #[doc = "Notify for the Q_PROPERTY"] #[cxx_name = #notify_cpp_str] fn #notify_rust(self: Pin<&mut #cpp_class_rust>); }; - let mut docs = vec![]; - while let Some(doc) = attribute_take_path(&mut method.attrs, &["doc"]) { - docs.push(doc); - } - - Some(ParsedSignal::from_property_method( - method, - notify.clone(), - qobject_names.name.rust_unqualified().clone(), - docs, - )) + Some(ParsedSignal::parse(method, Safety::Safe).unwrap()) } else { None } diff --git a/crates/cxx-qt-gen/src/generator/rust/signals.rs b/crates/cxx-qt-gen/src/generator/rust/signals.rs index a91d36871..e06f59419 100644 --- a/crates/cxx-qt-gen/src/generator/rust/signals.rs +++ b/crates/cxx-qt-gen/src/generator/rust/signals.rs @@ -251,6 +251,7 @@ mod tests { use super::*; use crate::generator::naming::qobject::tests::create_qobjectname; + use crate::parser::method::MethodFields; use crate::tests::assert_tokens_eq; use quote::{format_ident, quote}; use syn::{parse_quote, ForeignItemFn, Item}; @@ -378,7 +379,7 @@ mod tests { let method: ForeignItemFn = parse_quote! { fn ready(self: Pin<&mut MyObject>); }; - let qsignal = ParsedSignal::mock_with_method(&method); + let qsignal = ParsedSignal::mock(&method); let type_names = TypeNames::mock(); @@ -412,7 +413,7 @@ mod tests { let method: ForeignItemFn = parse_quote! { fn data_changed(self: Pin<&mut MyObject>, trivial: i32, opaque: UniquePtr); }; - let qsignal = ParsedSignal::mock_with_method(&method); + let qsignal = ParsedSignal::mock(&method); let qobject_names = create_qobjectname(); let mut type_names = TypeNames::mock(); @@ -553,10 +554,7 @@ mod tests { let method = parse_quote! { unsafe fn unsafe_signal(self: Pin<&mut MyObject>, param: *mut T); }; - let qsignal = ParsedSignal { - safe: false, - ..ParsedSignal::mock_with_method(&method) - }; + let qsignal = ParsedSignal::mock(&method); let qobject_names = create_qobjectname(); let mut type_names = TypeNames::mock(); @@ -700,7 +698,7 @@ mod tests { }; let qsignal = ParsedSignal { inherit: true, - ..ParsedSignal::mock_with_method(&method) + ..ParsedSignal::mock(&method) }; let qobject_names = create_qobjectname(); @@ -838,10 +836,14 @@ mod tests { let method: ForeignItemFn = parse_quote! { fn ready(self: Pin<&mut MyObject>); }; + let mock = ParsedSignal::mock(&method); let qsignal = ParsedSignal { - name: Name::new(format_ident!("ready")), + method_fields: MethodFields { + name: Name::new(format_ident!("ready")), + ..mock.method_fields + }, private: true, - ..ParsedSignal::mock_with_method(&method) + ..mock }; let type_names = TypeNames::mock(); diff --git a/crates/cxx-qt-gen/src/parser/inherit.rs b/crates/cxx-qt-gen/src/parser/inherit.rs index 989199d07..29d643205 100644 --- a/crates/cxx-qt-gen/src/parser/inherit.rs +++ b/crates/cxx-qt-gen/src/parser/inherit.rs @@ -5,28 +5,15 @@ use crate::parser::method::MethodFields; use crate::parser::{check_safety, separate_docs}; -use crate::{ - naming::Name, - parser::parameter::ParsedFunctionParameter, - syntax::{attribute::attribute_take_path, safety::Safety}, -}; +use crate::syntax::{attribute::attribute_take_path, safety::Safety}; +use core::ops::Deref; use quote::format_ident; use syn::{Attribute, ForeignItemFn, Ident, Result}; /// Describes a method found in an extern "RustQt" with #[inherit] pub struct ParsedInheritedMethod { - /// The original [syn::ForeignItemFn] of the inherited method declaration - pub method: ForeignItemFn, - /// The type of the self argument - pub qobject_ident: Ident, - /// whether the inherited method is marked as mutable - pub mutable: bool, - /// Whether the method is safe to call. - pub safe: bool, - /// the parameters of the method, without the `self` argument - pub parameters: Vec, - /// the name of the function in Rust, as well as C++ - pub name: Name, + /// The common fields which are available on all callable types + pub method_fields: MethodFields, /// All the docs (each line) of the inherited method pub docs: Vec, } @@ -36,24 +23,15 @@ impl ParsedInheritedMethod { check_safety(&method, &safety)?; let docs = separate_docs(&mut method); - let invokable_fields = MethodFields::parse(&method, docs)?; + let mut fields = MethodFields::parse(method)?; // This block seems unnecessary but since attrs are passed through on generator/rust/inherit.rs a duplicate attr would occur without it - attribute_take_path(&mut method.attrs, &["cxx_name"]); + attribute_take_path(&mut fields.method.attrs, &["cxx_name"]); - Ok(Self::from_invokable_fields(invokable_fields, method)) - } - - fn from_invokable_fields(fields: MethodFields, method: ForeignItemFn) -> Self { - Self { - method, - qobject_ident: fields.qobject_ident, - mutable: fields.mutable, - safe: fields.safe, - parameters: fields.parameters, - name: fields.name, - docs: fields.docs, - } + Ok(Self { + method_fields: fields, + docs, + }) } /// the name of the wrapper function in C++ @@ -62,6 +40,14 @@ impl ParsedInheritedMethod { } } +impl Deref for ParsedInheritedMethod { + type Target = MethodFields; + + fn deref(&self) -> &Self::Target { + &self.method_fields + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/cxx-qt-gen/src/parser/method.rs b/crates/cxx-qt-gen/src/parser/method.rs index c47b849a6..47a829786 100644 --- a/crates/cxx-qt-gen/src/parser/method.rs +++ b/crates/cxx-qt-gen/src/parser/method.rs @@ -8,10 +8,11 @@ use crate::{ parser::parameter::ParsedFunctionParameter, syntax::{attribute::attribute_take_path, safety::Safety}, }; +use core::ops::Deref; use std::collections::HashSet; -use syn::{Attribute, Error, ForeignItemFn, Ident, Result}; +use syn::{Error, ForeignItemFn, Ident, Result}; -use crate::parser::{check_safety, separate_docs}; +use crate::parser::check_safety; use crate::syntax::{foreignmod, types}; /// Describes a C++ specifier for the Q_INVOKABLE @@ -34,22 +35,14 @@ impl ParsedQInvokableSpecifiers { /// Describes a single method (which could be a Q_INVOKABLE) for a struct pub struct ParsedMethod { - /// The original [syn::ImplItemFn] of the invokable - pub method: ForeignItemFn, - /// The type of the self argument - pub qobject_ident: Ident, - /// Whether this invokable is mutable - pub mutable: bool, - /// Whether the method is safe to call. - pub safe: bool, - /// The parameters of the invokable - pub parameters: Vec, + /// The common fields which are available on all callable types + pub method_fields: MethodFields, /// Any specifiers that declared on the invokable pub specifiers: HashSet, /// Whether the method is qinvokable pub is_qinvokable: bool, - /// The rust and cxx name of the function - pub name: Name, + // No docs field since the docs should be on the method implementation outside the bridge + // This means any docs on the bridge declaration would be ignored } impl ParsedMethod { @@ -64,7 +57,10 @@ impl ParsedMethod { #[cfg(test)] pub fn make_mutable(self) -> Self { Self { - mutable: true, + method_fields: MethodFields { + mutable: true, + ..self.method_fields + }, ..self } } @@ -72,7 +68,10 @@ impl ParsedMethod { #[cfg(test)] pub fn make_unsafe(self) -> Self { Self { - safe: false, + method_fields: MethodFields { + safe: false, + ..self.method_fields + }, ..self } } @@ -82,21 +81,21 @@ impl ParsedMethod { Self { specifiers, ..self } } - pub fn parse(mut method: ForeignItemFn, safety: Safety) -> Result { + pub fn parse(method: ForeignItemFn, safety: Safety) -> Result { check_safety(&method, &safety)?; - let docs = separate_docs(&mut method); - let invokable_fields = MethodFields::parse(&method, docs)?; + let mut fields = MethodFields::parse(method)?; - if invokable_fields.name.namespace().is_some() { + if fields.name.namespace().is_some() { return Err(Error::new_spanned( - method.sig.ident, + fields.method.sig.ident, "Methods / QInvokables cannot have a namespace attribute", )); } // Determine if the method is invokable - let is_qinvokable = attribute_take_path(&mut method.attrs, &["qinvokable"]).is_some(); + let is_qinvokable = + attribute_take_path(&mut fields.method.attrs, &["qinvokable"]).is_some(); // Parse any C++ specifiers let mut specifiers = HashSet::new(); @@ -105,51 +104,41 @@ impl ParsedMethod { ParsedQInvokableSpecifiers::Override, ParsedQInvokableSpecifiers::Virtual, ] { - if attribute_take_path(&mut method.attrs, specifier.as_str_slice()).is_some() { + if attribute_take_path(&mut fields.method.attrs, specifier.as_str_slice()).is_some() { specifiers.insert(specifier); // Should a fn be able to be Override AND Virtual? } } - Ok(ParsedMethod::from_invokable_fields( - invokable_fields, - method, + Ok(Self { + method_fields: fields, specifiers, is_qinvokable, - )) + }) } +} - fn from_invokable_fields( - fields: MethodFields, - method: ForeignItemFn, - specifiers: HashSet, - is_qinvokable: bool, - ) -> Self { - Self { - method, - qobject_ident: fields.qobject_ident, - mutable: fields.mutable, - safe: fields.safe, - parameters: fields.parameters, - specifiers, - is_qinvokable, - name: fields.name, - } +impl Deref for ParsedMethod { + type Target = MethodFields; + + fn deref(&self) -> &Self::Target { + &self.method_fields } } /// Struct with common fields between Invokable types. /// These types are ParsedSignal, ParsedMethod and ParsedInheritedMethod +#[derive(Clone)] pub struct MethodFields { + pub method: ForeignItemFn, pub qobject_ident: Ident, pub mutable: bool, pub parameters: Vec, pub safe: bool, pub name: Name, - pub docs: Vec, // TODO: Remove this } impl MethodFields { - pub fn parse(method: &ForeignItemFn, docs: Vec) -> Result { + pub fn parse(method: ForeignItemFn) -> Result { let self_receiver = foreignmod::self_type_from_foreign_fn(&method.sig)?; let (qobject_ident, mutability) = types::extract_qobject_ident(&self_receiver.ty)?; let mutable = mutability.is_some(); @@ -159,12 +148,12 @@ impl MethodFields { let name = Name::from_rust_ident_and_attrs(&method.sig.ident, &method.attrs, None, None)?; Ok(MethodFields { + method, qobject_ident, mutable, parameters, safe, name, - docs, }) } } diff --git a/crates/cxx-qt-gen/src/parser/signals.rs b/crates/cxx-qt-gen/src/parser/signals.rs index 14f2fc0ad..b13fe90ad 100644 --- a/crates/cxx-qt-gen/src/parser/signals.rs +++ b/crates/cxx-qt-gen/src/parser/signals.rs @@ -3,12 +3,9 @@ // // SPDX-License-Identifier: MIT OR Apache-2.0 -use crate::{ - naming::Name, - parser::parameter::ParsedFunctionParameter, - syntax::{attribute::attribute_take_path, path::path_compare_str, safety::Safety}, -}; -use syn::{spanned::Spanned, Attribute, Error, ForeignItemFn, Ident, Result, Visibility}; +use crate::syntax::{attribute::attribute_take_path, path::path_compare_str, safety::Safety}; +use core::ops::Deref; +use syn::{spanned::Spanned, Attribute, Error, ForeignItemFn, Result, Visibility}; use crate::parser::method::MethodFields; use crate::parser::{check_safety, separate_docs}; @@ -16,18 +13,8 @@ use crate::parser::{check_safety, separate_docs}; #[derive(Clone)] /// Describes an individual Signal pub struct ParsedSignal { - /// The original [syn::ForeignItemFn] of the signal declaration - pub method: ForeignItemFn, - /// The type of the self argument - pub qobject_ident: Ident, - /// whether the signal is marked as mutable - pub mutable: bool, - /// Whether the method is safe to call. - pub safe: bool, - /// The parameters of the signal - pub parameters: Vec, - /// The name of the signal - pub name: Name, + /// The common fields which are available on all callable types + pub method_fields: MethodFields, /// If the signal is defined in the base class pub inherit: bool, /// Whether the signal is private @@ -37,29 +24,9 @@ pub struct ParsedSignal { } impl ParsedSignal { - /// Builds a signal from a given property method - pub fn from_property_method( - method: ForeignItemFn, - name: Name, - qobject_ident: Ident, - docs: Vec, - ) -> Self { - Self { - method, - qobject_ident, - mutable: true, - safe: true, - parameters: vec![], - name, - inherit: false, - private: false, - docs, - } - } - #[cfg(test)] /// Test fn for creating a mocked signal from a method body - pub fn mock_with_method(method: &ForeignItemFn) -> Self { + pub fn mock(method: &ForeignItemFn) -> Self { Self::parse(method.clone(), Safety::Safe).unwrap() } @@ -67,54 +34,43 @@ impl ParsedSignal { check_safety(&method, &safety)?; let docs = separate_docs(&mut method); - let invokable_fields = MethodFields::parse(&method, docs)?; + let mut fields = MethodFields::parse(method)?; - if invokable_fields.name.namespace().is_some() { + if fields.name.namespace().is_some() { return Err(Error::new_spanned( - method.sig.ident, + fields.method.sig.ident, "Signals cannot have a namespace attribute!", )); } - if !invokable_fields.mutable { + if !fields.mutable { return Err(Error::new( - method.span(), + fields.method.span(), "signals must be mutable, use Pin<&mut T> instead of T for the self type", )); } - let inherit = attribute_take_path(&mut method.attrs, &["inherit"]).is_some(); - let private = if let Visibility::Restricted(vis_restricted) = &method.vis { + let inherit = attribute_take_path(&mut fields.method.attrs, &["inherit"]).is_some(); + let private = if let Visibility::Restricted(vis_restricted) = &fields.method.vis { path_compare_str(&vis_restricted.path, &["self"]) } else { false }; - Ok(Self::from_invokable_fields( - invokable_fields, - method, + Ok(Self { + method_fields: fields, inherit, private, - )) + docs, + }) } +} - fn from_invokable_fields( - fields: MethodFields, - method: ForeignItemFn, - inherit: bool, - private: bool, - ) -> Self { - Self { - method, - qobject_ident: fields.qobject_ident, - mutable: fields.mutable, - safe: fields.safe, - parameters: fields.parameters, - name: fields.name, - inherit, - private, - docs: fields.docs, - } +impl Deref for ParsedSignal { + type Target = MethodFields; + + fn deref(&self) -> &Self::Target { + &self.method_fields } } @@ -124,6 +80,7 @@ mod tests { use super::*; + use crate::naming::Name; use crate::parser::tests::f64_type; use quote::format_ident;