Skip to content

Introduce temporary named expressions for match 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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

sterliakov
Copy link
Collaborator

@sterliakov sterliakov commented Jan 11, 2025

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 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.

This comment has been minimized.

@sterliakov
Copy link
Collaborator Author

Whoa, I expected this to produce at least a couple of unused ignore diagnostics, but apparently pattern matching isn't very popular in wild...

@sterliakov sterliakov marked this pull request as ready for review January 11, 2025 21:44

This comment has been minimized.

@sterliakov

This comment was marked as resolved.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@sterliakov
Copy link
Collaborator Author

This interferes with recently added narrowing by item/attribute (aka discriminated union, testMatchNarrowingUnionClassViaAttribute). So a simple solution like this won't fly. This information for index/attr should be stored in some other way to support narrowing of attr/item and the object itself at the same time. This may still work for other, less informative nodes (await?)

@sterliakov sterliakov closed this Mar 29, 2025
@sterliakov sterliakov reopened this Apr 4, 2025
@sterliakov sterliakov marked this pull request as draft April 4, 2025 22:41

This comment has been minimized.

@sterliakov
Copy link
Collaborator Author

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.

@sterliakov sterliakov marked this pull request as ready for review April 5, 2025 01:28

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.

Copy link
Member

@ilevkivskyi ilevkivskyi left a 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 = (
Copy link
Member

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:

from mypy.literals import literal  # name is confusing, but this is what you want
if not literal(expr):
    <make dummy name>

Copy link
Collaborator Author

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?

mypy/mypy/literals.py

Lines 117 to 124 in fc991a0

def literal(e: Expression) -> int:
"""Return the literal kind for an expression."""
if isinstance(e, ComparisonExpr):
return min(literal(o) for o in e.operands)
elif isinstance(e, OpExpr):
return min(literal(e.left), literal(e.right))

This will break testMatchOperations case added here, for example, as 1 < 2 will return LITERAL_YES, and we do not store types of comparison exprs in binder AFAIC.

Copy link
Member

Choose a reason for hiding this comment

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

Right, binder also does isinstance(expr, (IndexExpr, MemberExpr, NameExpr)) before checking for literal(). 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 about AssignmentExpr, what about something like x := y[z] (where z is not a literal)? This will have troubles with binder.

Copy link
Member

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] := z with non-literal y.

Copy link
Collaborator Author

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] := z with non-literal y

Did I miss a new PEP? If I didn't, this should be reported elsewhere as a SyntaxError...

Copy link
Collaborator Author

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".

Copy link
Collaborator Author

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.

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
if isinstance(subject, (NameExpr, AssignmentExpr)):
Copy link
Member

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:

        if isinstance(subject, (IndexExpr, MemberExpr, NameExpr) and literal(subject):
            <use existing expression>
        else:
            <create fake name>

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

github-actions bot commented Aug 3, 2025

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

@ilevkivskyi ilevkivskyi merged commit 555edf3 into python:master Aug 3, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants