Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.1k
Introduce temporary named expressions formatch subjects#18446
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Introduce temporary named expressions formatch subjects#18446
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment has been minimized.
This comment has been minimized.
sterliakov commentedJan 11, 2025
Whoa, I expected this to produce at least a couple of unused ignore diagnostics, but apparently pattern matching isn't very popular in wild... |
…ic-named-expr-in-match
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
…ic-named-expr-in-match
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ic-named-expr-in-match
This comment has been minimized.
This comment has been minimized.
…ic-named-expr-in-match
sterliakov commentedMar 29, 2025
This interferes with recently added narrowing by item/attribute (aka discriminated union, |
This comment has been minimized.
This comment has been minimized.
sterliakov commentedApr 5, 2025
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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ilevkivskyi left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LG, but I have one suggestion
mypy/checker.py Outdated
| 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= ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IIUC match statement handling re-uses binder in some code paths. Then I I think instead of this list we should do the same that binder does:
frommypy.literalsimportliteral# name is confusing, but this is what you wantifnotliteral(expr):<makedummyname>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice idea, but are you sure?
Lines 117 to 124 infc991a0
| defliteral(e:Expression)->int: | |
| """Return the literal kind for an expression.""" | |
| ifisinstance(e,ComparisonExpr): | |
| returnmin(literal(o)foroine.operands) | |
| elifisinstance(e,OpExpr): | |
| returnmin(literal(e.left),literal(e.right)) |
This will breaktestMatchOperations case added here, for example, as1 < 2 will returnLITERAL_YES, and we do not store types of comparison exprs in binder AFAIC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Right, binder also doesisinstance(expr, (IndexExpr, MemberExpr, NameExpr)) before checking forliteral(). But then this list will not be simplified much. Even then probably it would make sense to somehow re-use binder logic here.
Btw I am not super-confident aboutAssignmentExpr, what about something likex := y[z] (wherez isnot a literal)? This will have troubles with binder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, I meant the oppositex[y] := z with non-literaly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, I meant the opposite
x[y] := zwith non-literaly
Did I miss a new PEP? If I didn't, this should be reported elsewhere as a SyntaxError...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe we can add a common ancestor to{Str,Bytes,Int,Float,Complex}Expr? Semantically they should have a "primitive literal" superclass, then this list will look much saner - just "name or primitive".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hm. I only decided to allow primitive literals as an optimization, but nothing will break if we store their types. Matching of primitive literals sounds like something no one should be doing (often). Let's remove them altogether!
This comment has been minimized.
This comment has been minimized.
…hould not be common
This comment has been minimized.
This comment has been minimized.
mypy/checker.py Outdated
| 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 | ||
| ifisinstance(subject, (NameExpr,AssignmentExpr)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What I wanted is something like this:
ifisinstance(subject, (IndexExpr,MemberExpr,NameExpr)andliteral(subject):<useexistingexpression>else:<createfakename>
plus an explicit comment that this should be kept in sync with corresponding logic in binder. This way it will be easier to spot a bug if something changes in binder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hm, okay, you finally made me realize that the problem with walrus is not situated here - thanks! I think the new version should be closer.
Uh oh!
There was an error while loading.Please reload this page.
According tomypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
555edf3 intopython:masterUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixes#18440.Fixes#17230.Fixes#16650. Improves behavior in#14731 (but still thinks that match is non-exhaustive, "missing return" false positive remains).
#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 donot 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.