Narrow is with final types correctly
#6
Conversation
|
Diff from mypy_primer, showing the effect of this PR on open source code: koda-validate (https://github.com/keithasaurus/koda-validate)
+ koda_validate/typehints.py:170: error: Statement is unreachable [unreachable]
+ koda_validate/serialization/json_schema.py:85: error: Statement is unreachable [unreachable]
+ koda_validate/serialization/json_schema.py:377: error: Statement is unreachable [unreachable]
|
|
Problems looks real, checking. |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Diff from mypy_primer, showing the effect of this PR on open source code: trio (https://github.com/python-trio/trio)
+ src/trio/_core/_local.py:54: error: Unused "type: ignore" comment [unused-ignore]
+ src/trio/_core/_local.py:57: error: Unused "type: ignore" comment [unused-ignore]
steam.py (https://github.com/Gobot1234/steam.py)
+ steam/ext/commands/converters.py:532: error: Unused "type: ignore" comment [unused-ignore]
|
|
""" WalkthroughThis update introduces enhancements to type narrowing logic, particularly for final classes and type objects, within the type checking system. Internal logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant TypeChecker
participant covers_at_runtime
participant CallableType
participant TypeType
TypeChecker->>covers_at_runtime: covers_at_runtime(item, supertype)
covers_at_runtime->>covers_at_runtime: Check if item is AnyType or TypeType[AnyType]
covers_at_runtime-->>TypeChecker: Return False if so
covers_at_runtime->>CallableType: item.is_singleton_type()
covers_at_runtime->>TypeType: item.is_singleton_type()
covers_at_runtime-->>TypeChecker: Return True if singleton and subtype
covers_at_runtime->>covers_at_runtime: Continue with existing logic if not early exit
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code Graph Analysis (1)mypy/subtypes.py (2)
⏰ Context from checks skipped due to timeout of 90000ms (13)
🔇 Additional comments (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Diff from mypy_primer, showing the effect of this PR on open source code: pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/raises.py:1490: error: Unused "type: ignore" comment [unused-ignore]
steam.py (https://github.com/Gobot1234/steam.py)
+ steam/ext/commands/converters.py:532: error: Unused "type: ignore" comment [unused-ignore]
trio (https://github.com/python-trio/trio)
+ src/trio/_core/_local.py:54: error: Unused "type: ignore" comment [unused-ignore]
+ src/trio/_core/_local.py:57: error: Unused "type: ignore" comment [unused-ignore]
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to type narrowing for final types, which enhances the precision of type inference. The addition of new test cases is commendable, as it demonstrates a commitment to verifying the correctness of the changes. However, a thorough review of the performance implications and comprehensive coverage of edge cases is essential to ensure the robustness and efficiency of the new features.
Summary of Findings
- Type coverage checks: The changes introduce improved type narrowing for final classes and type identity checks, but it's important to ensure that all possible type combinations are thoroughly covered in the test cases. Consider adding more complex scenarios to verify the robustness of the new features.
- Runtime type checks: The code includes logic for runtime type checks, but it's crucial to ensure that these checks are efficient and don't introduce performance bottlenecks. Review the performance implications of the added checks, especially in frequently executed code paths.
Merge Readiness
The pull request is approaching readiness for merging, but there are a few issues that need to be addressed before it can be considered fully ready. Specifically, the high severity issue regarding the potential for runtime errors due to incomplete type coverage must be resolved. Additionally, the medium severity issue concerning the efficiency of runtime type checks should be investigated and optimized if necessary. Once these issues have been addressed and verified, the pull request can be confidently merged. I am unable to approve this pull request, and users should have others review and approve this code before merging.
| # Left `Any` or `Type[Any]` type will never be covered at runtime: | ||
| if isinstance(item, AnyType) or ( | ||
| isinstance(item, TypeType) and isinstance(item.item, AnyType) | ||
| ): | ||
| return False |
There was a problem hiding this comment.
This block prevents Any or Type[Any] from being covered at runtime. Is this the desired behavior? Should there be a comment explaining why this is necessary?
| # Left `Any` or `Type[Any]` type will never be covered at runtime: | |
| if isinstance(item, AnyType) or ( | |
| isinstance(item, TypeType) and isinstance(item.item, AnyType) | |
| ): | |
| return False | |
| # Left `Any` or `Type[Any]` type will never be covered at runtime: | |
| # This is because runtime type checks will not be able to infer | |
| # the underlying type of `Any` or `Type[Any]`. | |
| if isinstance(item, AnyType) or ( | |
| isinstance(item, TypeType) and isinstance(item.item, AnyType) | |
| ): |
| if isinstance(item, (CallableType, TypeType)) and item.is_singleton_type(): | ||
| if is_proper_subtype(item, supertype): | ||
| return True |
There was a problem hiding this comment.
This condition checks if the item is a singleton type and a proper subtype of the supertype. Can you add a comment explaining what a singleton type is in this context, and why this check is needed?
| if isinstance(item, (CallableType, TypeType)) and item.is_singleton_type(): | |
| if is_proper_subtype(item, supertype): | |
| return True | |
| # Check if the item is a singleton type (e.g., `None`, final classes) | |
| # and a proper subtype of the supertype. | |
| if isinstance(item, (CallableType, TypeType)) and item.is_singleton_type(): | |
| if is_proper_subtype(item, supertype): |
| def is_singleton_type(self) -> bool: | ||
| return (isinstance(self.item, Instance) and self.item.type.is_final) or isinstance( | ||
| self.item, NoneType | ||
| ) |
There was a problem hiding this comment.
This function determines if a TypeType is a singleton type. Why is NoneType considered a singleton type here?
| def is_singleton_type(self) -> bool: | |
| return (isinstance(self.item, Instance) and self.item.type.is_final) or isinstance( | |
| self.item, NoneType | |
| ) | |
| def is_singleton_type(self) -> bool: | |
| # A TypeType is a singleton type if its item is a final instance or None. | |
| return (isinstance(self.item, Instance) and self.item.type.is_final) or isinstance( | |
| self.item, NoneType | |
| ) |
| if x is Mark: | ||
| reveal_type(x) # N: Revealed type is "def () -> __main__.Mark" | ||
| else: | ||
| reveal_type(x) # N: Revealed type is "Any" |
There was a problem hiding this comment.
The revealed type here is def () -> __main__.Mark. Is this the intended behavior when x is Any and compared to a final class? Should it be Mark instead?
if x is Mark:
reveal_type(x) # N: Revealed type is "__main__.Mark"
else:
reveal_type(x) # N: Revealed type is "Any"
Now we can narrow
@finaltypes withisandis not.Closes python#15553
Summary by CodeRabbit
New Features
Bug Fixes
Tests