fix(io): update Reader.read to respect Liskov Substitution Principle#23
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefines the java.io.Reader.read API to a specific CharBuffer-based signature in both the runtime shim and stub, aligning it with the Java Reader contract and Liskov Substitution Principle. Class diagram for updated java_io_Reader_read_signatureclassDiagram
class Object
class Readable
class Closeable
class CharBuffer
class Reader{
+read(cb: CharBuffer) int
+mark(int) void
+markSupported() bool
+nullReader() Reader
+ready() bool
+reset() void
+skip(long) long
}
Reader --|> Object
Reader ..|> Readable
Reader ..|> Closeable
Reader --> CharBuffer : uses_in_read
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Narrowing
Reader.readfrom*args: Anyto a singleCharBufferparameter makes all other valid JavaReader.read(...)overloads impossible to express; consider using overloads or a more general signature (e.g., multiple@overloaddefinitions) instead of a singleCharBuffer-only variant. - If this change is specifically targeting the
Readable.read(CharBuffer)method, it may be clearer to model that as a separateReadable-interface method or a dedicated overload, rather than repurposing the primaryReader.readto only acceptCharBuffer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Narrowing `Reader.read` from `*args: Any` to a single `CharBuffer` parameter makes all other valid Java `Reader.read(...)` overloads impossible to express; consider using overloads or a more general signature (e.g., multiple `@overload` definitions) instead of a single `CharBuffer`-only variant.
- If this change is specifically targeting the `Readable.read(CharBuffer)` method, it may be clearer to model that as a separate `Readable`-interface method or a dedicated overload, rather than repurposing the primary `Reader.read` to only accept `CharBuffer`.
## Individual Comments
### Comment 1
<location path="stubs/stubs/java/io/__init__.pyi" line_range="116" />
<code_context>
@staticmethod
def nullReader() -> Reader: ...
- def read(self, *args: Any) -> int: ...
+ def read(self, cb: CharBuffer) -> int: ...
def ready(self) -> bool: ...
def reset(self) -> None: ...
</code_context>
<issue_to_address>
**suggestion:** The stub now models only a single `read(CharBuffer)` shape; consider overloads if other `read` variants are intended.
The old `*args: Any` signature was imprecise but did indicate multiple `read` variants. If this stub is meant to cover the full Java API (e.g., `read()`, `read(char[], int, int)`), representing each as an explicit `@overload` would be more accurate than a single `read(self, cb: CharBuffer) -> int` signature.
Suggested implementation:
```
@staticmethod
def nullReader() -> Reader: ...
@overload
def read(self) -> int: ...
@overload
def read(self, cb: CharBuffer) -> int: ...
@overload
def read(self, cbuf: list[str]) -> int: ...
@overload
def read(self, cbuf: list[str], off: int, length: int) -> int: ...
def ready(self) -> bool: ...
```
`.
Here are the edits:
<file_operations>
<file_operation operation="edit" file_path="stubs/stubs/java/io/__init__.pyi">
<<<<<<< SEARCH
@staticmethod
def nullReader() -> Reader: ...
def read(self, cb: CharBuffer) -> int: ...
def ready(self) -> bool: ...
=======
@staticmethod
def nullReader() -> Reader: ...
@overload
def read(self) -> int: ...
@overload
def read(self, cb: CharBuffer) -> int: ...
@overload
def read(self, cbuf: list[str]) -> int: ...
@overload
def read(self, cbuf: list[str], off: int, length: int) -> int: ...
def ready(self) -> bool: ...
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
1. Ensure `from typing import overload` is present at the top of `stubs/stubs/java/io/__init__.pyi`. If the file already imports from `typing`, extend that import to include `overload`.
2. If your codebase uses a different type to model Java `char[]` (e.g., `Sequence[str]` or a custom `CharArray`), adjust the `cbuf: list[str]` annotations accordingly to match your existing conventions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @staticmethod | ||
| def nullReader() -> Reader: ... | ||
| def read(self, *args: Any) -> int: ... | ||
| def read(self, cb: CharBuffer) -> int: ... |
There was a problem hiding this comment.
suggestion: The stub now models only a single read(CharBuffer) shape; consider overloads if other read variants are intended.
The old *args: Any signature was imprecise but did indicate multiple read variants. If this stub is meant to cover the full Java API (e.g., read(), read(char[], int, int)), representing each as an explicit @overload would be more accurate than a single read(self, cb: CharBuffer) -> int signature.
Suggested implementation:
@staticmethod
def nullReader() -> Reader: ...
@overload
def read(self) -> int: ...
@overload
def read(self, cb: CharBuffer) -> int: ...
@overload
def read(self, cbuf: list[str]) -> int: ...
@overload
def read(self, cbuf: list[str], off: int, length: int) -> int: ...
def ready(self) -> bool: ...
`.
Here are the edits:
<file_operations>
<file_operation operation="edit" file_path="stubs/stubs/java/io/init.pyi">
<<<<<<< SEARCH
@staticmethod
def nullReader() -> Reader: ...
def read(self, cb: CharBuffer) -> int: ...
def ready(self) -> bool: ...
@staticmethod
def nullReader() -> Reader: ...
@overload
def read(self) -> int: ...
@overload
def read(self, cb: CharBuffer) -> int: ...
@overload
def read(self, cbuf: list[str]) -> int: ...
@overload
def read(self, cbuf: list[str], off: int, length: int) -> int: ...
def ready(self) -> bool: ...
REPLACE
</file_operation>
</file_operations>
<additional_changes>
- Ensure
from typing import overloadis present at the top ofstubs/stubs/java/io/__init__.pyi. If the file already imports fromtyping, extend that import to includeoverload. - If your codebase uses a different type to model Java
char[](e.g.,Sequence[str]or a customCharArray), adjust thecbuf: list[str]annotations accordingly to match your existing conventions.
flagged by ty
Summary by Sourcery
Align Reader.read signature with the Java Reader.read(CharBuffer) contract to better respect type relationships.
Bug Fixes:
Enhancements: