python / typeshed

Collection of library stubs for Python, with static types
Other
4.37k stars 1.75k forks source link

SupportsGetItem name confusing #11822

Open anuraaga opened 6 months ago

anuraaga commented 6 months ago

I'm not sure if this is an issue with the type annotation for itemgetter or SupportsGetItem.

SupportsGetItem requires defining __contains__.

https://github.com/python/typeshed/blob/7ed91bc2e77ec18d28307dc6086a778ecc9c7bb3/stdlib/_typeshed/__init__.pyi#L156

This is surprising to me since I'd expect this trait to be about being able to index with [n], which only should need __getitem__. Perhaps there is a reason for it though, but then it means itemgetter can only be used with types that define __contains__.

https://github.com/python/typeshed/blob/7ed91bc2e77ec18d28307dc6086a778ecc9c7bb3/stdlib/_operator.pyi#L122

This is despite itemgetter not using in or __contains__, just indexing, so it should be allowed I think

https://github.com/python/cpython/blob/main/Lib/operator.py#L288

One important effect is that while you'd expect itemgetter to always work instead of a lambda x: x[0], it doesn't for these types.


Deferred until SupportsContainsAndGetItem has made it into type checkers. Most likely after mypy 1.11.0 has been released.

srittau commented 6 months ago

It's a bit weird that SupportsGetItem also requires __contains__ as this conflicts with our usual naming policy. This was probably done to support the old-style iteration protocol for container checks, but is highly confusing and misused in typeshed.

We should:

  1. At first:
    • Remove the "stable" marker from SupportsGetItem.
    • Add a SupportsGetItemAndContains SupportsContainsAndGetItem protocol and a "true", but temporary SupportsGetItem protocol, potentially named something intentionally awkward like TempSupportsGetItem.
    • Review existing stdlib annotations and use the new protocols where suitable.
  2. After TempSupportsGetItem has propagated to type checkers, we should replace annotations in stubs as well.
  3. After about a year, remove SupportsGetItem.
  4. After another grace period of maybe a year, rename TempSupportsGetItem to SupportsGetItem.
srittau commented 6 months ago

11825 shows that there's only one place in the stdlib that actually meant to use the existing SupportsGetItem, while there are multiple places that only require __getitem__.

Akuli commented 6 months ago

I'm getting the impression that most usages of the old SupportsGetItem would be converted to use the new SupportsGetItem (that is, they don't need __contains__). If so, then why don't we just delete __contains__ right away? It would make most stubs more correct.

srittau commented 6 months ago

SupportsGetItem is marked as "stable", so I'm wary changing it without a deprecation process. It would be much easier, of course.

srittau commented 6 months ago

Checking the third-party stubs, I only found one place where SupportsContainsAndGetItem would be used and several where SupportsGetItem would be correct.

Akuli commented 6 months ago

IMO stable shouldn't mean "will not receive bug fixes". I see this as a bug, not a feature. I would see it as a feature if many stubs relied on the current behavior.

But to me, the meaning of stable/bug/feature doesn't really matter, and it's more about "practicality beats purity".

srittau commented 6 months ago

Let's try this after SupportsContainsAndGetItem has made it into type checkers.