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

Do not count __slots__ as a protocol member #11885

Closed
wants to merge 2 commits into from
Closed

Do not count __slots__ as a protocol member #11885

wants to merge 2 commits into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Jan 2, 2022

Refs #9314
Closes #11884

@AlexWaygood
Copy link
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
Member Author

sobolevn commented Jan 2, 2022

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

New issue filed here: #11886

Copy link
Contributor

@A5rocks A5rocks left a comment

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

sobolevn commented Jan 2, 2022

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
Copy link
Contributor

A5rocks commented Jan 2, 2022

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
Copy link
Member Author

sobolevn commented Jan 3, 2022

We are blocked by #11891

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 3, 2022

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

sobolevn commented Jan 3, 2022

@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).

@github-actions

This comment has been minimized.

@github-actions
Copy link
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
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
Member Author

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

@HexDecimal
Copy link
Contributor

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

@sobolevn
Copy link
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