Skip to content

Do not count __slots__ as a protocol member - #11885

Closed
sobolevn wants to merge 2 commits into
python:masterfrom
sobolevn:issue-11884
Closed

Do not count __slots__ as a protocol member#11885
sobolevn wants to merge 2 commits into
python:masterfrom
sobolevn:issue-11884

Conversation

@sobolevn

Copy link
Copy Markdown
Member

Refs #9314
Closes#11884

@AlexWaygood

Copy link
Copy Markdown
Member

It's not so clear-cut, but would it be worth special-casing __class_getitem__ in the same way? It might avoid regressions like this (cc. @hauntsaninja)

@sobolevn

Copy link
Copy Markdown
MemberAuthor

It's not so clear-cut, but would it be worth special-casing class_getitem in the same way?

Can you please open a new issue for it? Probably we need to discuss / research all special cases separatelly. But, it looks like a good candidate 👍

@AlexWaygood

Copy link
Copy Markdown
Member

It's not so clear-cut, but would it be worth special-casing class_getitem in the same way?

Can you please open a new issue for it? Probably we need to discuss / research all special cases separatelly. But, it looks like a good candidate 👍

Sure!

@AlexWaygood

Copy link
Copy Markdown
Member

New issue filed here: #11886

@A5rocksA5rocks left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, though #11886 might be easily combined with this if __class_getitem__ is to be ignored (though keeping seperate PRs is admirable and perfectly fine too).

@sobolevn

Copy link
Copy Markdown
MemberAuthor

Now I am also thinking: what if I want to have a function / interface / contract / whatever that only works with slotted types? How can I express this?

For example:

defpretty_print_slots(klass_with_slots: ???) ->None:
...

I guess what Eric said in #11886 really makes sense. So, proably we can ignore protocols __slots__ = () definition as a "performance hack", we need to treat __slots__: str | Iterable[str] as a part of the protocol.

Do you agree?

@A5rocks

A5rocks commented Jan 2, 2022

Copy link
Copy Markdown
Collaborator

That sounds like a decent solution too, tried it out a bit and found some weird behavior in current mypy somewhat related to this, just bringing this up because it's a bit funny:

This fails:

importtyping@typing.runtime_checkableclassFoo(typing.Protocol):
__slots__= ()
classBar(Foo):
...
print(issubclass(Bar, Foo)) # this fails

and this works:

importtyping@typing.runtime_checkableclassFoo(typing.Protocol):
__slots__: str|typing.Iterable[str] = ()
classBar(Foo):
...
print(issubclass(Bar, Foo)) # this does not ?

(Anyways, that'll get fixed by this too, it's just some weird behavior...)

Basically yes, I agree that would make sense to me -- though I'm sure any solution would be nice.

@sobolevn

Copy link
Copy Markdown
MemberAuthor

We are blocked by #11891

@AlexWaygood

AlexWaygood commented Jan 3, 2022

Copy link
Copy Markdown
Member

We are blocked by #11891

I wonder if it's even helpful for typeshed to define __slots__ in the stub for object (as it does currently), or if it should be removed. Given that __slots__ are so special, it doesn't seem like it does much for the type-checker.

@sobolevn

Copy link
Copy Markdown
MemberAuthor

@AlexWaygood well, technically, object does not have __slots__.

>>> object.__slots__
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: type object 'object' has no attribute '__slots__'. Did you mean: '__class__'?
>>> object().__slots__
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'object' object has no attribute '__slots__'. Did you mean: '__class__'?

My vote is to remove them (my opinion is pretty simple: typeshed should be as close to source as possible).

Comment threadtest-data/unit/check-protocols.test Outdated
@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/commands/core.py:2333: error: Argument 1 to "is_owner" of "BotBase" has incompatible type "Union[discord.user.User, Member]"; expected "discord.abc.User"

@HexDecimal

Copy link
Copy Markdown
Contributor

What's the status of this?

Instead of hard-coding __slots__, shouldn't this check a collection of what to exclude so that what's excluded can be easily updated? From the discussion on #11886, apparently Python uses this collection to decide what to exclude from Protocol. Perhaps follow exactly what Python excludes rather than debating which ones should be special-cased in Mypy? I assume if this route is taken then Mypy would need a collection for each Python version with differing exclusions.

Maybe I should try making a separate PR with my suggestions?

@sobolevn
sobolevn marked this pull request as ready for review June 21, 2023 15:39
@sobolevn

Copy link
Copy Markdown
MemberAuthor

@HexDecimal I've deleted my fork by accident. You can re-submit this if you need this feature :)

@HexDecimal

Copy link
Copy Markdown
Contributor

I'll go ahead and attempt my own PR. I mostly want to fetch your test to work with.

@sobolevn

Copy link
Copy Markdown
MemberAuthor

Great, ping me to review your PR :)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

__slots__ and protocols don't work right for issubclass and typevar bounds

5 participants

@sobolevn@AlexWaygood@A5rocks@HexDecimal@hauntsaninja