fix(bind): four Python glue gaps found binding finance-query

docs/bindings.md

@@ -298,7 +298,8 @@ for now, so return an owned value there. A public field holding an
 exported type reads as a handle but has no setter: the handle Python passes
 in cannot be moved out of. A handle's `repr` lists its readable fields in
 declaration order (`Counter(value=10)`), leaving out fields holding another
-handle.
+handle. A tuple struct's positions read as `_0`, `_1`, ... in every
+language, since a bare index is not an identifier.
 
 The wheel ships `py.typed` and a stub, so editors and type checkers see the
 surface without importing the extension: every class with its `__init__`,

soothfast-bind/src/adt.rs

@@ -64,22 +64,25 @@ pub(crate) fn error_type(r: &mut Resolver, item: &Value, rust_path: String) -> E
 
 fn struct_fields(r: &mut Resolver, kind: &Value, at: &str) -> Vec<Field> {
     if let Some(plain) = kind.get("plain") {
-        return named_fields(r, &plain["fields"], at);
+        return named_fields(r, &plain["fields"], at, true);
     }
     if let Some(tuple) = kind.get("tuple").and_then(Value::as_array) {
         return positional_fields(r, tuple, at);
     }
     Vec::new()
 }
 
-fn named_fields(r: &mut Resolver, ids: &Value, at: &str) -> Vec<Field> {
+/// `reported` is false for an enum variant's own fields: a data-carrying
+/// enum always binds as an opaque handle, so nothing they hold ever crosses,
+/// and an unmapped one is not a gap regardless of its declared visibility.
+fn named_fields(r: &mut Resolver, ids: &Value, at: &str, reported: bool) -> Vec<Field> {
     let mut out = Vec::new();
     for field in items(r, ids.as_array().unwrap_or(&Vec::new())) {
         let name = field["name"].as_str().unwrap_or_default().to_string();
         let where_ = format!("{at}.{name}");
         let public = is_public(&field);
         out.push(Field {
-            ty: field_ty(r, &field, public, &where_),
+            ty: field_ty(r, &field, public && reported, &where_),
             public,
             doc: summary(&field),
             name,
@@ -136,12 +139,12 @@ fn items(r: &Resolver, ids: &[Value]) -> Vec<Value> {
 
 fn variant_fields(r: &mut Resolver, kind: &Value, at: &str) -> VariantFields {
     if let Some(st) = kind.get("struct") {
-        return VariantFields::Named(named_fields(r, &st["fields"], at));
+        return VariantFields::Named(named_fields(r, &st["fields"], at, false));
     }
     if let Some(tuple) = kind.get("tuple").and_then(Value::as_array) {
         let mut tys: Vec<Ty> = Vec::new();
         for (n, field) in items(r, tuple).into_iter().enumerate() {
-            tys.push(r.resolve(&field["inner"]["struct_field"], &format!("{at}.{n}")));
+            tys.push(r.resolve_unreported(&field["inner"]["struct_field"], &format!("{at}.{n}")));
         }
         return VariantFields::Tuple(tys);
     }

soothfast-bind/src/naming.rs

@@ -1,11 +1,16 @@
 //! Rust identifiers → target-language identifiers.
 //!
-//! Exported names are already legal Rust identifiers, so nothing needs
-//! sanitizing. The only collisions are with words a target language reserves,
-//! and each language's own module says which.
+//! Exported names are already legal Rust identifiers, so little needs
+//! sanitizing: collisions with words a target language reserves (each
+//! language's own module says which), and a tuple field's synthetic
+//! position name (`"0"`, `"1"`), legal as a Rust tuple index but not as an
+//! identifier anywhere else.
 
-/// Append an underscore to a name the target language will not accept.
+/// Escape a name the target language will not accept as-is.
 pub(crate) fn escape(name: &str, reserved: &[&str]) -> String {
+    if name.starts_with(|c: char| c.is_ascii_digit()) {
+        return format!("_{name}");
+    }
     if reserved.contains(&name) {
         format!("{name}_")
     } else {
@@ -53,6 +58,12 @@ mod tests {
         assert_eq!(escape("bump", &["from"]), "bump");
     }
 
+    #[test]
+    fn a_digit_leading_name_gains_a_leading_underscore() {
+        assert_eq!(escape("0", &[]), "_0");
+        assert_eq!(escape("1", &["from"]), "_1");
+    }
+
     #[test]
     fn ident_parts_drop_everything_illegal() {
         assert_eq!(ident_part("Vec<String>"), "VecString");

soothfast-bind/src/pyo3/glue.rs

@@ -189,11 +189,20 @@ fn plain_enum(class: &Class, krate: &str) -> String {
         .flatten()
         .map(|v| v.name.as_str())
         .collect();
-    let arms = |from: &str, to: &str| -> String {
-        names
+    // `inner` may be `#[non_exhaustive]`, so every conversion out of it
+    // carries a catch-all arm, unreachable for a matching binding.
+    let arms = |from: &str, to: &str, catch_all: bool| -> String {
+        let mut out: String = names
             .iter()
             .map(|n| format!("            {from}::{n} => {to}::{n},\n"))
-            .collect()
+            .collect();
+        if catch_all {
+            let _ = writeln!(
+                out,
+                "            _ => ::std::unreachable!(\"{from} gained a variant this binding was not generated for\"),"
+            );
+        }
+        out
     };
     format!(
         "
@@ -203,6 +212,7 @@ pub enum {name} {{
 {}}}
 
 impl ::std::convert::From<{inner}> for {name} {{
+    #[allow(unreachable_patterns)]
     fn from(value: {inner}) -> Self {{
         match value {{
 {}        }}
@@ -212,6 +222,7 @@ impl ::std::convert::From<{inner}> for {name} {{
 // A mirrored enum has no derived `Clone`; a field getter converts through
 // this one instead of cloning an owned copy just to consume it.
 impl ::std::convert::From<&{inner}> for {name} {{
+    #[allow(unreachable_patterns)]
     fn from(value: &{inner}) -> Self {{
         match value {{
 {}        }}
@@ -230,9 +241,9 @@ impl ::std::convert::From<{name}> for {inner} {{
             .iter()
             .map(|n| format!("    {n},\n"))
             .collect::<String>(),
-        arms(&inner, name),
-        arms(&inner, name),
-        arms(name, &inner),
+        arms(&inner, name, true),
+        arms(&inner, name, true),
+        arms(name, &inner, false),
     )
 }
 
@@ -468,19 +479,57 @@ fn out(expr: &str, ty: &Ty, plan: &BindingPlan) -> String {
                 format!("{expr}.map(::std::convert::Into::into)")
             }
             Ty::Class(name) => format!("{expr}.map({name})"),
+            _ if has_class(inner) => format!("{expr}.map(|value| {})", out("value", inner, plan)),
             _ => expr.to_string(),
         },
         Ty::List(inner) => match &**inner {
             Ty::Class(name) if plan.is_mirrored(name) => {
                 format!("{expr}.into_iter().map(::std::convert::Into::into).collect()")
             }
             Ty::Class(name) => format!("{expr}.into_iter().map({name}).collect()"),
+            _ if has_class(inner) => format!(
+                "{expr}.into_iter().map(|value| {}).collect()",
+                out("value", inner, plan)
+            ),
             _ => expr.to_string(),
         },
+        Ty::Map(_, value) if has_class(value) => format!(
+            "{expr}.into_iter().map(|(key, value)| (key, {})).collect()",
+            out("value", value, plan)
+        ),
+        Ty::Tuple(items) if items.iter().any(has_class) => tuple_out(expr, items, plan),
         _ => expr.to_string(),
     }
 }
 
+/// Whether a type nests an exported class at any depth, the signal `out`
+/// uses to tell a composition worth converting from one that crosses as-is.
+fn has_class(ty: &Ty) -> bool {
+    match ty {
+        Ty::Class(_) => true,
+        Ty::Optional(inner) | Ty::List(inner) => has_class(inner),
+        Ty::Map(_, value) => has_class(value),
+        Ty::Tuple(items) => items.iter().any(has_class),
+        _ => false,
+    }
+}
+
+/// A tuple leaving the user's crate, destructured so each position converts
+/// on its own terms.
+fn tuple_out(expr: &str, items: &[Ty], plan: &BindingPlan) -> String {
+    let names: Vec<String> = (0..items.len()).map(|i| format!("v{i}")).collect();
+    let converted: Vec<String> = names
+        .iter()
+        .zip(items)
+        .map(|(name, ty)| out(name, ty, plan))
+        .collect();
+    format!(
+        "{{ let ({}) = {expr}; ({}) }}",
+        names.join(", "),
+        converted.join(", ")
+    )
+}
+
 /// A returned type, where a sequence of one primitive comes back as its array
 /// class rather than a list of boxed numbers.
 fn returned_ty(ty: &Ty) -> String {
@@ -754,3 +803,64 @@ pub(super) fn docs(doc: Option<&str>, indent: &str) -> String {
         None => String::new(),
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::model::{Variant, VariantFields};
+
+    /// `Level` on the Rust side may be `#[non_exhaustive]`; the plan carries
+    /// no such flag, so the conversion out of it always needs a catch-all.
+    fn flag_class() -> Class {
+        Class {
+            rust_path: "acme::Level".into(),
+            name: "Level".into(),
+            doc: None,
+            send: true,
+            sync: true,
+            ctor: None,
+            accessors: Vec::new(),
+            methods: Vec::new(),
+            statics: Vec::new(),
+            variants: Some(vec![
+                Variant {
+                    name: "Low".into(),
+                    fields: VariantFields::Unit,
+                    doc: None,
+                },
+                Variant {
+                    name: "High".into(),
+                    fields: VariantFields::Unit,
+                    doc: None,
+                },
+            ]),
+        }
+    }
+
+    #[test]
+    fn a_plain_enum_conversion_out_of_the_foreign_type_has_a_catch_all_arm() {
+        let rendered = plain_enum(&flag_class(), "::acme");
+        let from_foreign = rendered
+            .split("impl ::std::convert::From<::acme::Level> for Level")
+            .nth(1)
+            .expect("renders the owned conversion");
+        assert!(
+            from_foreign.contains("#[allow(unreachable_patterns)]"),
+            "a catch-all arm is unreachable for a locally exhaustive enum: {rendered}"
+        );
+        assert!(
+            from_foreign.contains("_ => ::std::unreachable!"),
+            "without a catch-all this match fails to compile against a \
+             `#[non_exhaustive]` source enum: {rendered}"
+        );
+        let from_reverse = rendered
+            .split("impl ::std::convert::From<Level> for ::acme::Level")
+            .nth(1)
+            .expect("renders the reverse conversion");
+        assert!(
+            !from_reverse.contains("::std::unreachable!"),
+            "the mirrored enum is exhaustive on its own, so the reverse \
+             conversion needs no catch-all: {rendered}"
+        );
+    }
+}

soothfast-bind/tests/goldens/python/src/lib.rs

@@ -513,21 +513,25 @@ pub enum Level {
 }
 
 impl ::std::convert::From<::acme::Level> for Level {
+    #[allow(unreachable_patterns)]
     fn from(value: ::acme::Level) -> Self {
         match value {
             ::acme::Level::Low => Level::Low,
             ::acme::Level::High => Level::High,
+            _ => ::std::unreachable!("::acme::Level gained a variant this binding was not generated for"),
         }
     }
 }
 
 // A mirrored enum has no derived `Clone`; a field getter converts through
 // this one instead of cloning an owned copy just to consume it.
 impl ::std::convert::From<&::acme::Level> for Level {
+    #[allow(unreachable_patterns)]
     fn from(value: &::acme::Level) -> Self {
         match value {
             ::acme::Level::Low => Level::Low,
             ::acme::Level::High => Level::High,
+            _ => ::std::unreachable!("::acme::Level gained a variant this binding was not generated for"),
         }
     }
 }