Inhibit pos args and kw args after *

CHANGELOG.md

@@ -37,7 +37,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 * Usage of raw identifiers with `#[pyo3(set)]`. [#745](https://github.com/PyO3/pyo3/pull/745)
 * Usage of `PyObject` with `#[pyo3(get)]`. [#760](https://github.com/PyO3/pyo3/pull/760)
 * `#[pymethods]` used in conjunction with `#[cfg]`. #[769](https://github.com/PyO3/pyo3/pull/769)
-* Interpretation of `*`. #[792](https://github.com/PyO3/pyo3/pull/792)
+* Interpretation of `*` and some unreasonable behaviors of `#[args]`. #[792](https://github.com/PyO3/pyo3/pull/792)
 
 ### Removed
 

pyo3-derive-backend/src/pyfunction.rs

@@ -65,16 +65,10 @@ impl PyFunctionAttr {
             syn::Lit::Str(ref lits) => {
                 // "*"
                 if lits.value() == "*" {
-                    if self.has_kwargs {
+                    if self.has_kwargs || self.has_varargs {
                         return Err(syn::Error::new_spanned(
                             item,
-                            "syntax error, keyword self.arguments is defined",
-                        ));
-                    }
-                    if self.has_varargs {
-                        return Err(syn::Error::new_spanned(
-                            item,
-                            "self.arguments already define * (var args)",
+                            "* is not allowed after varargs(*) or kwargs(**)",
                         ));
                     }
                     self.has_varargs = true;
@@ -94,20 +88,52 @@ impl PyFunctionAttr {
     }
 
     fn add_work(&mut self, item: &NestedMeta, path: &Path) -> syn::Result<()> {
-        // self.arguments in form somename
-        if self.has_kwargs {
+        self.pos_arg_is_ok(item)?;
+        self.arguments.push(Argument::Arg(path.clone(), None));
+        Ok(())
+    }
+
+    fn pos_arg_is_ok(&self, item: &NestedMeta) -> syn::Result<()> {
+        if self.has_kw || self.has_kwargs {
             return Err(syn::Error::new_spanned(
                 item,
-                "syntax error, keyword self.arguments is defined",
+                "Positional argument or varargs(*) is not allowed after keyword arguments",
             ));
         }
-        if self.has_kw {
+        if self.has_varargs {
             return Err(syn::Error::new_spanned(
                 item,
-                "syntax error, argument is not allowed after keyword argument",
+                "Positional argument or varargs(*) is not allowed after *",
+            ));
+        }
+        Ok(())
+    }
+
+    fn kw_arg_is_ok(&self, item: &NestedMeta) -> syn::Result<()> {
+        if self.has_kwargs {
+            return Err(syn::Error::new_spanned(
+                item,
+                "Keyword argument or kwargs(**) is not allowed after kwargs(**)",
             ));
         }
-        self.arguments.push(Argument::Arg(path.clone(), None));
+        Ok(())
+    }
+
+    fn add_nv_common(
+        &mut self,
+        item: &NestedMeta,
+        name: &syn::Path,
+        value: String,
+    ) -> syn::Result<()> {
+        self.kw_arg_is_ok(item)?;
+        if self.has_varargs {
+            // kw only
+            self.arguments.push(Argument::Kwarg(name.clone(), value));
+        } else {
+            self.has_kw = true;
+            self.arguments
+                .push(Argument::Arg(name.clone(), Some(value)));
+        }
         Ok(())
     }
 
@@ -116,75 +142,23 @@ impl PyFunctionAttr {
             syn::Lit::Str(ref litstr) => {
                 if litstr.value() == "*" {
                     // args="*"
-                    if self.has_kwargs {
-                        return Err(syn::Error::new_spanned(
-                            item,
-                            "* - syntax error, keyword self.arguments is defined",
-                        ));
-                    }
-                    if self.has_varargs {
-                        return Err(syn::Error::new_spanned(item, "*(var args) is defined"));
-                    }
+                    self.pos_arg_is_ok(item)?;
                     self.has_varargs = true;
                     self.arguments.push(Argument::VarArgs(nv.path.clone()));
                 } else if litstr.value() == "**" {
                     // kwargs="**"
-                    if self.has_kwargs {
-                        return Err(syn::Error::new_spanned(
-                            item,
-                            "self.arguments already define ** (kw args)",
-                        ));
-                    }
+                    self.kw_arg_is_ok(item)?;
                     self.has_kwargs = true;
                     self.arguments.push(Argument::KeywordArgs(nv.path.clone()));
-                } else if self.has_varargs {
-                    self.arguments
-                        .push(Argument::Kwarg(nv.path.clone(), litstr.value()))
                 } else {
-                    if self.has_kwargs {
-                        return Err(syn::Error::new_spanned(
-                            item,
-                            "syntax error, keyword self.arguments is defined",
-                        ));
-                    }
-                    self.has_kw = true;
-                    self.arguments
-                        .push(Argument::Arg(nv.path.clone(), Some(litstr.value())))
+                    self.add_nv_common(item, &nv.path, litstr.value())?;
                 }
             }
             syn::Lit::Int(ref litint) => {
-                if self.has_varargs {
-                    self.arguments
-                        .push(Argument::Kwarg(nv.path.clone(), format!("{}", litint)));
-                } else {
-                    if self.has_kwargs {
-                        return Err(syn::Error::new_spanned(
-                            item,
-                            "syntax error, keyword self.arguments is defined",
-                        ));
-                    }
-                    self.has_kw = true;
-                    self.arguments
-                        .push(Argument::Arg(nv.path.clone(), Some(format!("{}", litint))));
-                }
+                self.add_nv_common(item, &nv.path, format!("{}", litint))?;
             }
             syn::Lit::Bool(ref litb) => {
-                if self.has_varargs {
-                    self.arguments
-                        .push(Argument::Kwarg(nv.path.clone(), format!("{}", litb.value)));
-                } else {
-                    if self.has_kwargs {
-                        return Err(syn::Error::new_spanned(
-                            item,
-                            "syntax error, keyword self.arguments is defined",
-                        ));
-                    }
-                    self.has_kw = true;
-                    self.arguments.push(Argument::Arg(
-                        nv.path.clone(),
-                        Some(format!("{}", litb.value)),
-                    ));
-                }
+                self.add_nv_common(item, &nv.path, format!("{}", litb.value))?;
             }
             _ => {
                 return Err(syn::Error::new_spanned(

tests/common.rs

@@ -18,7 +18,7 @@ macro_rules! py_expect_exception {
         let res = $py.run($code, None, Some(d));
         let err = res.unwrap_err();
         if !err.matches($py, $py.get_type::<pyo3::exceptions::$err>()) {
-            panic!(format!("Expected {} but got {:?}", stringify!($err), err))
+            panic!("Expected {} but got {:?}", stringify!($err), err)
         }
     }};
 }

tests/test_compile_error.rs

@@ -1,6 +1,7 @@
 #[test]
 fn test_compile_errors() {
     let t = trybuild::TestCases::new();
+    t.compile_fail("tests/ui/invalid_macro_args.rs");
     t.compile_fail("tests/ui/invalid_property_args.rs");
     t.compile_fail("tests/ui/invalid_pymethod_names.rs");
     t.compile_fail("tests/ui/missing_clone.rs");

tests/test_methods.rs

@@ -231,8 +231,13 @@ impl MethArgs {
         [a.to_object(py), args.into(), kwargs.to_object(py)].to_object(py)
     }
 
-    #[args("*", c = 10)]
-    fn get_pos_arg_kw_sep(&self, a: i32, b: i32, c: i32) -> PyResult<i32> {
+    #[args(a, b = 2, "*", c = 3)]
+    fn get_pos_arg_kw_sep1(&self, a: i32, b: i32, c: i32) -> PyResult<i32> {
+        Ok(a + b + c)
+    }
+
+    #[args(a, "*", b = 2, c = 3)]
+    fn get_pos_arg_kw_sep2(&self, a: i32, b: i32, c: i32) -> PyResult<i32> {
         Ok(a + b + c)
     }
 
@@ -299,15 +304,22 @@ fn meth_args() {
     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_arg_kw_sep(1, 2, c=3) == 6");
-    py_run!(py, inst, "assert inst.get_pos_arg_kw_sep(1, 2) == 13");
-    py_expect_exception!(py, inst, "assert inst.get_pos_arg_kw_sep(1)", TypeError);
-    py_expect_exception!(
+    py_run!(py, inst, "assert inst.get_pos_arg_kw_sep1(1) == 6");
+    py_run!(py, inst, "assert inst.get_pos_arg_kw_sep1(1, 2) == 6");
+    py_run!(
+        py,
+        inst,
+        "assert inst.get_pos_arg_kw_sep1(1, 2, c=13) == 16"
+    );
+    py_expect_exception!(py, inst, "inst.get_pos_arg_kw_sep1(1, 2, 3)", TypeError);
+
+    py_run!(py, inst, "assert inst.get_pos_arg_kw_sep2(1) == 6");
+    py_run!(
         py,
         inst,
-        "assert inst.get_pos_arg_kw_sep(1, 2, 3)",
-        TypeError
+        "assert inst.get_pos_arg_kw_sep2(1, b=12, c=13) == 26"
     );
+    py_expect_exception!(py, inst, "inst.get_pos_arg_kw_sep2(1, 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);

tests/test_module.rs

@@ -246,15 +246,14 @@ fn test_module_nesting() {
 }
 
 // Test that argument parsing specification works for pyfunctions
-
-#[pyfunction(a = 5, vararg = "*")]
+#[pyfunction(a, 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 = "*")]
+    #[pyfn(m, "int_vararg_fn", a, vararg = "*")]
     fn int_vararg_fn(py: Python, a: i32, vararg: &PyTuple) -> PyObject {
         ext_vararg_fn(py, a, vararg)
     }
@@ -270,9 +269,9 @@ fn test_vararg_module() {
     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) == [1, ()]");
     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) == [1, ()]");
     py_assert!(py, m, "m.int_vararg_fn(1, 2) == [1, (2,)]");
 }

tests/ui/invalid_macro_args.rs

@@ -0,0 +1,23 @@
+use pyo3::prelude::*;
+
+#[pyfunction(a = 5, b)]
+fn pos_after_kw(py: Python, a: i32, b: i32) -> PyObject {
+    [a.to_object(py), vararg.into()].to_object(py)
+}
+
+#[pyfunction(a, "*", b)]
+fn pos_after_separator(py: Python, a: i32, b: i32) -> PyObject {
+    [a.to_object(py), vararg.into()].to_object(py)
+}
+
+#[pyfunction(a = 5, vararg = "*")]
+fn args_after_kw(py: Python, a: i32, vararg: &PyTuple) -> PyObject {
+    [a.to_object(py), vararg.into()].to_object(py)
+}
+
+#[pyfunction(kwargs = "**", a = 5)]
+fn kw_after_kwargs(py: Python, kwargs: &PyDict, a: i32) -> PyObject {
+    [a.to_object(py), vararg.into()].to_object(py)
+}
+
+fn main() {}

tests/ui/invalid_macro_args.stderr

@@ -0,0 +1,23 @@
+error: Positional argument or varargs(*) is not allowed after keyword arguments
+ --> $DIR/invalid_macro_args.rs:3:21
+  |
+3 | #[pyfunction(a = 5, b)]
+  |                     ^
+
+error: Positional argument or varargs(*) is not allowed after *
+ --> $DIR/invalid_macro_args.rs:8:22
+  |
+8 | #[pyfunction(a, "*", b)]
+  |                      ^
+
+error: Positional argument or varargs(*) is not allowed after keyword arguments
+  --> $DIR/invalid_macro_args.rs:13:21
+   |
+13 | #[pyfunction(a = 5, vararg = "*")]
+   |                     ^^^^^^^^^^^^
+
+error: Keyword argument or kwargs(**) is not allowed after kwargs(**)
+  --> $DIR/invalid_macro_args.rs:18:29
+   |
+18 | #[pyfunction(kwargs = "**", a = 5)]
+   |                             ^^^^^