FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Do not cache fast container types inside lambdas by sterliakov · Pull Request #20166 · python/mypy · GitHub

  1. python
  2. mypy

Repository navigation

Do not cache fast container types inside lambdas - #20166

Merged
hauntsaninja merged 1 commit into
python:masterfrom
sterliakov:bugfix/gh-20163-fast-container-lambda
Nov 3, 2025
ConversationCommits (1)ChecksFiles changed
Merged

Do not cache fast container types inside lambdas#20166
hauntsaninja merged 1 commit into
python:masterfrom
sterliakov:bugfix/gh-20163-fast-container-lambda

Conversation

sterliakov commented Nov 2, 2025

Copy link
Copy Markdown
Collaborator

Fixes #20163. fast_container_type uses another layer of expression cache, it also has to be bypassed from within lambdas.

sterliakov changed the title Do not write "final" cache results for fast containers inside lambdas Do not cache fast container types inside lambdas Nov 2, 2025
github-actions Bot mentioned this pull request Nov 2, 2025
Closed

github-actions Bot commented Nov 2, 2025

Copy link
Copy Markdown
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

spark (https://github.com/apache/spark)
- python/pyspark/core/rdd.py:2210: error: Unused "type: ignore" comment  [unused-ignore]

sterliakov commented Nov 2, 2025

Copy link
Copy Markdown
Collaborator Author

And that's correct, pyspark error is not a false positive - StatCounter only accepts Iterable[float] but i is NumberOrArray that includes bare floats and some numpy stuff, so StatCounter(i) is invalid.

sterliakov requested a review from A5rocks November 2, 2025 15:12
A5rocks reviewed Nov 2, 2025
View reviewed changes
Comment thread mypy/checkexpr.py
if not self.in_lambda_expr:
# We cannot cache results in lambdas - their bodies can be accepted in
# error-suppressing watchers too early
self.resolved_type[e] = ct

A5rocks Nov 2, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Isn't there an expr cache that stores errors too? Is it possible to use that instead?

sterliakov Nov 3, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We already explicitly bypass the expr cache for lambdas, so probably no?

mypy/mypy/checkexpr.py

Lines 6035 to 6050 in 843d133

elif (
isinstance(node, (CallExpr, ListExpr, TupleExpr, DictExpr, OpExpr))
and not (self.in_lambda_expr or self.chk.current_node_deferred)
and not self.chk.options.disable_expression_cache
):
if (node, type_context) in self.expr_cache:
binder_version, typ, messages, type_map = self.expr_cache[(node, type_context)]
if binder_version == self.chk.binder.version:
self.chk.store_types(type_map)
self.msg.add_errors(messages)
else:
typ = self.accept_maybe_cache(node, type_context=type_context)
else:
typ = self.accept_maybe_cache(node, type_context=type_context)
else:
typ = node.accept(self)

A5rocks Nov 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That makes some sense, but I don't quite get the comment there. I also don't get why this extra cache exists...

I guess the comment is saying "the same expr can be evaluated multiple times in different contexts" which makes a bit of sense?

sterliakov Nov 3, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I also do not fully understand why a separate resolved_type storage is needed at all. It might well be just a remainder that wasn't cleaned up when expr_cache was introduced, but I'm not certain.

This lambda special-casing is coming from in #19408 and #19505, we discovered it independently. lambda exprs are handled in a completely different way with and without type context (see infer_lambda_type_using_context and branching on its result). There's a big difference between accepting a ReturnStmt and its expression alone - supporting both ways essentially means that we use different context stack entries on different paths for the same expression. This part is a bit difficult to reason about, but I still hope I got it correctly...

A5rocks Nov 3, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

OK, I guess it would make sense to try removing resolved_type cache in a followup...

Tinche commented Nov 3, 2025

Copy link
Copy Markdown
Contributor

Hey, thanks for working on this!

hauntsaninja merged commit 08a0b07 into python:master Nov 3, 2025
21 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

hauntsaninja hauntsaninja approved these changes
+1 more reviewer
A5rocks A5rocks approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

type checking in lambdas stops working when using Iterable instead of list

4 participants

Back | FazBrowse Home | New Git URL