Python: improve default values and use dataclasses to keep mypy happy. (#2552)

Improved how default values are handled in function signatures etc,
and more canonical use of Python dataclasses for records and enums,
all towards making mypy happier

Author: mhammond

CHANGELOG.md

@@ -29,6 +29,7 @@
   * Support throwing external error ([#2629](https://github.com/mozilla/uniffi-rs/pull/2629))
   * The `NoPointer` placeholder object used to create fake interface instances has been renamed to `NoHandle`
 - Python:
+  * Improved how default values are handled in function signatures etc and more canonical use of Python dataclasses, all towards making mypy happier ([#2552](https://github.com/mozilla/uniffi-rs/pull/2552))
   * Methods now have typing annotations for return values ([#2625](https://github.com/mozilla/uniffi-rs/issues/2625))
   * Fix relative imports ([#2657](https://github.com/mozilla/uniffi-rs/pull/2657))
   * Fix shadowing param names with internal variables in Python (#2628/#2645)

fixtures/keywords/rust/Cargo.toml

@@ -12,9 +12,6 @@ name = "uniffi_keywords_rust"
 thiserror = "2"
 uniffi = { workspace = true }
 
-[build-dependencies]
-uniffi = { workspace = true, features = ["build"] }
-
 [dev-dependencies]
 uniffi = { workspace = true, features = ["bindgen-tests"] }
 

fixtures/keywords/rust/build.rs

@@ -2,11 +2,16 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+//! A combined Rust + Python keywords test, ensuring we overlap enough so we have
+//! good tests for both.
+
 use std::{collections::HashMap, sync::Arc};
 
+#[uniffi::export]
 pub fn r#if(_async: u8) {}
 
 #[allow(non_camel_case_types)]
+#[derive(uniffi::Object)]
 pub struct r#break {}
 
 impl r#break {
@@ -16,17 +21,31 @@ impl r#break {
     pub fn r#break(&self, v: HashMap<u8, Arc<r#break>>) -> Option<HashMap<u8, Arc<r#break>>> {
         Some(v)
     }
-    fn r#continue(&self, _v: Vec<Box<dyn r#continue>>) {}
+    pub fn r#continue(&self, _v: Vec<Box<dyn r#continue>>) {}
     pub fn r#yield(&self, _async: u8) {}
     pub fn r#async(&self, _yield: Option<u8>) {}
 }
 
 #[allow(non_camel_case_types)]
-pub trait r#continue {
+#[uniffi::export]
+pub trait r#continue: Send + Sync {
+    fn r#return(&self, _v: r#return) -> r#return;
+    fn r#continue(&self) -> Option<Arc<dyn r#continue>>;
+    fn r#break(&self, _v: Option<Arc<r#break>>) -> HashMap<u8, Arc<r#break>>;
+    fn r#while(&self, _v: Vec<r#while>) -> r#while;
+    fn r#yield(&self, _v: HashMap<u8, Vec<r#yield>>) -> Option<HashMap<u8, Vec<r#yield>>>;
+}
+
+#[allow(non_camel_case_types)]
+#[derive(uniffi::Object)]
+pub struct r#in {}
+
+#[uniffi::export]
+impl r#continue for r#in {
     fn r#return(&self, _v: r#return) -> r#return {
         unimplemented!()
     }
-    fn r#continue(&self) -> Option<Box<dyn r#continue>> {
+    fn r#continue(&self) -> Option<Arc<dyn r#continue>> {
         unimplemented!()
     }
     fn r#break(&self, _v: Option<Arc<r#break>>) -> HashMap<u8, Arc<r#break>> {
@@ -41,20 +60,19 @@ pub trait r#continue {
 }
 
 #[allow(non_camel_case_types)]
-#[derive(uniffi::Object)]
-pub struct r#in {}
-
-#[uniffi::export]
-impl r#continue for r#in {}
-
-#[allow(non_camel_case_types)]
-#[derive(Debug)]
+#[derive(uniffi::Record, Debug)]
 pub struct r#return {
+    #[uniffi(default)]
     r#yield: u8,
+    #[uniffi(default)]
     r#async: Option<u8>,
+    // bool for python/mypy - #2552
+    #[uniffi(default)]
+    bool: bool,
 }
 
 #[allow(non_camel_case_types)]
+#[derive(uniffi::Record)]
 pub struct r#while {
     r#return: r#return,
     r#yield: Vec<r#yield>,
@@ -64,19 +82,21 @@ pub struct r#while {
 }
 
 #[allow(non_camel_case_types)]
+#[derive(uniffi::Enum)]
 pub enum r#async {
     #[allow(non_camel_case_types)]
-    r#as { r#async: u8 },
+    // bool for python/mypy, #2552
+    r#as { r#async: u8, bool: bool },
 }
 
 #[allow(non_camel_case_types)]
-#[derive(Debug)]
+#[derive(uniffi::Enum, Debug)]
 pub enum r#yield {
     r#async,
 }
 
 #[allow(non_camel_case_types)]
