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.

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

Positional-only args #1925

Merged
merged 10 commits into from Oct 19, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Add commonly-used sequence methods to `PyList` and `PyTuple`. [#1849](https://github.com/PyO3/pyo3/pull/1849)
- Add `as_sequence` methods to `PyList` and `PyTuple`. [#1860](https://github.com/PyO3/pyo3/pull/1860)
- Add `abi3-py310` feature. [#1889](https://github.com/PyO3/pyo3/pull/1889)
- Add support for positional-only arguments in `#[pyfunction]` [#1439](https://github.com/PyO3/pyo3/issues/1439)
aganders3 marked this conversation as resolved.
Show resolved Hide resolved

### Changed

Expand Down
11 changes: 11 additions & 0 deletions pyo3-macros-backend/src/method.rs
Expand Up @@ -449,6 +449,17 @@ impl<'a> FnSpec<'a> {
None
}

pub fn is_pos_only(&self, name: &syn::Ident) -> bool {
for s in self.attrs.iter() {
if let Argument::PosOnlyArg(path, _) = s {
if path.is_ident(name) {
return true;
}
}
}
false
}

pub fn is_kw_only(&self, name: &syn::Ident) -> bool {
for s in self.attrs.iter() {
if let Argument::Kwarg(path, _) = s {
Expand Down
8 changes: 6 additions & 2 deletions pyo3-macros-backend/src/params.rs
Expand Up @@ -78,6 +78,7 @@ pub fn impl_arg_params(
};

let mut positional_parameter_names = Vec::new();
let mut positional_only_parameters = 0usize;
let mut required_positional_parameters = 0usize;
let mut keyword_only_parameters = Vec::new();

Expand All @@ -86,6 +87,7 @@ pub fn impl_arg_params(
continue;
}
let name = arg.name.unraw().to_string();
let posonly = spec.is_pos_only(arg.name);
let kwonly = spec.is_kw_only(arg.name);
let required = !(arg.optional.is_some() || spec.default_value(arg.name).is_some());

Expand All @@ -100,6 +102,9 @@ pub fn impl_arg_params(
if required {
required_positional_parameters += 1;
}
if posonly {
positional_only_parameters += 1;
aganders3 marked this conversation as resolved.
Show resolved Hide resolved
}
positional_parameter_names.push(name);
}
}
Expand Down Expand Up @@ -154,8 +159,7 @@ pub fn impl_arg_params(
cls_name: #cls_name,
func_name: stringify!(#python_name),
positional_parameter_names: &[#(#positional_parameter_names),*],
// TODO: https://github.com/PyO3/pyo3/issues/1439 - support specifying these
positional_only_parameters: 0,
positional_only_parameters: #positional_only_parameters,
required_positional_parameters: #required_positional_parameters,
keyword_only_parameters: &[#(#keyword_only_parameters),*],
accept_varargs: #accept_args,
Expand Down
26 changes: 25 additions & 1 deletion pyo3-macros-backend/src/pyfunction.rs
Expand Up @@ -22,9 +22,11 @@ use syn::{

#[derive(Debug, Clone, PartialEq)]
pub enum Argument {
PosOnlyArgsSeparator,
VarArgsSeparator,
VarArgs(syn::Path),
KeywordArgs(syn::Path),
PosOnlyArg(syn::Path, Option<String>),
Arg(syn::Path, Option<String>),
Kwarg(syn::Path, Option<String>),
}
Expand All @@ -34,6 +36,7 @@ pub enum Argument {
pub struct PyFunctionSignature {
pub arguments: Vec<Argument>,
has_kw: bool,
has_posonly_args: bool,
has_varargs: bool,
has_kwargs: bool,
}
Expand Down Expand Up @@ -126,7 +129,20 @@ impl PyFunctionSignature {
self.arguments.push(Argument::VarArgsSeparator);
Ok(())
}
_ => bail_spanned!(item.span() => "expected \"*\""),
syn::Lit::Str(lits) if lits.value() == "/" => {
// "/"
self.posonly_arg_is_ok(item)?;
self.has_posonly_args = true;
// any arguments _before_ this become positional-only
self.arguments.iter_mut().for_each(|a| {
if let Argument::Arg(path, name) = a {
*a = Argument::PosOnlyArg(path.clone(), name.clone());
}
birkenfeld marked this conversation as resolved.
Show resolved Hide resolved
});
self.arguments.push(Argument::PosOnlyArgsSeparator);
Ok(())
}
_ => bail_spanned!(item.span() => "expected \"/\" or \"*\""),
}
}

Expand All @@ -143,6 +159,14 @@ impl PyFunctionSignature {
Ok(())
}

fn posonly_arg_is_ok(&self, item: &NestedMeta) -> syn::Result<()> {
ensure_spanned!(
!(self.has_posonly_args || self.has_kwargs || self.has_varargs),
item.span() => "/ is not allowed after /, varargs(*), or kwargs(**)"
);
Ok(())
}

fn vararg_is_ok(&self, item: &NestedMeta) -> syn::Result<()> {
ensure_spanned!(
!(self.has_kwargs || self.has_varargs),
Expand Down
141 changes: 141 additions & 0 deletions tests/test_methods.rs
Expand Up @@ -215,6 +215,36 @@ impl MethArgs {
[a.to_object(py), args.into(), kwargs.to_object(py)].to_object(py)
}

#[args(a, b, "/")]
fn get_posargs_only(&self, a: i32, b: i32) -> i32 {
a + b
}

#[args(a, "/", b)]
fn get_posargs_only_with_posargs(&self, a: i32, b: i32) -> i32 {
birkenfeld marked this conversation as resolved.
Show resolved Hide resolved
a + b
}

#[args(a, "/", b, c = 5)]
fn get_posargs_only_with_posargs_and_kwargs(&self, a: i32, b: i32, c: i32) -> i32 {
a + b + c
}

#[args(a, "/", b, "*", c, d = 5)]
fn get_all_arg_types_together(&self, a: i32, b: i32, c: i32, d: i32) -> i32 {
a + b + c + d
}

#[args(a, "/", "*", b)]
fn get_posargs_only_and_kwargs_only(&self, a: i32, b: i32) -> i32 {
a + b
}

#[args(a, "/", "*", b = 3)]
fn get_posargs_only_and_kwargs_only_with_defaults(&self, a: i32, b: i32) -> i32 {
a + b
}

birkenfeld marked this conversation as resolved.
Show resolved Hide resolved
#[args("*", a = 2, b = 3)]
fn get_kwargs_only_with_defaults(&self, a: i32, b: i32) -> i32 {
a + b
Expand Down Expand Up @@ -308,6 +338,117 @@ fn meth_args() {
py_expect_exception!(py, inst, "inst.get_pos_arg_kw(1, a=1)", PyTypeError);
py_expect_exception!(py, inst, "inst.get_pos_arg_kw(b=2)", PyTypeError);

py_run!(py, inst, "assert inst.get_posargs_only(3, 5) == 8");
py_expect_exception!(
py,
inst,
"assert inst.get_posargs_only(a = 3, b = 5)",
PyTypeError
);
py_expect_exception!(
py,
inst,
"assert inst.get_posargs_only(3, b = 5)",
PyTypeError
);

py_run!(
py,
inst,
"assert inst.get_posargs_only_with_posargs(1, 2) == 3"
);
py_run!(
py,
inst,
"assert inst.get_posargs_only_with_posargs(1, b = 2) == 3"
);
py_expect_exception!(
py,
inst,
"assert inst.get_posargs_only_with_posargs(a = 1, b = 2)",
PyTypeError
);
py_run!(
py,
inst,
"assert inst.get_posargs_only_with_posargs_and_kwargs(1, 2) == 8"
);
py_run!(
py,
inst,
"assert inst.get_posargs_only_with_posargs_and_kwargs(1, 2, c = 3) == 6"
);
py_expect_exception!(
py,
inst,
"assert inst.get_posargs_only_with_posargs_and_kwargs(a = 1, b = 2)",
PyTypeError
);

py_run!(
py,
inst,
"assert inst.get_all_arg_types_together(1, 2, c = 3, d = 3) == 9"
);
py_run!(
py,
inst,
"assert inst.get_all_arg_types_together(1, 2, c = 3) == 11"
);
py_expect_exception!(
py,
inst,
"assert inst.get_all_arg_types_together(a = 1, b = 2, d = 3)",
PyTypeError
);

py_run!(
py,
inst,
"assert inst.get_posargs_only_and_kwargs_only(3, b = 5) == 8"
);
py_expect_exception!(
py,
inst,
"assert inst.get_posargs_only_and_kwargs_only(3, 5)",
PyTypeError
);
py_expect_exception!(
py,
inst,
"assert inst.get_posargs_only_and_kwargs_only(a = 3, b = 5)",
PyTypeError
);

py_run!(
py,
inst,
"assert inst.get_posargs_only_and_kwargs_only_with_defaults(3) == 6"
);
py_run!(
py,
inst,
"assert inst.get_posargs_only_and_kwargs_only_with_defaults(3, b = 5) == 8"
);
py_expect_exception!(
py,
inst,
"assert inst.get_posargs_only_and_kwargs_only_with_defaults(3, 5)",
PyTypeError
);
py_expect_exception!(
py,
inst,
"assert inst.get_posargs_only_and_kwargs_only_with_defaults(a = 3, b = 5)",
PyTypeError
);
py_expect_exception!(
py,
inst,
"assert inst.get_posargs_only_and_kwargs_only_with_defaults(a = 3)",
PyTypeError
);

py_run!(py, inst, "assert inst.get_kwargs_only_with_defaults() == 5");
py_run!(
py,
Expand Down