Support type aliases, NamedTuple and TypedDict in constrained TypeVar defaults
#2
Conversation
…8862-typevar-default-alias
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Thank you, I tried your changes locally and I believe it also fixes python#17686. |
|
Missed that one, thanks! |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
1 similar comment
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
WalkthroughThe changes update the logic for checking if a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MypyChecker
participant TypeVar
participant is_same_type
User->>MypyChecker: Define TypeVar with constraints and default
MypyChecker->>TypeVar: Check if default is among constraints
TypeVar->>is_same_type: Compare default with each constraint
is_same_type-->>TypeVar: Return comparison result
TypeVar-->>MypyChecker: Valid/invalid default determination
MypyChecker-->>User: Accept or report error
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🧰 Additional context used🧬 Code Graph Analysis (1)mypy/checker.py (1)
🔇 Additional comments (9)
✨ 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. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/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
The pull request addresses an issue where TypeVar defaults were not being correctly checked against their constraints, especially when type aliases, NamedTuple, or TypedDict were involved. The changes include modifications to the checker and checkexpr modules to use is_same_type for comparing types, and the addition of new test cases to cover various scenarios. Overall, the changes seem well-structured and address the reported issues effectively.
Summary of Findings
- Type comparison: The change from
==tois_same_typeensures that type aliases and special types are correctly compared when checking TypeVar defaults against constraints. This is crucial for the correctness of type checking. - Test coverage: The addition of new test cases covering type aliases, NamedTuple, and TypedDict significantly improves the test coverage for TypeVar defaults and constraints.
Merge Readiness
The pull request appears to be well-structured and addresses the reported issues effectively. The addition of new test cases significantly improves the test coverage. I recommend merging this pull request after addressing the review comments. I am unable to directly approve the pull request, and users should have others review and approve this code before merging.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request addresses issues related to type aliases, NamedTuple, and TypedDict in constrained TypeVar defaults. The changes involve modifications to the checker and checkexpr modules to improve type checking accuracy, along with the addition of new test cases to ensure the correct handling of these features. Overall, the changes seem well-structured and contribute to the stability and correctness of the type checker.
Summary of Findings
- Severity Levels: The review settings requested that review comments be provided for issues at the
medium,high, andcriticallevels of severity. No issues at these levels were found.
Merge Readiness
The pull request appears to be in good shape for merging. The code changes are focused and address the specified issues. The addition of comprehensive test cases further increases confidence in the correctness of the implementation. Since no issues at the critical, high, or medium levels of severity were found, the pull request seems ready to be merged, though as always, another reviewer should look over the changes before merging. I am unable to directly approve the pull request, and that users should have others review and approve this code before merging.
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: TypeVar Defaults Type Comparison👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| if not is_subtype(tv.default, tv.upper_bound): | ||
| self.fail("TypeVar default must be a subtype of the bound type", tv) | ||
| if tv.values and not any(tv.default == value for value in tv.values): | ||
| if tv.values and not any(is_same_type(tv.default, value) for value in tv.values): |
There was a problem hiding this comment.
Type Comparison Issue
Using equality (==) instead of is_same_type() for type comparison can lead to incorrect constraint validation. This may cause TypeVar defaults that are semantically equivalent but not identical to be incorrectly rejected, reducing type system reliability.
if tv.values and not any(is_same_type(tv.default, value) for value in tv.values):
Commitable Suggestion
| if tv.values and not any(is_same_type(tv.default, value) for value in tv.values): | |
| if tv.values and not any(is_same_type(tv.default, value) for value in tv.values): |
Standards
- ISO-IEC-25010-Functional-Correctness
- ISO-IEC-25010-Reliability-Maturity
| if not is_subtype(p_default, e.upper_bound): | ||
| self.chk.fail("TypeVar default must be a subtype of the bound type", e) | ||
| if e.values and not any(p_default == value for value in e.values): | ||
| if e.values and not any(is_same_type(p_default, value) for value in e.values): |
There was a problem hiding this comment.
Inconsistent Type Comparison
Using equality (==) instead of is_same_type() for type comparison in TypeVar expression validation. This creates inconsistent behavior between checker.py and checkexpr.py, potentially causing different validation results for the same types.
if e.values and not any(is_same_type(p_default, value) for value in e.values):
Commitable Suggestion
| if e.values and not any(is_same_type(p_default, value) for value in e.values): | |
| if e.values and not any(is_same_type(p_default, value) for value in e.values): |
Standards
- ISO-IEC-25010-Functional-Correctness
- ISO-IEC-25010-Reliability-Maturity
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: TypeVar Default Type Comparison👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| if not is_subtype(tv.default, tv.upper_bound): | ||
| self.fail("TypeVar default must be a subtype of the bound type", tv) | ||
| if tv.values and not any(tv.default == value for value in tv.values): | ||
| if tv.values and not any(is_same_type(tv.default, value) for value in tv.values): |
There was a problem hiding this comment.
Type Comparison Logic
The change from equality check to is_same_type() affects type comparison behavior. While this fixes the immediate issue, it may have unintended consequences for other type comparisons that relied on exact equality. This could lead to incorrect type validation in other scenarios.
# Use is_same_type for structural equivalence of types including aliases
if tv.values and not any(is_same_type(tv.default, value) for value in tv.values):
Commitable Suggestion
| if tv.values and not any(is_same_type(tv.default, value) for value in tv.values): | |
| # Use is_same_type for structural equivalence of types including aliases | |
| if tv.values and not any(is_same_type(tv.default, value) for value in tv.values): |
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- ISO-IEC-25010-Reliability-Maturity
| if not is_subtype(p_default, e.upper_bound): | ||
| self.chk.fail("TypeVar default must be a subtype of the bound type", e) | ||
| if e.values and not any(p_default == value for value in e.values): | ||
| if e.values and not any(is_same_type(p_default, value) for value in e.values): |
There was a problem hiding this comment.
Inconsistent Type Comparison
The same type comparison logic change was made in checkexpr.py, creating a parallel modification. If these functions are used in different contexts, inconsistent behavior could occur if one is updated without the other in future changes, leading to reliability issues.
# Use is_same_type for structural equivalence of types including aliases
if e.values and not any(is_same_type(p_default, value) for value in e.values):
Commitable Suggestion
| if e.values and not any(is_same_type(p_default, value) for value in e.values): | |
| # Use is_same_type for structural equivalence of types including aliases | |
| if e.values and not any(is_same_type(p_default, value) for value in e.values): |
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- ISO-IEC-25010-Reliability-Maturity
| if not is_subtype(tv.default, tv.upper_bound): | ||
| self.fail("TypeVar default must be a subtype of the bound type", tv) | ||
| if tv.values and not any(tv.default == value for value in tv.values): | ||
| if tv.values and not any(is_same_type(tv.default, value) for value in tv.values): |
There was a problem hiding this comment.
Duplicate Equality Check
The equality check was changed from '==' to 'is_same_type()' function. This same change was also made in checkexpr.py, suggesting a common operation that could be extracted to avoid duplication and ensure consistent behavior across the codebase.
Standards
- Clean-Code-DRY
- Maintainability-Quality-Consistency
- Refactoring-Extract-Method
3 participants
Fixes python#18862. Fixes python#17686.
Summary by CodeRabbit
Bug Fixes
Tests