Skip to content

Conversation

@patrick91
Copy link
Contributor

@patrick91 patrick91 commented Oct 22, 2020

Closes #9315

This PR aims to support Annotated to create type Aliases when passing a non type as annotation, like this:

class Meta:
    ...

x = Annotated[int, Meta()]

You can test with:

pytest -k testAnnotatedSecondParamNonType --pdb

@patrick91 patrick91 marked this pull request as draft October 22, 2020 00:21
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

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) :)

@patrick91 patrick91 marked this pull request as ready for review October 22, 2020 00:27
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
Collaborator

@hauntsaninja hauntsaninja Oct 22, 2020

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
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.

Thanks! I'm okay with this, but I'll leave it for a bit to see if someone else has something to say (I don't really know much about plugins)

@JukkaL JukkaL merged commit 161ee24 into python:master Oct 24, 2020
@JukkaL
Copy link
Collaborator

JukkaL commented Oct 24, 2020

Looks good. Thanks for the PR!

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.

Type alias for Annotated[some_type, non_type] not supported

3 participants