Skip to content

Support type aliases, NamedTuple and TypedDict in constrained TypeVar defaults #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

arvi18
Copy link

@arvi18 arvi18 commented Apr 23, 2025

Fixes python#18862. Fixes python#17686.

Summary by CodeRabbit

  • Bug Fixes

    • Improved accuracy when checking if a TypeVar default matches its constraints, particularly when using type aliases or special types.
  • Tests

    • Added comprehensive test cases to verify correct handling of TypeVar defaults with constraints, type aliases, and special types.
    • Enhanced test coverage for type inference and error detection involving TypeVar defaults and constraints.

@arvi18
Copy link
Author

arvi18 commented Apr 23, 2025

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@arvi18
Copy link
Author

arvi18 commented Apr 23, 2025

Thank you, I tried your changes locally and I believe it also fixes python#17686.

@arvi18
Copy link
Author

arvi18 commented Apr 23, 2025

Missed that one, thanks!

@arvi18
Copy link
Author

arvi18 commented Apr 23, 2025

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

1 similar comment
@arvi18
Copy link
Author

arvi18 commented Apr 23, 2025

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Copy link

coderabbitai bot commented Apr 23, 2025

Walkthrough

The changes update the logic for checking if a TypeVar's default type matches one of its constraint types in the mypy type checker. Instead of using direct equality, the code now uses a more precise type comparison function (is_same_type) in both the main checker and expression checker. Several new test cases are added to verify correct behavior when type aliases, special types, and generics are used as constraints or defaults for TypeVar. These tests ensure that type inference and error detection work as intended with these scenarios.

Changes

File(s) Change Summary
mypy/checker.py
mypy/checkexpr.py
Replaced direct == comparison with is_same_type for checking if a TypeVar's default matches any constraint type, improving type equivalence checking.
test-data/unit/check-python312.test Added test case testPEP695TypeVarConstraintsDefaultAliases to verify TypeVar defaults and constraints using type aliases and generics.
test-data/unit/check-python313.test Added test case testTypeVarConstraintsDefaultAliasesInline to test inline type alias usage in TypeVar constraints and defaults.
test-data/unit/check-typevar-defaults.test Added multiple test cases covering TypeVar defaults with type aliases, TypeAliasType, TypedDict, NamedTuple, and generics; cleaned up two blank lines.

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
Loading

Poem

In the warren of types, a new check hops in,
No longer just equals, but truly akin.
Aliases and lists, even tuples parade,
As defaults for TypeVars, their roles now well-played.
With tests as our carrots, correctness we seek—
A hop, a compare, and no more mystique!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 380cb8d and 57799f1.

📒 Files selected for processing (5)
  • mypy/checker.py (1 hunks)
  • mypy/checkexpr.py (1 hunks)
  • test-data/unit/check-python312.test (1 hunks)
  • test-data/unit/check-python313.test (1 hunks)
  • test-data/unit/check-typevar-defaults.test (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
mypy/checker.py (1)
mypy/subtypes.py (1)
  • is_same_type (254-282)
🔇 Additional comments (9)
mypy/checker.py (1)

7661-7661: Good improvement to type equality checking

Replacing the direct equality comparison (==) with is_same_type() is a great enhancement for handling type aliases, NamedTuple, and TypedDict in constrained TypeVar defaults. The is_same_type() function performs a more precise semantic type equivalence check compared to plain equality, which ensures that types with different representations but the same semantic meaning (like type aliases pointing to the same type) are correctly recognized as equivalent.

This change will help prevent false positives when TypeVars have defaults that are semantically equivalent to their constraints but represented differently in the type system.

mypy/checkexpr.py (1)

6213-6213: Good improvement to type comparison logic.

Changing from direct equality (==) to is_same_type() provides more accurate structural type comparison and enables proper support for type aliases, NamedTuple, and TypedDict when used as defaults in constrained TypeVars. This will handle cases where types are structurally equivalent but represented differently.

test-data/unit/check-python313.test (1)

278-291: Good addition of tests for type alias handling with inline TypeVar constraints

This test case effectively validates that type aliases work correctly as both constraints and defaults in TypeVar definitions using the inline PEP 695 syntax. The test verifies the correct resolution to int in all cases, which is important for ensuring proper type inference.

test-data/unit/check-python312.test (1)

2046-2068: Well-structured test for TypeVar constraints with type aliases

This test case properly covers the TypeVar constructor approach for using type aliases in both constraints and defaults. Good combination of tests with different alias types (K, V, L) demonstrating the correct type resolution.

test-data/unit/check-typevar-defaults.test (5)

751-772: Great test for TypeAliasType with constrained TypeVar defaults

This test case correctly validates that TypeAliasType from typing_extensions works properly when used as constraints and defaults in TypeVar definitions. The revealed types demonstrate correct inference.


774-783: Concise test for implicit type aliases

Good coverage of the implicit type alias case, ensuring that variables assigned to types work as TypeVar constraints and defaults.


785-794: Comprehensive test for explicit TypeAlias annotation

This test properly covers TypeAlias-annotated variables used in TypeVar constraints and defaults, completing coverage of different alias declaration styles.


796-816: Important test for TypedDict and NamedTuple special types

Excellent addition that tests special types (TypedDict and NamedTuple) in TypeVar constraints and defaults, ensuring they're correctly recognized and handled by the type system.


818-850: Thorough test for generic special types

This test case effectively covers the more complex scenario of generic TypedDict and NamedTuple types in TypeVar constraints and defaults, including valid and invalid combinations. The error cases (T4, T5) correctly verify that TypeVar defaults must match constraint types exactly.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@visz11
Copy link
Collaborator

visz11 commented Apr 23, 2025

@coderabbitai full review

Copy link

coderabbitai bot commented Apr 23, 2025

✅ Actions performed

Full review triggered.

@visz11
Copy link
Collaborator

visz11 commented Apr 23, 2025

@coderabbitai full review

Copy link

coderabbitai bot commented Apr 23, 2025

✅ Actions performed

Full review triggered.

@visz11
Copy link
Collaborator

visz11 commented Apr 24, 2025

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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 == to is_same_type ensures 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.

@visz11
Copy link
Collaborator

visz11 commented Apr 24, 2025

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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, and critical levels 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants