Skip to content

Conversation

@tungol
Copy link
Contributor

@tungol tungol commented Oct 5, 2024

This is an updated version of #11187

In addition to shuffling defintions between files, these things got tweaked:

  • Removed several redundant definitions of __enter__. These seem to be an artifact of using TypeVar instead of Self originally
  • seek and truncate have inconsistent variable names. They're also positional only, so they were in the stubtest allowlist before, but I went ahead and made them explicit in the stubs
  • BytesIO.readlines shouldn't have been on the allowlist in the first place. It differs not only in variable name, but also default value.
  • A big block of functions in TextIOWrapper were commented that mypy needed them, but I don't think it does anymore, unless there's a problem with subclassing TextIOWrapper that doesn't show up in typeshed itself. No indication in the history about that.
  • In the implementation, the concrete classes inherit from the private implementation _*IOBase classes, not the classes in io which are actually metaclasses, but they are registered to those metaclasses at runtime. It wasn't technically required for any reason, but I split the difference on this by keeping the _*IOBase classes in their base classes directly. I think it's a bit of a reminder of the actual implementation, and means that a stubtest check for inheritance will show that typeshed is adding to the base classes, rather than replacing the base class, and I think that's a little cleaner.

Partially related to #3968

This is an updated version of python#11187

In addition to shuffling defintions between files, these
things got tweaked:

 - Removed several redundant definitions of __enter__. These
   seem to be an artifact of using TypeVar instead of Self originally
 - seek and truncate have inconsistent variable names. They're also
   positional only, so they were in the stubtest allowlist before,
   but I went ahead and made them explicit in the stubs
 - BytesIO.readlines shouldn't have been on the allowlist in
   the first place. It differs not only in variable name, but
   also default value.
 - A big block of functions in TextIOWrapper were commented that
   mypy needed them, but I don't think it does anymore, unless
   there's a problem with subclassing TextIOWrapper that doesn't
   show up in typeshed itself. No indication in the history about that.
 - In the implementation, the concrete classes inherit from the
   private implementation _*IOBase classes, not the classes in io
   which are actually metaclasses, but they are registered to those
   metaclasses at runtime. It wasn't technically required for any reason,
   but I split the difference on this by keeping the _*IOBase classes
   in their base classes directly. I think it's a bit of a reminder
   of the actual implementation, and means that a stubtest check for
   inheritance will show that typeshed is adding to the base classes,
   rather than replacing the base class, and I think that's a little cleaner.

Partially related to python#3968
@tungol tungol marked this pull request as draft October 5, 2024 20:39
@github-actions

This comment has been minimized.

@tungol tungol marked this pull request as ready for review October 5, 2024 21:08
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2024

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

