TypeNames::cxx_qualified(): Error on onknown type

LeonMatthesKDAB · ahayzen-kdab · commit a877c24cc31b · 2024-03-15T09:34:12.000Z
diff --git a/crates/cxx-qt-gen/src/generator/cpp/constructor.rs b/crates/cxx-qt-gen/src/generator/cpp/constructor.rs
@@ -162,6 +162,12 @@ mod tests {
 
     use syn::parse_quote;
 
+    fn type_names_with_qobject() -> TypeNames {
+        let mut type_names = TypeNames::mock();
+        type_names.insert("QObject", None, None, None);
+        type_names
+    }
+
     fn qobject_for_testing() -> GeneratedCppQObject {
         GeneratedCppQObject {
             ident: "MyObject".to_string(),
@@ -197,7 +203,7 @@ mod tests {
             &[],
             "BaseClass".to_owned(),
             &["member1(1)".to_string(), "member2{ 2 }".to_string()],
-            &TypeNames::default(),
+            &type_names_with_qobject(),
         )
         .unwrap();
 
@@ -227,7 +233,7 @@ mod tests {
             &[],
             "BaseClass".to_owned(),
             &[],
-            &TypeNames::default(),
+            &type_names_with_qobject(),
         )
         .unwrap();
 
@@ -258,7 +264,7 @@ mod tests {
             &[],
             "BaseClass".to_owned(),
             &[],
-            &TypeNames::default(),
+            &type_names_with_qobject(),
         )
         .unwrap();
 
@@ -290,7 +296,7 @@ mod tests {
             }],
             "BaseClass".to_owned(),
             &[],
-            &TypeNames::default(),
+            &type_names_with_qobject(),
         )
         .unwrap();
 
@@ -340,7 +346,7 @@ mod tests {
             }],
             "BaseClass".to_owned(),
             &["initializer".to_string()],
-            &TypeNames::default(),
+            &type_names_with_qobject(),
         )
         .unwrap();
 
@@ -394,7 +400,7 @@ mod tests {
             ],
             "BaseClass".to_owned(),
             &["initializer".to_string()],
-            &TypeNames::default(),
+            &type_names_with_qobject(),
         )
         .unwrap();
 
diff --git a/crates/cxx-qt-gen/src/generator/cpp/method.rs b/crates/cxx-qt-gen/src/generator/cpp/method.rs
@@ -228,8 +228,10 @@ mod tests {
         ];
         let qobject_idents = create_qobjectname();
 
-        let generated =
-            generate_cpp_methods(&invokables, &qobject_idents, &TypeNames::default()).unwrap();
+        let mut type_names = TypeNames::mock();
+        type_names.insert("QColor", None, None, None);
+
+        let generated = generate_cpp_methods(&invokables, &qobject_idents, &type_names).unwrap();
 
         // methods
         assert_eq!(generated.methods.len(), 5);
diff --git a/crates/cxx-qt-gen/src/generator/cpp/property/mod.rs b/crates/cxx-qt-gen/src/generator/cpp/property/mod.rs
@@ -81,8 +81,9 @@ mod tests {
         ];
         let qobject_idents = create_qobjectname();
 
-        let generated =
-            generate_cpp_properties(&properties, &qobject_idents, &TypeNames::mock()).unwrap();
+        let mut type_names = TypeNames::mock();
+        type_names.insert("QColor", None, None, None);
+        let generated = generate_cpp_properties(&properties, &qobject_idents, &type_names).unwrap();
 
         // metaobjects
         assert_eq!(generated.metaobjects.len(), 2);
