Skip to content

Commit

Permalink
Merge pull request #1525 from davidhewitt/fix-issue-1506
Browse files Browse the repository at this point in the history 
1506: fixes to macro hygiene
  • Loading branch information
@davidhewitt
davidhewitt committed Mar 30, 2021
2 parents bc826d1 + ce851ad commit 3663d3f
Show file tree
Hide file tree
Showing 3 changed files with 137 additions and 56 deletions.
9 changes: 5 additions & 4 deletions pyo3-macros-backend/src/module.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,17 +225,18 @@ fn function_c_wrapper(
};
slf_module = None;
};
let body = impl_arg_params(spec, None, cb)?;
let py = syn::Ident::new("_py", Span::call_site());
let body = impl_arg_params(spec, None, cb, &py)?;
Ok(quote! {
unsafe extern "C" fn #wrapper_ident(
_slf: *mut pyo3::ffi::PyObject,
_args: *mut pyo3::ffi::PyObject,
_kwargs: *mut pyo3::ffi::PyObject) -> *mut pyo3::ffi::PyObject
{
pyo3::callback::handle_panic(|_py| {
pyo3::callback::handle_panic(|#py| {
#slf_module
let _args = _py.from_borrowed_ptr::<pyo3::types::PyTuple>(_args);
let _kwargs: Option<&pyo3::types::PyDict> = _py.from_borrowed_ptr_or_opt(_kwargs);
let _args = #py.from_borrowed_ptr::<pyo3::types::PyTuple>(_args);
let _kwargs: Option<&pyo3::types::PyDict> = #py.from_borrowed_ptr_or_opt(_kwargs);

#body
})
Expand Down
118 changes: 66 additions & 52 deletions pyo3-macros-backend/src/pymethod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,17 +94,18 @@ pub fn impl_wrap_cfunction_with_keywords(
) -> Result<TokenStream> {
let body = impl_call(cls, &spec);
let slf = self_ty.receiver(cls);
let body = impl_arg_params(&spec, Some(cls), body)?;
let py = syn::Ident::new("_py", Span::call_site());
let body = impl_arg_params(&spec, Some(cls), body, &py)?;
Ok(quote! {{
unsafe extern "C" fn __wrap(
_slf: *mut pyo3::ffi::PyObject,
_args: *mut pyo3::ffi::PyObject,
_kwargs: *mut pyo3::ffi::PyObject) -> *mut pyo3::ffi::PyObject
{
pyo3::callback::handle_panic(|_py| {
pyo3::callback::handle_panic(|#py| {
#slf
let _args = _py.from_borrowed_ptr::<pyo3::types::PyTuple>(_args);
let _kwargs: Option<&pyo3::types::PyDict> = _py.from_borrowed_ptr_or_opt(_kwargs);
let _args = #py.from_borrowed_ptr::<pyo3::types::PyTuple>(_args);
let _kwargs: Option<&pyo3::types::PyDict> = #py.from_borrowed_ptr_or_opt(_kwargs);

#body
})
Expand Down Expand Up @@ -138,7 +139,8 @@ pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> Result<TokenStream>
let name = &spec.name;
let names: Vec<syn::Ident> = get_arg_names(&spec);
let cb = quote! { #cls::#name(#(#names),*) };
let body = impl_arg_params(spec, Some(cls), cb)?;
let py = syn::Ident::new("_py", Span::call_site());
let body = impl_arg_params(spec, Some(cls), cb, &py)?;

Ok(quote! {{
#[allow(unused_mut)]
Expand All @@ -149,12 +151,12 @@ pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> Result<TokenStream>
{
use pyo3::callback::IntoPyCallbackOutput;

pyo3::callback::handle_panic(|_py| {
let _args = _py.from_borrowed_ptr::<pyo3::types::PyTuple>(_args);
let _kwargs: Option<&pyo3::types::PyDict> = _py.from_borrowed_ptr_or_opt(_kwargs);
pyo3::callback::handle_panic(|#py| {
let _args = #py.from_borrowed_ptr::<pyo3::types::PyTuple>(_args);
let _kwargs: Option<&pyo3::types::PyDict> = #py.from_borrowed_ptr_or_opt(_kwargs);

let initializer: pyo3::PyClassInitializer::<#cls> = #body.convert(_py)?;
let cell = initializer.create_cell_from_subtype(_py, subtype)?;
let initializer: pyo3::PyClassInitializer::<#cls> = #body.convert(#py)?;
let cell = initializer.create_cell_from_subtype(#py, subtype)?;
Ok(cell as *mut pyo3::ffi::PyObject)
})
}
Expand All @@ -167,8 +169,8 @@ pub fn impl_wrap_class(cls: &syn::Type, spec: &FnSpec<'_>) -> Result<TokenStream
let name = &spec.name;
let names: Vec<syn::Ident> = get_arg_names(&spec);
let cb = quote! { pyo3::callback::convert(_py, #cls::#name(&_cls, #(#names),*)) };

let body = impl_arg_params(spec, Some(cls), cb)?;
let py = syn::Ident::new("_py", Span::call_site());
let body = impl_arg_params(spec, Some(cls), cb, &py)?;

Ok(quote! {{
#[allow(unused_mut)]
Expand All @@ -177,10 +179,10 @@ pub fn impl_wrap_class(cls: &syn::Type, spec: &FnSpec<'_>) -> Result<TokenStream
_args: *mut pyo3::ffi::PyObject,
_kwargs: *mut pyo3::ffi::PyObject) -> *mut pyo3::ffi::PyObject
{
pyo3::callback::handle_panic(|_py| {
let _cls = pyo3::types::PyType::from_type_ptr(_py, _cls as *mut pyo3::ffi::PyTypeObject);
let _args = _py.from_borrowed_ptr::<pyo3::types::PyTuple>(_args);
let _kwargs: Option<&pyo3::types::PyDict> = _py.from_borrowed_ptr_or_opt(_kwargs);
pyo3::callback::handle_panic(|#py| {
let _cls = pyo3::types::PyType::from_type_ptr(#py, _cls as *mut pyo3::ffi::PyTypeObject);
let _args = #py.from_borrowed_ptr::<pyo3::types::PyTuple>(_args);
let _kwargs: Option<&pyo3::types::PyDict> = #py.from_borrowed_ptr_or_opt(_kwargs);

#body
})
Expand All @@ -194,8 +196,8 @@ pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> Result<TokenStrea
let name = &spec.name;
let names: Vec<syn::Ident> = get_arg_names(&spec);
let cb = quote! { pyo3::callback::convert(_py, #cls::#name(#(#names),*)) };

let body = impl_arg_params(spec, Some(cls), cb)?;
let py = syn::Ident::new("_py", Span::call_site());
let body = impl_arg_params(spec, Some(cls), cb, &py)?;

Ok(quote! {{
#[allow(unused_mut)]
Expand All @@ -204,9 +206,9 @@ pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> Result<TokenStrea
_args: *mut pyo3::ffi::PyObject,
_kwargs: *mut pyo3::ffi::PyObject) -> *mut pyo3::ffi::PyObject
{
pyo3::callback::handle_panic(|_py| {
let _args = _py.from_borrowed_ptr::<pyo3::types::PyTuple>(_args);
let _kwargs: Option<&pyo3::types::PyDict> = _py.from_borrowed_ptr_or_opt(_kwargs);
pyo3::callback::handle_panic(|#py| {
let _args = #py.from_borrowed_ptr::<pyo3::types::PyTuple>(_args);
let _kwargs: Option<&pyo3::types::PyDict> = #py.from_borrowed_ptr_or_opt(_kwargs);

#body
})
Expand Down Expand Up @@ -349,6 +351,7 @@ pub fn impl_arg_params(
spec: &FnSpec<'_>,
self_: Option<&syn::Type>,
body: TokenStream,
py: &syn::Ident,
) -> Result<TokenStream> {
if spec.args.is_empty() {
return Ok(body);
Expand Down Expand Up @@ -382,11 +385,20 @@ pub fn impl_arg_params(
}

let num_params = positional_parameter_names.len() + keyword_only_parameters.len();
let args_array = syn::Ident::new("output", Span::call_site());

let mut param_conversion = Vec::new();
let mut option_pos = 0;
for (idx, arg) in spec.args.iter().enumerate() {
param_conversion.push(impl_arg_param(&arg, &spec, idx, self_, &mut option_pos)?);
param_conversion.push(impl_arg_param(
&arg,
&spec,
idx,
self_,
&mut option_pos,
py,
&args_array,
)?);
}

let (mut accept_args, mut accept_kwargs) = (false, false);
Expand Down Expand Up @@ -422,8 +434,8 @@ pub fn impl_arg_params(
accept_varkeywords: #accept_kwargs,
};

let mut output = [None; #num_params];
let (_args, _kwargs) = DESCRIPTION.extract_arguments(_args, _kwargs, &mut output)?;
let mut #args_array = [None; #num_params];
let (_args, _kwargs) = DESCRIPTION.extract_arguments(_args, _kwargs, &mut #args_array)?;

#(#param_conversion)*

Expand All @@ -440,92 +452,94 @@ fn impl_arg_param(
idx: usize,
self_: Option<&syn::Type>,
option_pos: &mut usize,
py: &syn::Ident,
args_array: &syn::Ident,
) -> Result<TokenStream> {
// Use this macro inside this function, to ensure that all code generated here is associated
// with the function argument
macro_rules! quote_arg_span {
($($tokens:tt)*) => { quote_spanned!(arg.ty.span() => $($tokens)*) }
}

let arg_name = syn::Ident::new(&format!("arg{}", idx), Span::call_site());

if arg.py {
return Ok(quote_spanned! {
arg.ty.span() =>
let #arg_name = _py;
});
return Ok(quote_arg_span! { let #arg_name = #py; });
}

let ty = arg.ty;
let name = arg.name;
let transform_error = quote! {
|e| pyo3::derive_utils::argument_extraction_error(_py, stringify!(#name), e)
let transform_error = quote_arg_span! {
|e| pyo3::derive_utils::argument_extraction_error(#py, stringify!(#name), e)
};

if spec.is_args(&name) {
ensure_spanned!(
arg.optional.is_none(),
arg.name.span() => "args cannot be optional"
);
return Ok(quote_spanned! {
arg.ty.span() =>
let #arg_name = _args.unwrap().extract()
.map_err(#transform_error)?;
return Ok(quote_arg_span! {
let #arg_name = _args.unwrap().extract().map_err(#transform_error)?;
});
} else if spec.is_kwargs(&name) {
ensure_spanned!(
arg.optional.is_some(),
arg.name.span() => "kwargs must be Option<_>"
);
return Ok(quote_spanned! {
arg.ty.span() =>
return Ok(quote_arg_span! {
let #arg_name = _kwargs.map(|kwargs| kwargs.extract())
.transpose()
.map_err(#transform_error)?;
});
}

let arg_value = quote!(output[#option_pos]);
let arg_value = quote_arg_span!(#args_array[#option_pos]);
*option_pos += 1;

let default = match (spec.default_value(name), arg.optional.is_some()) {
(Some(default), true) if default.to_string() != "None" => quote! { Some(#default) },
(Some(default), _) => quote! { #default },
(None, true) => quote! { None },
(None, false) => quote! { panic!("Failed to extract required method argument") },
(Some(default), true) if default.to_string() != "None" => {
quote_arg_span! { Some(#default) }
}
(Some(default), _) => quote_arg_span! { #default },
(None, true) => quote_arg_span! { None },
(None, false) => quote_arg_span! { panic!("Failed to extract required method argument") },
};

let extract = if let Some(FromPyWithAttribute(expr_path)) = &arg.attrs.from_py_with {
quote! {#expr_path(_obj).map_err(#transform_error)?}
quote_arg_span! { #expr_path(_obj).map_err(#transform_error)?}
} else {
quote! {_obj.extract().map_err(#transform_error)?}
quote_arg_span! { _obj.extract().map_err(#transform_error)?}
};

return if let syn::Type::Reference(tref) = arg.optional.as_ref().unwrap_or(&ty) {
let (tref, mut_) = preprocess_tref(tref, self_);
let (target_ty, borrow_tmp) = if arg.optional.is_some() {
// Get Option<&T> from Option<PyRef<T>>
(
quote! { Option<<#tref as pyo3::derive_utils::ExtractExt>::Target> },
quote_arg_span! { Option<<#tref as pyo3::derive_utils::ExtractExt>::Target> },
if mut_.is_some() {
quote! { _tmp.as_deref_mut() }
quote_arg_span! { _tmp.as_deref_mut() }
} else {
quote! { _tmp.as_deref() }
quote_arg_span! { _tmp.as_deref() }
},
)
} else {
// Get &T from PyRef<T>
(
quote! { <#tref as pyo3::derive_utils::ExtractExt>::Target },
quote! { &#mut_ *_tmp },
quote_arg_span! { <#tref as pyo3::derive_utils::ExtractExt>::Target },
quote_arg_span! { &#mut_ *_tmp },
)
};

Ok(quote_spanned! {
arg.ty.span() =>
Ok(quote_arg_span! {
let #mut_ _tmp: #target_ty = match #arg_value {
Some(_obj) => #extract,
None => #default,
};
let #arg_name = #borrow_tmp;
})
} else {
Ok(quote_spanned! {
arg.ty.span() =>
Ok(quote_arg_span! {
let #arg_name = match #arg_value {
Some(_obj) => #extract,
None => #default,
Expand Down
66 changes: 66 additions & 0 deletions tests/test_methods.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -753,3 +753,69 @@ issue_1505!(
fn issue_1505(&self, _py: Python<'_>) {}
}
);

// Regression test for issue 1506 - incorrect macro hygiene.
// By applying the `#[pymethods]` attribute inside a macro_rules! macro, this separates the macro
// call scope from the scope of the impl block. For this to work our macros must be careful to not
// cheat hygeine!

#[pyclass]
struct Issue1506 {}

macro_rules! issue_1506 {
(#[pymethods] $($body:tt)*) => {
#[pymethods]
$($body)*
};
}

issue_1506!(
#[pymethods]
impl Issue1506 {
fn issue_1506(
&self,
_py: Python<'_>,
_arg: &PyAny,
_args: &PyTuple,
_kwargs: Option<&PyDict>,
) {
}

#[new]
fn issue_1506_new(
_py: Python<'_>,
_arg: &PyAny,
_args: &PyTuple,
_kwargs: Option<&PyDict>,
) -> Self {
Issue1506 {}
}

#[getter("foo")]
fn issue_1506_getter(&self, _py: Python<'_>) -> i32 {
5
}

#[setter("foo")]
fn issue_1506_setter(&self, _py: Python<'_>, _value: i32) {}

#[staticmethod]
fn issue_1506_static(
_py: Python<'_>,
_arg: &PyAny,
_args: &PyTuple,
_kwargs: Option<&PyDict>,
) {
}

#[classmethod]
fn issue_1506_class(
_cls: &PyType,
_py: Python<'_>,
_arg: &PyAny,
_args: &PyTuple,
_kwargs: Option<&PyDict>,
) {
}
}
);

0 comments on commit 3663d3f

Please sign in to comment.