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

Add support for typing_extensions.Annotated #7021

Closed
ilevkivskyi opened this issue Jun 18, 2019 · 6 comments · Fixed by #7292
Closed

Add support for typing_extensions.Annotated #7021

ilevkivskyi opened this issue Jun 18, 2019 · 6 comments · Fixed by #7292

Comments

@ilevkivskyi
Copy link
Collaborator

@ilevkivskyi ilevkivskyi commented Jun 18, 2019

It would be great to support Annotated types as specified by PEP 593. This should be pretty easy to implement.

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jun 19, 2019

If nobody else is working on this, I'd be happy to do it.

@ilevkivskyi
Copy link
Collaborator Author

@ilevkivskyi ilevkivskyi commented Jun 19, 2019

OK, go ahead and try it!

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Aug 5, 2019

Doing this right turned out to be significantly more difficult than I anticipated.

It's trivial to blindly strip away annotations when encountered, or to support anything that can be put in a Literal... But the PEP seems to allow literally any expression, such as function calls like struct2.ctype("<10s") and ValueRange(-20, 3). A proper implementation would most likely require deferring these expressions until a new, later pass, or somehow validating and re-stringifying the expressions... and I don't know enough about mypy's internals to implement that.

If we're okay with just relying on plugins to handle any fancy business, then dropping all annotations and resolving to the inner type, I can still do that fairly easily. But I'm not quite sure what the vision is here going forward.

@msullivan
Copy link
Collaborator

@msullivan msullivan commented Aug 5, 2019

For a first pass, I think it is totally fine to just discard the additional annotations. The important bit is that we are able to process code with Annotated, ignoring any annotations. (Given that we don't support any of them yet!)

In order to support actually using Annotated things, we probably want to be more liberal in what we shove into RawExpressionType, and actually store an Expression there. Then we would push back evaluation of that into actual literal values back into type analysis. Pinging @Michael0x2a to see if that makes sense?
But that doesn't need to be present in the initial pass.

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Aug 6, 2019

Okay, thanks for the clarification. It looks like plugins will be able to use get_type_analyze_hook to make any changes before annotations are discarded, too.

I'll have something up this week for review, hopefully.

@Michael0x2a
Copy link
Collaborator

@Michael0x2a Michael0x2a commented Aug 6, 2019

In order to support actually using Annotated things, we probably want to be more liberal in what we shove into RawExpressionType, and actually store an Expression there. Then we would push back evaluation of that into actual literal values back into type analysis.

Yeah, this seems pretty reasonable to me. The only caveat is that we currently do a bit of hackery in the earlier parsing and semantic analysis layers to handle some str/unicode/bytes nonsense when constructing RawExpressionTypes. I'm not sure how complicated pushing this logic up into type analysis will be -- probably it won't be too bad?

But if it gets too annoying, we could maybe sidestep the issue by having RawExpressionType contain a union of Expression and the existing allowed values. That would let us leave most of the existing hackery alone, just at the cost of introducing even more hackery. ¯\_(ツ)_/¯

The only other main constraint is that we need to make sure RawExpressionTypes don't leak out of the semantic analysis/type analysis phase -- but the strategy of delegating everything to plugins and/or dropping all annotations ought to handle that.

msullivan added a commit that referenced this issue Aug 6, 2019
Closes #7021. This adds basic support for `typing_extensions.Annotated`. Currently, we just discard all annotations and resolve to the inner type.

Plugins can use `get_type_analyze_hook` to try to intervene before this step, but most of the arguments that aren't just names or `Literal`-able values will have already been mangled by this point. Nothing other than the type sees any sort of validation, either. See the issue discussion for more on this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants