Thread pyo3's path through the builder functions #3907
Conversation
mejrs
force-pushed
the
non_local_definitions
branch
from
d407acb to
5ca78a7
Compare
|
Apparently this does something weird with the spans in some error messages. Any ideas on how to fix that? |
|
Uff, that does not surprise me. I'll try to read and think later ASAP, but I'm currently preparing & rehearsing material for a meetup on Thursday (presenting on PyO3!) so worst case I may not be able to give this headspace until Friday. |
| impl PyO3CratePath { | ||
| pub fn to_tokens_spanned(&self, span: Span) -> TokenStream { | ||
| match self { | ||
| Self::Given(path) => quote::quote_spanned! { span => #path }, | ||
| Self::Default => quote::quote_spanned! { span => ::pyo3 }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The idea of adding this here is that this gave back control to the span that the ::pyo3 default path was being emitted with.
I applied in manually in the places where it mattered. If we wanted to force ourselves to think about it we could remove quote::ToTokens implementation and require ourselves to always call this method. I quite like the convenience of the ToTokens implementation though :)
|
Great - @mejrs I needed to play around with this to get a sense of what might solve this. It ended up being pretty reasonable so I pushed that as an additional commit. Hopefully that unblocks making the rest of the PR work (I see we made a few merge conflicts already since you started this 🙈). |
mejrs
force-pushed
the
non_local_definitions
branch
from
a72ea70 to
2838c15
Compare
mejrs
force-pushed
the
non_local_definitions
branch
from
2838c15 to
2e0686e
Compare
They have been resolved. I'd like to merge this as is (if Ci clears 🙏 ), and address the rest in their own PR. |
| quotes::map_result_into_ptr( | ||
| quotes::ok_wrap( | ||
| quote! { | ||
| ::std::clone::Clone::clone(&(#slf.#field_token)) | ||
| }, | ||
| ctx, | ||
| ), | ||
| ctx, | ||
| ) |
There was a problem hiding this comment.
I wonder if it makes sense for future refactoring like moving these quotes functions to be methods on Ctx. A follow up for anyone interested I think, rather than part of this PR.
| @@ -31,4 +31,4 @@ error[E0592]: duplicate definitions with name `__pymethod___richcmp____` | |||
| | duplicate definitions for `__pymethod___richcmp____` | |||
| | other definition for `__pymethod___richcmp____` | |||
| | | |||
| = note: this error originates in the macro `_pyo3::impl_::pyclass::generate_pyclass_richcompare_slot` which comes from the expansion of the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info) | |||
| = note: this error originates in the macro `::pyo3::impl_::pyclass::generate_pyclass_richcompare_slot` which comes from the expansion of the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info) | |||
There was a problem hiding this comment.
TBH this error looks better than the previous form 👍
The first part of addressing #3903
This gets rid of all the
use #krate as _pyo3in our macro code.I did not address the
use #pyo3_path::prelude::PyAnyMethods;imports because that looks like a lot of work so splitting it up seemed like a good idea.