Skip to content

Add Python return type for async functions #1984

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

Merged
merged 3 commits into from
Feb 8, 2024

Conversation

giarc3
Copy link
Contributor

@giarc3 giarc3 commented Feb 6, 2024

As discussed in #1970, uniffi currently doesn't put a return type on generated async functions in Python.

This is my attempt at correcting the function signatures it generates. Because the diff is a bit bad, I'll summarize the changes:

  • For async functions, match on if they have a return type
    a. If they do, add async to the front of the function, add the -> ___ return type, and add await to the return statement.
    b. If they don't, still add async to the front of the function and await to the return.
  • Remove the match meth.return_type() cases from the body of the function, as it's now being checked outside of the function

The changes are repeated in macros.py and TopLevelFunctionTemplate.py

b. technically could remain unchanged, but it seemed more correct to make the generated functions match each other.

Note that I'm by no means a Python expert, so please let me know if there's a more ergonomic/preferred way to accomplish all this.

@giarc3 giarc3 requested a review from a team as a code owner February 6, 2024 23:59
@giarc3 giarc3 requested review from jhugman and removed request for a team February 6, 2024 23:59
@mhammond
Copy link
Member

mhammond commented Feb 7, 2024

Thanks! This is a larger change than I was anticipating - which isn't necessarily a problem, just needs a closer look. I'll look in more detail over the next day or 2.

@mhammond mhammond self-requested a review February 7, 2024 00:09
Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Thanks! Please let me know if I missed something here...

@giarc3
Copy link
Contributor Author

giarc3 commented Feb 7, 2024

@mhammond I pushed a change to add the None and reduce the code duplication as you indicated (much cleaner diff!).

In doing testing of this against a local project, I happened upon the Protocol classes getting generated, and hit the extent of my knowledge. As I didn't change Protocol.py, the functions in the protocol classes remain without async and without the return type (if present). Do you know if this is an issue? I would have expected the lack of async to cause a problem, but I haven't run into anything.

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

I think the lack of async in protocol should be OK as async is just sugar - the prototype code still returns a coroutine. The lack of annotations there isn't ideal, but I don't see how it hurts. The fact our CI is green and this has tests means I think we should land this - if it does turn out we broke something then we'll fix it and add more tests :)

Thanks!

@mhammond mhammond merged commit 545bcd7 into mozilla:main Feb 8, 2024
@giarc3 giarc3 deleted the python-async-return-type branch February 8, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants