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

Also fix some other validation issues and add more tests.

fixes #420

CHANGELOG.md

@@ -44,6 +44,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
 

pyo3-derive-backend/src/pymethod.rs

@@ -450,14 +450,17 @@ 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(
+                _py,
                 Some(_LOCATION),
                 PARAMS,
-                &_args,
+                &mut _args,
-                _kwargs,
+                &mut _kwargs,
                 #accept_args,
                 #accept_kwargs,
                 &mut output

src/derive_utils.rs

@@ -8,10 +8,10 @@ use crate::err::PyResult;
 use crate::exceptions::TypeError;
 use crate::ffi;
 use crate::init_once;
-use crate::types::{PyAny, PyDict, PyModule, PyString, PyTuple};
+use crate::types::{PyAny, PyDict, PyModule, PyTuple};
 use crate::GILPool;
+use crate::IntoPyObject;
 use crate::Python;
-use crate::{IntoPyObject, PyTryFrom};
 use std::ptr;
 
 /// Description of a python parameter; used for `parse_args()`.
@@ -34,75 +34,88 @@ pub struct ParamDescription {
 /// * output: Output array that receives the arguments.
 ///           Must have same length as `params` and must be initialized to `None`.
 pub fn parse_fn_args<'p>(
+    py: Python<'p>,
     fname: Option<&str>,
     params: &[ParamDescription],
-    args: &'p PyTuple,
+    args: &mut &'p PyTuple,
-    kwargs: Option<&'p PyDict>,
+    kwargs: &mut Option<&'p PyDict>,
     accept_args: bool,
     accept_kwargs: bool,
     output: &mut [Option<&'p PyAny>],
 ) -> PyResult<()> {
     let nargs = args.len();
-    let nkeywords = kwargs.map_or(0, PyDict::len);
+    let mut used_args = 0;
-    if !accept_args && !accept_kwargs && (nargs + nkeywords > params.len()) {
-        return Err(TypeError::py_err(format!(
-            "{} takes at most {} argument{} ({} given)",
-            fname.unwrap_or("function"),
-            params.len(),
-            if params.len() == 1 { "" } else { "s" },
-            nargs + nkeywords
-        )));
-    }
-    let mut used_keywords = 0;
     // 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)) {
             Some(kwarg) => {
                 *out = Some(kwarg);
-                used_keywords += 1;
                 if i < nargs {
                     return Err(TypeError::py_err(format!(
-                        "Argument given by name ('{}') and position ({})",
+                        "{} got multiple values for argument '{}'",
-                        p.name,
+                        fname.unwrap_or("function"),
-                        i + 1
+                        p.name
                     )));
                 }
+                kwargs.as_ref().unwrap().del_item(p.name).unwrap();
             }
             None => {
                 if p.kw_only {
                     if !p.is_optional {
                         return Err(TypeError::py_err(format!(
-                            "Required argument ('{}') is keyword only argument",
+                            "{} missing required keyword-only argument '{}'",
+                            fname.unwrap_or("function"),
                             p.name
                         )));
                     }
                     *out = None;
                 } else if i < nargs {
                     *out = Some(args.get_item(i));
+                    used_args += 1;
                 } else {
                     *out = None;
                     if !p.is_optional {
                         return Err(TypeError::py_err(format!(
-                            "Required argument ('{}') (pos {}) not found",
+                            "{} missing required positional argument: '{}'",
-                            p.name,
+                            fname.unwrap_or("function"),
-                            i + 1
+                            p.name
                         )));
                     }
                 }
             }
         }
     }
-    if !accept_kwargs && used_keywords != nkeywords {
+    // check for extraneous keyword arguments
-        // check for extraneous keyword arguments
+    if !accept_kwargs && !kwargs.as_ref().map_or(true, |d| d.is_empty()) {
-        for item in kwargs.unwrap().items().iter() {
+        for (key, _) in kwargs.unwrap().iter() {
-            let item = <PyTuple as PyTryFrom>::try_from(item)?;
+            // raise an error with any of the keys
-            let key = <PyString as PyTryFrom>::try_from(item.get_item(0))?.to_string()?;
+            return Err(TypeError::py_err(format!(
-            if !params.iter().any(|p| p.name == key) {
+                "{} got an unexpected keyword argument '{}'",
-                return Err(TypeError::py_err(format!(
+                fname.unwrap_or("function"),
-                    "'{}' is an invalid keyword argument for this function",
+                key
-                    key
+            )));
-                )));
+        }
-            }
+    }
+    // check for extraneous positional arguments
+    if !accept_args && used_args < nargs {
+        return Err(TypeError::py_err(format!(
+            "{} takes at most {} positional argument{} ({} given)",
+            fname.unwrap_or("function"),
+            used_args,
+            if used_args == 1 { "" } else { "s" },
+            nargs
+        )));
+    }
+    // adjust the remaining args
+    if accept_args {
+        let slice = args
+            .slice(used_args as isize, nargs as isize)
+            .into_object(py);
+        *args = py.checked_cast_as(slice).unwrap();
+    }
+    if accept_kwargs {
+        if kwargs.map_or(false, |d| d.is_empty()) {
+            *kwargs = None;
         }
     }
     Ok(())

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]