Merge pull request #450 from birkenfeld/parse_args_msg

Adjust the varargs/kwds objects to remove arguments consumed by parameters

CHANGELOG.md

@@ -56,6 +56,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
    the items are not hashable.
  * Fixed building using `venv` on Windows.
  * `PyTuple::new` now returns `&PyTuple` instead of `Py<PyTuple>`.
+ * Fixed several issues with argument parsing; notable, the `*args` and `**kwargs`
+   tuple/dict now doesn't contain arguments that are otherwise assigned to parameters.
 
 ## [0.6.0] - 2018-03-28
 

guide/src/function.md

@@ -46,6 +46,34 @@ fn module_with_functions(py: Python, m: &PyModule) -> PyResult<()> {
 # fn main() {}
 ```
 
+## Argument parsing
+
+Both the `#[pyfunction]` and `#[pyfn]` attributes support specifying details of
+argument parsing.  The details are given in the section "Method arguments" in
+the [Classes](class.md) chapter.  Here is an example for a function that accepts
+arbitrary keyword arguments (`**kwargs` in Python syntax) and returns the number
+that was passed:
+
+```rust
+# extern crate pyo3;
+use pyo3::prelude::*;
+use pyo3::wrap_pyfunction;
+use pyo3::types::PyDict;
+
+#[pyfunction(kwds="**")]
+fn num_kwds(kwds: Option<&PyDict>) -> usize {
+    kwds.map_or(0, |dict| dict.len())
+}
+
+#[pymodule]
+fn module_with_functions(py: Python, m: &PyModule) -> PyResult<()> {
+    m.add_wrapped(wrap_pyfunction!(num_kwds)).unwrap();
+    Ok(())
+}
+
+# fn main() {}
+```
+
 ### Making the function signature available to Python
 
 In order to make the function signature available to Python to be retrieved via

pyo3-derive-backend/src/pymethod.rs

@@ -470,13 +470,15 @@ pub fn impl_arg_params(spec: &FnSpec<'_>, body: TokenStream) -> TokenStream {
         ];
 
         let mut output = [#(#placeholders),*];
+        let mut _args = _args;
+        let mut _kwargs = _kwargs;
 
         // Workaround to use the question mark operator without rewriting everything
         let _result = (|| {
-            pyo3::derive_utils::parse_fn_args(
+            let (_args, _kwargs) = pyo3::derive_utils::parse_fn_args(
                 Some(_LOCATION),
                 PARAMS,
-                &_args,
+                _args,
                 _kwargs,
                 #accept_args,
                 #accept_kwargs,

src/derive_utils.rs

@@ -7,9 +7,9 @@
 use crate::err::PyResult;
 use crate::exceptions::TypeError;
 use crate::init_once;
-use crate::types::{PyAny, PyDict, PyModule, PyString, PyTuple};
+use crate::instance::PyNativeType;
+use crate::types::{PyAny, PyDict, PyModule, PyTuple};
 use crate::GILPool;
-use crate::PyTryFrom;
 use crate::Python;
 use crate::{ffi, IntoPy, PyObject};
 use std::ptr;
@@ -41,72 +41,76 @@ pub fn parse_fn_args<'p>(
     accept_args: bool,
     accept_kwargs: bool,
     output: &mut [Option<&'p PyAny>],
-) -> PyResult<()> {
+) -> PyResult<(&'p PyTuple, Option<&'p PyDict>)> {
     let nargs = args.len();
-    let nkeywords = kwargs.map_or(0, PyDict::len);
-    if !accept_args && !accept_kwargs && (nargs + nkeywords > params.len()) {
-        return Err(TypeError::py_err(format!(
-            "{}{} takes at most {} argument{} ({} given)",
-            fname.unwrap_or("function"),
-            if fname.is_some() { "()" } else { "" },
-            params.len(),
-            if params.len() == 1 { "s" } else { "" },
-            nargs + nkeywords
-        )));
+    let mut used_args = 0;
+    macro_rules! raise_error {
+        ($s: expr $(,$arg:expr)*) => (return Err(TypeError::py_err(format!(
+            concat!("{} ", $s), fname.unwrap_or("function") $(,$arg)*
+        ))))
     }
-    let mut used_keywords = 0;
+    // Copy kwargs not to modify it
+    let kwargs = match kwargs {
+        Some(k) => Some(k.copy()?),
+        None => None,
+    };
     // Iterate through the parameters and assign values to output:
     for (i, (p, out)) in params.iter().zip(output).enumerate() {
-        match kwargs.and_then(|d| d.get_item(p.name)) {
+        *out = match kwargs.and_then(|d| d.get_item(p.name)) {
             Some(kwarg) => {
-                *out = Some(kwarg);
-                used_keywords += 1;
                 if i < nargs {
-                    return Err(TypeError::py_err(format!(
-                        "Argument given by name ('{}') and position ({})",
-                        p.name,
-                        i + 1
-                    )));
+                    raise_error!("got multiple values for argument: {}", p.name)
                 }
+                kwargs.as_ref().unwrap().del_item(p.name).unwrap();
+                Some(kwarg)
             }
             None => {
                 if p.kw_only {
                     if !p.is_optional {
-                        return Err(TypeError::py_err(format!(
-                            "Required argument ('{}') is keyword only argument",
-                            p.name
-                        )));
+                        raise_error!("missing required keyword-only argument: {}", p.name)
                     }
-                    *out = None;
+                    None
                 } else if i < nargs {
-                    *out = Some(args.get_item(i));
+                    used_args += 1;
+                    Some(args.get_item(i))
                 } else {
-                    *out = None;
                     if !p.is_optional {
-                        return Err(TypeError::py_err(format!(
-                            "Required argument ('{}') (pos {}) not found",
-                            p.name,
-                            i + 1
-                        )));
+                        raise_error!("missing required positional argument: {}", p.name)
                     }
+                    None
                 }
             }
         }
     }
-    if !accept_kwargs && used_keywords != nkeywords {
-        // check for extraneous keyword arguments
-        for item in kwargs.unwrap().items().iter() {
-            let item = <PyTuple as PyTryFrom>::try_from(item)?;
-            let key = <PyString as PyTryFrom>::try_from(item.get_item(0))?.to_string()?;
-            if !params.iter().any(|p| p.name == key) {
-                return Err(TypeError::py_err(format!(
-                    "'{}' is an invalid keyword argument for this function",
-                    key
-                )));
-            }
-        }
+    let is_kwargs_empty = kwargs.as_ref().map_or(true, |dict| dict.is_empty());
+    // Raise an error when we get an unknown key
+    if !accept_kwargs && !is_kwargs_empty {
+        let (key, _) = kwargs.unwrap().iter().next().unwrap();
+        raise_error!("got an unexpected keyword argument: {}", key)
     }
-    Ok(())
+    // Raise an error when we get too many positional args
+    if !accept_args && used_args < nargs {
+        raise_error!(
+            "takes at most {} positional argument{} ({} given)",
+            used_args,
+            if used_args == 1 { "" } else { "s" },
+            nargs
+        )
+    }
+    // Adjust the remaining args
+    let args = if accept_args {
+        let py = args.py();
+        let slice = args.slice(used_args as isize, nargs as isize).into_py(py);
+        py.checked_cast_as(slice).unwrap()
+    } else {
+        args
+    };
+    let kwargs = if accept_kwargs && is_kwargs_empty {
+        None
+    } else {
+        kwargs
+    };
+    Ok((args, kwargs))
 }
 
 /// Builds a module (or null) from a user given initializer. Used for `#[pymodule]`.

tests/test_methods.rs

@@ -232,6 +232,28 @@ impl MethArgs {
     ) -> PyResult<PyObject> {
         Ok([args.into(), kwargs.to_object(py)].to_object(py))
     }
+
+    #[args(args = "*", kwargs = "**")]
+    fn get_pos_arg_kw(
+        &self,
+        py: Python,
+        a: i32,
+        args: &PyTuple,
+        kwargs: Option<&PyDict>,
+    ) -> PyObject {
+        [a.to_object(py), args.into(), kwargs.to_object(py)].to_object(py)
+    }
+
+    #[args(kwargs = "**")]
+    fn get_pos_kw(&self, py: Python, a: i32, kwargs: Option<&PyDict>) -> PyObject {
+        [a.to_object(py), kwargs.to_object(py)].to_object(py)
+    }
+
+    // "args" can be anything that can be extracted from PyTuple
+    #[args(args = "*")]
+    fn args_as_vec(&self, args: Vec<i32>) -> i32 {
+        args.iter().sum()
+    }
 }
 
 #[test]
@@ -259,6 +281,27 @@ fn meth_args() {
         inst,
         "assert inst.get_kwargs(1,2,3,t=1,n=2) == [(1,2,3), {'t': 1, 'n': 2}]"
     );
+
+    py_run!(py, inst, "assert inst.get_pos_arg_kw(1) == [1, (), None]");
+    py_run!(
+        py,
+        inst,
+        "assert inst.get_pos_arg_kw(1, 2, 3) == [1, (2, 3), None]"
+    );
+    py_run!(
+        py,
+        inst,
+        "assert inst.get_pos_arg_kw(1, b=2) == [1, (), {'b': 2}]"
+    );
+    py_run!(py, inst, "assert inst.get_pos_arg_kw(a=1) == [1, (), None]");
+    py_expect_exception!(py, inst, "inst.get_pos_arg_kw()", TypeError);
+    py_expect_exception!(py, inst, "inst.get_pos_arg_kw(1, a=1)", TypeError);
+    py_expect_exception!(py, inst, "inst.get_pos_arg_kw(b=2)", TypeError);
+
+    py_run!(py, inst, "assert inst.get_pos_kw(1, b=2) == [1, {'b': 2}]");
+    py_expect_exception!(py, inst, "inst.get_pos_kw(1,2)", TypeError);
+
+    py_run!(py, inst, "assert inst.args_as_vec(1,2,3) == 6");
 }
 
 #[pyclass]

tests/test_module.rs

@@ -1,6 +1,6 @@
 use pyo3::prelude::*;
 
-use pyo3::types::IntoPyDict;
+use pyo3::types::{IntoPyDict, PyTuple};
 
 mod common;
 
@@ -194,3 +194,35 @@ fn test_module_nesting() {
         "supermodule.submodule.subfunction() == 'Subfunction'"
     );
 }
+
+// Test that argument parsing specification works for pyfunctions
+
+#[pyfunction(a = 5, vararg = "*")]
+fn ext_vararg_fn(py: Python, a: i32, vararg: &PyTuple) -> PyObject {
+    [a.to_object(py), vararg.into()].to_object(py)
+}
+
+#[pymodule]
+fn vararg_module(_py: Python, m: &PyModule) -> PyResult<()> {
+    #[pyfn(m, "int_vararg_fn", a = 5, vararg = "*")]
+    fn int_vararg_fn(py: Python, a: i32, vararg: &PyTuple) -> PyObject {
+        ext_vararg_fn(py, a, vararg)
+    }
+
+    m.add_wrapped(pyo3::wrap_pyfunction!(ext_vararg_fn))
+        .unwrap();
+    Ok(())
+}
+
+#[test]
+fn test_vararg_module() {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+    let m = pyo3::wrap_pymodule!(vararg_module)(py);
+
+    py_assert!(py, m, "m.ext_vararg_fn() == [5, ()]");
+    py_assert!(py, m, "m.ext_vararg_fn(1, 2) == [1, (2,)]");
+
+    py_assert!(py, m, "m.int_vararg_fn() == [5, ()]");
+    py_assert!(py, m, "m.int_vararg_fn(1, 2) == [1, (2,)]");
+}