@@ -356,7 +357,7 @@ mod tests {
     fn test_generate_cpp_properties_mapped_cxx_name() {
         let properties = vec![ParsedQProperty {
             ident: format_ident!("mapped_property"),
-            ty: parse_quote! { A1 },
+            ty: parse_quote! { A },
         }];
         let qobject_idents = create_qobjectname();
 
diff --git a/crates/cxx-qt-gen/src/generator/cpp/qenum.rs b/crates/cxx-qt-gen/src/generator/cpp/qenum.rs
@@ -53,7 +53,7 @@ pub fn generate(
     let mut generated = GeneratedCppQObjectBlocks::default();
 
     for qenum in qenums {
-        let mut qualified_name = type_names.cxx_qualified(&qenum.ident);
+        let mut qualified_name = type_names.cxx_qualified(&qenum.ident)?;
         let enum_name = type_names.cxx_unqualified(&qenum.ident)?;
         // TODO: this is a workaround for type_names.cxx_qualified not always returning a fully-qualified
         // identifier.
diff --git a/crates/cxx-qt-gen/src/generator/cpp/signal.rs b/crates/cxx-qt-gen/src/generator/cpp/signal.rs
@@ -66,7 +66,7 @@ fn parameter_types_and_values(
     let parameter_named_types = parameter_named_types_with_self.join(", ");
 
     // Insert the extra argument into the closure
-    let self_ty = type_names.cxx_qualified(self_ty);
+    let self_ty = type_names.cxx_qualified(self_ty)?;
     parameter_named_types_with_self.insert(0, format!("{self_ty}& self"));
     parameter_types_with_self.insert(0, format!("{self_ty}&"));
     parameter_values_with_self.insert(0, "self".to_owned());
@@ -92,7 +92,7 @@ pub fn generate_cpp_signal(
         .insert("#include <cxx-qt/signalhandler.h>".to_owned());
 
     // Build a namespace that includes any namespace for the T
-    let qobject_ident_namespaced = type_names.cxx_qualified(qobject_ident);
+    let qobject_ident_namespaced = type_names.cxx_qualified(qobject_ident)?;
 
     // Prepare the idents
     let idents = QSignalName::from(signal);
@@ -250,8 +250,9 @@ mod tests {
         }];
         let qobject_idents = create_qobjectname();
 
-        let generated =
-            generate_cpp_signals(&signals, &qobject_idents, &TypeNames::mock()).unwrap();
+        let mut type_names = TypeNames::mock();
+        type_names.insert("QColor", None, None, None);
+        let generated = generate_cpp_signals(&signals, &qobject_idents, &type_names).unwrap();
 
         assert_eq!(generated.methods.len(), 1);
         let header = if let CppFragment::Header(header) = &generated.methods[0] {
@@ -331,13 +332,13 @@ mod tests {
     fn test_generate_cpp_signals_mapped_cxx_name() {
         let signals = vec![ParsedSignal {
             method: parse_quote! {
-                fn data_changed(self: Pin<&mut MyObject>, mapped: A1);
+                fn data_changed(self: Pin<&mut MyObject>, mapped: A);
             },
             qobject_ident: format_ident!("MyObject"),
             mutable: true,
             parameters: vec![ParsedFunctionParameter {
                 ident: format_ident!("mapped"),
-                ty: parse_quote! { A1 },
+                ty: parse_quote! { A },
             }],
             ident: CombinedIdent {
                 cpp: format_ident!("dataChanged"),
diff --git a/crates/cxx-qt-gen/src/naming/cpp.rs b/crates/cxx-qt-gen/src/naming/cpp.rs
@@ -209,7 +209,7 @@ fn path_segment_to_string(segment: &PathSegment, type_names: &TypeNames) -> Resu
     let ident_string = ident.to_string();
 
     // If we are a Pin<T> then for C++ it becomes just T
-    let args = match &*ident_string {
+    let arg = match &*ident_string {
         "Pin" => {
             let mut args =
                 path_argument_to_string(&segment.arguments, type_names)?.unwrap_or_else(Vec::new);
@@ -231,25 +231,27 @@ fn path_segment_to_string(segment: &PathSegment, type_names: &TypeNames) -> Resu
     };
 
     // If there are template args check that its a supported template type.
-    let ident = if args.is_some() {
+    if let Some(arg) = arg {
         // A built in template base cannot have a cxx_name or a namespace
-        if let Some(built_in) = possible_built_in_template_base(&ident_string) {
-            built_in.to_owned()
+        if let Some(ident) = possible_built_in_template_base(&ident_string) {
+            Ok(format!("{ident}<{arg}>"))
         } else {
-            return Err(Error::new_spanned(
+            Err(Error::new_spanned(
                 ident,
                 format!("Unsupported template base: {ident}"),
-            ));
+            ))
         }
     } else {
-        type_names.cxx_qualified(&segment.ident)
-    };
-
-    Ok(format!(
-        "{ident}{args}",
-        ident = ident,
-        args = args.map_or_else(|| "".to_owned(), |arg| format!("<{arg}>"))
-    ))
+        // Currently, type_names only includes the names of types that are declared within the bridge.
+        // Some types are built-in and available without declaration though.
+        // Check whether its one of those, as otherwise the call to `cxx_qualified` will result in
+        // an "unknown type" error.
+        if let Some(built_in) = possible_built_in(&segment.ident.to_string()) {
+            Ok(built_in.to_owned())
+        } else {
+            type_names.cxx_qualified(&segment.ident)
+        }
+    }
 }
 
 /// Convert any built in types to known C++ equivalents
@@ -322,9 +324,14 @@ mod tests {
 
     macro_rules! test_syn_types_to_cpp_types {
         [$($input_type:tt => $output_type:literal),*] => {
+            let mut type_names = TypeNames::default();
+            // Add some types to the list of available types so we can use them in tests.
+            type_names.insert("T", None, None, None);
+            type_names.insert("QColor", None, None, None);
+            type_names.insert("QPoint", None, None, None);
             $(
             assert_eq!(
-                syn_type_to_cpp_type(&parse_quote! $input_type, &TypeNames::default()).unwrap(),
+                syn_type_to_cpp_type(&parse_quote! $input_type, &type_names).unwrap(),
                 $output_type);
             )*
         }
diff --git a/crates/cxx-qt-gen/src/naming/name.rs b/crates/cxx-qt-gen/src/naming/name.rs
@@ -93,4 +93,12 @@ impl Name {
             module: module.clone(),
         })
     }
+
+    /// Get the unqualified name of the type in C++.
+    /// This is either:
+    /// - The cxx_name attribute value, if one is provided
+    /// - The original ident, if no cxx_name was provided
+    pub(super) fn cxx_unqualified(&self) -> String {
+        self.cxx.clone().unwrap_or_else(|| self.rust.to_string())
+    }
 }
diff --git a/crates/cxx-qt-gen/src/naming/type_names.rs b/crates/cxx-qt-gen/src/naming/type_names.rs
@@ -177,6 +177,10 @@ impl TypeNames {
         Ok(())
     }
 
+    fn unknown_type(&self, ident: &Ident) -> syn::Error {
+        syn::Error::new_spanned(ident, format!("Undeclared type: `{ident}`!"))
+    }
+
     /// For a given rust ident return the CXX name with its namespace
     ///
     /// Ideally we'd want this type name to always be **fully** qualified, starting with `::`.
@@ -187,35 +191,28 @@ impl TypeNames {
     /// This needs to be considered in many places (properties, signals, invokables, etc.)
     /// Therefore, for now we'll use the qualified, but not fully qualified version of `namespace::type`.
     /// This should work in most cases, but it's not perfect.
-    pub fn cxx_qualified(&self, ident: &Ident) -> String {
+    pub fn cxx_qualified(&self, ident: &Ident) -> Result<String> {
         // Check if there is a cxx_name or namespace to handle
-        let name = self.names.get(ident);
-
-        if name.is_none() {
-            return ident.to_string();
-        }
-        let name = name.unwrap();
+        let name = self
+            .names
+            .get(ident)
+            .ok_or_else(|| self.unknown_type(ident))?;
 
-        let cxx_name = name.cxx.clone().unwrap_or_else(|| name.rust.to_string());
+        let cxx_name = name.cxx_unqualified();
 
         if let Some(namespace) = &name.namespace {
-            format!("{namespace}::{cxx_name}")
+            Ok(format!("{namespace}::{cxx_name}"))
         } else {
-            cxx_name
+            Ok(cxx_name)
         }
     }
 
-    fn unknown_type(&self, ident: &Ident) -> syn::Error {
-        syn::Error::new_spanned(ident, format!("Undeclared type: `{ident}`!"))
-    }
-
     /// For a given rust ident return the CXX name **without** its namespace
     pub fn cxx_unqualified(&self, ident: &Ident) -> Result<String> {
-        if let Some(name) = self.names.get(ident) {
-            Ok(name.cxx.clone().unwrap_or_else(|| ident.to_string()))
-        } else {
-            Err(self.unknown_type(ident))
-        }
+        self.names
+            .get(ident)
+            .ok_or_else(|| self.unknown_type(ident))
+            .map(Name::cxx_unqualified)
     }
 
     /// For a given rust ident return the namespace if it's not empty
@@ -249,6 +246,7 @@ impl TypeNames {
         module_ident: &Ident,
     ) -> Result<()> {
         let name = Name::from_ident_and_attrs(ident, attrs, parent_namespace, module_ident)?;
+        // TODO: Check for duplicates
         self.names.insert(name.rust.clone(), name);
         Ok(())
     }
@@ -306,8 +304,8 @@ mod tests {
         assert_eq!(types.num_types(), 0);
 
         assert!(types.cxx_unqualified(&format_ident!("A")).is_err());
+        assert!(types.cxx_qualified(&format_ident!("A")).is_err());
         assert!(types.namespace(&format_ident!("A")).is_err());
-        // assert!(types.cxx_qualified(&format_ident!("A")).is_err());
     }
 
     #[test]
@@ -320,7 +318,7 @@ mod tests {
 
         assert_eq!(types.num_types(), 1);
         assert_eq!(types.rust_qualified(&ident), parse_quote! { ffi::A });
-        assert_eq!(types.cxx_qualified(&ident), "A");
+        assert_eq!(types.cxx_qualified(&ident).unwrap(), "A");
         assert!(types.namespace(&ident).unwrap().is_none());
     }
 
@@ -338,7 +336,7 @@ mod tests {
             .is_ok());
 
         assert_eq!(types.num_types(), 1);
-        assert_eq!(types.cxx_qualified(&ident), "B");
+        assert_eq!(types.cxx_qualified(&ident).unwrap(), "B");
         assert!(types.namespace(&ident).unwrap().is_none());
         assert_eq!(types.rust_qualified(&ident), parse_quote! { ffi::A });
     }
@@ -397,7 +395,7 @@ mod tests {
             .populate(&ident, &[], Some("bridge_namespace"), &format_ident!("ffi"))
             .is_ok());
 
-        assert_eq!(types.cxx_qualified(&ident), "bridge_namespace::A");
+        assert_eq!(types.cxx_qualified(&ident).unwrap(), "bridge_namespace::A");
         assert_eq!(
             types.namespace(&ident).unwrap().unwrap(),
             "bridge_namespace"
@@ -443,7 +441,7 @@ mod tests {
         let type_names = parse_cxx_item(item);
         let ident = format_ident!("A");
         assert_eq!(type_names.num_types(), 1);
-        assert_eq!(type_names.cxx_qualified(&ident), "B");
+        assert_eq!(type_names.cxx_qualified(&ident).unwrap(), "B");
 
         assert_eq!(type_names.rust_qualified(&ident), parse_quote! { ffi::A });
     }
@@ -476,15 +474,15 @@ mod tests {
         assert_eq!(types.num_types(), 3);
 
         assert_eq!(
-            &types.cxx_qualified(&format_ident!("A")),
+            types.cxx_qualified(&format_ident!("A")).unwrap(),
             "type_namespace::B"
         );
         assert_eq!(
-            &types.cxx_qualified(&format_ident!("C")),
+            types.cxx_qualified(&format_ident!("C")).unwrap(),
             "extern_namespace::D"
         );
         assert_eq!(
-            &types.cxx_qualified(&format_ident!("E")),
+            types.cxx_qualified(&format_ident!("E")).unwrap(),
             "bridge_namespace::E"
         );
 
@@ -534,7 +532,10 @@ mod tests {
             type_names.namespace(&ident).unwrap().unwrap(),
             "enum_namespace"
         );
-        assert_eq!(type_names.cxx_qualified(&ident), "enum_namespace::EnumB");
+        assert_eq!(
+            type_names.cxx_qualified(&ident).unwrap(),
+            "enum_namespace::EnumB"
+        );
         assert_eq!(
             type_names.rust_qualified(&ident),
             parse_quote! { ffi::EnumA }
@@ -556,7 +557,10 @@ mod tests {
 
         assert_eq!(types.num_types(), 1);
         assert_eq!(types.cxx_unqualified(&ident).unwrap(), "StructB");
-        assert_eq!(types.cxx_qualified(&ident), "struct_namespace::StructB");
+        assert_eq!(
+            types.cxx_qualified(&ident).unwrap(),
+            "struct_namespace::StructB"
+        );
         assert_eq!(
             types.namespace(&ident).unwrap().unwrap(),
             "struct_namespace"
diff --git a/crates/cxx-qt-gen/test_inputs/invokables.rs b/crates/cxx-qt-gen/test_inputs/invokables.rs
diff --git a/crates/cxx-qt-gen/test_inputs/signals.rs b/crates/cxx-qt-gen/test_inputs/signals.rs
diff --git a/crates/cxx-qt-gen/test_outputs/invokables.rs b/crates/cxx-qt-gen/test_outputs/invokables.rs
diff --git a/crates/cxx-qt-gen/test_outputs/signals.rs b/crates/cxx-qt-gen/test_outputs/signals.rs

Original file line number	Diff line number	Diff line change
`@@ -93,4 +93,12 @@ impl Name {`
`93`	`93`	`module: module.clone(),`
`94`	`94`	`})`
`95`	`95`	`}`
	`96`	`+`
	`97`	`+ /// Get the unqualified name of the type in C++.`
	`98`	`+ /// This is either:`
	`99`	`+ /// - The cxx_name attribute value, if one is provided`
	`100`	`+ /// - The original ident, if no cxx_name was provided`
	`101`	`+ pub(super) fn cxx_unqualified(&self) -> String {`
	`102`	`+ self.cxx.clone().unwrap_or_else(\|\| self.rust.to_string())`
	`103`	`+ }`
`96`	`104`	`}`