Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the interpretation of vararg separator #792

Merged
merged 4 commits into from
Mar 7, 2020
Merged

Fix the interpretation of vararg separator #792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
25069ba
Fix the interpretation of '*'
kngwyu Mar 4, 2020
cea707d
Inhibit positional args after *
kngwyu Mar 6, 2020
26fe29f
Some refactorings for pyfunction.rs
kngwyu Mar 6, 2020
b7c4fdb
Update CHANGELOG.md
kngwyu Mar 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +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)
* `"*"` in a `#[pyfunction()]` argument list incorrectly accepting any number of positional arguments (use `args = "*"` when this behaviour is desired). #[792](https://github.com/PyO3/pyo3/pull/792)

### Removed

Expand Down
20 changes: 0 additions & 20 deletions pyo3-derive-backend/src/method.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,17 +206,6 @@ impl<'a> FnSpec<'a> {
false
}

pub fn accept_args(&self) -> bool {
for s in self.attrs.iter() {
match *s {
Argument::VarArgs(_) => return true,
Argument::VarArgsSeparator => return true,
_ => (),
}
}
false
}

pub fn is_kwargs(&self, name: &syn::Ident) -> bool {
for s in self.attrs.iter() {
if let Argument::KeywordArgs(ref path) = s {
Expand All @@ -226,15 +215,6 @@ impl<'a> FnSpec<'a> {
false
}

pub fn accept_kwargs(&self) -> bool {
for s in self.attrs.iter() {
if let Argument::KeywordArgs(_) = s {
return true;
}
}
false
}

pub fn default_value(&self, name: &syn::Ident) -> Option<TokenStream> {
for s in self.attrs.iter() {
match *s {
Expand Down
161 changes: 64 additions & 97 deletions pyo3-derive-backend/src/pyfunction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,139 +52,109 @@ impl PyFunctionAttr {
NestedMeta::Lit(ref lit) => {
self.add_literal(item, lit)?;
}
_ => {
return Err(syn::Error::new_spanned(item, "Unknown argument"));
NestedMeta::Meta(syn::Meta::List(ref list)) => {
return Err(syn::Error::new_spanned(
list,
"List is not supported as argument",
));
Comment on lines +55 to +59
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :)

}
}

Ok(())
}

fn add_literal(&mut self, item: &NestedMeta, lit: &syn::Lit) -> syn::Result<()> {
match lit {
syn::Lit::Str(ref lits) => {
syn::Lit::Str(ref lits) if lits.value() == "*" => {
// "*"
if lits.value() == "*" {
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,
"self.arguments already define * (var args)",
));
}
self.has_varargs = true;
self.arguments.push(Argument::VarArgsSeparator);
} else {
return Err(syn::Error::new_spanned(lits, "Unknown string literal"));
}
}
_ => {
return Err(syn::Error::new_spanned(
item,
format!("Only string literal is supported, got: {:?}", lit),
));
self.vararg_is_ok(item)?;
self.has_varargs = true;
self.arguments.push(Argument::VarArgsSeparator);
Ok(())
}
};
Ok(())
_ => Err(syn::Error::new_spanned(
item,
format!("Only \"*\" is supported here, got: {:?}", lit),
)),
}
}

fn add_work(&mut self, item: &NestedMeta, path: &Path) -> syn::Result<()> {
// self.arguments in form somename
if self.has_kwargs {
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 *",
));
}
self.arguments.push(Argument::Arg(path.clone(), None));
Ok(())
}

fn vararg_is_ok(&self, item: &NestedMeta) -> syn::Result<()> {
if self.has_kwargs || self.has_varargs {
return Err(syn::Error::new_spanned(
item,
"* is not allowed after varargs(*) or kwargs(**)",
));
}
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(**)",
));
}
Ok(())
}

fn add_nv_common(
Copy link
Member Author

@kngwyu kngwyu Mar 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I separated this function for refactoring.
The logic is not changed except *, **, name=lit is inhibited(the old code does

        if self.has_varargs {
           // No check here
           ...
        } else {
            self.kw_arg_is_ok(item)?;
            ...
        }

).

&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(())
}

fn add_name_value(&mut self, item: &NestedMeta, nv: &syn::MetaNameValue) -> syn::Result<()> {
match nv.lit {
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.vararg_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(
Expand Down Expand Up @@ -221,11 +191,8 @@ pub fn parse_name_attribute(attrs: &mut Vec<syn::Attribute>) -> syn::Result<Opti
*span,
"Expected string literal for #[name] argument",
)),
// TODO: The below pattern is unstable, so instead we match the wildcard.
// slice_patterns due to be stable soon: https://github.com/rust-lang/rust/issues/62254
// [(_, span), _, ..] => {
_ => Err(syn::Error::new(
name_attrs[0].1,
[(_, span), ..] => Err(syn::Error::new(
*span,
"#[name] can not be specified multiple times",
)),
}
Expand Down
25 changes: 12 additions & 13 deletions pyo3-derive-backend/src/pymethod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,15 +406,6 @@ fn impl_call(_cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream {
quote! { _slf.#fname(#(#names),*) }
}

/// Converts a bool to "true" or "false"
fn bool_to_ident(condition: bool) -> syn::Ident {
Copy link
Member Author

@kngwyu kngwyu Mar 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this function since just quote! { #bool_value } works.

if condition {
syn::Ident::new("true", Span::call_site())
} else {
syn::Ident::new("false", Span::call_site())
}
}

fn impl_arg_params_(spec: &FnSpec<'_>, body: TokenStream, into_result: TokenStream) -> TokenStream {
if spec.args.is_empty() {
return quote! {
Expand All @@ -431,8 +422,8 @@ fn impl_arg_params_(spec: &FnSpec<'_>, body: TokenStream, into_result: TokenStre
continue;
}
let name = arg.name;
let kwonly = bool_to_ident(spec.is_kw_only(&arg.name));
let opt = bool_to_ident(arg.optional.is_some() || spec.default_value(&arg.name).is_some());
let kwonly = spec.is_kw_only(&arg.name);
let opt = arg.optional.is_some() || spec.default_value(&arg.name).is_some();

params.push(quote! {
pyo3::derive_utils::ParamDescription {
Expand All @@ -449,8 +440,16 @@ fn impl_arg_params_(spec: &FnSpec<'_>, body: TokenStream, into_result: TokenStre
param_conversion.push(impl_arg_param(&arg, &spec, idx, &mut option_pos));
}

let accept_args = bool_to_ident(spec.accept_args());
let accept_kwargs = bool_to_ident(spec.accept_kwargs());
let (mut accept_args, mut accept_kwargs) = (false, false);

for s in spec.attrs.iter() {
use crate::pyfunction::Argument;
match s {
Argument::VarArgs(_) => accept_args = true,
Argument::KeywordArgs(_) => accept_kwargs = true,
_ => continue,
}
}
let num_normal_params = params.len();
// create array of arguments, and then parse
quote! {
Expand Down
2 changes: 1 addition & 1 deletion src/derive_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ pub fn parse_fn_args<'p>(
if i < nargs {
raise_error!("got multiple values for argument: {}", p.name)
}
kwargs.as_ref().unwrap().del_item(p.name).unwrap();
kwargs.as_ref().unwrap().del_item(p.name)?;
Some(kwarg)
}
None => {
Expand Down
2 changes: 1 addition & 1 deletion tests/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}};
}
1 change: 1 addition & 0 deletions tests/test_compile_error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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");
Expand Down
30 changes: 28 additions & 2 deletions tests/test_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,11 +231,20 @@ impl MethArgs {
[a.to_object(py), args.into(), kwargs.to_object(py)].to_object(py)
}

#[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)
}

#[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 {
Expand Down Expand Up @@ -264,7 +273,7 @@ fn meth_args() {
py_run!(py, inst, "assert inst.get_default() == 10");
py_run!(py, inst, "assert inst.get_default(100) == 100");
py_run!(py, inst, "assert inst.get_kwarg() == 10");
py_run!(py, inst, "assert inst.get_kwarg(100) == 10");
py_expect_exception!(py, inst, "inst.get_kwarg(100)", TypeError);
py_run!(py, inst, "assert inst.get_kwarg(test=100) == 100");
py_run!(py, inst, "assert inst.get_kwargs() == [(), None]");
py_run!(py, inst, "assert inst.get_kwargs(1,2,3) == [(1,2,3), None]");
Expand Down Expand Up @@ -295,6 +304,23 @@ 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_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_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);

Expand Down
Loading