scrapy (https://github.com/scrapy/scrapy)
+ scrapy/core/downloader/handlers/ftp.py:61: error: Incompatible types in assignment (expression has type "BufferedIOBase", variable has type "BinaryIO")  [assignment]

operator (https://github.com/canonical/operator)
- ops/pebble.py:1878: error: Argument 1 of "write" is incompatible with supertype "BufferedIOBase"; supertype defines the argument type as "Buffer"  [override]
+ ops/pebble.py:1878: error: Argument 1 of "write" is incompatible with supertype "_BufferedIOBase"; supertype defines the argument type as "Buffer"  [override]
- ops/pebble.py:1902: error: Return type "str | bytes" of "read" incompatible with return type "bytes" in supertype "BufferedIOBase"  [override]
+ ops/pebble.py:1902: error: Return type "str | bytes" of "read" incompatible with return type "bytes" in supertype "_BufferedIOBase"  [override]
- ops/pebble.py:1902: error: Argument 1 of "read" is incompatible with supertype "BufferedIOBase"; supertype defines the argument type as "int | None"  [override]
+ ops/pebble.py:1902: error: Argument 1 of "read" is incompatible with supertype "_BufferedIOBase"; supertype defines the argument type as "int | None"  [override]
- ops/pebble.py:1935: error: Return type "str | bytes" of "read1" incompatible with return type "bytes" in supertype "BufferedIOBase"  [override]
+ ops/pebble.py:1935: error: Return type "str | bytes" of "read1" incompatible with return type "bytes" in supertype "_BufferedIOBase"  [override]

paasta (https://github.com/yelp/paasta)
- paasta_tools/cli/cmds/local_run.py:980: error: Argument 1 to "write" of "TextIOBase" has incompatible type "Union[str, bytes]"; expected "str"  [arg-type]
+ paasta_tools/cli/cmds/local_run.py:980: error: Argument 1 to "write" of "_TextIOBase" has incompatible type "Union[str, bytes]"; expected "str"  [arg-type]

@tungol
Copy link
Contributor Author

tungol commented Oct 5, 2024

I'm struggling to understand the mypy_primer result from scrapy.

The line in question is self.body: BinaryIO = open(filename, "wb") if filename else BytesIO()

I believe that should come out to BufferedWriter or BytesIO, whose definitions in this MR change from:

class BufferedWriter(BufferedIOBase, BinaryIO):
class BytesIO(BufferedIOBase, BinaryIO):

to

class BufferedWriter(BufferedIOBase, _BufferedIOBase, BinaryIO):
class BytesIO(BufferedIOBase, _BufferedIOBase, BinaryIO):

Am I missing something? The other mypy_primer lines are just swapping one name for another in an existing error, but this one is weird to me and I don't like it very much. Something to do with how the bases are re-arranged?

@tungol
Copy link
Contributor Author

tungol commented Oct 5, 2024

Seems like that error doesn't happen if the order is

class BytesIO(_BufferedIOBase, BinaryIO, BufferedIOBase):
class BufferedWriter(_BufferedIOBase, BinaryIO, BufferedIOBase):

I can do that, but it creates the same problem if you want to assign it to BufferedIOBase instead of BinaryIO. I tried some other variations to no better result.

@JelleZijlstra
Copy link
Member

I guess this is a quirk of mypy's join operator in the presence of multiple inheritance. Here's a simplified example (https://mypy-play.net/?mypy=latest&python=3.12&gist=7db4e657e8a94a117c8a71b6d974a478):

class A: pass
class B: pass
class C: pass

class Cls1(A, B, C): pass
class Cls2(A, B, C): pass

def func(cond: bool, c1: Cls1, c2: Cls2) -> None:
    a: A = c1 if cond else c2
    b: B = c1 if cond else c2
    c: C = c1 if cond else c2
    print(a, b, c)

Mypy errors on the assignments to B and C, but not A. This feels like a manifestation of python/mypy#4373.

@JelleZijlstra
Copy link
Member

Given that it's only one mypy-primer hit and it's pretty clearly buggy behavior in mypy, I think it's fine to ignore this case.

@srittau srittau added the topic: io I/O related issues label Oct 8, 2024
@srittau
Copy link
Collaborator

srittau commented Oct 8, 2024

Thanks! All these changes sound reasonable, but I'd prefer the move and the other improvements to be in two separate PRs: This makes is easier to review the actual changes and it will also lead to a cleaner commit history in typeshed, which could be important in changes to these foundational classes.

@tungol
Copy link
Contributor Author

tungol commented Oct 8, 2024

That makes sense, I'll split it up.

@tungol tungol closed this Oct 8, 2024
tungol added a commit to tungol/typeshed that referenced this pull request Oct 8, 2024
This version keeps it simple and clean: No changes to class bodies.
The only changes here are moving between files and updating the
naming and inheritance.

Related to python#3968 and split from python#12740.
JelleZijlstra pushed a commit that referenced this pull request Oct 9, 2024

This version keeps it simple and clean: No changes to class bodies.
The only changes here are moving between files and updating the
naming and inheritance.

Related to #3968 and split from #12740.
@tungol tungol deleted the io-2 branch October 23, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: io I/O related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants