Skip to content
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

[red-knot] support narrowing on or patterns in matches #17030

Merged
merged 5 commits into from
Mar 28, 2025

Conversation

ericmarkmartin
Copy link
Contributor

Summary

Part of #13694

Narrow in or-patterns by taking the type union of the type constraints in each disjunct pattern.

Test Plan

Add new tests to narrow/match.md

@ericmarkmartin ericmarkmartin marked this pull request as draft March 28, 2025 06:07
Copy link
Contributor

github-actions bot commented Mar 28, 2025

mypy_primer results

Changes were detected when running on open source projects
rich (https://github.com/Textualize/rich)
+ error[lint:invalid-argument-type] /tmp/mypy_primer/projects/rich/rich/control.py:198:27: Object of type `dict | @Todo[crates/red_knot_python_semantic/src/types/infer.rs:3616]` cannot be assigned to parameter 2 (`table`) of bound method `translate`; expected type `_TranslateTable`
- error[lint:invalid-argument-type] /tmp/mypy_primer/projects/rich/rich/control.py:198:27: Object of type `dict | @Todo[crates/red_knot_python_semantic/src/types/infer.rs:3611]` cannot be assigned to parameter 2 (`table`) of bound method `translate`; expected type `_TranslateTable`

Move the pattern guard field from PatternPredicateKind to
PatternPredicate. This better matches the ast and saves us from storing
a bunch of Nones in or patterns. Also, it will make things a bit
cleaner if we start to look at guards in the match case narrowing
@ericmarkmartin ericmarkmartin marked this pull request as ready for review March 28, 2025 07:42
case "foo" | 42 | None:
reveal_type(x) # revealed: Literal["foo", 42] | None
case "foo" | tuple():
# kill lamumbo
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this been randomly generated by an LLM or what is going on?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also curious how this came to be here 😆 but the PR looks great otherwise, so I'm just going to remove this line and merge.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Thank you!

case "foo" | 42 | None:
reveal_type(x) # revealed: Literal["foo", 42] | None
case "foo" | tuple():
# kill lamumbo
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also curious how this came to be here 😆 but the PR looks great otherwise, so I'm just going to remove this line and merge.

@carljm carljm enabled auto-merge (squash) March 28, 2025 14:24
@carljm carljm merged commit 6417174 into astral-sh:main Mar 28, 2025
22 checks passed
@dhruvmanila dhruvmanila added the red-knot Multi-file analysis & type inference label Mar 28, 2025
@ericmarkmartin ericmarkmartin deleted the match-or-narrowing branch March 28, 2025 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants