8000 Representation of ParamSpecs at runtime compared to Callable · Issue #102615 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

Representation of ParamSpecs at runtime compared to Callable #102615

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.

Sign up for GitHub

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

Jump to bottom
Closed
Gobot1234 opened this issue Mar 12, 2023 · 14 comments
Closed

Representation of ParamSpecs at runtime compared to Callable #102615

Gobot1234 opened this issue Mar 12, 2023 · 14 comments
Labels
stdlib Python modules in the Lib dir topic-typing

Comments

@Gobot1234
Copy link
Contributor
Gobot1234 commented Mar 12, 2023 •
edited by bedevere-bot
Loading 
          I think a list representation is more consistent with the text of PEP 612 and the runtime representation of Callable. We should change the runtime repr of ParamSpec generics to be more like Callable, because right now they're inconsistent:

>>> Callable[[int, str], bool]
typing.Callable[[int, str], bool]
>>> P = ParamSpec("P")
>>> T = TypeVar("T")
>>> class MyCallable(Generic[P, T]): pass
... 
>>> MyCallable[[int, str], bool]
__main__.MyCallable[(<class 'int'>, <class 'str'>), bool]
>>> get_args(Callable[[int, str], bool])
([<class 'int'>, <class 'str'>], <class 'bool'>)
>>> get_args(MyCallable[[int, str], bool])
((<class 'int'>, <class 'str'>), <class 'bool'>)

Originally posted by @JelleZijlstra in python/typing#1274 (comment)

Linked PRs
@Fidget-Spinner
Copy link
Member

Just clarifying for anyone who plans to contribute: underneath both __args__ are the same (a flat non-nested tuple of t ypes) get_args just shows the string representation of Callable to wrap [] around the args.

@sobolevn
Copy link
Member

underneath both args are the same (a flat non-nested tuple of t ypes)

No, they are different:

>>> MyCallable[[int, str], bool].__args__
((<class 'int'>, <class 'str'>), <class 'bool'>)
>>> Callable[[int, str], bool].__args__
(<class 'int'>, <class 'str'>, <class 'bool'>)

I've sent a PR with the repr change.

sobolevn added a commit to sobolevn/cpython that referenced this issue Mar 13, 2023
@sobolevn
pythongh-102615: Use list instead of tuple in repr of paramspec
8e4620c
@bedevere-bot bedevere-bot mentioned this issue Mar 13, 2023
Merged
@Fidget-Spinner
Copy link
Member

underneath both args are the same (a flat non-nested tuple of t ypes)

No, they are different:

>>> MyCallable[[int, str], bool].__args__
((<class 'int'>, <class 'str'>), <class 'bool'>)
>>> Callable[[int, str], bool].__args__
(<class 'int'>, <class 'str'>, <class 'bool'>)

I've sent a PR with the repr change.

Oh woops, I'm wrong then :). That's a bug. The tuples should be flattened and they should be the same. I remember trying to flatten all the args representations when I implemented PEP 612.

>>> Callable[P, T][[int, str], bool].__args__
(<class 'int'>, <class 'str'>, <class 'bool'>)
>>> collections.abc.Callable[P, T][[int, str], bool].__args__
(<class 'int'>, <class 'str'>, <class 'bool'>)

There's a very important reason why they need to be flattened -- generic expects a list of types in __args__, it does not handle tuples. So double substitution will break if there are tuples.

E.g this works.

>>> Callable[P, T][[P, str], bool][int]
typing.Callable[[int, str], bool]

But ugh these two are broken

# Broken in collections.abc :(, needs fixing
>>> collections.abc.Callable[P, T][[P, str], bool][int]
TypeError: Callable must be used as Callable[[arg, ...], result].
# Broken in typing
>>> MyCallable[P, T][[P, str], bool][int]
TypeError: __main__.MyCallable[(~P, <class 'str'>), bool] is not a generic clas

@sobolevn
Copy link
Member

That's a bug

Yeah, I was not sure if this is intentional or a bug. Got it now!
Will change my PR to address it soon :)

@sobolevn sobolevn removed the easy label Mar 13, 2023
@sobolevn
Copy link
Member

After reading quite a lot of tests, I think that

>>> MyCallable[[int, str], bool].__args__
((<class 'int'>, <class 'str'>), <class 'bool'>)

is correct. For example, take a look at this test:

def test_multiple_paramspecs_in_user_generics(self):
        P = ParamSpec("P")
        P2 = ParamSpec("P2")

        class X(Generic[P, P2]):
            f: Callable[P, int]
            g: Callable[P2, str]

        G1 = X[[int, str], [bytes]]
        G2 = X[[int], [str, bytes]]
        self.assertNotEqual(G1, G2)
        self.assertEqual(G1.__args__, ((int, str), (bytes,)))
        self.assertEqual(G2.__args__, ((int,), (str, bytes)))

If we are going to flatten G1 = X[[int, str], [bytes]] to have (int, str, bytes) - it won't be clear what we mean here.

So, I think it is intentional: args are not flattened for ParamSpec substitution.

@Fidget-Spinner, however, your examples are broken indeed.

@Fidget-Spinner
Copy link
Member

Thanks for looking into this. I vaguely recall that for user defined generics with ParamSpec, we don't flatten because of the following:

Generic[P1, int, P2]

The subsitution of the above, if flattened, would be impossible to reconstruct with get_args.

I need to dig through the history. I think I left a comment somewhere about this.

@Fidget-Spinner
Copy link
Member

I think the only thing that needs fixing here is the repr and collections.abc.Callable then. Thanks again Nikita!

