X Tutup
The Wayback Machine - https://web.archive.org/web/20221225010140/https://github.com/python/mypy/pull/11345
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

Jump to bottom

Allow slice syntax in e.g. Annotated[int, 1:3] and TensorType["batch":..., float] #11345

Merged
merged 2 commits into from Oct 18, 2021
Merged

Allow slice syntax in e.g. Annotated[int, 1:3] and TensorType["batch":..., float] #11345

merged 2 commits into from Oct 18, 2021

Conversation

Zac-HD
Copy link
Contributor

@Zac-HD Zac-HD commented Oct 16, 2021 •
edited

Consider the following trivial Python file, using the torchtyping library - if only this typechecked, we could write some fancy ML code!

from typing import Annotated, Dict
from torchtyping import TensorType  # type: ignore

a: Annotated[int, 1:2]
b: Dict[int, x:y]
c: Dict[x:y]
t: TensorType["batch":..., float]

On master, we get syntax-level errors due to the use of : slice syntax, which prevents any semantic checking - and can't even be disabled by a config file!

$ mypy --show-column-numbers t.py
t.py:4:4: error: Slice usage in type annotation is invalid
t.py:5:4: error: Slice usage in type annotation is invalid
t.py:6:4: error: Slice usage in type annotation is invalid
t.py:7:4: error: Slice usage in type annotation is invalid
Found 4 errors in 1 file (errors prevented further checking)

Following this patch, mypy identifies the problems with our Dict annotations, but is happy with Annotated and can be directed to ignore TensorType:

$ mypy --show-column-numbers t.py
t.py:5:14: error: Invalid type comment or annotation
t.py:5:14: note: did you mean to use ',' instead of ':' ?
t.py:6:4: error: "dict" expects 2 type arguments, but 1 given
t.py:6:9: error: Invalid type comment or annotation
t.py:6:9: note: did you mean to use ',' instead of ':' ?
Found 3 errors in 1 file (checked 1 source file)

This PR therefore fixes #10266; and closes #11279. Thanks to @sobolevn for pointing me in the right direction!

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
sobolevn
sobolevn reviewed Oct 16, 2021
View changes
Copy link
Member

@sobolevn sobolevn left a comment

except that there's some weird interaction on Python < 3.9, which I don't understand well enough to track down quickly. Any suggestions?

Python3.9 has different ast.Subscript structure. This helper might be useful to understand how it changed: https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/compat/functions.py#L32-L45

@Zac-HD

This comment has been minimized.

Sign in to view
@sobolevn
Copy link
Member

sobolevn commented Oct 16, 2021

I get a different ordering of messages for same-line errors on different Python versions

Try to run your tests with --show-column-numbers, I suspect that it might be the issue. Since all three items are on the same line:

  main:3: error: "dict" expects 2 type arguments, but 1 given  [type-arg]
  main:3: error: Invalid type comment or annotation  [valid-type]
  main:3: note: did you mean to use ',' instead of ':' ?

Column numbers might be different here: https://github.com/python/mypy/pull/11345/files#diff-a7d2c40f4be6322cee498432ca9dbbc3a356c3cbcb61029231452f6638bb11b5R1565-R1572

@Zac-HD Zac-HD force-pushed the allow-slice-syntax branch 6 times, most recently from 1cd724d to e095098 Compare Oct 17, 2021
@Zac-HD
Copy link
Contributor Author

Zac-HD commented Oct 17, 2021

Alright, I've fixed the pre-Python-3.9 issues: with message ordering (by propagating column numbers), and with Index nodes so that the int in Annotated[int, 3:4] is still recognised 🎉

However... this causes a problem, where on Python 3.9 mypyc complains that mypy/fastparse.py:1558: error: "Index" has no attribute "value". On one level this is fair enough - Index nodes are deprecated - but I can't even # type: ignore the problem because that's an unused ignore on older versions! Any further ideas @sobolevn?

@sobolevn
Copy link
Member

sobolevn commented Oct 17, 2021

@Zac-HD will conditional type alias work in this case? 🤔

if sys.version_info >= (3, 9):
    AstIndex = Any
else:
   AstIndex = ast.Index

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Oct 18, 2021

Or just imitate ASTConvertor.visit_index with return self.visit(cast(Any, n).value)? Let's see...

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Oct 18, 2021

Woohoo, it's all working! Thanks for your help @sobolevn 🥰

Ping @JelleZijlstra for review?

JelleZijlstra
JelleZijlstra reviewed Oct 18, 2021
View changes
mypy/fastparse.py Outdated Show resolved Hide resolved
JelleZijlstra
JelleZijlstra reviewed Oct 18, 2021
View changes
mypy/fastparse.py Outdated Show resolved Hide resolved
@Zac-HD
Allow slice syntax
22e8e3a 
This is useful for Annotated, and crucial for downstream libraries like torchtyping.
@Zac-HD Zac-HD requested a review from JelleZijlstra Oct 18, 2021
@Zac-HD Zac-HD force-pushed the allow-slice-syntax branch 2 times, most recently from 619cf28 to 83ee297 Compare Oct 18, 2021
@JelleZijlstra JelleZijlstra merged commit 9aaeef5 into python:master Oct 18, 2021
14 checks passed
@Zac-HD Zac-HD deleted the allow-slice-syntax branch Oct 18, 2021
@Zac-HD Zac-HD mentioned this pull request Oct 29, 2021
Merged
ilevkivskyi pushed a commit that referenced this pull request Oct 29, 2021
@Zac-HD
Allow slice syntax (#11406)
e4c52ee 
This is useful for Annotated, and crucial for downstream libraries like torchtyping.

This PR cherrypicks #11345 onto the 0.920 release branch
tushar-deepsource pushed a commit to deepsourcelabs/mypy that referenced this pull request Jan 20, 2022
@Zac-HD @tushar-deepsource
Allow slice syntax in e.g. Annotated[int, 1:3] and `TensorType["bat…
ebd6586 
…ch":..., float]` (python#11345)

This is useful for Annotated, and crucial for downstream libraries like torchtyping.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra

@sobolevn sobolevn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash with slices e.g. Annotated[int, 3:4]
3 participants
@Zac-HD @sobolevn @JelleZijlstra
X Tutup