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

Fix usage of byte2int with bytes #9152

Merged
merged 5 commits into from
Nov 12, 2022
Merged

Conversation

Avasam
Copy link
Sponsor Collaborator

@Avasam Avasam commented Nov 10, 2022

Fixes #9145 . Restores functionality specifically with bytes.
Works around a mypy bug that thinks SupportsGetItem[int, <nothing>] is expected when passed bytes. May be python/mypy#14032

indexbytes seems unnafected

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

Could you add a test case for this? Also worth reporting as a separate mypy bug I think.

@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Nov 10, 2022

I was going to but I can't replicate it anymore O_o

from typing import Any, Protocol, TypeVar, Union

_T = TypeVar("_T")
_KT_contra = TypeVar("_KT_contra", contravariant=True)
_VT_co = TypeVar("_VT_co", covariant=True)


class SupportsGetItem(Protocol[_KT_contra, _VT_co]):
    def __contains__(self, __x: Any) -> bool:
        ...

    def __getitem__(self, __key: _KT_contra) -> _VT_co:
        ...


###


def foo(obj: SupportsGetItem[int, _T]) -> _T:
    return obj[0]


bar: int = foo(b"1")

###

import six

six.byte2int(b"1")

###


def byte2int(c: Union[bytes, int]) -> int:
    if isinstance(c, bytes):
        return six.byte2int(c)
    return c


bar = byte2int(b"1")

mypy 0.990 (compiled: yes)
Python 3.9.13

@JelleZijlstra
Copy link
Member

Interesting, different mypy version?

@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Nov 10, 2022

Actually it differs whether I point to my local typeshed repo or not... Even if I revert the change to byte2int.

The other culprit could be SupportsGetItem. In my example above, I redefined it, and it's the same in my local typeshed repo (same as develop). But the typeshed stubs shipped with mypy still has def __contains__(self, __x: object) -> bool: ....

The change from object to any was actually important as part of #9117

Edit: Changing __x: Any back to __x: object in SupportsGetItem indeed reproduces the issue

Then would this PR make sense as a stop-gap until type-checkers update their shipped typeshed stubs?

@github-actions

This comment has been minimized.

@Avasam
Copy link
Sponsor Collaborator Author

Avasam commented Nov 10, 2022

For the record, having to use Any instead of object or _VT_co sounds an awful lot like the comment in Mapping (which references discussion of python/typing#273 )

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

A better solution might be to just copy the new version of SupportsGetItem into the stubs for six, and then use that directly instead of importing it from _typeshed (with a TODO comment saying we should switch to the _typeshed version once mypy updates its vendored copy of typeshed and makes a new release). Then we wouldn't have to make the function an overloaded function.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks @Avasam!

@AlexWaygood AlexWaygood merged commit 14a43b8 into python:main Nov 12, 2022
@Avasam Avasam deleted the six-buffer-index branch November 12, 2022 15:59
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.

Issue with annotation six.byte2int starting 1.16.21.2
3 participants