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

Fix for pep585 with __future__ import #9608

Closed
wants to merge 1 commit into from

Conversation

cdce8p
Copy link
Collaborator

@cdce8p cdce8p commented Oct 17, 2020

Description

Fix PR fixes an issue introduced with #7963. When using from __future__ import annotations with py37 and above, the following should pass:

from __future__ import annotations

t1: list[int]
T2 = list[int]  # This line caused an error

Test Plan

I've added some tests to verify that the change works as intended.

--
cc: @TH3CHARLie

@TH3CHARLie
Copy link
Collaborator

TH3CHARLie commented Oct 17, 2020

I don't think the case you presented is valid Python so mypy does the right thing about reporting error here.

@cdce8p
Copy link
Collaborator Author

cdce8p commented Oct 17, 2020

If I'm not missing something, this should be a valid type alias. It might not be that useful for list[int] but image something a little more complex:

MyCustomType = dict[str, typing.Optional[tuple[int, float]]
my_var: MyCustomType = {...}

https://docs.python.org/3/library/typing.html#type-aliases

@TH3CHARLie
Copy link
Collaborator

If I'm not missing something, this should be a valid type alias. It might not be that useful for list[int] but image something a little more complex:

MyCustomType = dict[str, typing.Optional[tuple[int, float]]
my_var: MyCustomType = {...}

https://docs.python.org/3/library/typing.html#type-aliases

Sorry it's my miss that I didn't see that the PR is related to PEP585, yeah it should support alias.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Oct 17, 2020

It is not a valid type alias for Python 3.8 and lower, regardless of whether from __future__ import annotations is used.

While this is valid code in Python 3.9, we definitely should still complain about this for older Python versions.

@cdce8p
Copy link
Collaborator Author

cdce8p commented Oct 17, 2020

@hauntsaninja I didn't follow the discussion exactly around PEP585 so I might be missing something. However as far as I understand it, the backwards compatibility (until 37) is only dependent on the typechecker understanding it [1]. Type aliases are already supported [2]. Again I might be missing something.

[1] https://www.python.org/dev/peps/pep-0585/#id5
[2] https://docs.python.org/3.7/library/typing.html#type-aliases

@hauntsaninja
Copy link
Collaborator

Maybe I'm missing something too! :-) What I'm trying to say is that mypy should mirror runtime behaviour by complaining about this snippet for Python versions where we get TypeErrors:

~/tmp λ cat test74.py
from __future__ import annotations

t1: list[int]
T2 = list[int]
print("okay!")

~/tmp λ python3.7 test74.py
Traceback (most recent call last):
  File "test74.py", line 4, in <module>
    T2 = list[int]
TypeError: 'type' object is not subscriptable

~/tmp λ python3.8 test74.py
Traceback (most recent call last):
  File "test74.py", line 4, in <module>
    T2 = list[int]
TypeError: 'type' object is not subscriptable

~/tmp λ python3.9 test74.py
okay!

There's a difference between a type annotation and a type alias. PEP 563 goes into more detail here.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Oct 17, 2020

Note that once mypy supports PEP 613 (#9404) we should probably also allow T2: TypeAlias = "list[str]" on Python 3.7 (note the quotes).

@cdce8p
Copy link
Collaborator Author

cdce8p commented Oct 17, 2020

I think I understand your point now. You're right! I was testing with py39 which is why I didn't recognize the error 🙈
Thanks for pointing it out. I'll close this PR. #9564 should implement that case for py39.

@cdce8p cdce8p closed this Oct 17, 2020
@cdce8p cdce8p deleted the fix-future-annotation-pep585 branch October 17, 2020 21:01
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.

3 participants