diff --git a/docs/bindings.md b/docs/bindings.md index a98810c..593d6c2 100644 --- a/docs/bindings.md +++ b/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__`, diff --git a/soothfast-bind/src/adt.rs b/soothfast-bind/src/adt.rs index 3ee575c..1ddc0db 100644 --- a/soothfast-bind/src/adt.rs +++ b/soothfast-bind/src/adt.rs @@ -64,7 +64,7 @@ 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 { 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); @@ -72,14 +72,17 @@ fn struct_fields(r: &mut Resolver, kind: &Value, at: &str) -> Vec { Vec::new() } -fn named_fields(r: &mut Resolver, ids: &Value, at: &str) -> Vec { +/// `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 { 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 { 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 = 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); } diff --git a/soothfast-bind/src/naming.rs b/soothfast-bind/src/naming.rs index c5b5b34..522043f 100644 --- a/soothfast-bind/src/naming.rs +++ b/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"), "VecString"); diff --git a/soothfast-bind/src/pyo3/glue.rs b/soothfast-bind/src/pyo3/glue.rs index 2883589..c6c0942 100644 --- a/soothfast-bind/src/pyo3/glue.rs +++ b/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::(), - arms(&inner, name), - arms(&inner, name), - arms(name, &inner), + arms(&inner, name, true), + arms(&inner, name, true), + arms(name, &inner, false), ) } @@ -468,6 +479,7 @@ 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 { @@ -475,12 +487,49 @@ fn out(expr: &str, ty: &Ty, plan: &BindingPlan) -> String { 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 = (0..items.len()).map(|i| format!("v{i}")).collect(); + let converted: Vec = 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 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}" + ); + } +} diff --git a/soothfast-bind/tests/goldens/python/src/lib.rs b/soothfast-bind/tests/goldens/python/src/lib.rs index 80fc6ae..9f872e4 100644 --- a/soothfast-bind/tests/goldens/python/src/lib.rs +++ b/soothfast-bind/tests/goldens/python/src/lib.rs @@ -513,10 +513,12 @@ 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"), } } } @@ -524,10 +526,12 @@ impl ::std::convert::From<::acme::Level> for Level { // 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"), } } } diff --git a/soothfast-bind/tests/idioms.rs b/soothfast-bind/tests/idioms.rs index a45d6ff..6b9a084 100644 --- a/soothfast-bind/tests/idioms.rs +++ b/soothfast-bind/tests/idioms.rs @@ -796,3 +796,120 @@ fn a_field_holding_a_clone_exported_type_reads_as_a_fresh_handle_in_python_only( go.gaps ); } + +/// A tuple struct's own document, isolated from `doc()`: rustdoc names a +/// positional field by its index, which is a legal Rust tuple index but not +/// an identifier anywhere else. +fn tuple_struct_doc() -> Value { + json!({ + "index": { + "1": { + "name": "Wrap", "docs": Value::Null, "attrs": [], "visibility": "public", + "inner": { "struct": { + "kind": { "tuple": [2] }, + "generics": { "params": [], "where_predicates": [] }, + "impls": [] } }, + }, + "2": field("0", prim("i64"), true), + }, + "paths": { + "1": { "crate_id": 0, "path": ["shape", "Wrap"], "kind": "struct" }, + }, + }) +} + +#[test] +fn a_tuple_structs_position_becomes_a_legal_identifier() { + let records = vec![record("shape::Wrap", "struct")]; + let (surface, gaps) = + surface(&tuple_struct_doc(), &TypeTable::with_defaults(), &records).expect("walks"); + assert!( + !gaps.iter().any(|g| g.at() == "shape::Wrap.0"), + "a plain i64 position needs no mapping: {gaps:?}" + ); + + let files = BindKind::Python + .emit(&surface, gaps, &opts()) + .expect("emits"); + let glue = &files.files["src/lib.rs"]; + assert!(glue.contains("fn _0(&self) -> i64"), "{glue}"); + let stub = &files.files["acme_core.pyi"]; + assert!(stub.contains("def _0(self) -> int"), "{stub}"); + assert!(glue.contains("Wrap(_0={:?})"), "{glue}"); +} + +/// An exported `Item`, isolated from `doc()`, reached through a map and a +/// map of lists: neither composition is `Ty::Class` itself, so the value at +/// its leaf needs its own conversion, not the whole expression's. +fn map_of_classes_doc() -> Value { + json!({ + "index": { + "1": struct_item("Item", &[10], &[11]), + "10": field("id", prim("i64"), true), + "11": auto_impl("Clone", false), + "2": struct_item("Store", &[20, 21], &[]), + "20": field( + "by_name", + path("HashMap", 90, &[path("String", 91, &[]), path("Item", 1, &[])]), + true, + ), + "21": field( + "groups", + path( + "HashMap", + 90, + &[path("String", 91, &[]), path("Vec", 92, &[path("Item", 1, &[])])], + ), + true, + ), + "3": func( + "all", + &[], + path("HashMap", 90, &[path("String", 91, &[]), path("Item", 1, &[])]), + false, + ), + }, + "paths": { + "1": { "crate_id": 0, "path": ["shape", "Item"], "kind": "struct" }, + "2": { "crate_id": 0, "path": ["shape", "Store"], "kind": "struct" }, + "3": { "crate_id": 0, "path": ["shape", "all"], "kind": "function" }, + }, + }) +} + +#[test] +fn a_class_nested_inside_a_map_or_a_map_of_lists_is_wrapped_at_the_leaf() { + let records = vec![ + record("shape::Item", "struct"), + record("shape::Store", "struct"), + record("shape::all", "fn"), + ]; + let (surface, gaps) = + surface(&map_of_classes_doc(), &TypeTable::with_defaults(), &records).expect("walks"); + let files = BindKind::Python + .emit(&surface, gaps, &opts()) + .expect("emits"); + let glue = &files.files["src/lib.rs"]; + assert!( + glue.contains( + "fn by_name(&self) -> ::std::collections::HashMap {\n \ + self.0.by_name.clone().into_iter().map(|(key, value)| (key, Item(value))).collect()" + ), + "{glue}" + ); + assert!( + glue.contains( + "fn groups(&self) -> ::std::collections::HashMap> {\n \ + self.0.groups.clone().into_iter().map(|(key, value)| \ + (key, value.into_iter().map(Item).collect())).collect()" + ), + "{glue}" + ); + assert!( + glue.contains( + "fn all() -> ::std::collections::HashMap {\n \ + ::acme::all().into_iter().map(|(key, value)| (key, Item(value))).collect()" + ), + "{glue}" + ); +} diff --git a/soothfast-bind/tests/surface.rs b/soothfast-bind/tests/surface.rs index 42b2104..5152175 100644 --- a/soothfast-bind/tests/surface.rs +++ b/soothfast-bind/tests/surface.rs @@ -259,3 +259,40 @@ fn an_exported_type_taken_by_value_is_reported_rather_than_copied() { Gap::HandleByValue { at, ty } if at == "acme::merge" && ty == "Counter" ))); } + +/// A data-carrying enum always binds as an opaque handle (see +/// `pyo3::glue::plain_enum`'s counterpart for plain ones), so a variant's +/// own payload never crosses on any backend, mapped or not. +#[test] +fn an_unmapped_type_inside_a_variant_payload_is_not_gapped() { + use fixture::{enum_item, field, path, record, variant}; + use serde_json::json; + + let doc = json!({ + "index": { + "1": enum_item("Stamped", &[2], &[]), + "2": variant("At", json!({ "tuple": [3] })), + "3": field("0", path("Duration", 4, &[]), true), + }, + "paths": { + "1": { "crate_id": 0, "path": ["shape", "Stamped"], "kind": "enum" }, + "4": { "crate_id": 1, "path": ["std", "time", "Duration"], "kind": "struct" }, + }, + }); + let table = TypeTable::default(); + let records = vec![record("shape::Stamped", "enum")]; + let (surface, gaps) = surface(&doc, &table, &records).expect("walks"); + + assert_eq!(surface.types.len(), 1); + assert!( + matches!(&surface.types[0].kind, TypeKind::Enum(v) if v[0].fields == VariantFields::Tuple(vec![Ty::Opaque("std::time::Duration".into())])), + "{:?}", + surface.types[0].kind + ); + assert!( + !gaps + .iter() + .any(|g| matches!(g, Gap::UnmappedForeign { .. })), + "a variant's payload can never cross, so its unmapped type is not a gap: {gaps:?}" + ); +} diff --git a/soothfast-demo/bindings/python/src/lib.rs b/soothfast-demo/bindings/python/src/lib.rs index 2ecb793..754e72d 100644 --- a/soothfast-demo/bindings/python/src/lib.rs +++ b/soothfast-demo/bindings/python/src/lib.rs @@ -274,12 +274,14 @@ pub enum Metric { } impl ::std::convert::From<::soothfast_demo::Metric> for Metric { + #[allow(unreachable_patterns)] fn from(value: ::soothfast_demo::Metric) -> Self { match value { ::soothfast_demo::Metric::Median => Metric::Median, ::soothfast_demo::Metric::Mad => Metric::Mad, ::soothfast_demo::Metric::Min => Metric::Min, ::soothfast_demo::Metric::Max => Metric::Max, + _ => ::std::unreachable!("::soothfast_demo::Metric gained a variant this binding was not generated for"), } } } @@ -287,12 +289,14 @@ impl ::std::convert::From<::soothfast_demo::Metric> for Metric { // 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<&::soothfast_demo::Metric> for Metric { + #[allow(unreachable_patterns)] fn from(value: &::soothfast_demo::Metric) -> Self { match value { ::soothfast_demo::Metric::Median => Metric::Median, ::soothfast_demo::Metric::Mad => Metric::Mad, ::soothfast_demo::Metric::Min => Metric::Min, ::soothfast_demo::Metric::Max => Metric::Max, + _ => ::std::unreachable!("::soothfast_demo::Metric gained a variant this binding was not generated for"), } } }