sobolevn added a commit to sobolevn/cpython that referenced this issue Mar 14, 2023
@sobolevn
pythongh-102615: Fix type vars substitution of `collections.abc.Calla…
2731844 
…ble` and custom generics with `ParamSpec`
@sobolevn sobolevn mentioned this issue Mar 14, 2023
Closed
@AlexWaygood
Copy link
Member
AlexWaygood commented Mar 14, 2023 •
edited
Loading

But ugh these two are broken

>>> # Broken in collections.abc :(, needs fixing
>>> collections.abc.Callable[P, T][[P, str], bool][int]
TypeError: Callable must be used as Callable[[arg, ...], result].
>>> # Broken in typing
>>> MyCallable[P, T][[P, str], bool][int]
TypeError: __main__.MyCallable[(~P, <class 'str'>), bool] is not a generic clas

Are these two examples valid uses of ParamSpec? I thought you had to use Concatenate if you wanted to prepend parameters to a ParamSpec variable in a parameters list. And I thought appending parameters to a ParamSpec variable in a parameters list was disallowed altogether by PEP-612.

So surely the runtime is correct in disallowing both of these: they both involve a ParamSpec being substituted with an invalid parameters list?

@AlexWaygood AlexWaygood added the stdlib Python modules in the Lib dir label Mar 14, 2023
@Fidget-Spinner
Copy link
Member

@AlexWaygood thanks. Sorry I recalled wrongly the semantics of ParamSpec. I now see that PEP 612 forbids these uses. So does this mean the actual fix is to

a. disallow it in typing (because currently only collections.abc and typing.Generic correctly fails) OR
b. do nothing?

which do you prefer?

@sobolevn I'm sorry for leading you on a wild goose chase. Thank you for your efforts in all of this. Sorry for wasting your time. In the end, the right fix might truly be just to fix the repr. I think I'll wait for a week or so to see the consensus on this thread.

TLDR: I'm wrong and I think Alex is right. Nikita has spent a great deal of effort on understanding this issue. So I'll defer judgement to both Nikita and Alex, I think I'm too rusty with typing right now to make the right call.

@AlexWaygood
Copy link
Member
AlexWaygood commented Mar 14, 2023 •
edited
Loading

@Fidget-Spinner I don't think you've got anything to apologise for — the spec is horrendously complicated for PEP-612! I only spotted the error on, like, the 5th read :)

My top priority here would be improving the error message so that it's clearer for users (including typing maintainers, including me!) exactly why the runtime is raising a TypeError here. It's too easy to forget the finer points of PEP-612.

In terms of harmonising the behaviour between collections.abc.Callable and typing.Callable — if we can disallow the invalid uses in typing as well, then I'm open to it. But if it would add significant complexity, I'd probably be inclined to just leave it.

@sobolevn
Copy link
Member
sobolevn commented Mar 14, 2023 •
edited
Loading

@Fidget-Spinner no worries, I've spent my time on a very interesting topic :)

I now agree that there are several related, but distinct problems:

  1. repr of ParamSpec in user generics

  2. typing.Callable behaves differently and allows incorrect substitution that collections.abc.Callable does not allow:

     >>> import typing
 >>> P = typing.ParamSpec('P')
 >>> T = typing.TypeVar('T')
 >>> typing.Callable[P, T][[P, str], bool][int]
 typing.Callable[[int, str], bool]

    While:

     >>> import collections.abc
 >>> collections.abc.Callable[P, T][[P, str], bool][int]
 Traceback (most recent call last):
   File "<stdin>", line 1, in <module>
   File "/Users/sobolev/Desktop/cpython/Lib/_collections_abc.py", line 501, in __getitem__
     return _CallableGenericAlias(Callable, tuple(new_args))
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   File "/Users/sobolev/Desktop/cpython/Lib/_collections_abc.py", line 456, in __new__
     raise TypeError(
 TypeError: Callable must be used as Callable[[arg, ...], result].

  3. There's a suspicious if case that is probably not tested / works incorrectly: gh-102615: Fix type vars substitution of collections.abc.Callable and custom generics with ParamSpec #102681 (comment)

  4. There are not enough tests for all the corner cases we have discussed here

So, my plan is:

  1. Fix the repr
  2. We can fix the typing.Callable to behave the same way (why?), or just ignore it, because it is quite complex to get right
  3. Run tests with coverage to find out what is going on and fix it / add tests / remove these lines
  4. Add new tests

Please, bare with me and my multiple PRs 😆

Thanks everyone for the productive discussion!

@sobolevn
Copy link
Member

Oh, and better errors (if we can) as 5.

AlexWaygood added a commit that referenced this issue Mar 15, 2023
@sobolevn @AlexWaygood
gh-102615: Use list instead of tuple in repr of paramspec (#102637
2b5781d 
)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@sobolevn sobolevn mentioned this issue Mar 15, 2023
Closed
@AlexWaygood
Copy link
Member

The original issue here has been fixed, and we won't be backporting the fix (see #102637 (review) for the rationale). As such, I'm going to close this issue and open followup issues for the other problems we identified in this thread. @sobolevn has already opened the first followup issue:

This was referenced Mar 15, 2023
Closed
Closed
@AlexWaygood
Copy link
Member

Two more followup issues:

Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this issue Mar 27, 2023
@sobolevn @AlexWaygood @Fidget-Spinner
pythongh-102615: Use list instead of tuple in repr of paramspec (…
4de1e65 
…python#102637)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
warsaw pushed a commit to warsaw/cpython that referenced this issue Apr 11, 2023
@sobolevn @AlexWaygood @warsaw
pythongh-102615: Use list instead of tuple in repr of paramspec (…
e5a5a95 
…python#102637)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
stdlib Python modules in the Lib dir topic-typing
Projects
None yet
Milestone
No milestone
4 participants
@sobolevn @Fidget-Spinner @Gobot1234 @AlexWaygood
0