Code review feedback

Author: Tpt

pyo3-introspection/src/introspection.rs

@@ -101,70 +101,83 @@ fn convert_members(
                 )?);
             }
             Chunk::Class { name, id } => {
-                let (_, _, mut methods) = convert_members(
-                    chunks_by_parent
-                        .get(&id.as_str())
-                        .map(Vec::as_slice)
-                        .unwrap_or_default(),
-                    chunks_by_id,
-                    chunks_by_parent,
-                )?;
-                // We sort methods to get a stable output
-                methods.sort_by(|l, r| match l.name.cmp(&r.name) {
-                    Ordering::Equal => {
-                        // We put the getter before the setter
-                        if l.decorators.iter().any(|d| d == "property") {
-                            Ordering::Less
-                        } else if r.decorators.iter().any(|d| d == "property") {
-                            Ordering::Greater
-                        } else {
-                            // We pick an ordering based on decorators
-                            l.decorators.cmp(&r.decorators)
-                        }
-                    }
-                    o => o,
-                });
-                classes.push(Class {
-                    name: name.into(),
-                    methods,
-                })
+                classes.push(convert_class(id, name, chunks_by_id, chunks_by_parent)?)
             }
             Chunk::Function {
                 name,
                 id: _,
                 arguments,
                 parent: _,
                 decorators,
-            } => functions.push(Function {
-                name: name.into(),
-                decorators: decorators.clone(),
-                arguments: Arguments {
-                    positional_only_arguments: arguments
-                        .posonlyargs
-                        .iter()
-                        .map(convert_argument)
-                        .collect(),
-                    arguments: arguments.args.iter().map(convert_argument).collect(),
-                    vararg: arguments
-                        .vararg
-                        .as_ref()
-                        .map(convert_variable_length_argument),
-                    keyword_only_arguments: arguments
-                        .kwonlyargs
-                        .iter()
-                        .map(convert_argument)
-                        .collect(),
-                    kwarg: arguments
-                        .kwarg
-                        .as_ref()
-                        .map(convert_variable_length_argument),
-                },
-            }),
+            } => functions.push(convert_function(name, arguments, decorators)),
         }
     }
     Ok((modules, classes, functions))
 }
 
+fn convert_class(
+    id: &str,
+    name: &str,
+    chunks_by_id: &HashMap<&str, &Chunk>,
+    chunks_by_parent: &HashMap<&str, Vec<&Chunk>>,
+) -> Result<Class> {
+    let (nested_modules, nested_classes, mut methods) = convert_members(
+        chunks_by_parent
+            .get(&id)
+            .map(Vec::as_slice)
+            .unwrap_or_default(),
+        chunks_by_id,
+        chunks_by_parent,
+    )?;
+    ensure!(
+        nested_modules.is_empty(),
+        "Classes cannot contain nested modules"
+    );
+    ensure!(
+        nested_classes.is_empty(),
+        "Nested classes are not supported yet"
+    );
+    // We sort methods to get a stable output
+    methods.sort_by(|l, r| match l.name.cmp(&r.name) {
+        Ordering::Equal => {
+            // We put the getter before the setter
+            if l.decorators.iter().any(|d| d == "property") {
+                Ordering::Less
+            } else if r.decorators.iter().any(|d| d == "property") {
+                Ordering::Greater
+            } else {
+                // We pick an ordering based on decorators
+                l.decorators.cmp(&r.decorators)
+            }
+        }
+        o => o,
+    });
+    Ok(Class {
+        name: name.into(),
+        methods,
+    })
+}
+
+fn convert_function(name: &str, arguments: &ChunkArguments, decorators: &[String]) -> Function {
+    Function {
+        name: name.into(),
+        decorators: decorators.to_vec(),
+        arguments: Arguments {
+            positional_only_arguments: arguments.posonlyargs.iter().map(convert_argument).collect(),
+            arguments: arguments.args.iter().map(convert_argument).collect(),
+            vararg: arguments
+                .vararg
+                .as_ref()
+                .map(convert_variable_length_argument),
+            keyword_only_arguments: arguments.kwonlyargs.iter().map(convert_argument).collect(),
+            kwarg: arguments
+                .kwarg
+                .as_ref()
+                .map(convert_variable_length_argument),
+        },
+    }
+}
+
 fn convert_argument(arg: &ChunkArgument) -> Argument {
     Argument {
         name: arg.name.clone(),

pyo3-macros-backend/src/introspection.rs

@@ -110,7 +110,7 @@ pub fn function_introspection_code(
     if let Some(parent) = parent {
         desc.insert(
             "parent",
-            IntrospectionNode::IntrospectionId(Some(parent.clone())),
+            IntrospectionNode::IntrospectionId(Some(Cow::Borrowed(parent))),
         );
     }
     IntrospectionNode::Map(desc).emit(pyo3_crate_path)
@@ -219,7 +219,7 @@ fn argument_introspection_data<'a>(
 
 enum IntrospectionNode<'a> {
     String(Cow<'a, str>),
-    IntrospectionId(Option<Type>),
+    IntrospectionId(Option<Cow<'a, Type>>),
     Map(HashMap<&'static str, IntrospectionNode<'a>>),
     List(Vec<IntrospectionNode<'a>>),
 }
@@ -369,10 +369,12 @@ fn unique_element_id() -> u64 {
     hasher.finish()
 }
 
-fn ident_to_type(ident: &Ident) -> Type {
-    TypePath {
-        path: ident.clone().into(),
-        qself: None,
-    }
-    .into()
+fn ident_to_type(ident: &Ident) -> Cow<'static, Type> {
+    Cow::Owned(
+        TypePath {
+            path: ident.clone().into(),
+            qself: None,
+        }
+        .into(),
+    )
 }

pyo3-macros-backend/src/pymethod.rs

@@ -165,8 +165,8 @@ impl<'a> PyMethod<'a> {
         meth_attrs: &mut Vec<syn::Attribute>,
         options: PyFunctionOptions,
     ) -> Result<Self> {
-        ensure_function_options_valid(&options)?;
         check_generic(sig)?;
+        ensure_function_options_valid(&options)?;
         let spec = FnSpec::parse(sig, meth_attrs, options)?;
 
         let method_name = spec.python_name.to_string();