-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add support for union types as X | Y (PEP 604) #9647
Conversation
…responding pull_request in cpython). Then, it's possible to use str | int in place of Union[str,int].
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@hauntsaninja What is your opinion regarding the documentation? |
My weakly held opinion is that we should leave the docs alone for now. It would be consistent with the docs not using PEP 585 / Python 3.6 is alive and kicking / IMO |
I agree. I'll push a commit removing all references in the docs. If we later decide to keep them, I can just reverse the commit. |
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 this! Looks good to me, but leaving unmerged in case another maintainer has thoughts. Specifically, I'm wondering if it's worth it to try and make this conditional on from __future__ import annotations
(knowledge of that unfortunately comes later on in the process).
Also are you @pprados ? If not, we should make sure to add Co-authored-by when merging.
@hauntsaninja You might have a point there. Without
No, I'm not. Originally I opened #9610 for it and somebody linked him. However since he didn't respond, I though I open the PR myself to start working on implementing it upstream. I really like the new notation and already started using it in my personal projects together with this mypy implementation 😃 |
I went ahead and added a check for Regarding the error message: |
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, this looks good! I just have some minor nits:
- Rename the attribute
is_binary_op
touses_pep604_syntax
- I think we should mention it; maybe
X | Y syntax for unions requires Python 3.10, but can be used in annotations via PEP 563 on {options.python_version}
.
Python 3.9 is going to be around for a long time and it's very much intentional on our part that we're supporting new syntax for it. I wish there was a shorter way to describe from __future__ import annotations
that isn't a PEP number, but I do feel like the error message I propose is a not misleading summary of the situation.
I'm not sure what the best way to fix the type error is; I don't know what implications changing SemanticAnalyzerCoreInterface has. I will say that it's not the worst to drop stub file support. Typeshed is conservative in its use of new syntax and worst case one could use the future import in stubs if you wanted to be able to type check Python 3.9
@hauntsaninja Thanks for your review.
My reasoning for not adding a note to
I might not have described it clearly enough. As far as I know, those will work without issues and you don't even need the future import. mypy/test-data/unit/check-union-or-syntax.test Lines 77 to 81 in ae5771e
Before, I wasn't sure how I should check for those, since the |
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.
Some more suggestions:
- Could you add a test with generics, e.g.
List[int | str]
- Could you test whether it works when quoted, e.g.
def f(x: "int | str"): ...
- Let's move is_type_comment to being an attribute of TypeConverter, rather than changing visit.
- I think you'll need to set is_type_comment for functions somewhere else in fastparse. Might be worth grepping through wherever we instantiate TypeConverter
pep604_syntax.uses_pep604_syntax
is a little bit of a mouthful. Maybe rename tosyntax.uses_pep604
orsyntax_form.uses_pep604
? Actually, maybe the right thing to do is un-nest it and just have two attributes:uses_pep604
andis_evaluated
, whereis_evaluated
is True if it's not a type comment or string literal.is_evaluated
is something we could generalise beyond UnionType, eg for Detect if required string literal escapes are missing from a type #948 or for helping guide PEP 585 usage.
And okay, we can leave out the PEP 563 mention. You're right that it's the more conservative change; we can always reconsider if we find people are confused :-) |
@hauntsaninja Thanks again. Regarding your points:
Good catch. Turns out that it wouldn't have worked. I also added a test case for generics.
Great idea. I should have though of that. It just makes much more sense.
I've changed it to just use I was thinking, we don't have any documentation for this change. Although, as discussed, replacing |
* Added tests * Rename/refactor variables * Fixed issue with type strings e.g. "int | str"
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, this looks great!
One last nit: I think the is_type_comment
attribute on TypeConverter should be renamed to is_evaluated
, since we call parse_type_comment
from parse_type_string
.
Documentation is a good idea! https://mypy.readthedocs.io/en/stable/kinds_of_types.html#union-types seems like the natural place
@hauntsaninja I changed the attribute name and added a few more test cases. The commit also includes my first draft for the documentation. Let me know what you think :) |
Any updates? What is left to do before it can be merged? |
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.
Documentation looks good, but might be a little long for the page it's on. Should probably move the details onto a separate page that also discusses PEP 585 support. But we can do that in another PR. Thanks again! :-)
@hauntsaninja Thanks for the reviews. Those changes really improved this PR! |
See python/mypy#9647 for details. The X | Y syntax is only valid starting in Python 3.10 and not yet fully supported by mypy.
fwiw, @stefanv and I were indeed having a confused conversation about this on the scikit-image Zulip chat 😂 (needs sign-up/login, can use GH login). TLDR, thank you both for implementing this, it is just sooooo much nicer to use this syntax, but yes, it would be great to have a better place than this issue to tell people, hey, you can use this today in 3.7+ with In short, we'd favour some documentation around this and I'm happy to take a stab at a short sentence/paragraph if someone points me to where they'd like it to go. =) Thank you again! |
I changed the documentation already. There's a new section that should help with this sort of question: https://mypy.readthedocs.io/en/stable/runtime_troubles.html. |
Lovely, thank you! 🙏 |
Description
This PR attempts to add the original implementation for PEP 604 proposed by @pprados
The PEP will be added to Python in
3.10
. However, since it's already acceptable syntax, we can even use it for older versions withfrom __future__ import annotations
, in type comments or stub files. python/typing#429 (comment)Example
Test Plan
Tests were included with the original proposal.
Todo
3.10
isn't out yet, I would suggest to exclude the doc changes for now.Edit history
[2020-10-26] Rebased onto
master
, resolved conflicts[2020-10-26] Review changes + test fix
[2020-10-26] Removed documentation changes
--
Fixes: #9610