-
Notifications
You must be signed in to change notification settings - Fork 791
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
Fix function name shadowing #3022
Conversation
pyo3-macros-backend/src/pymethod.rs
Outdated
let associated_method = quote! { | ||
unsafe fn #wrapper_ident( | ||
#py: _pyo3::Python<'_>, | ||
_raw_slf: *mut _pyo3::ffi::PyObject, | ||
#(#arg_idents: #arg_types),* | ||
) -> _pyo3::PyResult<#ret_ty> { | ||
let function = #cls::#name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply that fn function
is now broken as a #[pymethod]
instead? Maybe we need let _pyo3_function
instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I also recommend adding a comment describing why we are doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply that fn function is now broken as a #[pymethod] instead?
No, why would it? That works fine.
Agreed. I also recommend adding a comment describing why we are doing this.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I see, because the previous function
binding would be correctly rebound by the let
. Ignore me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice to add a newsfragment for this? Otherwise lgtm, thanks!
@mejrs I've squashed and added a newsfragment. Hope that's ok. bors r+ |
Build succeeded: |
Fixes #3017