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

use tag builder to avoid materializing result #3666

Closed

Conversation

davidhewitt
Copy link
Member

@adamreichold possible solution for https://github.com/PyO3/pyo3/pull/3661/files#r1431111113

I'm unsure what I think of the horrors here, but it solves the problem 😂

@adamreichold
Copy link
Member

possible solution for https://github.com/PyO3/pyo3/pull/3661/files#r1431111113

To be honest, I am not sure what the specific problem there is, hence I am not yet confident to decided whether this additional complexity is worth it or not.

#[allow(unreachable_code)]
let _type_hinter = || #cls::#fn_ident(#(#unreachable_args),*);
let tag = (&_pyo3::callback::TagBuilder::new(&_type_hinter)).#tag();
tag.convert(py, #call)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be simpler if we tried to wrap call directly, e.g.

let call = move || #call;
let tag = (&_pyo3::callback::TagBuilder::new(&call)).#tag();
tag.convert(py, call());

or does that break the lifetime extension as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually exactly what I tried first! 😂

The problem is that #call might contain diverging branches to return errors (e.g. by ?), and the return type of the function call is generally incompatible with this (e.g. because we OkWrap non-results).

Given that we know the full function signature, another option is to just carry the return type through the macro as tokens rather than rely on inference. The only complication here is that any lifetimes in the return type need to be stripped (we used to have code to do that; it wasn't much fun and quite complex, though this hack with _type_hinter is on a similar level of evil).

Copy link
Member

Choose a reason for hiding this comment

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

This complexity is why I would like to take one step back to actually look at a breaking test case. I mean, __next__ and __anext__ will not take any arguments besides self and the GIL token, right? At least the existing code from our repo seems to compile without the single-expression approach to the conversion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely, sorry, I should have started there - see #3661 (comment), which hopefully you also find gets broken without a single expression.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking back when we introduced the requirement for single expression form, I would even say that using the alternative from back then (the holder bindings) might actually be preferable as it would avoid complicating the already complicated specialization and would more generally allow us to inspect the return value using arbitrary code, not just specialization.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think you might be right. At the time I was hopeful that was a good simplification, but it's caused us pain a few times now.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I will try to resurrect that PR if I can find it and try to get that into mergeable form before continuing in #3661.

Copy link
Member

Choose a reason for hiding this comment

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

Seems it was #3140...

@davidhewitt
Copy link
Member Author

I think that this isn't needed after #3667, so will close.

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