Skip to content

Improve annotated support with non types #9625

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 24, 2020
Merged

Improve annotated support with non types #9625

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f2de9c3
Add failing test
patrick91 Oct 22, 2020
c2358cb
Don't try to convert annotated's second param to type
patrick91 Oct 22, 2020
da5e9f6
Use fullname to check if the current expression is Annotated
patrick91 Oct 22, 2020
8ac1694
Improve check for annotation
patrick91 Oct 22, 2020
32e2beb
Only return the annotation's type
patrick91 Oct 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion mypy/exprtotype.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ def expr_to_unanalyzed_type(expr: Expression, _parent: Optional[Expression] = No
args = expr.index.items
else:
args = [expr.index]
base.args = tuple(expr_to_unanalyzed_type(arg, expr) for arg in args)
if base.name == 'Annotated':
base.args = (expr_to_unanalyzed_type(args[0], expr), args[1])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this seems to work, but it will probably fail the type checking since now the args are of different types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it feels a bit hacky, let me know what you think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the check is potentially wrong too, but I think it might be worth checking this PR and maybe try to find a better way to do this 😊

Copy link
Collaborator

@hauntsaninja hauntsaninja Oct 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You're correct that you're failing type checking. You can probably fix the first type error by narrowing / guarding expr.base to the type that it is when its typing.Annotated (probably isinstance(expr.base, RefExpr)?)

The second type error is trickier and might indicate a fundamental problem. If there isn't a way around this, we could band aid this problem by just returning expr_to_unanalyzed_type(args[0], expr). I'd imagine the implication of this is that mypy and mypy plugins won't recognise the alias as being Annotated, but at least we no longer have the false positive.

Finally, Annotated is variadic, so it can have arbitrarily many parameters, so you don't want to just hardcode it to two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct that you're failing type checking. You can probably fix the first type error by narrowing / guarding expr.base to the type that it is when its typing.Annotated (probably isinstance(expr.base, RefExpr)?)

Done :)

The second type error is trickier and might indicate a fundamental problem. If there isn't a way around this, we could band aid this problem by just returning expr_to_unanalyzed_type(args[0], expr). I'd imagine the implication of this is that mypy and mypy plugins won't recognise the alias as being Annotated, but that's not a regression and we no longer have the false positive.

I followed your suggestion for now, mostly to see if there's any other failing test.
I wonder if need the annotations anywhere else (I guess it might useful for plugins 🤔)

Happy to change it to something else, but I'm not sure what can we change now (I'm still unfamiliar with mypy's source code) :)

else:
base.args = tuple(expr_to_unanalyzed_type(arg, expr) for arg in args)
if not base.args:
base.empty_tuple_index = True
return base
Expand Down
10 changes: 10 additions & 0 deletions test-data/unit/check-annotated.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,3 +116,13 @@ Alias = Annotated[Union[T, str], ...]
x: Alias[int]
reveal_type(x) # N: Revealed type is 'Union[builtins.int, builtins.str]'
[builtins fixtures/tuple.pyi]

[case testAnnotatedSecondParamNonType]
from typing_extensions import Annotated

class Meta:
...

x = Annotated[int, Meta()]
reveal_type(x) # N: Revealed type is 'def () -> builtins.int'
[builtins fixtures/tuple.pyi]