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

implement PEP 585 Generic Builtins (Partially solves #7907) #9564

Merged
merged 28 commits into from Nov 26, 2020

Conversation

AllanDaemon
Copy link
Contributor

@AllanDaemon AllanDaemon commented Oct 9, 2020

Description

Partially Solves #7907

Instead of using the nongen_builtins dictionary with the non generic builtins names, it uses a function to get it that takes the python version. It returns an empty dict if the version is 3.9 or greater.

I'm opening the PR to review the code and the way it solves. Some discussion was made in the #7907 thread. I'll add the new tests to the code.

Test Plan

I'm writing some tests for this, taking in consideration the different versions. But I'm having some difficulties to get the testing stuff right, so it may take a little bit. If wanted, you can merge this and I'll open a PR with the new tests latter.

mypy/nodes.py Outdated Show resolved Hide resolved
mypy/nodes.py Show resolved Hide resolved
mypy/nodes.py Outdated Show resolved Hide resolved
@cdce8p
Copy link
Collaborator

cdce8p commented Oct 24, 2020

@AllanDaemon I've written some test cases. Take a look at AllanDaemon#1

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Two quick comments:

  1. Could you also gate the GENERIC_STUB_NOT_AT_RUNTIME_TYPES in get_omitted_any behind a python version check?
  2. Could you add some tests for tuple? It's a bit of a special snowflake and I'm concerned it won't work

mypy/typeanal.py Outdated Show resolved Hide resolved
AllanDaemon and others added 2 commits October 24, 2020 21:12
Co-authored-by: cdce8p <30130371+cdce8p@users.noreply.github.com>
@AllanDaemon
Copy link
Contributor Author

@AllanDaemon I've written some test cases. Take a look at AllanDaemon#1

Thanks. I wrote a few tests but I got stuck with the issue that the type as it's on a different part of the code that I hadn't found yet, but your PR solves it. Now I gonna add some of my tests but the PR seems good to go.

@AllanDaemon
Copy link
Contributor Author

AllanDaemon commented Oct 25, 2020

Two quick comments:

  1. Could you also gate the GENERIC_STUB_NOT_AT_RUNTIME_TYPES in get_omitted_any behind a python version check?

IDK. I'm not familiar yet with the code, but as far as related with PEP 585, those 2 classes are not mentioned there. I also couldn't find a code that triggers this. It this should be gated by python version, I think it would be better to open an new issue / PR.

  1. Could you add some tests for tuple? It's a bit of a special snowflake and I'm concerned it won't work

I will.

@AllanDaemon
Copy link
Contributor Author

By now, just the test that handles the reveal_type() is failing. As I wrote here (#7907 (comment)), I think this should be handled in another PR, in particular after discussing a few things about how reaveal_type() should be handled in some cases (like type, and tuple).

So, for this PR, after I disable the tests that fail because of reveal_type(), this PR should be good to go.

So, is there anything else that should be handled before it could be merged?

mypy/typeanal.py Outdated Show resolved Hide resolved
AllanDaemon and others added 2 commits October 25, 2020 18:21
Co-authored-by: cdce8p <30130371+cdce8p@users.noreply.github.com>
@AllanDaemon AllanDaemon changed the title implement PEP 585 Generic Builtins (closes #7907) implement PEP 585 Generic Builtins (Partially solves #7907) Oct 25, 2020
@AllanDaemon
Copy link
Contributor Author

So, what else is missing for this PR that I need to do?

Copy link
Collaborator

@ethanhs ethanhs left a comment

Choose a reason for hiding this comment

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

This looks good, thank you for implementing PEP 585 support! I just had one minor change but otherwise I think this is good to land.

test-data/unit/check-generic-alias.test Outdated Show resolved Hide resolved
Co-authored-by: Ethan Smith <ethan@ethanhs.me>
@cdce8p
Copy link
Collaborator

cdce8p commented Nov 15, 2020

Any updates?

@hauntsaninja hauntsaninja merged commit 52225ab into python:master Nov 26, 2020
@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Nov 26, 2020

Thanks for your work on this! Took the liberty of tidying up / merging some tests.

I'm also closing #7907. We can open a more specific issue for the low priority additions of deprecation warnings / making reveal_type reveal collections classes. We already have #9522 for the higher priority tuple reveal type issue.

@hauntsaninja hauntsaninja mentioned this pull request Nov 26, 2020
6 tasks
@wbadart
Copy link

wbadart commented Jan 25, 2021

Hey team- just upgraded to 0.800 and found that mypy might not recognize PEP585-type syntax in type aliases:

T = list[int]
foo.py:1: error: "list" is not subscriptable  [misc]
    T = list[int]
        ^

Let me know if you have trouble reproducing the error and/or if there's anything I can do to help!

@JelleZijlstra
Copy link
Member

@wbadart that syntax doesn't work at runtime in 3.9 yet. See #9442 and #9880.

@wbadart
Copy link

wbadart commented Jan 25, 2021

Yup. Sorry, I should have noted that I'm using from __future__ import annotations in the actual code where I noticed this, so no runtime errors. In any case, I just meant to note how mypy treats this syntax (regardless of what the Python interpreter makes of it).

@JelleZijlstra
Copy link
Member

The future import doesn't make a difference to type aliases, because that code is still evaluated at runtime.

@wbadart
Copy link

wbadart commented Jan 25, 2021

Ahh... Okay, I'm following now. That would explain why we don't want that code to type-check. Thanks for the clarification!

@rben01
Copy link

rben01 commented Mar 1, 2021

Hey team- just upgraded to 0.800 and found that mypy might not recognize PEP585-type syntax in type aliases:

T = list[int]
foo.py:1: error: "list" is not subscriptable  [misc]
    T = list[int]
        ^

Let me know if you have trouble reproducing the error and/or if there's anything I can do to help!

Also seeing this on 0.812

@cdce8p
Copy link
Collaborator

cdce8p commented Mar 1, 2021

@rben01 Please read the comments above. The PEP 585 syntax in Type Aliases requires at least Python 3.9. Regardless if from __future__ import annotations is used or not. The mypy documentation has a good page on exactly that problem, since it can be quite confusing sometimes: https://mypy.readthedocs.io/en/stable/runtime_troubles.html

Once mypy supports PEP 613 you should however be able to declare them as strings in versions prior to 3.9.

T: TypeAlias = "list[int]"

@rben01
Copy link

rben01 commented Mar 1, 2021

@cdce8p Sorry for skipping over the previous comments. But after (re-)reading all the prior material I'm still confused. Pep 585 says this:

Starting with Python 3.7, when from future import annotations is used, function and variable annotations can parameterize standard collections directly. Example:

from __future__ import annotations
def find(haystack: dict[str, list[int]]) -> int:
   ...

Usefulness of this syntax before PEP 585 is limited as external tooling like Mypy does not recognize standard collections as generic. Moreover, certain features of typing like type aliases or casting require putting types outside of annotations, in runtime context. While these are relatively less common than type annotations, it's important to allow using the same type syntax in all contexts. This is why starting with Python 3.9, the following collections become generic using class_getitem() to parameterize contained types:

I think that that Pep is saying that using list[int] as a type annotation is allowed in Python 3.7+ as long as you import annotations from __future__, and that it's only disallowed in Python 3.7 and 3.8 when it used as an r-value, such as when it's the first argument to cast or in an expression such as type_alias = list[int].

I tested this in an interpreter and it seems like this is correct:

Text available below

Text version
In [66]: sys.version
Out[66]: '3.8.7 (default, Feb 10 2021, 03:56:21) \n[Clang 7.1.0 (tags/RELEASE_710/final)]'

In [67]: def find(haystack: dict[str, list[int]]) -> int:
    ...:     pass
    ...:     

In [68]: type_alias = list[int]
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-68-1bf51d638f86> in <module>
----> 1 type_alias = list[int]

TypeError: 'type' object is not subscriptable

Python's type system is evolving way too quickly for me to keep up, and the subtle behavior differences between Python versions are a lot to keep track of so I'll defer to the Mypy team. But this was my understanding of Pep 585: that list[int] is valid as a type annotation, but prior to Python 3.9 it may not appear as an r-value/be evaluated at runtime.

@JelleZijlstra
Copy link
Member

@rben01 That's right; I think the part that you're misunderstanding is that type_alias = list[int] isn't a type annotation, it's a runtime use of the type. Therefore, from __future__ import annotations doesn't help: when you run the code Python still tries to actually execute list[int] and fails.

@rben01
Copy link

rben01 commented Mar 1, 2021

@JelleZijlstra Ok, sorry for the confusion. I saw the error message error: "list" is not subscriptable without considering the context. I am having the following problem, which I now realize is not the same as wdabart's, which is that mypy flags this as invalid for me:

from __future__ import annotations
x: list[int] = [1]

I think this is definitely a bug with mypy? I can open a new issue if there isn't one for this specific case.

@JelleZijlstra
Copy link
Member

That should be allowed, yes (assuming mypy is running at least --python-version 3.7). If it doesn't that's a bug.

@rben01
Copy link

rben01 commented Mar 1, 2021

Ah, sorry, the underlying issue was with having two versions of mypy installed and Visual Studio Code not using the right one when linting. Ignore me 🤦 .

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.

None yet

9 participants