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

@sobolevn sobolevn commented Jan 2, 2022

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

sobolevn commented Jan 2, 2022

Copy link
Copy Markdown
Member Author

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

@A5rocks A5rocks 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

sobolevn commented Jan 2, 2022

Copy link
Copy Markdown
Member Author

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:

def pretty_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:

import typing

@typing.runtime_checkable
class Foo(typing.Protocol):
    __slots__ = ()


class Bar(Foo):
    ...

print(issubclass(Bar, Foo))  # this fails

and this works:

import typing

@typing.runtime_checkable
class Foo(typing.Protocol):
    __slots__: str | typing.Iterable[str]  = ()


class Bar(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

sobolevn commented Jan 3, 2022

Copy link
Copy Markdown
Member Author

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

sobolevn commented Jan 3, 2022

Copy link
Copy Markdown
Member Author

@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 thread test-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 sobolevn closed this Jun 21, 2023
@sobolevn

Copy link
Copy Markdown
Member Author

@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
Member Author

Great, ping me to review your PR :)

Sign up for free to 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