-#[derive(Debug, thiserror::Error)]
+#[derive(uniffi::Error, Debug, thiserror::Error)]
 pub enum r#for {
     #[error("return")]
     r#return { r#return: r#return },
@@ -94,4 +114,4 @@ pub fn get_else(e: r#else) -> r#else {
     e
 }
 
-uniffi::include_scaffolding!("keywords");
+uniffi::setup_scaffolding!("keywords_rust");

fixtures/keywords/rust/src/keywords.udl

@@ -9,3 +9,6 @@
 import keywords_rust
 # but might as well call something.
 keywords_rust._if(0)
+
+# python/mypy tests - not keywords, but type names in strange places.
+assert(keywords_rust.Return().bool == False)

fixtures/keywords/rust/src/lib.rs

@@ -14,10 +14,53 @@ pub fn pass(namespace: &mut Namespace) -> Result<()> {
 
     namespace.try_visit_mut(|default: &mut DefaultValueNode| {
         default.py_default = render_default(&default.default)?;
+        // Many types can use the appropriate literal directly.
+        default.arg_literal = match &default.default {
+            DefaultValue::Default(tn) => match &tn.ty {
+                Type::UInt8
+                | Type::UInt16
+                | Type::UInt32
+                | Type::UInt64
+                | Type::Int8
+                | Type::Int16
+                | Type::Int32
+                | Type::Int64
+                | Type::Float32
+                | Type::Float64
+                | Type::Boolean
+                | Type::Bytes
+                | Type::String
+                | Type::Optional { .. } => default.py_default.clone(),
+                _ => "_DEFAULT".to_string(),
+            },
+            DefaultValue::Literal(ln) => {
+                if can_use_literal(&ln.lit) {
+                    default.py_default.clone()
+                } else {
+                    "_DEFAULT".to_string()
+                }
+            }
+        };
+        default.is_arg_literal = default.py_default == default.arg_literal;
         Ok(())
     })
 }
 
+pub fn can_use_literal(literal: &Literal) -> bool {
+    match literal {
+        Literal::Boolean(_)
+        | Literal::String(_)
+        | Literal::UInt(_, _, _)
+        | Literal::Int(_, _, _)
+        | Literal::Float(_, _)
+        | Literal::Enum(_, _) => true,
+        Literal::Some { inner } => match &**inner {
+            DefaultValue::Literal(inner_lit) => can_use_literal(&inner_lit.lit),
+            _ => false,
+        },
+        _ => false,
+    }
+}
 pub(super) fn render_default(default: &DefaultValue) -> Result<String> {
     Ok(match default {
         DefaultValue::Default(tn) => match &tn.ty {

fixtures/keywords/rust/tests/bindings/test_keywords.py

@@ -177,6 +177,10 @@ pub struct DefaultValueNode {
     pub default: DefaultValue,
     /// The default value rendered as a Python string
     pub py_default: String,
+    /// The default value as specified as a literal in function args.
+    pub arg_literal: String,
+    /// Convenience - whether the arg literal is our literal.
+    pub is_arg_literal: bool,
 }
 
 #[derive(Debug, Clone, Node, Eq, PartialEq, Hash)]

uniffi_bindgen/src/bindings/python/pipeline/default.rs

@@ -1,7 +1,7 @@
 {%- for arg in callable.arguments -%}
     {{ arg.name }}
     {%- match arg.default %}
-    {%- when Some(literal) %}: typing.Union[object, {{ arg.ty.type_name }}] = _DEFAULT
+    {%- when Some(default) %}: {% if !default.is_arg_literal %}typing.Union[object, {{ arg.ty.type_name }}]{% else %}{{ arg.ty.type_name }}{% endif %} = {{ default.arg_literal }}
     {%- else %}: {{ arg.ty.type_name }}
     {%- endmatch %}
     {%- if !loop.last %},{% endif -%}

uniffi_bindgen/src/bindings/python/pipeline/nodes.rs

@@ -1,8 +1,10 @@
 {%- for arg in callable.arguments %}
 {%- if let Some(default) = arg.default %}
-if {{ arg.name }} is _DEFAULT:
+{%- if !default.is_arg_literal %}
+if {{ arg.name }} is {{ default.arg_literal }}:
     {{ arg.name }} = {{ default.py_default }}
 {%- endif %}
+{%- endif %}
 {{ arg.ty.ffi_converter_name }}.check_lower({{ arg.name }})
 {% endfor -%}