-
Notifications
You must be signed in to change notification settings - Fork 911
Move TypeHint into a Python expression AST #5671
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
base: main
Are you sure you want to change the base?
Conversation
Implement a subset of Python expr AST Adds to the "name" construct a kind to distinguish global (global name that does not need to be resolved against the current module) vs local (names that needs to be resolved). This consolidates the previous builtin/local/module construct by taking into account that modules can be relative (for example if in a #[pyclass] the user set `module=` to the submodule name) Adds also the support for `Callable[[int], float]` and a beginning of constructs to represent constants (currently only None but will be useful for typing.Literal support) TODO: - update the macro code - update the introspection code
davidhewitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this.
My initial reaction reading the code is that PyStaticExpr is the right data format but the API for for type hints specifically is less nice, e.g. compare the union constructor we had before with the bit_or constructor we have now.
Due to the limitations of const fn (no allocations etc) I think the only way to get the ergonomics back would be a macro in that case, e.g. type_hint_union!(&a, &b, &c) instead of PyStaticExpr(&a, &PyStaticExpr::bit_or(&b, &c)).
Maybe for others like TypeHint::module_attr it might be able to have a normal const fn if we wanted to keep that API.
I'm also not sure if we should keep struct TypeHint(PyStaticExpr) as a way to have a namespace for type hint constructors specifically.
Wonder what you think of those kinds of points?
| pub enum PyStaticConstant { | ||
| /// None | ||
| None, | ||
| // TODO: add Bool(bool), String(&'static str)... (is useful for Literal["foo", "bar"] types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String(&'static str) would also be interesting because it can be used as an escape hatch for forward-reference style annotations for things we can't parse.
e.g. we could have "tuple[()]" already as a type hint until we can support it natively (with a PyStaticCall ast node, I guess).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes for String(&'static str)! I am planning to do it in a follow up if it's fine with you (to get it done properly, it needs a JSON escaping implementation working in const...).
For tuple[()] it's possible with this MR with the ::Tuple(&[]) variant.
|
Thank you!
Yes! It might make sense to have them for usual operations (union and subscript). Will add them.
If we had to end up with macros anyway, then the "namespace for constructors" usecase seems less interesting to me. What I can do is experiment a bit with macros and see what it looks like? |
|
👍 sounds good to me, thanks! |
Implement a subset of Python expr AST Adds to the "name" construct a kind to distinguish global (global name that does not need to be resolved against the current module) vs local (names that needs to be resolved). This consolidates the previous builtin/local/module construct by taking into account that modules can be relative (for example if in a #[pyclass] the user set `module=` to the submodule name) Adds also the support for `Callable[[int], float]` and a beginning of constructs to represent constants (currently only None but will be useful for typing.Literal support) TODO: - update the macro code - update the introspection code
|
@davidhewitt Experiment done with macros. I have replaced the const constructor with them. Does it looks good to you? If yes, I will move toward the next steps (same kind of changes in macro and introspection code) |
davidhewitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The macro constructors seem to work nicely to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems justifiable to extract into a separate PR.
Amazing! Will finish this MR then. |
|
I don't think we are fully there yet in term of imports generation in the stubs: if the module argument of Then for custom type hints, I guess the best approach is to ask the user to write imports from them in the
This way we should hopefully get something correct. And |
Implement a subset of Python expr AST
Adds to the "name" construct a kind to distinguish global (global name that does not need to be resolved against the current module) vs local (names that needs to be resolved).
This consolidates the previous builtin/local/module construct by taking into account that modules can be relative (for example if in a
#[pyclass]the user setmodule=to the submodule name)Adds also the support for
Callable[[int], float]and a beginning of constructs to represent constants (currently only None but will be useful fortyping.Literalsupport)The interesting part of this PR is the
inspect/mod.rsfile