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

Inconsistent __args__ between typing.Callable and collections.abc.Callable #86361

Closed
Zac-HD mannequin opened this issue Oct 29, 2020 · 27 comments
Closed

Inconsistent __args__ between typing.Callable and collections.abc.Callable #86361

Zac-HD mannequin opened this issue Oct 29, 2020 · 27 comments
Labels
3.9 3.10 stdlib type-bug

Comments

@Zac-HD
Copy link
Mannequin

@Zac-HD Zac-HD mannequin commented Oct 29, 2020

BPO 42195
Nosy @gvanrossum, @serhiy-storchaka, @ilevkivskyi, @corona10, @Zac-HD, @miss-islington, @isidentical, @hauntsaninja, @Fidget-Spinner
PRs
  • #23060
  • #23765
  • #23839
  • #23852
  • #23915
  • #23916
  • #24059
  • 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 = <Date 2020-12-14.16:34:44.125>
    created_at = <Date 2020-10-29.12:25:19.859>
    labels = ['type-bug', 'library', '3.9', '3.10']
    title = 'Inconsistent __args__ between typing.Callable and collections.abc.Callable'
    updated_at = <Date 2021-01-02.16:19:18.916>
    user = 'https://github.com/Zac-HD'

    bugs.python.org fields:

    activity = <Date 2021-01-02.16:19:18.916>
    actor = 'miss-islington'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-12-14.16:34:44.125>
    closer = 'gvanrossum'
    components = ['Library (Lib)']
    creation = <Date 2020-10-29.12:25:19.859>
    creator = 'Zac Hatfield-Dodds'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42195
    keywords = ['patch']
    message_count = 27.0
    messages = ['379869', '379913', '379918', '380031', '380040', '380048', '380059', '380175', '380181', '381064', '381065', '381068', '381073', '381402', '381642', '381643', '381644', '381647', '382103', '382108', '382213', '382936', '382991', '382992', '383668', '383669', '384227']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'serhiy.storchaka', 'levkivskyi', 'corona10', 'Zac Hatfield-Dodds', 'miss-islington', 'BTaskaya', 'hauntsaninja', 'kj']
    pr_nums = ['23060', '23765', '23839', '23852', '23915', '23916', '24059']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue42195'
    versions = ['Python 3.9', 'Python 3.10']

    @Zac-HD
    Copy link
    Mannequin Author

    @Zac-HD Zac-HD mannequin commented Oct 29, 2020

    The two ways of getting a parametrised Callable have inconsistent __args__:

        >>> import collections.abc, typing
        >>> typing.Callable[[int, int], int].__args__
        (int, int, int)
        >>> collections.abc.Callable[[int, int], int].__args__
        ([int, int], int)

    I discovered this while working on PEP-585 support in Hypothesis [1], where it is easy enough to work around but carries a potentially serious performance cost - the list means we cannot use the type as a cache key for non-... argument types.

    https://bugs.python.org/issue40494 and https://bugs.python.org/issue40398 may be related.

    [1] HypothesisWorks/hypothesis#2653

    @Zac-HD Zac-HD mannequin added 3.9 3.10 stdlib type-bug labels Oct 29, 2020
    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Oct 30, 2020

    Good find! I see that typing.Callable has adopted this structure precisely to enable caching.

    We should see if we can fix _collections_abc.Callable. It's still early in the life of 3.9 so I think this is reasonable.

    We'll need a subclass of GenericAlias so that the repr() of Callable[[int, int], int] still comes out correctly. This is similar to how typing.Callable solves it.

    Do you feel up to submitting a PR for this? Otherwise maybe Batuhan feels like contributing a fix for this?

    @Zac-HD
    Copy link
    Mannequin Author

    @Zac-HD Zac-HD mannequin commented Oct 30, 2020

    Unfortunately I'm overcommitted for the next few weeks-months :-/

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Oct 31, 2020

    @corona10 Do I hear that you'd like to work on this?

    @corona10
    Copy link
    Member

    @corona10 corona10 commented Oct 31, 2020

    @gvanrossum

    Sorry, Not this time. I just add myself to observe how to solve this issue.

    Maybe Batuhan is the proper member to handle this issue.

    Just question to this issue.
    Since GenericAlias does not have a Py_TPFLAGS_BASETYPE flag, IMHO we have to add Py_TPFLAGS_BASETYPE to type declaration and this behavior does not occur any regression?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Oct 31, 2020

    Would not be better to change typing.Callable?

    Attributes __origin__, __args__ and __parameters__ are not documented in the typing module. They are also not mentioned in any PEP except PEP-585. So I don't know what is intention and correct value of these attributes.

    @Fidget-Spinner
    Copy link
    Member

    @Fidget-Spinner Fidget-Spinner commented Oct 31, 2020

    Hi, I submitted a PR for a proof-of-concept on how the code which subclasses GenericAlias might look like for everyone's consideration.

    Personally, I'd lean more towards what Serhiy said and change typing.Callable rather than GenericAlias which affects many other classes' __class_getitem__. However, considering that typing.Callable has been around for years, I'm hesitant to change its __args__ which might break programs which rely on it.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Nov 2, 2020

    Actually you can't really change typing.Callable's __args__, because it must be hashable, and lists aren't.

    If GenericAlias doesn't cache yet, it might very well do so in the future to gain some speed when e.g. list[int] is used at runtime outside annotations, e.g. in cast(), so it will be important there too.

    @Fidget-Spinner
    Copy link
    Member

    @Fidget-Spinner Fidget-Spinner commented Nov 2, 2020

    Dear Guido, from what I can see in the typing module, _CallableType already casts the args list to a tuple before creating the __CallableGenericAlias, so it should support cacheing. This is taken from the from _CallableType::

    def __getitem__(self, params):
        ...  # (some checking code here)
        args, result = params
        ...  # (some checking code here)
    
            params = (tuple(args), result)  # args is cast to a tuple
        return self.__getitem_inner__(params)

    @tp_cache # the cache
    def __getitem_inner
    _(self, params):
    args, result = params
    ... # (some checking code here)

    \# This is the suspect code causing the flattening of args
    params = args + (result,)     
    return self.copy_with(params)
    
    def copy_with(self, params):
        return _CallableGenericAlias(self.__origin__, params,
                                     name=self._name, inst=self._inst)

    Changing the suspect code from params = args + (result,) to params = (args, result) allows typing.Callable to be consistent with collections.abc.Callable's GenericAlias, and also allows for cacheing.

    With that change:

    >>> from typing import Callable
    >>> Callable[[int, ], str].__args__
    ((<class 'int'>,), <class 'str'>)  # note the args is a tuple
    
    >>> from collections.abc import Callable
    >>> Callable[[int, ], str].__args__
    ([<class 'int'>], <class 'str'>)   # note the args is a list

    This isn't fully consistent with collections.abc.Callable's GenericAlias just yet, but it's close.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Nov 16, 2020

    @Hatfield-Dodds, if we changed typing.Callable to return ((int, int), str) but collections.abc.Callable continued to return ([int, int], str), would that suffice for your purposes?

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Nov 16, 2020

    Also, maybe we should make builtins.callable generic as well?

    @Zac-HD
    Copy link
    Mannequin Author

    @Zac-HD Zac-HD mannequin commented Nov 16, 2020

    @Hatfield-Dodds, if we changed typing.Callable to return ((int, int), str) but collections.abc.Callable continued to return ([int, int], str), would that suffice for your purposes?

    For performance reasons I'd prefer that the return value be hashable for both, but we've already shipped the workarounds [0,1,2] for 3.9.0 and will maintain that until 3.9 reaches EOL in any case.

    Whether we return (int, int, str) or ((int, int), str) doesn't make much difference to me, the latter will require a trivial patch to [2] so please do whatever makes most sense upstream.

    Also, maybe we should make builtins.callable generic as well?

    I like this idea :-)

    [0] https://hypothesis.readthedocs.io/en/latest/changes.html#v5-39-0
    [1] https://github.com/HypothesisWorks/hypothesis/pull/2653/files#diff-c56f048e926cce76dc6cd811924136f5c97e0f68f59625869b4ab01f1dbe10e0L1473-R1480
    [2] https://github.com/HypothesisWorks/hypothesis/pull/2653/files#diff-f6a209c019f3f6af11a027a0035e3fc736935d9920fd85da726f9abf4c325d6bR562-R567

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Nov 16, 2020

    In that case I prefer ((int, int), str), in case we ever end up needing to add additional parameters to Callable. I propose we first fix https://bugs.python.org/issue42102.

    @Fidget-Spinner
    Copy link
    Member

    @Fidget-Spinner Fidget-Spinner commented Nov 19, 2020

    I tried to implement Callable[[int, int], str] as ((int, int), str). However, it breaks much of typing's tests and requires recursion to account for the nested tuples, in both typing, and in the C implementation of GenericAlias.

    I'd like to humbly propose a less breaking solution: express __args__ of Callable[[int, int], str] as (Tuple[int, int], str). Almost all the current code in the typing library already supports this. As for collections.abc.Callable, its __args__ simply needs to be expressed as (tuple[int, int], str). This is also an easy fix.

    Semantically, this makes sense to me too. Both of the above changes will also still allow caching since Tuple[x] is hashable. This will allow us to fix this issue without depending on bpo-42102, or at least it can be a stop gap measure. If bpo-42102 has a resolution, the C implementation can just replace the Python one directly.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Nov 23, 2020

    I'm still not sold on __args__ == (Tuple[int, int], str); it looks too weird.

    However if we introduced a new private type for this purpose that might work? I see that the definition of Tuple in typing.py is

    Tuple = _TupleType(tuple, -1, inst=False, name='Tuple')

    Maybe we could do something like

    _PosArgs = _TupleType(tuple, -1, inst=False, name='_PosArgs')

    ?

    Then __args__ could be (_PosArgs[int, int], str).

    However this still leaves collections.abc.Callable different. (We really don't want to import typing there.)

    Then again, maybe we should still not rule out ((int, int), str)? It feels less hackish than the others.

    And yet another solution would be to stick with (int, int, str) and change collections.abc.Callable to match that. Simple, and more backward compatible for users of the typing module (since no changes at all there).

    @Fidget-Spinner
    Copy link
    Member

    @Fidget-Spinner Fidget-Spinner commented Nov 23, 2020

    @guido,
    Aesthetics-wise, I agree that ((int, int), str) looks by far the best. My gripe with it lies with the implementation - almost every function in typing currently assumes that every object in __args__ is a type, having (int, int) - the tuple object - requires many changes especially to TypeVar substitution and repr.

    I thought that (tuple[int, int], str) looked fine because the positional arguments passed to a function can be represented by a single tuple (and also no need for imports in collections.abc :)! ). However, I support (int, int, str) too for the backwards-compatibility reasons you stated. The only downside to that is that Callable's __args__ will be slightly harder to parse if more args representing other things are added in the future.

    @hauntsaninja
    Copy link
    Contributor

    @hauntsaninja hauntsaninja commented Nov 23, 2020

    I think ((int, int), str) is superior to the others and if it can be made to work, that's what we should do.

    Note that if variadic type proposals go anywhere (https://docs.google.com/document/d/1oXWyAtnv0-pbyJud8H5wkpIk8aajbkX-leJ8JXsE318/edit#), my understanding is we'd probably have to loosen the assumption that __args__ only contains types for that to work too.

    (int, int, str) is temptingly easy, but I think it's a bad idea. For starters, PEP-612 loosens what X can be in Callable[X, str] and life is too short to spend time figuring out whether (P, str) is meant to be Callable[P, str] or Callable[[P], str].

    I'm still trying to figure out how I feel about (Tuple[int, int], str). My initial reaction was negative / I think I'd be more comfortable with a (_Posargs[int, int], str). I don't think Tuple[int, int] would be a workable solution in the event that variadic types pose a similar problem.

    @Fidget-Spinner
    Copy link
    Member

    @Fidget-Spinner Fidget-Spinner commented Nov 23, 2020

    A possible workaround for _PosArgs in collections.abc without depending on typing could be like this:

    _PosArgs = type('_PosArgs', (tuple, ), {})

    This would help out the future type proposals too, because GenericAlias accepts non-type args.

    __args__ of Callable in typing and collections.abc won't be == though. Because all typing types aren't equal to their builtin counterparts. Eg. Tuple[int] == tuple[int] is False.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Nov 30, 2020

    Now that I've seen and reviewed KJ's implementation using _PosArgs, I am worried about it, as it looks quite complicated. @shantanu, do we really need to worry that Callable[P, R] could be ambiguous? If P is a ParamSpec, wouldn't Callable[[P], R] be invalid?

    @hauntsaninja
    Copy link
    Contributor

    @hauntsaninja hauntsaninja commented Nov 30, 2020

    You're right that Callable[[P], int] is invalid, so a flat representation isn't currently ambiguous. But a flat representation would foist the responsibility for checking that to all users of __args__. If your code isn't ParamSpec aware, your failures will probably be less obvious or even silent.

    The flat representation trades off a) implementation complexity with b) usability / aesthetics, c) future proof-ness. I'm not morally opposed to such a tradeoff / am happy to defer to people with more experience in making such tradeoffs.

    @Fidget-Spinner
    Copy link
    Member

    @Fidget-Spinner Fidget-Spinner commented Dec 1, 2020

    FWIW, current code for extracting args type and return type from Callable seems to be something like this (at least from the typing module):

    arg_types = __args__[:-1]
    return_type = __args__[-1]

    Once ParamSpec is added in, library authors would need to check specifically for that in arg_types and return_types. Other than that, I don't have enough expertise or experience to say what will/won't break.

    IMO a flat tuple is ok for now: AFAIK (and I might be completely wrong here), most runtime type checking libraries I've used don't validate the arguments passed in because being too strict may interfere with keyword arguments. (Eg. Callable in Pydantic https://pydantic-docs.helpmanual.io/usage/types/#callable). So the slightly more inconvenient way of reading Callable's __args__ seems to be an acceptable tradeoff for backwards compatibility and simplicity.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 13, 2020

    New changeset 463c7d3 by kj in branch 'master':
    bpo-42195: Ensure consistency of Callable's __args__ in collections.abc and typing (GH-23060)
    463c7d3

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 14, 2020

    New changeset 33b3fed by kj in branch '3.9':
    [3.9] bpo-42195: Ensure consistency of Callable's __args__ in collections.abc and typing (GH-23765)
    33b3fed

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 14, 2020

    Looks like we can close this. Thanks for the report Zac (hopefully you can live with the resolution), and thanks a bundle Ken Jin for all your work on this! I hope it was fun and educational for you. I know it was for me.

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Dec 24, 2020

    New changeset 6dd3da3 by kj in branch 'master':
    bpo-42195: Override _CallableGenericAlias's __getitem__ (GH-23915)
    6dd3da3

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Dec 24, 2020

    New changeset a125198 by Miss Islington (bot) in branch '3.9':
    bpo-42195: Override _CallableGenericAlias's __getitem__ (GH-23915)
    a125198

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Jan 2, 2021

    New changeset 49cd68f by Ken Jin in branch 'master':
    bpo-42195: Disallow isinstance/issubclass for subclasses of genericaliases in Union (GH-24059)
    49cd68f

    @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.9 3.10 stdlib type-bug
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants