Introduce temporary named expressions for match subjects
#4
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
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Whoa, I expected this to produce at least a couple of unused ignore diagnostics, but apparently pattern matching isn't very popular in wild... |
|
Diff from mypy_primer, showing the effect of this PR on open source code: schemathesis (https://github.com/schemathesis/schemathesis)
- File "/tmp/mypy_primer/old_mypy/venv/bin/mypy", line 8, in <module>
+ File "/tmp/mypy_primer/new_mypy/venv/bin/mypy", line 8, in <module>
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/__main__.py", line 15, in console_entry
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/__main__.py", line 15, in console_entry
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 119, in main
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 119, in main
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 203, in run_build
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 203, in run_build
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 191, in build
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 191, in build
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 267, in _build
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 267, in _build
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 2947, in dispatch
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 2947, in dispatch
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 976, in write_deps_cache
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 976, in write_deps_cache
|
|
Meh, the mypyc failure is unrelated and didn't happen during first run, likely a glitch. |
|
Diff from mypy_primer, showing the effect of this PR on open source code: schemathesis (https://github.com/schemathesis/schemathesis)
- File "/tmp/mypy_primer/old_mypy/venv/bin/mypy", line 8, in <module>
+ File "/tmp/mypy_primer/new_mypy/venv/bin/mypy", line 8, in <module>
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/__main__.py", line 15, in console_entry
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/__main__.py", line 15, in console_entry
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 119, in main
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 119, in main
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 203, in run_build
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/main.py", line 203, in run_build
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 191, in build
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 191, in build
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 267, in _build
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 267, in _build
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 2947, in dispatch
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 2947, in dispatch
- File "/tmp/mypy_primer/old_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 976, in write_deps_cache
+ File "/tmp/mypy_primer/new_mypy/venv/lib/python3.12/site-packages/mypy/build.py", line 976, in write_deps_cache
|
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Warning Rate limit exceeded@visz11 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes introduce a new helper method in the Changes
Sequence Diagram(s)sequenceDiagram
participant UserCode as User Code
participant TypeChecker
participant AST as AST Nodes
UserCode->>TypeChecker: Defines a match statement with a complex subject
TypeChecker->>AST: Inspects match subject expression
TypeChecker->>TypeChecker: Calls _make_named_statement_for_match
alt Subject is named/literal/ComplexExpr
TypeChecker->>TypeChecker: Use subject as-is
else
TypeChecker->>TypeChecker: Generate unique dummy name
TypeChecker->>AST: Create NameExpr for dummy
end
TypeChecker->>TypeChecker: Use named subject for type narrowing
TypeChecker->>UserCode: Performs type checking and narrowing in match cases
Poem
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 (
|
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
This interferes with recently added narrowing by item/attribute (aka discriminated union, |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
To support narrowing the subject and its "dependencies" simultaneously, I propose to just push the narrowed type of subject to typemap as the type of its original form, allowing for any inference on it. |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mypy/checker.py(7 hunks)test-data/unit/check-python310.test(2 hunks)
🧰 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 (16)
- GitHub Check: Type check our own code (py39-ubuntu)
- GitHub Check: Type check our own code (py39-windows-64)
- GitHub Check: Formatting and code style with Black + ruff
- GitHub Check: mypyc runtime tests with py39-macos
- GitHub Check: Test suite with py313-ubuntu, mypyc-compiled
- GitHub Check: Test suite with py311-ubuntu, mypyc-compiled
- GitHub Check: Test suite with py310-ubuntu
- GitHub Check: Test suite with py312-ubuntu, mypyc-compiled
- GitHub Check: Test suite with py39-windows-64
- GitHub Check: Test suite with py39-ubuntu, mypyc-compiled
- GitHub Check: Test mypyc suite with 32-bit Python
- GitHub Check: Run mypy_primer (3)
- GitHub Check: Run mypy_primer (4)
- GitHub Check: Run mypy_primer (1)
- GitHub Check: Run mypy_primer (2)
- GitHub Check: Run mypy_primer (0)
🔇 Additional comments (7)
mypy/checker.py (1)
70-70: Import looks good.Adding
ComplexExprto the import list is necessary for the new helper and is correctly placed alongside the other node classes.test-data/unit/check-python310.test (6)
1305-1319: Test case now expected to pass rather than failThis change removes the
-xfailmarker from thetestMatchCapturePatternFromAsyncFunctionReturningUniontest case, indicating that it's now expected to pass. This is consistent with the PR's goal of improving match subject handling for complex expressions like the result of an async function call.
2589-2601: Good test coverage for matching on function call resultsThis new test case verifies that pattern matching on function call expressions works correctly, with proper type narrowing and unreachable path detection. The test confirms that the type checker can accurately track the types for each match case and detect when a case is unreachable.
2602-2615: Good test coverage for matching on attribute accessThis test case validates pattern matching on attribute access expressions. The test ensures proper type narrowing when matching on a class attribute that has a union type, and correctly identifies unreachable code paths.
2616-2643: Comprehensive tests for matching on unary and binary operationsThese test cases verify pattern matching functionality for unary operations (like negation) and binary operations (addition and comparison). They test both literal matching and type-based matching scenarios, confirming that the type checker correctly narrows types and identifies unreachable branches.
2644-2659: Good test for matching on dictionary item accessThis test case ensures that pattern matching works correctly when the subject is a dictionary item access expression. It properly tests type narrowing for a union typed value and identifies unreachable code paths.
2660-2671: Good pathological case test for literal value matchingThis test verifies a corner case where matching on a literal value works correctly. It ensures the type checker properly handles matching a literal value against itself and identifies subsequent unreachable branches.
| # A helper state to produce unique temporary names on demand. | ||
| _unique_id: int | ||
|
|
There was a problem hiding this comment.
_unique_id must be added to __slots__ to avoid AttributeError.
TypeChecker uses __slots__ to save memory. Writing self._unique_id = 0 in __init__ will raise at runtime unless the new attribute is appended to the __slots__ tuple defined near the top of the class.
Suggested follow‑up (illustrative – adjust exact location):
class TypeChecker(TraverserVisitor):
- __slots__ = (
- "...",
- # existing slots
+ __slots__ = (
+ "...",
+ "_unique_id", # ← add here
)Committable suggestion skipped: line range outside the PR's diff.
| def _make_named_statement_for_match(self, s: MatchStmt) -> Expression: | ||
| """Construct a fake NameExpr for inference if a match clause is complex.""" | ||
| subject = s.subject | ||
| 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, | ||
| ) | ||
| if isinstance(subject, expressions_to_preserve): | ||
| return subject | ||
| elif s.subject_dummy is not None: | ||
| return s.subject_dummy | ||
| 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 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve source‑location info when synthesising a dummy subject.
NameExpr() defaults to line/column = -1, which degrades error positioning and can break linters relying on accurate locations.
Copy the location from the original subject so diagnostics continue to point to the correct place.
- named_subject = NameExpr(name)
+ named_subject = NameExpr(name)
+ # Keep error messages anchored to the original expression
+ named_subject.set_line(subject.line)
+ named_subject.set_column(subject.column)(If set_column is unavailable, use copy_location(named_subject, subject) helper that is used elsewhere in mypy.)
Optionally, also populate v.line = subject.line for completeness.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _make_named_statement_for_match(self, s: MatchStmt) -> Expression: | |
| """Construct a fake NameExpr for inference if a match clause is complex.""" | |
| subject = s.subject | |
| 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, | |
| ) | |
| if isinstance(subject, expressions_to_preserve): | |
| return subject | |
| elif s.subject_dummy is not None: | |
| return s.subject_dummy | |
| 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 | |
| def _make_named_statement_for_match(self, s: MatchStmt) -> Expression: | |
| """Construct a fake NameExpr for inference if a match clause is complex.""" | |
| subject = s.subject | |
| 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, | |
| ) | |
| if isinstance(subject, expressions_to_preserve): | |
| return subject | |
| elif s.subject_dummy is not None: | |
| return s.subject_dummy | |
| 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) | |
| # Keep error messages anchored to the original expression | |
| named_subject.set_line(subject.line) | |
| named_subject.set_column(subject.column) | |
| named_subject.node = v | |
| s.subject_dummy = named_subject | |
| return named_subject |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Diff from mypy_primer, showing the effect of this PR on open source code: vision (https://github.com/pytorch/vision)
- torchvision/utils.py:270: error: Unused "type: ignore" comment [unused-ignore]
|
Fixes python#18440. Fixes python#17230.
python#16503 did this specifically for
CallExpr, but that isn't the only kind of such statements. I propose to expand this for more general expressions and believe that a blacklist is more reasonable here: we do not want to introduce a temporary name only for certain expressions that either are already named or can be used to infer contained variables (inline tuple/list/dict/set literals).Writing logic to generate a name for every other kind of expression would be quite cumbersome - I circumvent this by using a simple counter to generate unique names on demand.
Summary by CodeRabbit