Don't skip Option for nullable collections where applicable

Nullable collection were represented as a `Vec<_>` or `[_]`, but an empty
collection is not the same as an undefined collection.

Don't do this for return and output values though: in practice the C
implementations return NULL when the size is 0. So it doesn't make much
sense using an `Option<_>` in that case.

Finally, Gir now parses the `zero-terminated` annotation which allows
detecting functions returning an array for which the length is not defined
by a length argument. Those functions used to require an explicit `manual`
declaration in the `Gir.toml` and are automatically commented out now.

Fixes: #1133
See also: gtk-rs/gtk-rs-core#1257

Author: fengalin

src/analysis/class_builder.rs

@@ -137,6 +137,7 @@ fn analyze_property(
         func_name: String::new(),
         func_name_alias: None,
         nullable,
+        is_array: prop.is_array,
         get_out_ref_mode,
         set_in_ref_mode,
         set_bound: None,

src/analysis/properties.rs

@@ -26,6 +26,7 @@ pub struct Property {
     pub func_name: String,
     pub func_name_alias: Option<String>,
     pub nullable: library::Nullable,
+    pub is_array: bool,
     pub get_out_ref_mode: RefMode,
     pub set_in_ref_mode: RefMode,
     pub bounds: Bounds,
@@ -265,6 +266,7 @@ fn analyze_property(
             func_name: get_func_name,
             func_name_alias: get_prop_name,
             nullable,
+            is_array: prop.is_array,
             get_out_ref_mode,
             set_in_ref_mode,
             set_bound: None,
@@ -318,6 +320,7 @@ fn analyze_property(
             func_name: set_func_name,
             func_name_alias: set_prop_name,
             nullable,
+            is_array: prop.is_array,
             get_out_ref_mode,
             set_in_ref_mode,
             set_bound,

src/analysis/return_value.rs

@@ -120,19 +120,32 @@ pub fn analyze(
 
     let parameter = parameter.as_ref().map(|lib_par| {
         let par = analysis::Parameter::from_return_value(env, lib_par, configured_functions);
-        if let Ok(rust_type) = RustType::builder(env, typ)
-            .direction(par.lib_par.direction)
-            .try_from_glib(&par.try_from_glib)
-            .try_build()
+        if par.lib_par.is_array
+            && !par.lib_par.zero_terminated.unwrap_or(true)
+            && par.lib_par.array_length.is_none()
         {
-            used_types.extend(rust_type.into_used_types());
-        }
+            // The array length for the return value of this function can not be inferred
+            // from any argument. How the length should be inferred is usually specified
+            // in the doc of the function, so there's nothing we can do automatically.
+            // Currently the functions matching this condition are explicitely marked
+            // `manual = true` in the matching Gir.toml.
+            // FIXME: `FixedArray`s could be handled automatically
+            commented = true;
+        } else {
+            if let Ok(rust_type) = RustType::builder(env, typ)
+                .direction(par.lib_par.direction)
+                .try_from_glib(&par.try_from_glib)
+                .try_build()
+            {
+                used_types.extend(rust_type.into_used_types());
+            }
 
-        commented = RustType::builder(env, typ)
-            .direction(func.ret.direction)
-            .try_from_glib(&par.try_from_glib)
-            .try_build_param()
-            .is_err();
+            commented = RustType::builder(env, typ)
+                .direction(func.ret.direction)
+                .try_from_glib(&par.try_from_glib)
+                .try_build_param()
+                .is_err();
+        }
 
         par
     });

src/analysis/rust_type.rs

@@ -251,7 +251,6 @@ impl<'env> RustTypeBuilder<'env> {
         let ok = |s: &str| Ok(RustType::from(s));
         let ok_and_use = |s: &str| Ok(RustType::new_and_use(&s));
         let err = |s: &str| Err(TypeError::Unimplemented(s.into()));
-        let mut skip_option = false;
         let type_ = self.env.library.type_(self.type_id);
         let mut rust_type = match *type_ {
             Basic(fund) => {
@@ -355,7 +354,6 @@ impl<'env> RustTypeBuilder<'env> {
             List(inner_tid) | SList(inner_tid) | CArray(inner_tid) | PtrArray(inner_tid)
                 if ConversionType::of(self.env, inner_tid) == ConversionType::Pointer =>
             {
-                skip_option = true;
                 let inner_ref_mode = match self.env.type_(inner_tid) {
                     Class(..) | Interface(..) => RefMode::None,
                     Record(record) => match RecordType::of(record) {
@@ -409,7 +407,6 @@ impl<'env> RustTypeBuilder<'env> {
                     };
 
                     if let Some(s) = array_type {
-                        skip_option = self.direction == ParameterDirection::Return;
                         if self.ref_mode.is_ref() {
                             Ok(format!("[{s}]").into())
                         } else {
@@ -604,7 +601,7 @@ impl<'env> RustTypeBuilder<'env> {
             }
         }
 
-        if *self.nullable && !skip_option {
+        if *self.nullable {
             match ConversionType::of(self.env, self.type_id) {
                 ConversionType::Pointer | ConversionType::Scalar => {
                     rust_type = rust_type.map_any(|rust_type| {

src/codegen/properties.rs

@@ -102,7 +102,7 @@ fn declaration(env: &Env, prop: &Property) -> String {
         let dir = library::ParameterDirection::Return;
         let ret_type = RustType::builder(env, prop.typ)
             .direction(dir)
-            .nullable(prop.nullable)
+            .nullable(prop.nullable & !prop.is_array)
             .ref_mode(prop.get_out_ref_mode)
             .try_build_param()
             .into_string();

src/library.rs

@@ -3,7 +3,7 @@ use std::{
     collections::{BTreeMap, BTreeSet, HashMap, HashSet},
     fmt,
     iter::Iterator,
-    ops::{Deref, DerefMut},
+    ops::{BitAnd, Deref, DerefMut},
     str::FromStr,
 };
 
@@ -135,6 +135,14 @@ impl DerefMut for Nullable {
     }
 }
 
+impl BitAnd<bool> for Nullable {
+    type Output = Nullable;
+
+    fn bitand(self, rhs: bool) -> Nullable {
+        Nullable(*self & rhs)
+    }
+}
+
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
 pub struct Mandatory(pub bool);
 
@@ -513,6 +521,7 @@ pub struct Property {
     pub typ: TypeId,
     pub c_type: Option<String>,
     pub transfer: Transfer,
+    pub is_array: bool,
     pub version: Option<Version>,
     pub deprecated_version: Option<Version>,
     pub doc: Option<String>,
@@ -531,6 +540,7 @@ pub struct Parameter {
     pub transfer: Transfer,
     pub caller_allocates: bool,
     pub nullable: Nullable,
+    pub is_array: bool,
     pub array_length: Option<u32>,
     pub zero_terminated: Option<bool>,
     pub is_error: bool,

src/parser.rs

@@ -17,6 +17,7 @@ const EMPTY_CTYPE: &str = "/*EMPTY*/";
 struct ParsedType {
     tid: TypeId,
     c_type: Option<String>,
+    is_array: bool,
     array_length: Option<u32>,
     zero_terminated: Option<bool>,
 }
@@ -1267,7 +1268,7 @@ impl Library {
         let transfer = elem
             .attr_from_str("transfer-ownership")?
             .unwrap_or(Transfer::None);
-        let nullable = elem.attr_bool("nullable", false);
+        let mut nullable = elem.attr_bool("nullable", false);
         let scope = elem.attr_from_str("scope")?.unwrap_or(ParameterScope::None);
         let closure = elem.attr_from_str("closure")?;
         let destroy = elem.attr_from_str("destroy")?;
@@ -1305,6 +1306,7 @@ impl Library {
                 }
 
                 typ = Some(elem_type);
+
                 Ok(())
             }
             "varargs" => {
@@ -1317,9 +1319,32 @@ impl Library {
         })?;
 
         if let Some(mut elem_type) = typ {
-            if for_method {
-                elem_type.array_length = elem_type.array_length.map(|l| l + 1);
+            if elem_type.is_array {
+                // Note: these could be handled in the `analysis` part, however that would be quite
+                //       annoying because many code places check the parsed `nullable` instead of
+                //       the `analysis` result. Gir necessitates a rewrite...
+                use ParameterDirection::*;
+                match direction {
+                    Return | Out | InOut => {
+                        // Some arrays in return position come with nullable=true & zero-terminated=false
+                        // annotations. In practice, the C implementations return NULL when the size is 0.
+                        // So it doesn't make much sense using an `Option<_>` by default for nullable arrays.
+                        nullable = false
+                    }
+                    _ => {
+                        // We can only generate a Rust `Option` from a nullable array annotation
+                        // if the array is not zero terminated. Otherwise we would need an extra
+                        // indication to differentiate an empty array from an undefined value,
+                        // which should be indicated in the doc and would need manual implementation.
+                        nullable &= !elem_type.zero_terminated.unwrap_or(true);
+                    }
+                }
+
+                if for_method {
+                    elem_type.array_length = elem_type.array_length.map(|l| l + 1);
+                }
             }
+
             Ok(Parameter {
                 name: param_name.into(),
                 typ: elem_type.tid,
@@ -1329,6 +1354,7 @@ impl Library {
                 transfer,
                 caller_allocates,
                 nullable: Nullable(nullable),
+                is_array: elem_type.is_array,
                 array_length: elem_type.array_length,
                 zero_terminated: elem_type.zero_terminated,
                 is_error: false,
@@ -1451,6 +1477,7 @@ impl Library {
             construct,
             construct_only,
             transfer,
+            is_array: elem_type.is_array,
             typ: elem_type.tid,
             c_type: elem_type.c_type,
             version,
@@ -1468,7 +1495,7 @@ impl Library {
         ns_id: u16,
         elem: &Element,
     ) -> Result<ParsedType, String> {
-        let type_name = elem
+        let mut type_name = elem
             .attr("name")
             .or_else(|| {
                 if elem.name() == "array" {
@@ -1508,19 +1535,15 @@ impl Library {
                     "<type> element is missing an inner element type",
                     elem.position(),
                 ))
-            } else if type_name == "gboolean" && c_type.as_deref() == Some("_Bool") {
-                Ok(ParsedType {
-                    tid: self.find_or_stub_type(ns_id, "bool"),
-                    c_type,
-                    array_length,
-                    zero_terminated,
-                })
             } else {
+                if type_name == "gboolean" && c_type.as_deref() == Some("_Bool") {
+                    type_name = "bool";
+                }
+
                 Ok(ParsedType {
                     tid: self.find_or_stub_type(ns_id, type_name),
                     c_type,
-                    array_length,
-                    zero_terminated,
+                    ..Default::default()
                 })
             }
         } else {
@@ -1541,6 +1564,7 @@ impl Library {
             Ok(ParsedType {
                 tid,
                 c_type,
+                is_array: true,
                 array_length,
                 zero_terminated,
             })
@@ -1637,4 +1661,89 @@ mod tests {
         })
         .unwrap();
     }
+
+    #[test]
+    fn array_param_nullable_zero_terminated_default() {
+        XmlParser::new(
+            br#"<?xml version="1.0"?>
+            <parameter name="data" transfer-ownership="none" nullable="1">
+              <array length="2" type="const guchar*">
+                <type name="guint8" type="guchar"/>
+              </array>
+            </parameter>"#
+                .as_slice(),
+        )
+        .document(|p, _| {
+            p.element_with_name("parameter", |p, elem| {
+                let elem = Library::new("Glib")
+                    .read_parameter(p, MAIN_NAMESPACE, elem, false, false)
+                    .unwrap();
+                assert_eq!(elem.c_type, "const guchar*");
+                // We can only generate a Rust `Option` from a nullable array annotation
+                // if the array is not zero terminated. Otherwise we would need an extra
+                // indication to differentiate an empty array from an undefined value,
+                // which should be indicated in the doc and would need manual implementation.
+                assert!(!*elem.nullable);
+                // the default seems to be true
+                assert!(elem.zero_terminated.is_none());
+
+                Ok(())
+            })
+        })
+        .unwrap();
+    }
+
+    #[test]
+    fn array_param_nullable_zero_terminated() {
+        XmlParser::new(
+            br#"<?xml version="1.0"?>
+            <parameter name="data" transfer-ownership="none" nullable="1">
+              <array length="2" zero-terminated="1" type="const guchar*">
+                <type name="guint8" type="guchar"/>
+              </array>
+            </parameter>"#
+                .as_slice(),
+        )
+        .document(|p, _| {
+            p.element_with_name("parameter", |p, elem| {
+                let elem = Library::new("Glib")
+                    .read_parameter(p, MAIN_NAMESPACE, elem, false, false)
+                    .unwrap();
+                assert_eq!(elem.c_type, "const guchar*");
+                // We can only generate a Rust `Option` from a nullable array annotation
+                // if the array is not zero terminated. Otherwise we would need an extra
+                // indication to differentiate an empty array from an undefined value,
+                // which should be indicated in the doc and would need manual implementation.
+                assert!(!*elem.nullable);
+                assert!(elem.zero_terminated.unwrap());
+
+                Ok(())
+            })
+        })
+        .unwrap();
+    }
+
+    #[test]
+    fn array_prop() {
+        XmlParser::new(
+            br#"<?xml version="1.0"?>
+            <property name="advertised-protocols" version="2.60" writable="1" transfer-ownership="none" setter="set_advertised_protocols">
+              <array>
+                <type name="utf8"/>
+              </array>
+            </property>"#
+                .as_slice(),
+        )
+        .document(|p, _| {
+            p.element_with_name("property", |p, elem| {
+                let elem = Library::new("Glib")
+                    .read_property(p, MAIN_NAMESPACE, elem, "")
+                    .unwrap().unwrap();
+                assert!(elem.is_array);
+
+                Ok(())
+            })
+        })
+        .unwrap();
+    }
 }

src/xmlparser.rs

@@ -31,6 +31,7 @@ pub struct XmlParser<'a> {
     error_emitter: Rc<ErrorEmitter>,
 }
 
+#[derive(Debug)]
 struct ErrorEmitter {
     /// Path to currently parsed document.
     path: Option<PathBuf>,
@@ -61,6 +62,7 @@ impl ErrorEmitter {
 pub struct Document;
 
 /// A wrapper for `XmlEvent::StartElement` which doesn't have its own type.
+#[derive(Debug)]
 pub struct Element {
     name: OwnedName,
     attributes: Vec<OwnedAttribute>,