-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Do not cache fast container types inside lambdas #20166
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
Do not cache fast container types inside lambdas #20166
Conversation
|
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]
|
|
And that's correct, pyspark error is not a false positive - |
| 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 |
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.
Isn't there an expr cache that stores errors too? Is it possible to use that instead?
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.
We already explicitly bypass the expr cache for lambdas, so probably no?
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) |
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.
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?
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.
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...
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.
OK, I guess it would make sense to try removing resolved_type cache in a followup...
|
Hey, thanks for working on this! |
Fixes #20163.
fast_container_typeuses another layer of expression cache, it also has to be bypassed from within lambdas.