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

fix compile failure for getter with return lifetime of self #3347

Merged
merged 3 commits into from
Jul 30, 2023

Conversation

davidhewitt
Copy link
Member

While testing main downstream I noticed a compile failure caused by #3318, when the value returned from a #[getter] has the same lifetime as the &self receiver.

This PR adds a corresponding test and fixes it. The solution is the usual trend for the macro function bodies towards expressions; there is a sequence of statements along the lines of let item = ...; return convert(item), the PR changes this to just be convert(...).

This doesn't need a changelog entry as the compile failure never hit a released version.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Jul 28, 2023
@davidhewitt davidhewitt mentioned this pull request Jul 28, 2023
Copy link
Member Author

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Ok, I've pushed a commit which improves the implementation of impl_py_getter_def a little bit.

I've identified a follow-up which will move code out of the macro-generated code and into the trampolines, so I'd like to ask to merge this as-is (even if the macro expression is still a bit verbose) because I'll simplify further in the follow up.

pyo3-macros-backend/src/quotes.rs Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

I also pushed a commit which moves all usage of the _py ident to py, which allowed a general reduction in complexity across the macro code (including inside this impl_py_getter_def once I moved to the ok_wrap form).

Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

While I welcome the reduction in Perl-style sigils by not quoting the GIL token, I somewhat fear regressions in macro hygiene due to this.

I am also not sure what the motivation for doing that change now is? Sure, this implies another reduction in sigil noise in the proc macro code, but to me, it appears small compared to possible regression in macro hygiene, especially which such a common identifier as py (at least in PyO3 code bases).

I am pretty sure I am missing something, because this appears somewhat risky to me.

@davidhewitt
Copy link
Member Author

That's a reasonable concern. The py ident should always have call_site hygiene, and the only case that it's necessary to explicitly construct the ident is when mixing with quote_spanned to emit other tokens with a different hygiene. (The interpolated #py in a quote_spanned will keep its original span.)

The py ident was parameterised further in some functions because we have used both py and _py as the identifier inside the macros codebase.

To remove the hygiene concerns from this PR I've moved that commit to #3356 and replaced it here with a simpler commit which unifies all idents as py in the macros codebase. That's sufficient to allow the ok_wrap helper to work without needing to take py ident as an argument.

@davidhewitt davidhewitt added this pull request to the merge queue Jul 30, 2023
Merged via the queue into PyO3:main with commit 5562052 Jul 30, 2023
31 checks passed
@davidhewitt davidhewitt deleted the getter-lifetime branch July 30, 2023 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants