Skip to content

Introspection: Adds basic input type annotations #5089

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Tpt
Copy link
Contributor

@Tpt Tpt commented Apr 24, 2025

Implementation notes:

  • The type annotation is a plain string. Indeed, it is not possible at my knowledge to properly concatenate strings from const code (hence without macros), making advanced structs not possible. Moreover, because it is not possible to use generic types in const code, we won't be able to generate list[int] types at the moment but only list. This limitation will be lifted if min_generic_const_args unstable feature lands at some point. However it should be possible to write a macro that validates the types and wrap them in some TypeAnnotation(&'static str) struct. Note that because our macros are aware of the Option<T> parameters annotations like T | None are properly generated. We might expand that to other common containers like Vec or HashMap.
  • The stub generator autogenerates import with very naive code. I am not sure we want to keep autogenerating them with I fear more complexity or request the user to write them by hand. If we auto generate them we must make it more robust (maybe in a follow up?)
  • The stubs generator does not support maximum line length yet, the test now reformats the stubs before doing string equality
  • I have only written types for a few built-in elements to not overload this MR
  • I have not added test coverage of types generated from #[pyclass]. We will get that for free after merging Implements basic method introspection #5087
  • The default when no type is set is typing.Any. It might be cleaner to make INPUT_TYPE an Option<&'static str> and use None as the default value to not set an annotation instead.

@Tpt Tpt added the CI-skip-changelog Skip checking changelog entry label Apr 24, 2025
@Tpt Tpt force-pushed the type-annotation branch 5 times, most recently from 9367241 to e3546aa Compare April 24, 2025 13:37
@davidhewitt davidhewitt mentioned this pull request May 2, 2025
Copy link
Member

@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.

Sorry for the slow-ish review by me. I guess we will miss the 0.25 release for this (my bad).

Overall looks great, with just a few suggestions. 👍

@Tpt Tpt force-pushed the type-annotation branch from e3546aa to 1eda0cb Compare May 13, 2025 11:28
@Tpt
Copy link
Contributor Author

Tpt commented May 13, 2025

Thank you for the review.

I guess we will miss the 0.25 release for this (my bad).

There is still a bunch of follow ups to get something working properly so it's definitely not a big deal.

Copy link
Member

@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.

Thanks, given all the comments I think this is good to merge (and I guess I can rebase the 0.25 branch to include this after all 😂)

@davidhewitt davidhewitt enabled auto-merge May 13, 2025 11:33
@Tpt
Copy link
Contributor Author

Tpt commented May 13, 2025

Thanks, given all the comments I think this is good to merge

Amazing!

(and I guess I can rebase the 0.25 branch to include this after all 😂)

I would feel better to have it in 0.26 to get time to do more testing, add more types across the codebase, deprecate the input_type and output_type methods...

@davidhewitt davidhewitt added this pull request to the merge queue May 13, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 13, 2025
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