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

Support annotations in signature strings. #81677

Open
ericsnowcurrently opened this issue Jul 3, 2019 · 15 comments
Open

Support annotations in signature strings. #81677

ericsnowcurrently opened this issue Jul 3, 2019 · 15 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Jul 3, 2019

BPO 37496
Nosy @gvanrossum, @ambv, @ericsnowcurrently, @ilevkivskyi, @pablogsal, @isidentical, @shihai1991, @potomak, @FFY00

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2019-07-03.23:45:26.337>
labels = ['type-feature', 'library', '3.9']
title = 'Support annotations in signature strings.'
updated_at = <Date 2020-05-17.19:37:08.940>
user = 'https://github.com/ericsnowcurrently'

bugs.python.org fields:

activity = <Date 2020-05-17.19:37:08.940>
actor = 'FFY00'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2019-07-03.23:45:26.337>
creator = 'eric.snow'
dependencies = []
files = []
hgrepos = []
issue_num = 37496
keywords = []
message_count = 10.0
messages = ['347247', '348139', '348182', '348496', '348556', '349184', '349210', '357676', '357679', '369042']
nosy_count = 10.0
nosy_names = ['gvanrossum', 'lukasz.langa', 'eric.snow', 'levkivskyi', 'pablogsal', 'BTaskaya', 'Eric Wieser', 'shihai1991', 'potomak', 'FFY00']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue37496'
versions = ['Python 3.9']

Linked PRs

@ericsnowcurrently
Copy link
Member Author

In early 2014 (3.3), when argument clinic was added, we added support for turning func.__text_signature__ into an inspect.Signature object. However, at that time we did not add support for annotations (see the nested "parse_name()" in _signature_fromstr(). Annotations should be supported.

I'd expect that PEP-563 (Postponed Evaluation of Annotations) could be leveraged.

@ericsnowcurrently ericsnowcurrently added 3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jul 3, 2019
@potomak
Copy link
Mannequin

potomak mannequin commented Jul 19, 2019

I'd like to work on this, but I'm kind of new to the codebase. Do you think I should leave this task to someone more expert on the matter?

I took a look at the function you mentioned and I was able to support simple annotations, for instance x: int, by evaluating node.annotation.id. This method doesn't work with more complex annotations thought. For instance a simple type alias (Foo = int, x: Foo) breaks this naive implementation. Another error happens if the annotation is not a simple id, but a subscript function application, for instance x: List[int].

Do you have any suggestion for how to continue working on this task?

@ilevkivskyi
Copy link
Member

You might want to look into how PEP-563 is implemented, it has a utility to turn an AST back into a string (I assume this is what you want).

@ericsnowcurrently
Copy link
Member Author

+1 on using a string for Parameter.annotation and Signature.return_annotation.

@potomak
Copy link
Mannequin

potomak mannequin commented Jul 27, 2019

You might want to look into how PEP-563 is implemented, it has a utility to turn an AST back into a string (I assume this is what you want).

Thanks for your suggestion @levkivskyi.

I took a look at #4390, that implements PEP-563.

In that PR ast_unparse.c has been introduced, that is

a close translation of the relevant parts of [Tools/unparse.py](https://github.com/python/cpython/blob/main/Tools/unparse.py)

Source: #4390 (comment)

I have a couple of questions about how to use ast_unparse.c's expr_as_unicode function:

  1. Should I create a new function in the ast module that exposes that C function in Python in order to use it in [Lib/inspect.py](https://github.com/python/cpython/blob/main/Lib/inspect.py)?
  2. Would it be better to just re-use the AST to string implementation in [Tools/unparse.py](https://github.com/python/cpython/blob/main/Tools/unparse.py)?

On a side note: would it make sense to reconcile the two "unparse AST" implementations?

+1 on using a string for Parameter.annotation and Signature.return_annotation.

Thanks for your comment @eric.snow.

From what I saw in the test I've written, right now Parameter.annotation and Signature.return_annotation return Python type classes.

At the beginning, in order to keep Parameter.annotation's return type consistent with the current implementation, I tried to evaluate the annotation's "unparse AST" string output, but I was getting errors evaluating type aliases and refined types.

Returning strings would make parse_name's implementation easier, but I'm wondering if would be backward compatible.

@ilevkivskyi
Copy link
Member

I have a couple of questions about how to use ast_unparse.c's expr_as_unicode function:

  1. Should I create a new function in the ast module that exposes that C function in Python in order to use it in [Lib/inspect.py](https://github.com/python/cpython/blob/main/Lib/inspect.py)?
  2. Would it be better to just re-use the AST to string implementation in [Tools/unparse.py](https://github.com/python/cpython/blob/main/Tools/unparse.py)?

Hm, TBH I am not sure which one is better, I am leaning towards option 1, but I add more people to get some opinions.

@gvanrossum
Copy link
Member

  1. Should I create a new function in the ast module that exposes that C function in Python in order to use it in [Lib/inspect.py](https://github.com/python/cpython/blob/main/Lib/inspect.py)?
  2. Would it be better to just re-use the AST to string implementation in [Tools/unparse.py](https://github.com/python/cpython/blob/main/Tools/unparse.py)?

I would vote for #1. If it is not up to the task that probably points to a flaw anyway.

@isidentical
Copy link
Sponsor Member

We have exposed Tools/unparse.py under ast module, so that option also can help. @potomak are you still interested in working on this issue?

@potomak
Copy link
Mannequin

potomak mannequin commented Dec 1, 2019

Thanks for the info.

Yes, I’m still interested in working on the issue, but lately I didn’t have
much time to do it.

If this is a high priority feel free to re-assign to someone else,
otherwise I’ll start working on it next week.

On Sun, Dec 1, 2019 at 6:59 AM Batuhan <report@bugs.python.org> wrote:

Batuhan <batuhanosmantaskaya@gmail.com> added the comment:

We have exposed Tools/unparse.py under ast module, so that option also can
help. @potomak are you still interested in working on this issue?

----------
nosy: +BTaskaya, pablogsal


Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue37496\>


@EricWieser
Copy link
Mannequin

EricWieser mannequin commented May 16, 2020

This seems somewhat related to https://bugs.python.org/issue31939

@skirpichev
Copy link
Contributor

It seems nobody is working on this issue, so I'll try in #101872.

skirpichev added a commit to skirpichev/cpython that referenced this issue Sep 3, 2023
skirpichev added a commit to skirpichev/cpython that referenced this issue Oct 24, 2023
@skirpichev
Copy link
Contributor

@encukou, maybe this should be closed like #93865?

@encukou
Copy link
Member

encukou commented Feb 26, 2024

I think that's @ericsnowcurrently's call, as he filed the issue.
While the straightforward way to do this didn't work out (particularly: making the current approach public/supported/documented probably isn't a good idea), there are ideas of other ways to tackle the issue.

@ericsnowcurrently
Copy link
Member Author

I don't recall having any need for this, nor do I recall why I opened this issue. It does still make sense to me that we would support annotations in __text_signature__ though.

Also, note that support for annotations in signature strings extends beyond __text_signature__. Even though the docs say:

If the passed object has a __signature__ attribute, this function returns it without further computations.

...the implementation does further computations, including handling a string (even if imperfectly):

cpython/Lib/inspect.py

Lines 2520 to 2521 in 53b84e7

if isinstance(sig, str):
sig = _signature_fromstr(sigcls, obj, sig)

So I think this issue should stay open unless we don't plan on fully supporting str signatures in __signature__.

@skirpichev
Copy link
Contributor

skirpichev commented Feb 27, 2024

It does still make sense to me that we would support annotations in text_signature though.

I did this in #101872 (now this pr has several commits more, after 177c993, needed for my PEP draft), but @encukou was against this.

making the current approach public/supported/documented probably isn't a good idea)

BTW, making string processing public wasn't part of this proposal or #101872.

implementation does further computations, including handling a string (even if imperfectly)

#115937

@erlend-aasland erlend-aasland removed the 3.9 only security fixes label Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

7 participants