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 inspect.get_annotations() #87983

Closed
larryhastings opened this issue Apr 12, 2021 · 46 comments
Closed

Add inspect.get_annotations() #87983

larryhastings opened this issue Apr 12, 2021 · 46 comments
Assignees
Labels
3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@larryhastings
Copy link
Contributor

BPO 43817
Nosy @gvanrossum, @warsaw, @larryhastings, @ericvsmith, @methane, @ambv, @JelleZijlstra, @tirkarthi, @Fidget-Spinner
PRs
  • bpo-43817: Add inspect.get_annotations(). #25522
  • Files
  • get_annotations.py
  • 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 = 'https://github.com/larryhastings'
    closed_at = <Date 2021-04-30.04:17:28.356>
    created_at = <Date 2021-04-12.17:39:44.372>
    labels = ['type-feature', 'library', '3.10']
    title = 'Add inspect.get_annotations()'
    updated_at = <Date 2021-04-30.04:17:28.355>
    user = 'https://github.com/larryhastings'

    bugs.python.org fields:

    activity = <Date 2021-04-30.04:17:28.355>
    actor = 'larry'
    assignee = 'larry'
    closed = True
    closed_date = <Date 2021-04-30.04:17:28.356>
    closer = 'larry'
    components = ['Library (Lib)']
    creation = <Date 2021-04-12.17:39:44.372>
    creator = 'larry'
    dependencies = []
    files = ['49968']
    hgrepos = []
    issue_num = 43817
    keywords = ['patch']
    message_count = 46.0
    messages = ['390874', '390875', '390876', '390877', '390878', '390881', '391172', '391203', '391213', '391222', '391228', '391496', '391497', '391506', '391507', '391510', '391525', '391583', '391589', '391590', '391591', '391594', '391759', '391763', '391765', '391766', '391767', '391769', '391770', '391919', '391945', '391988', '391989', '391991', '392000', '392001', '392003', '392004', '392005', '392006', '392009', '392011', '392012', '392304', '392379', '392380']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'barry', 'larry', 'eric.smith', 'methane', 'lukasz.langa', 'JelleZijlstra', 'xtreak', 'kj']
    pr_nums = ['25522']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue43817'
    versions = ['Python 3.10']

    @larryhastings
    Copy link
    Contributor Author

    Currently, with PEP-563 looming over us, people who use annotations for something besides type hints are kind of stuck. Converting stringized annotations back into useful values is a complicated problem, and there's only function in the standard library that will do it for you: typing.get_type_hints(). However, typing.get_type_hints() deals specifically with *type hints*. Type hints are a subset of annotations, and they have an additional set of rules they want to obey. As such typing.get_type_hints() is quite opinionated and makes additional changes to the annotations that someone who only wants to un-stringize their annotations likely does not want.

    I therefore propose adding a new function to typing:

        typing.get_annotations(o)

    This would behave similarly to typing.get_type_hints(o), except it would be agnostic about the contents of the annotations. It would simply evaluate them, without further changing them as typing.get_type_hints() does. Specifically:

    • It would ignore the "__no_type_check__" attribute.
    • It would not wrap string annotations with ForwardRef().
    • It would not wrap values with Optional[].
    • It would not turn an annotation of None into type(None).
    • It would not otherwise change the value of the annotation, in case I missed any.

    Since the two functions would be so similar, hopefully either they would share a common implementation, or typing.get_type_hints() could be implemented on top of typing.get_annotations().

    Guido, Eric, any thoughts?

    @larryhastings larryhastings added 3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Apr 12, 2021
    @larryhastings
    Copy link
    Contributor Author

    (realized mid-issue-creation that it should have a different name)

    @larryhastings larryhastings changed the title Add typing.eval_annotations() Add typing.get_annotations() Apr 12, 2021
    @larryhastings larryhastings changed the title Add typing.eval_annotations() Add typing.get_annotations() Apr 12, 2021
    @gvanrossum
    Copy link
    Member

    It's fair that get_type_hints() does too much. But what does your proposed function do besides this?

    {k: eval(v) for k, b in x.__annotations__.items()}
    

    ?

    @larryhastings
    Copy link
    Contributor Author

    To be honest, I'm not 100% sure. But I observe that typing.get_type_hints() is about fifty lines of code, and very few of them are the opinionated lines I want to avoid.

    Some things typing.get_type_hints() seems to do for you:

    • Work around the "classes inherit annotations" design bug.
    • Unwrap wrapped functions.
    • "Return empty annotations for something that _could_ have them."

    @gvanrossum
    Copy link
    Member

    I'd say just submit the PR. Shouldn't be problematic. (Have you decided
    what to do if the eval() fails?)

    @larryhastings
    Copy link
    Contributor Author

    I'd say just submit the PR. Shouldn't be problematic.

    Okey-doke, I can do it. Though I have plenty to do at the moment, though, so it wouldn't be this week.

    Is there anybody who would *enjoy* taking this on, who we could farm it out to? If not, that's no problem, I should be able to get it done before 3.10b1.

    (Have you decided what to do if the eval() fails?)

    My experience is that typing.get_type_hints() doesn't catch exceptions, e.g. NameError. I assumed that typing.get_annotations() should behave the same way.

    @methane
    Copy link
    Member

    methane commented Apr 16, 2021

    Should this feature implemented in typing module? How about inspect module?

    Additionally, how about adding get_annotation_str(), which returns str always?
    Some use cases (e.g. help(f)) need just string. I want to skip eval() in it.

    @larryhastings
    Copy link
    Contributor Author

    Should this feature implemented in typing module? How about inspect module?

    That's a reasonable idea.

    Part of the reasoning behind putting it in the typing module was to share its implementation with typing.get_type_hints(). However, I was reading the source to typing.get_type_hints() yesterday, and its implementation is completely based on the assumption that the annotations are type hints. It might be possible to rework its implementation to isolate these assumptions, but the remaining shared code would be probably less than fifty lines. So the code reuse isn't a big deal.

    If the two functions don't share an implementation, you're right, the inspect module is probably a better place for this new function.

    Additionally, how about adding get_annotation_str(), which returns str always? Some use cases (e.g. help(f)) need just string. I want to skip eval() in it.

    I don't know if I'd want to add a third function. Perhaps a parameter to get_annotation? eval_str=True?

    @gvanrossum
    Copy link
    Member

    Also, for crosslinking, see bpo-43463.

    @larryhastings
    Copy link
    Contributor Author

    Assuming this function is added for Python 3.10, I propose that uses of typing.get_type_hints() in the standard library that aren't specifically dealing with type hints should change to calling this function instead. I currently know of two sites:

    • inspect.signature()
    • functools.singledispatch()

    @gvanrossum
    Copy link
    Member

    Putting it in inspect makes sense then.

    @larryhastings larryhastings changed the title Add typing.get_annotations() Add inspect.get_annotations() Apr 20, 2021
    @larryhastings larryhastings changed the title Add typing.get_annotations() Add inspect.get_annotations() Apr 20, 2021
    @larryhastings
    Copy link
    Contributor Author

    Just over twelve hours ago, the Python Steering Committee announced that stringized annotations would no longer be default behavior in Python 3.10. They will go back to being gated with "from __future__ import annotations". I think we still need this function, but naturally the implementation will now be a bit different.

    It's going to take time to change the default semantics of annotations back to the way they were in Python 3.9. In the meantime, I've coded up a first draft of inspect.get_annotations(), written assuming stringized annotations are optional. It's attached below. I'll also paste in the definition and the docstring into the text box here for your reading convenience.

    I assert that get_annotations() should definitely try to un-stringize stringized annotations by default. The default behavior of eval_str is sliightly magical, but I'm not sure I can do anything any smarter (apart from "resist the temptation to guess"). Unsophisticated users will want to see real values, and it may be important to give sophisticated users control over whether or not eval() is called.

    By the same token: if eval_str stays as part of the interface, I propose exposing it from the library functions that will call get_annotations():

    • inspect.signature()
    • functools.singledispatch()
      That way, a project using stringized annotations due to gnarly circular imports/definitions problems could still use inspect.signature(), without having to worry about worrying about whether or not all the symbols were defined at runtime.

    I'll interpret a lack of feedback as a sort of mumbled encouraging consensus.

    -----

    def get_annotations(obj, globals=None, locals=None, *, eval_str=ONLY_IF_ALL_STR):
      
    Compute the annotations dict for an object.

    obj may be a callable, class, or module.
    Passing in any other type of object raises TypeError.

    This function handles several details for you:

    • Values of type str may be un-stringized using eval(),
      depending on the value of eval_str. This is intended
      for use with stringized annotations
      (from __future__ import annotations).
    • If obj doesn't have an annotations dict, returns an
      empty dict. (Functions and methods always have an
      annotations dict; classes, modules, and other types of
      callables may not.)
    • Ignores inherited annotations on classes. If a class
      doesn't have its own annotations dict, returns an empty dict.
    • Always, always, always returns a dict.

    eval_str controls whether or not values of type str are replaced
    with the result of calling eval() on those values:

    • If eval_str is true, eval() is called on values of type str.
    • If eval_str is false, values of type str are unchanged.
    • If eval_str is the special value inspect.ONLY_IF_ALL_STR,
      which is the default, eval() is called on values of type str
      only if *every* value in the dict is of type str. This is a
      heuristic; the goal is to only eval() stringized annotations.
      (If, in a future version of Python, get_annotations() is able
      to determine authoritatively whether or not an annotations
      dict contains stringized annotations, inspect.ONLY_IF_ALL_STR
      will use that authoritative source instead of the heuristic.)

    globals and locals are passed in to eval(); see the documentation
    for eval() for more information. If globals is None,
    get_annotations() uses a context-specific default value,
    contingent on type(obj):

    • If obj is a module, globals defaults to obj.__dict__.
    • If obj is a class, globals defaults to
      sys.modules[obj.__module__].__dict__.
    • If obj is a callable, globals defaults to obj.__globals__,
      although if obj is a wrapped function (using
      functools.update_wrapper()) it is first unwrapped.

    @larryhastings
    Copy link
    Contributor Author

    Hmm. If o.__annotations__ is None, should this function set the empty dict on the object? That seems slightly too opinionated to me. On the other hand, the user would probably expect that they could change the dict they got back. (If Python shipped with a builtin "frozen dict", I suppose I could safely return one of those.)

    Note that the danger of this happening should drop to about zero, assuming I push through my other small patch:

    https://bugs.python.org/issue43901

    @ericvsmith
    Copy link
    Member

    If o.__annotations__ is None, should this function set the empty dict on the object? That seems slightly too opinionated to me. On the other hand, the user would probably expect that they could change the dict they got back.

    Are you saying the user would expect to be able to change __annotations__ my modifying the dict they get back?

    Is it ever the case that the user can modify __annotations__ through the dict that's returned? That is: does __annotations__ itself ever get returned?

    I think you'd either want __annotations__ returned all the time, or never returned. Otherwise some cases could modify __annotations__, and some couldn't.

    If __annotations__ is never returned, then I wouldn't set __annotations__ in this case.

    @larryhastings
    Copy link
    Contributor Author

    Are you saying the user would expect to be able to change __annotations__ my modifying the dict they get back?

    As the docs are currently written, it's ambiguous.

    Is it ever the case that the user can modify __annotations__ through the dict that's returned? That is: does __annotations__ itself ever get returned?

    Yes. I could enumerate the cases in which that is true but I don't think it would shed light. Suffice to say, as currently written, get_annotations() currently returns the original dict unmodified when it can, and returns a freshly-created dict when it can't.

    I think you'd either want __annotations__ returned all the time, or never returned. Otherwise some cases could modify __annotations__, and some couldn't.

    I think you're right! And since there are definitely circumstances in which it can't return __annotations__ directly, that indicates that it should never return __annotations__ directly. Good call!

    @larryhastings
    Copy link
    Contributor Author

    Just to round the bases: get_annotations() won't return an unmodified __annotations__ dict, but it *could* return a *consistent* dict. It could keep a cache (lru or otherwise) of all responses so far. I don't think that's what we want, I just thought I should point out the option.

    @ericvsmith
    Copy link
    Member

    And since there are definitely circumstances in which it can't return __annotations__ directly, that indicates that it should never return __annotations__ directly.

    And the follow on here is that get_annotations() shouldn't set __annotations__ if it's None. I realize I'm just stating the obvious.

    @larryhastings
    Copy link
    Contributor Author

    When I proposed this new function, stringized annotations were the default behavior in Python 3.10, and there were two calls to typing.get_type_hints() in the standard library:

    • inspect.signature()
    • functools.singledispatch()

    Now that stringized annotations are no longer the default in 3.10, we've reverted that change to inspect.signature(). The call is still present in functools.singledispatch(), but that's been there for a while--it's in 3.9.4 for example.

    It's worth considering changing inspect.signature() to use get_annotations(). To be honest, I've never been really sure if the idea of PEP-563 stringized annotations are supposed to be a hidden implementation detail, or a first-class concept that the user (and the standard library) is expected to deal with. My current get_annotations() implementation assumes the former, and eval()s the annotations back into real Python values if it thinks that's the right thing to do. Personally I'd prefer that inspect.signature() un-stringized annotations for me--I would see that as a useful feature--but I could be completely wrong on this.

    For now, I'm going to plan on modifying inspect.signature() so it calls get_annotations() as part of this PR.

    (I'll also modify inspect.signature() to take a "eval_str" keyword-only parameter, and pass that through to get_annotations(). That gives the caller control over this un-stringizing feature, in case they need to turn it off.)

    Łukasz: what about functools.singledispatch()? IIUC, it uses typing.get_type_hints() to get the type of the first parameter of a function, so it can use that as an index into a dict mapping first-argument types to functions.. Since it's not a user-visible detail perhaps it doesn't matter. But it seems a little strange to me that it's using typing.get_type_hints()--ISTM it should be calling inspect.signature(), or possibly this proposed new inspect.get_annotations(). What do you think?

    @ericvsmith
    Copy link
    Member

    To be honest, I've never been really sure if the idea of PEP-563 stringized annotations are supposed to be a hidden implementation detail, or a first-class concept that the user (and the standard library) is expected to deal with.

    This is an excellent question. My assumption with dataclasses has been that it's something the user is expected to deal with. It's a user-visible change, that all users of __annotations__ are going to have to deal with. I understood PEP-573 as saying: if you don't want to see a stringized annotation, call typing.get_type_hints() yourself. My assumption is that neither @DataClass nor anybody else is going to do that for you.

    However, not everyone agreed. See for example bpo-39442.

    I do think that wherever we end up with __annotations__ in 3.11, we should be explicit about answering your question.

    @larryhastings
    Copy link
    Contributor Author

    I think it gets a little murkier when we talk about *annotations* vs *type hints*. Type hints have a defined meaning for a string: a string is a sort of forward declaration, and you eval() the string to get the real value. (Or, not, if you're comfortable working with the stringized version of the type hint.) So typing.get_type_hints() calls eval() on *every* annotation value of type str.

    But inspect.get_annotations() can't be so opinionated. If the user entered a string as their annotation, it should assume they want the string to show up in the annotations dict. This is why I'm trying to be so smart with the "eval_str" default value heuristic.

    The text in the docs about a "future version of Python" is pursuant to my vague "PEP-1212" idea, which would let get_annotations() determine or not whether the annotations were stringized by the compiler. Or whatever we wind up deciding to do for Python 3.11--which, as you say, will hopefully disambiguate this question.

    p.s. assuming you meant PEP-563, not PEP-573.

    @ericvsmith
    Copy link
    Member

    p.s. assuming you meant PEP-563, not PEP-573.

    Yes. Someday I'll figure out this keyboard.

    @larryhastings
    Copy link
    Contributor Author

    Time runs short for Python 3.10b1. Can I get a review from anybody here, say over the weekend?

    @methane
    Copy link
    Member

    methane commented Apr 24, 2021

    I'm not sure ONLY_IF_ALL_STR is worth enough.

    Will anyone use it after Python 3.10? inspect.signature()? Pydantic?

    @larryhastings
    Copy link
    Contributor Author

    For what it's worth: I changed the name to ONLY_IF_STRINGIZED in the PR.

    Since I propose that it be the default, everyone who called inspect.get_annotations(), and inspect.signature(), would use it. I think Pydantic would prefer it, because Pydantic wants to see the real objects at runtime, rather than the stringized annotations.

    If stringized annotations were deprecated in the future, then eval_str should be deprecated at the same time.

    @methane
    Copy link
    Member

    methane commented Apr 24, 2021

    I think Pydantic would prefer it, because Pydantic wants to see the real objects at runtime, rather than the stringized annotations.

    If so, why don't they use eval_str=True?

    I can not find any use cases where eval_str= ONLY_IF_ALL_STR is better than eval_str=True.

    On the other hand, I think "return string annotation instead of raising error when eval failed" option is useful.

    @larryhastings
    Copy link
    Contributor Author

    The difference between eval_str=True and eval_str=ONLY_IF_STRINGIZED:

    def foo(a:int, b:"howdy howdy"): ...

    inspect.get_annotations(foo, eval_str=True) throws an exception.
    inspect.get_annotations(foo, eval_str=ONLY_IF_STRINGIZED) returns {'a': int, b: 'howdy howdy'}

    Type hints have a convention that string annotations are a "forward declaration" and should be eval()uated. Annotations don't have such a convention--a string is a legal annotation, and is not required to be valid Python.

    @methane
    Copy link
    Member

    methane commented Apr 24, 2021

    The difference between eval_str=True and eval_str=ONLY_IF_STRINGIZED:

    def foo(a:int, b:"howdy howdy"): ...

    inspect.get_annotations(foo, eval_str=True) throws an exception.
    inspect.get_annotations(foo, eval_str=ONLY_IF_STRINGIZED) returns {'a': int, b: 'howdy howdy'}

    Type hints have a convention that string annotations are a "forward declaration" and should be eval()uated. Annotations don't have such a convention--a string is a legal annotation, and is not required to be valid Python.

    For such use case, ONLY_IF_STRINGIZED thorows an exception for def foo(a: "howdy howdy") anyway.
    In such cases, they should use eval_str=False, or eval_str=True
    and return_str_when_eval_failed=True option.

    @larryhastings
    Copy link
    Contributor Author

    Perhaps eval_str=ONLY_IF_STRINGIZED should also add the semantics "if evaluating any string fails, behave as if eval_str=false". I would *not* propose adding that for eval_str=true. But people keep asking for this. Hmm.

    The heuristic is a tricky thing. That's why in my "PEP-1212" idea I proposed a way that a function like get_annotations() could determine unambiguously whether or not the annotations for an object were stringized. (And, if we did do something like that in a future Python version, this would also change the "if evaluating any string fails" behavior I just proposed.)

    @larryhastings
    Copy link
    Contributor Author

    I keep thinking about it, and I think letting inspect.get_annotations() and inspect.signature() raise exceptions is the right API choice.

    I note that that's what typing.get_type_hints() did in Python 3.9. During the development of Python 3.10, this was changed; typing.get_type_hints() started catching Exception and returning the original annotation dictionary (with un-eval'd strings). This change has been reverted, and as of the current moment typing.get_type_hints() no longer catches exceptions on eval().

    I think there are two types of people who will have string annotations that throw an exception when eval'd:

    1. People who made a typo or other mistake.
    2. People who deliberately use undefined identifiers in their annotations, due to circular import / circular dependencies in their code bases.

    The people who are saying "just catch the exception and let it pass silently" seem to be in group 2. I suggest that people in group 2 are sophisticated enough to start passing in eval_str=False to inspect.signature(). And I think people in group 1 would want to be alerted to their mistake, rather than have the library silently catch the error and mysteriously change its behavior.

    @gvanrossum
    Copy link
    Member

    I'm not a big user of the inspect module, but I always thought that its use was so that you could look at a function (or other object) *produced by a 3rd party* and learn something about it.

    Asking for the signature is one such operation, and I'd be rather upset if I couldn't inspect the annotation if that 3rd party happened to have added a bad string annotation. especially if I wasn't interested in the annotation part of the signature in the first place. (There's a lot of other useful info to be gleaned from it.)

    OTOH I think it's fine for get_annotations() to raise by default if there's a bad annotation.

    @larryhastings
    Copy link
    Contributor Author

    I'm not a big user of the inspect module, but I always thought that
    its use was so that you could look at a function (or other object)
    *produced by a 3rd party* and learn something about it.

    That's interesting! I always thought its main use was the opposite--it was so third parties could introspect *your* functions. Like, Pydantic examines your function's signature so it can wrap it with runtime validation. (Though in this specific case, I believe Pydantic examines __annotations__ directly.) I also dimly recall a Python library that built COM bindings around your code, but I can't find it right now.

    I can't remember ever writing application code that used inspect.signature() to inspect library functions--but I've certainly written library functions that used inspect.signature() to inspect application code.

    Asking for the signature is one such operation, and I'd be rather
    upset if I couldn't inspect the annotation if that 3rd party happened
    to have added a bad string annotation.

    Do you think that's likely? I'd be quite surprised if it was common for third party libraries to have string annotations--either manual or using automatic stringizing--in the first place. I thought string annotations were really only used as part of type hints, and the type-hinted code was all internal code bases from large organizations, not third-party libraries. But I concede I know little about it.

    Following on to that, the annotations would have to be bad, which means again either someone made a typo or it was done deliberately. The latter reason would be simply obnoxious if done by a third-party library--third-party libraries should not have the circular imports problem used to justifiy such practices.

    I suppose third-party libraries are just as error-prone as anybody else, so if they were manually stringizing their annotations, it could happen there. Which I agree would be annoying, just like any bug in a third-party library. But I wouldn't agree this specific error is so special that we need to suppress it.

    @JelleZijlstra
    Copy link
    Member

    I agree with Guido that it's better to design inspect.signature to not throw an error for annotations that don't eval() cleanly.

    I use inspect.signature for getting information about callables (third-party and first-party) in my type checker: https://github.com/quora/pyanalyze/blob/master/pyanalyze/arg_spec.py#L436.
    In that context, I'd much rather get string annotations that I can process myself later than get an exception if the annotations aren't valid at runtime. In the former case I can still get useful information out of the signature even if I don't know how to process some annotations. For example, I'll still know what parameters exist even if I don't know what their types are. In the latter case, I don't get any useful signature data.

    @gvanrossum
    Copy link
    Member

    There may be a (deliberate? :-) misunderstanding. When I wrote about "you" inspecting code by a "3rd party" I meant that as a symmetric relationship -- the "you" could be a library and from the library's POV the "3rd party" could be you (or me).

    Either way, given that inspect.signature() collects many disparate aspects of the parameters of a function, it seems a bad design for it to raise an exception if any one of those aspects of any one of those parameters doesn't match an expectation (other than an expectation enforced by the parser+compiler themselves, like the constraint that positional-only parameters must precede other types of parameters).

    I looked at the code, and there's a special value that Parameter().annotation is set if there's no annotation (Parameter.empty). Maybe you can follow that model and add another special value for errors during evaluation?

    @larryhastings
    Copy link
    Contributor Author

    I use inspect.signature for getting information about callables
    (third-party and first-party) in my type checker:
    https://github.com/quora/pyanalyze/blob/master/pyanalyze/arg_spec.py#L436.
    In that context, I'd much rather get string annotations that I can
    process myself later than get an exception if the annotations aren't
    valid at runtime.

    This use case is what the eval_str parameter is for. Since you're dealing specifically with type hints (as opposed to annotations generally), you don't really care whether you get strings or valid objects--you need to handle both. So in Python 3.10+ you can--and should!--call inspect.get_annotations() and inspect.signature() with eval_str=False. If you do that you won't have any trouble.

    Since I keep getting new proposals on how to suppress eval() errors in inspect.signature(), I think I need to zoom out and talk about why I don't want to do it at all. Forgive me, but this is long--I'm gonna start from first principles.

    Changing inspect.signature() so it calls eval() on annotations is a change in 3.10. And, due to the fact that it's possible (how likely? nobody seems to know) that there are malformed string annotations lurking in user code, this has the possibility of being a breaking change.

    In order to handle this correctly, I think you need to start with a more fundamental question: are *stringized* annotations supposed to be a hidden implementation detail, and Python should present annotations as real values whether or not they were internally stringized? Or: if the user adds "from __future__ import annotation" to their module, is this them saying they explicitly want their annotations as strings and they should always see them as strings?

    (Again, this is specifically when the *language* turns your annotations into strings with the "from __future" import. I think if the user enters annotations as strings, the language should preserve those annotations as strings, and that includes the library.)

    It seems to me that the first answer is right. PEP-563 talks a lot about "here's how you turn your stringized annotations back into objects". In particular, it recommends calling typing.get_type_hints(), which AFAIK has always called eval() to turn string annotations back into objects. It's worth noting here that, in both 3.9 and in the current Python trunk, typing.get_type_hints() doesn't catch exceptions.

    Also, during the development of Python 3.10, during a time when stringized annotations had become the default behavior, inspect.signature() was changed to call typing.get_type_hints(). Presumably to have typing.get_type_hints() handle the tricky work of calling eval().

    (I had problems with this specific approach. "annotations" and "type hints" aren't the same thing, so having inspect.signature() e.g. wrap some annotations with Optional, and change None to NoneType, was always a mistake. Also, typing.get_type_hints() was changed at this time to catch "Exception" and suppress *all* errors raised during the eval() call. Happily both these changes have since been backed out.)

    From that perspective, *not* having inspect.signature() turn stringized annotations back into strings from the very beginning was a bug. And changing inspect.signature() in Python 3.10 so it calls eval() on string annotations is a bug fix. So far folks seem to agree--the pushback I'm getting is regarding details of my approach, not on the idea of doing it at all.

    Now we hit our second question. It's possible that inspect.signature() can't eval() every stringized annotation back into a Python value, due to a malformed annotation expression that wasn't caught at compile-time. This means inspect.signature() calls that worked in 3.9 could potentially start failing in 3.10. How should we address it?

    From the perspective of "string annotations are a hidden implementation detail, and users want to see real objects", I think the fact that it wasn't already failing was also a bug. If your annotations are malformed, surely you want to know about it. If you have a malformed annotation, and you don't turn on stringized annotations, you get an exception when the annotation expression is evaluated at module import time, in any Python version up to an including 3.10. Stringized annotations delays this evaluation to when the annotations are examined--which means the exception for a malformed expression gets delayed too. Which in turn means, from my perspective, the fact that inspect.signature() didn't raise on malformed annotation expressions was a bug.

    I just don't agree that silently changing the failed eval()'d string annotation into something else is the right approach. There have been a bunch of proposals along these lines, and while I appreciate the creative contributions and the attempts at problem-solving, I haven't liked any of them. But it's not that I find that particular spelling awful, and if we could work together we'll find a spelling that I like. I don't like the basic approach of silently suppressing these errors--I think inspect.signature() raising an exception is *already* the right behavior. Again I'll quote from the Zen: "errors should never pass silently unless explicitly silenced", and, "special cases aren't special enough to break the rules".

    (For the record, Guido's new proposal of "add a field to the Parameter object indicating the error" is the best proposal yet. I don't want to do this at all, but if somehow it became simply unavoidable, ATM that seems like the best approach to take.)

    I must admit I'm a little puzzled by the pushback I'm getting. I thought the pushback would be "having inspect.signature() call eval() at all is a breaking change, it's too late to change it". But the pushback has been all about coddling users with malformed annotations, by making the errors pass silently. We're talking about user code that has errors, and these errors have been passing silently, potentially for years. I regret the inconvenience that 3.10 will finally start raising an exception for these errors--and yet it still seems like an obviously better design than silently suppressing these errors.

    (I also think that catching errors quickly goes down a rabbit hole. Should inspect.signature() also suppress MemoryError? RecursionError? ImportError?)

    Finally, there's an extant use case for code bases to deliberately provide malformed, un-evaluatable annotations. This is due to structural problems endemic with large code bases (circular dependencies / circular imports). Again I think the Zen's guidance here is right, and I think it's reasonable to expect code bases that deliberately break the rules to work around this change. I realize that this bug fix in the standard library will inconvenience them--but that's why I made sure to have something like eval_str=False. I have empathy for the bind these users find themselves in, and I want to ensure they have the tools they need to succeed. But I think they need to work around bug fixes in the library, rather than preserve the old bug they were implicitly relying on.

    And there's one more loose thread that I need to tie off here: what about users with objects with manually-entered string annotations that are deliberately not valid Python, who examine these objects with inspect.signature()? I regret that this is collateral damage. "from __future__ import annotations" doesn't give us any way to distinguish between "this annotation was automatically stringized by Python" and "this annotation is a string explicitly entered by the user". My heuristic with the default behavior of inspect.get_annotations() and inspect.signature() (see eval_str=ONLY_IF_STRINGIZED) is an attempt to accommodate such usage. But this heuristic won't do the right thing if the user manually stringizes all the annotations for an object. In this case, this is legitimately permissible code, which will start failing in Python 3.10. I apologize in advance to such users. But my guess is this is exceedingly rare, and again such users can switch to eval_str=False at which point they'll be back in business.

    @ericvsmith
    Copy link
    Member

    I'd like to see the default behavior be to raise an exception if eval fails on any annotation.

    I think it's reasonable to provide a way to find out which specific keys have problems, but I don't think that should be the default. Wouldn't it be good enough to have a flag which says "just return me a dict which only has keys for items which don't contain errors"? Let's call it silence_errors for discussion sake. You could then figure out which ones contain errors by:

    getattr(obj, "__annotations__", {}).keys() - get_annotations(obj, silence_errors=True).keys()

    That is, silence_errors works on a per-key basis, not the call to get_annotations() as a whole.

    @larryhastings
    Copy link
    Contributor Author

    There may be a (deliberate? :-) misunderstanding. When I wrote about
    "you" inspecting code by a "3rd party" I meant that as a symmetric
    relationship -- the "you" could be a library and from the library's
    POV the "3rd party" could be you (or me).

    I wasn't deliberately misunderstanding you. And I understand what you're saying. But the relationship isn't perfectly symmetric from a pragmatic perspective.

    Let's say I write some code, and I also call into a third-party library. I've annotated one of my objects, and the third-party library calls inspect.signature() on my object. If my annotations are strings, and they aren't eval()uatable, then boom! it raises an exception, showing me a bug in my code, and I can fix it straight away.

    On the other hand: let's say I write some code, and I call into a third-party library. If the third-party library has an annotated object, and I call inspect.signature() on that object, and the annotations happen to be strings that aren't evaluatable, boom! it's showing me a bug in the third-party library. Now I have a much larger headache: I have to notify this third-party vendor, convince them to fix the bug, wait for a new release, etc etc etc. (And in the meantime maybe I add eval_str=False to my inspect.signature() call.)

    It's my assumption that the former scenario is far more likely than the latter, which is good news because it's way easier to deal with. The latter scenario is plausible, and much more inconvenient, but I think it's far less likely. If I'm wrong, and there are lots of third-party libraries with stringized annotations that could have errors lurking in them, I'd very much like to know that.

    @larryhastings
    Copy link
    Contributor Author

    I like Eric's suggestion best of all. I'd be willing to add a "silence errors on a case-by-case basis" flag to inspect.signature(). I imagine that would add a new field to the Parameter object (as Guido suggested) indicating which objects failed. Would that be acceptable all around?

    @larryhastings
    Copy link
    Contributor Author

    Just to be clear: I would *not* want this new mode to be the *default* behavior. So far I think ONLY_IF_STRINGIZED is the best compromise for default behavior.

    @larryhastings
    Copy link
    Contributor Author

    One final thought on that idea. Part of the reason why I didn't go back to the drawing board and re-think the API was because I thought folks were pushing back on the idea of *default* behavior possibly raising exceptions.

    If I misread it, and the pushback was "we want real Python values when we can get them, and strings when we can't, and it's acceptable if we have to explicitly request that behavior from inspect.signature() and inspect.get_annotations()", then hot diggity!, maybe we've finally found a compromise we can all agree on.

    @gvanrossum
    Copy link
    Member

    I want to be as short as possible.

    The user may have annotations because they are using mypy. They may be
    using a library that calls inspect.signature() on some of their functions
    to retrieve the parameter names conveniently.

    All this has worked for years, the library never caring about annotations,
    the user probably adding a few more annotations as they get more value out
    of mypy.

    Now one day, using Python 3.9, the user adds "from __future__ import
    annotations" to their module, and removes the quotes from annotations
    containing forward references or things imported under "if TYPE_CHECKING".

    This works in 3.9. But you want to break it in 3.10. This is what I object
    to.

    I realize this also broke in the 3.10 alphas where PEP-563 was the default.
    But that was rolled back for a reason! And we changed inspect.signature()
    to call get_type_hints() (i.e. call eval() and break if it fails) because
    it wasn't just affecting users PEP-563, it was affecting everyone who *was*
    expecting a type object rather than a string in Parameter.annotation. That
    bug also exists in 3.9, but only for users of PEP-563 (most of whom didn't
    care about runtime inspection of annotations, so it didn't matter to them
    that Parameter.annotation got a string.

    You can draw the matrix of use cases and Python versions yourself.

    Given that we're not going whole-hog on PEP-563 in 3.10, I think we should
    either leave sleeping dogs lie, and continue to return a string as
    Parameter.annotation if PEP-563 was in effect, or else we should at least
    make inspect.signature() not fail if the string fails to evaluate. Note
    that the version of inspect.signature() that I remember being checked into
    3.10 did catch exceptions from get_type_hints() and used the original
    __annotations__ dict (i.e. with all strings) if there was an eval error (or
    any other error, IIRC). Please go look for it if you don't believe me (it
    would have been rolled back last week in the Great Rollback).

    @gvanrossum
    Copy link
    Member

    No, I want the *default* not to raise when eval() fails.

    @methane
    Copy link
    Member

    methane commented Apr 27, 2021

    Just to be clear: I would *not* want this new mode to be the *default* behavior. So far I think ONLY_IF_STRINGIZED is the best compromise for default behavior.

    I don't think ONLY_IF_STRINGIZED is the best compromise. I don't think it solve any issue.

    if False:
        from typing import List
    
    def f1() -> 'List[str]':
        pass
    
    def f2(a:int) -> 'List[str]':
        pass
    

    In this example, both of help(f1) and help(f2) must show signature, not raise an error.

    @larryhastings
    Copy link
    Contributor Author

    Ie debated about this with myself (and with a friend!) over the last few days, and I've concluded that evaluating strings by default is too opinionated for the standard library. I've changed both inspect.signature() and inspect.get_annotations() so eval_str is False by default (and removed the ONLY_IF_STRINGIZED logic entirely). Both functions will still raise an exception if eval_str=True, but this will only happen if the user explicitly requests evaluating the strings.

    I've already updated the PR.

    @larryhastings
    Copy link
    Contributor Author

    New changeset 74613a4 by larryhastings in branch 'master':
    bpo-43817: Add inspect.get_annotations(). (bpo-25522)
    74613a4

    @larryhastings
    Copy link
    Contributor Author

    Thanks for your feedback, everybody! It's now checked in.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants