[red-knot] Handle multiple comprehension targets #13213
Conversation
dhruvmanila
changed the title
dhruvmanila
requested review from
carljm,
MichaReiser and
AlexWaygood
as code owners
|
| @@ -156,7 +156,8 @@ pub(crate) struct ForStmtDefinitionNodeRef<'a> { | |||
|
|
|||
| #[derive(Copy, Clone, Debug)] | |||
| pub(crate) struct ComprehensionDefinitionNodeRef<'a> { | |||
| pub(crate) node: &'a ast::Comprehension, | |||
| pub(crate) iterable: &'a ast::Expr, | |||
There was a problem hiding this comment.
I was wondering if we'll also need access to the if portion of the generator here, but I think we won't; that expression should be handled separately as a Constraint, it's not part of the Definition.
There was a problem hiding this comment.
Yeah, that's what I thought as well which is the reason to limit this. It can easily be reverted in the future if there's a need.
| self.infer_definition(comprehension); | ||
| for expr in &comprehension.ifs { | ||
| self.infer_expression(expr); | ||
| fn infer_comprehensions_scope(&mut self, comprehensions: &[ast::Comprehension]) { |
There was a problem hiding this comment.
We've inherited some awfully confusing naming from the CPython grammar and AST :( If we have [y for x in [] for y in x], that's a single "list comprehension", whose AST node has a generators attribute containing two... Comprehensions, for x in [] and for y in x. So not only is the term "generator" overloaded here from its usual meaning, but we're also using the term "comprehension" itself to mean two entirely different things, one of which is part of the other 🤯
(To be clear, this is not a critique of this PR, just a lament for the state of naming in this part of the AST. We could fix this in our AST, but then we'd be inventing our own naming scheme that doesn't match the CPython AST, and that doesn't seem great either.)
One way to maybe clarify this naming a bit would be to switch from list_comprehension, dict_comprehension, etc all through these method names to instead use listcomp, dictcomp, etc when we are referring to the outer expression, and reserve the full word "comprehension" for the thing actually named Comprehension in the AST, which is just the inner for x in y part. I wouldn't exactly call that clear, but given what we're working with, it might at least be better? It at least matches the names used in the AST and avoids using the word "comprehension" to mean two different things.
In any case, I don't like the names infer_comprehensions_scope and infer_comprehension_scope for these two methods. The individual "Comprehension" nodes inside a list/set/dict/gen-comp are not different scopes, so the use of "scope" here is misleading.
If we go with my suggestion above to rename e.g. infer_list_comprehension_expression to infer_listcomp_expression, then I think we could name these two just infer_comprehensions and infer_comprehension.
There was a problem hiding this comment.
I agree with the sentiment, thanks for sharing that.
(To be clear, this is not a critique of this PR, just a lament for the state of naming in this part of the AST. We could fix this in our AST, but then we'd be inventing our own naming scheme that doesn't match the CPython AST, and that doesn't seem great either.)
I think, if we want, we can change the names in our AST as we've done so in the past as well (#8064, #6379, #6253, etc.). There are other recommendations here: #6183
One way to maybe clarify this naming a bit would be to switch from
list_comprehension,dict_comprehension, etc all through these method names to instead uselistcomp,dictcomp, etc when we are referring to the outer expression, and reserve the full word "comprehension" for the thing actually namedComprehensionin the AST, which is just the innerfor x in ypart.
From my perspective, both listcomp and list_comprehension seems equivalent as the former seems like a short version of the latter. Is the reason for recommending listcomp because of it's presence in the CPython AST?
In any case, I don't like the names
infer_comprehensions_scopeandinfer_comprehension_scopefor these two methods. The individual "Comprehension" nodes inside a list/set/dict/gen-comp are not different scopes, so the use of "scope" here is misleading.
Makes sense. I can change this.
There was a problem hiding this comment.
I've not renamed the list_comprehension to listcomp as I feel like the "expression" prefix in list_comprehension_expression gives a signal that this is different than a simple "comprehension". Regardless, I don't have a strong opinion, so I'm happy to rename it if you think listcomp is better.
There was a problem hiding this comment.
Yeah, mostly what my suggestion was aiming for was to hew very closely to the AST naming, in hopes that this would at least give some guideposts to readers. But I'm OK with your approach and relying on _expression to distinguish.
CodSpeed Performance ReportMerging #13213 will improve performances by 4.66%Comparing Summary
Benchmarks breakdown
|
Summary
Part of #13085, this PR updates the comprehension definition to handle multiple targets.
Test Plan
Update existing semantic index test case for comprehension with multiple targets. Running corpus tests shouldn't panic.