GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — builtins: Audit bytes arguments by JelleZijlstra · Pull Request #7631 · python/typeshed

Conversation

JelleZijlstra commented Apr 16, 2022

Copy link

Copy Markdown

Member

As a followup from #7589 (comment),
I audited all occurrences of bytes in builtins.pyi by reading the corresponding C code
on CPython main.

Most use the C buffer protocol, so _typeshed.ReadableBuffer is the right type. A few
check specifically for bytes and bytearray.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          builtins: Audit bytes arguments

8df4803

As a followup from python#7589 (comment),
I audited all occurrences of bytes in builtins.pyi by reading the corresponding C code
on CPython main.

Most use the C buffer protocol, so _typeshed.ReadableBuffer is the right type. A few
check specifically for bytes and bytearray.

JelleZijlstra commented

Apr 16, 2022

View reviewed changes

Comment thread

stdlib/builtins.pyi

		def __new__(cls: type[Self], __x: str \| bytes \| SupportsInt \| SupportsIndex \| SupportsTrunc = ...) -> Self: ...
		def __new__(cls: type[Self], __x: str \| ReadableBuffer \| SupportsInt \| SupportsIndex \| SupportsTrunc = ...) -> Self: ...
		@overload
		def __new__(cls: type[Self], __x: str \| bytes \| bytearray, base: SupportsIndex) -> Self: ...

JelleZijlstra Apr 16, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

>>> int(memoryview(b"0xdeadbeef"), 16)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: int() can't convert non-string with explicit base
>>> int(memoryview(b"123"))
123

Showing that the first overload accepts buffers but the second doesn't.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

hauntsaninja Apr 16, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

https://github.com/bradfitz/deadbeef

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra and Akuli reacted with laugh emoji

All reactions

😄 2 reactions

Comment thread

stdlib/builtins.pyi

		def from_bytes(
		cls: type[Self],
		bytes: Iterable[SupportsIndex] \| SupportsBytes, # TODO buffer object argument
		bytes: Iterable[SupportsIndex] \| SupportsBytes \| ReadableBuffer,

JelleZijlstra Apr 16, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

>>> int.from_bytes([1, 2, 3])
66051
>>> int.from_bytes(memoryview(b"123"))
3224115

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

stdlib/builtins.pyi

		self, __sub: ReadableBuffer \| SupportsIndex, __start: SupportsIndex \| None = ..., __end: SupportsIndex \| None = ...
		) -> int: ...
		if sys.version_info >= (3, 8):
		def hex(self, sep: str \| bytes = ..., bytes_per_sep: SupportsIndex = ...) -> str: ...

JelleZijlstra Apr 16, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

>>> b"xy".hex(memoryview(b"x"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: sep must be str or bytes.
>>> b"xy".hex(bytearray(b"x"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: sep must be str or bytes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

stdlib/builtins.pyi Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

stdlib/builtins.pyi

		@overload
		def __getitem__(self, __s: slice) -> bytes: ...
		def __add__(self, __s: bytes) -> bytes: ...
		def __add__(self, __s: ReadableBuffer) -> bytes: ...

JelleZijlstra Apr 16, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

>>> b"x" + memoryview(b"y")
b'xy'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

stdlib/builtins.pyi

		def __setitem__(self, __s: slice, __x: Iterable[SupportsIndex] \| bytes) -> None: ...
		def __delitem__(self, __i: SupportsIndex \| slice) -> None: ...
		def __add__(self, __s: bytes) -> bytearray: ...
		def __iadd__(self: Self, __s: Iterable[int]) -> Self: ...

JelleZijlstra Apr 16, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This was wrong; ba += [1, 2, 3] fails

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

stdlib/builtins.pyi

		opener: _Opener \| None = ...,
		) -> IO[Any]: ...
		def ord(__c: str \| bytes) -> int: ...
		def ord(__c: str \| bytes \| bytearray) -> int: ...

JelleZijlstra Apr 16, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

>>> ord(memoryview(b"x"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ord() expected string of length 1, but memoryview found

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Update stdlib/builtins.pyi

446691f

This comment has been minimized.


          type ignore

a7b6ab8

This comment has been minimized.

1 similar comment

github-actions Bot commented Apr 16, 2022

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

srittau approved these changes

Apr 16, 2022

View reviewed changes

srittau left a comment

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks, I didn't double check, but the changes look reasonable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

stdlib/builtins.pyi

		def join(self, __iterable_of_bytes: Iterable[ByteString \| memoryview]) -> bytes: ...
		def ljust(self, __width: SupportsIndex, __fillchar: bytes = ...) -> bytes: ...
		def join(self, __iterable_of_bytes: Iterable[ReadableBuffer]) -> bytes: ...
		def ljust(self, __width: SupportsIndex, __fillchar: bytes \| bytearray = ...) -> bytes: ...

srittau Apr 16, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unfortunately, this will also accept memoryview at the moment, but having it more explicit can't hurt.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra Apr 16, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

That's a mypy bug :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

srittau Apr 16, 2022

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's working as documented. In the past when reviewing I've always asked people to remove bytearray from argument types due to that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

srittau merged commit ee09d9e into python:master

Apr 16, 2022

JelleZijlstra deleted the bytes branch

April 16, 2022 13:57

Reviewers

srittau

srittau approved these changes

hauntsaninja

hauntsaninja left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.