Clone bugfix/st synthetic named expr in match #19
Conversation
…ic-named-expr-in-match
…ic-named-expr-in-match
…ic-named-expr-in-match
…ic-named-expr-in-match
…ic-named-expr-in-match
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis change enhances mypy's match statement type checking by introducing a unique ID generator in the TypeChecker class to create deterministic, collision-free dummy names. A new public method Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances MyPy's type checking capabilities for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
codeant-ai
bot
added
the
size:L
Sequence DiagramThe PR creates synthetic (unique) named expressions for complex match subjects, uses them when inferring pattern capture types, and copies inferred constraints back to the original subject so narrowing works for attribute/item/call subjects. This diagram shows the main success path added by the changes. sequenceDiagram
participant AST as MatchStmt (AST)
participant Checker as TypeChecker
participant Pattern as PatternChecker
AST->>Checker: visit_match_stmt(s)
Checker->>Checker: _make_named_statement_for_match(s) -> create or reuse dummy NameExpr for subject
Checker->>Checker: evaluate subject type (expr_checker.accept)
Checker->>Pattern: infer patterns using named_subject
Pattern-->>Checker: pattern_map keyed by named_subject (and captures)
Checker->>Checker: copy constraints from named_subject -> original s.subject; propagate and push typemaps
Checker->>AST: type-check match arms using pushed typemaps (accept bodies)
Generated by CodeAnt AI |
Refacto PR SummaryEnhanced match statement type inference by improving synthetic named expression handling for complex subjects. The implementation creates unique dummy variables for non-literal match subjects and propagates type constraints between original and synthetic expressions to enable better type narrowing and exhaustivity checking. Key Changes:
Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant MS as MatchStmt
participant TC as TypeChecker
participant NE as NamedExpr
participant PM as PatternMatcher
MS->>TC: visit_match_stmt(complex_subject)
TC->>TC: _make_named_statement_for_match()
alt Complex Expression
TC->>TC: new_unique_dummy_name("match")
TC->>NE: Create synthetic NameExpr
TC->>MS: Store as subject_dummy
else Primitive Literal
TC->>TC: Return original subject
end
TC->>PM: Process patterns with named_subject
PM->>TC: Return pattern_map, else_map
TC->>TC: Propagate constraints to original subject
TC-->>MS: Complete type inference
Testing GuideClick to expand
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![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 a bugfix for type checking match statements with complex subjects, such as function calls or attribute access. The core idea is to create a synthetic named expression for these complex subjects, allowing the existing type narrowing logic to function correctly. The changes are well-implemented, and the new helper methods _make_named_statement_for_match and new_unique_dummy_name improve code organization. The added tests are comprehensive and cover a good range of cases.
I have one suggestion to improve maintainability and performance by moving a constant tuple to the class level.
| expressions_to_preserve = ( | ||
| # Already named - we should infer type of it as given | ||
| NameExpr, | ||
| AssignmentExpr, | ||
| # Primitive literals - their type is known, no need to name them | ||
| IntExpr, | ||
| StrExpr, | ||
| BytesExpr, | ||
| FloatExpr, | ||
| ComplexExpr, | ||
| EllipsisExpr, | ||
| ) |
There was a problem hiding this comment.
For better performance and maintainability, consider defining expressions_to_preserve as a class-level constant on TypeChecker, since it's a constant value. This avoids recreating the tuple on every call to _make_named_statement_for_match.
For example:
class TypeChecker(...):
_MATCH_SUBJECT_EXPRESSIONS_TO_PRESERVE: Final = (
NameExpr,
AssignmentExpr,
# ...
)
# ...
def _make_named_statement_for_match(self, s: MatchStmt) -> Expression:
subject = s.subject
if isinstance(subject, self._MATCH_SUBJECT_EXPRESSIONS_TO_PRESERVE):
return subject
# ...
Nitpicks 🔍
|
|
CodeAnt AI finished reviewing your PR. |
Code Review: Match Statement Type Inference EnhancementPR Confidence Score: 🟨 4 / 5👍 Well Done
📁 Selected files for review (2)
🎯 Custom Instructions
📝 Additional Comments
🧰 Additional context used
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| CallExpr, | ||
| ClassDef, | ||
| ComparisonExpr, | ||
| ComplexExpr, |
There was a problem hiding this comment.
Missing ComplexExpr Import
Code adds ComplexExpr to imports and uses it in expressions_to_preserve tuple, but the import may not be properly resolved. This could cause NameError at runtime when type checker processes complex number literals in match statements.
ComplexExpr,
Commitable Suggestion
| ComplexExpr, | |
| ComplexExpr, |
Standards
- Type-Safety-Import-Resolution
- API-Contract-Consistency
Context References
mypy/nodes.py-ComplexExpr class definition confirms it exists and should be importable
|
|
||
| def new_unique_dummy_name(self, namespace: str) -> str: | ||
| """Generate a name that is guaranteed to be unique for this TypeChecker instance.""" | ||
| name = f"dummy-{namespace}-{self._unique_id}" |
There was a problem hiding this comment.
Magic String Pattern
Hardcoded string format creates coupling between dummy name generation and string parsing logic. Changes to naming convention require coordinated updates across multiple locations.
Standards
- Clean-Code-Functions
- SOLID-OCP
- Maintainability-Quality-Coupling
Context References
filepath- ``definition- ``
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test-data/unit/check-python310.test (1)
2661-2671: Question: IsLiteral[0]?the expected type here?The test expects
reveal_type(i)to showLiteral[0]?. The?suffix typically indicates an optional/nullable type, which seems unusual for matching against a literal0. Can you confirm this is the intended behavior for this edge case?If this is indeed the correct expected output, consider adding a comment explaining why the type is
Literal[0]?rather than justLiteral[0]to help future maintainers understand this pathological case.mypy/checker.py (2)
5420-5458: Match subject aliasing via syntheticNameExprlooks sound; consider copying source location onto the dummyThe new
_make_named_statement_for_matchhelper plusnamed_subjectusage invisit_match_stmtnicely centralize the “synthetic subject name” logic:
Simple/“already named” or literal subjects (
NameExpr,AssignmentExpr, primitive literal exprs includingComplexExprandEllipsisExpr) are preserved, avoiding unnecessary indirection.Complex subjects now get a cached
NameExprstored inMatchStmt.subject_dummy, ensuring we reuse the sameExpressionobject across pattern passes and when buildingpattern_map/else_map.The extra remapping
if pattern_map and named_subject in pattern_map: pattern_map[s.subject] = pattern_map[named_subject] if else_map and named_subject in else_map: else_map[s.subject] = else_map[named_subject]bridges from the synthetic
NameExprback to the original subject expression so downstream logic (includingpropagate_up_typemap_infoand later binder state) still sees constraints keyed by the real subject as well. This avoids losing information on the original expression while giving you a stable key for the synthetic one.One small improvement you might want (mainly for debugging and error reporting) is to copy source location from the original subject onto the synthetic
NameExprand its backingVar, so any diagnostics that happen to mention this dummy node don’t show line-1:Optional tweak to preserve subject location on the dummy name
def _make_named_statement_for_match(self, s: MatchStmt) -> Expression: @@ - else: - # Create a dummy subject expression to handle cases where a match statement's subject - # is not a literal value. This lets us correctly narrow types and check exhaustivity - # This is hack! - name = self.new_unique_dummy_name("match") - v = Var(name) - named_subject = NameExpr(name) - named_subject.node = v - s.subject_dummy = named_subject - return named_subject + else: + # Create a dummy subject expression to handle cases where a match statement's subject + # is not a literal value. This lets us correctly narrow types and check exhaustivity. + name = self.new_unique_dummy_name("match") + v = Var(name) + v.line = subject.line + named_subject = NameExpr(name) + named_subject.set_line(subject) + named_subject.node = v + s.subject_dummy = named_subject + return named_subjectThis is purely optional; the existing behavior is functionally correct.
Also applies to: 5506-5534
7915-7919:new_unique_dummy_namehelper is fine; could be reused if more synthetic names are neededThe
new_unique_dummy_name(self, namespace: str)helper is a clean, low‑overhead way to generate reproducible dummy identifiers (dummy-<ns>-<n>) perTypeCheckerinstance. It’s appropriately narrow in scope and decoupled from symbol tables, which is fine for non‑declared internal names like the synthetic match subjects.If you later find yourself needing symbol‑table‑aware uniqueness (e.g. for fake
TypeInfos), you could consider layering this overgen_unique_nameand a relevantSymbolTable, but that’s not necessary for the current match‑subject use case.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mypy/checker.pytest-data/unit/check-python310.test
🧰 Additional context used
🧬 Code graph analysis (1)
mypy/checker.py (1)
mypy/nodes.py (23)
ComplexExpr(1783-1797)MatchStmt(1640-1667)Expression(201-207)NameExpr(1875-1895)AssignmentExpr(2074-2087)IntExpr(1696-1710)StrExpr(1719-1733)BytesExpr(1740-1763)FloatExpr(1766-1780)EllipsisExpr(1800-1806)name(234-235)name(353-354)name(532-533)name(571-577)name(809-810)name(907-908)name(1068-1069)name(2287-2288)name(2572-2573)name(3205-3207)name(3752-3753)name(3864-3865)Var(973-1118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Type check our own code (py39-ubuntu)
- GitHub Check: Test suite with py39-ubuntu, mypyc-compiled
- GitHub Check: Formatting and code style with Black + ruff
- GitHub Check: Test suite with py312-ubuntu, mypyc-compiled
- GitHub Check: Test suite with py310-ubuntu
- GitHub Check: Test suite with py313-ubuntu, mypyc-compiled
- GitHub Check: Type check our own code (py39-windows-64)
- GitHub Check: Test suite with py39-windows-64
- GitHub Check: Test suite with py311-ubuntu, mypyc-compiled
- GitHub Check: Run mypy_primer (2)
- GitHub Check: Test mypyc suite with 32-bit Python
- GitHub Check: Run mypy_primer (1)
- GitHub Check: Run mypy_primer (4)
- GitHub Check: Run mypy_primer (0)
- GitHub Check: Run mypy_primer (3)
🔇 Additional comments (7)
test-data/unit/check-python310.test (5)
1305-1305: Good: Previously failing test now passes.The removal of the
-xfailsuffix confirms this test case now passes after the bugfix for synthetic named expressions in match statements. This validates that match statements on async function calls returning unions are now handled correctly.
2589-2601: Well-designed test for function call match subjects.This test case properly validates that matching on a function call result correctly narrows types in each branch and detects unreachable code when all union alternatives are exhausted.
2602-2615: LGTM: Attribute access match subject test.This test case validates that matching on attribute access expressions correctly generates synthetic named expressions and performs proper type narrowing.
2616-2644: Comprehensive test for operation match subjects.This test case thoroughly validates matching on various operation types (unary, binary, comparison) and confirms that synthetic named expressions work correctly for these complex subjects.
2645-2659: LGTM: Dictionary item match subject test.This test case validates that matching on dictionary subscript expressions correctly handles synthetic named expressions and type narrowing.
mypy/checker.py (2)
70-70: ImportingComplexExpris consistent with new match-subject handlingBringing
ComplexExprinto this module so it can be treated as a primitive literal in_make_named_statement_for_matchaligns with how other literal expr nodes are handled. No change needed.
354-356: Per‑instance_unique_idcounter is correctly scoped and initializedUsing an instance field
_unique_idplus initializing it to 0 in__init__gives deterministic, collision‑free dummy names within aTypeCheckerand works fine with the incrementalreset()flow (which reuses existingsubject_dummynodes instead of regenerating them).Also applies to: 421-421
|
Diff from mypy_primer, showing the effect of this PR on open source code: core (https://github.com/home-assistant/core)
- homeassistant/components/asuswrt/bridge.py:114: error: Incompatible return value type (got "dict[str, object]", expected "dict[str, float | str | None] | dict[str, float]") [return-value]
discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/commands/hybrid.py:510: error: Overlap between argument names and ** TypedDict items: "name", "description" [misc]
+ discord/ext/commands/hybrid.py:510: error: Overlap between argument names and ** TypedDict items: "description", "name" [misc]
- discord/ext/commands/hybrid.py:631: error: Overlap between argument names and ** TypedDict items: "name", "description" [misc]
+ discord/ext/commands/hybrid.py:631: error: Overlap between argument names and ** TypedDict items: "description", "name" [misc]
|
CodeAnt-AI Description
Fix match subject type inference for complex expressions
What Changed
Impact
✅ Clearer pattern capture types in match statements✅ Fewer false "unreachable" match branches✅ More accurate reveal_type results in match tests💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.