Create a LocalScope for CollectionComprehensions and generate a Declaration
#2019
Conversation
KuechA
requested review from
maximiliankaul,
lshala,
konradweiss and
oxisto
as code owners
| * Works similar to [apply] but before executing [block], it enters the scope for this object | ||
| * and afterward leaves the scope again. | ||
| */ | ||
| private inline fun <T : Node> T.applyWithScope(block: T.() -> Unit): T { |
There was a problem hiding this comment.
This could be a helpful bit, also for the future node builder API, should we move this to the scope manager? Or keep it intentionally private for now?
There was a problem hiding this comment.
I thought the same and wasn't sure where would be a good place. Maybe Node directly?
There was a problem hiding this comment.
When in doubt place it in Extensions.kt:D
| @@ -129,10 +141,10 @@ class ExpressionHandler(frontend: PythonLanguageFrontend) : | |||
| * a [CollectionComprehension]. | |||
| */ | |||
| private fun handleSetComprehension(node: Python.AST.SetComp): CollectionComprehension { | |||
| return newCollectionComprehension(rawNode = node).apply { | |||
| return newCollectionComprehension(rawNode = node).applyWithScope { | |||
There was a problem hiding this comment.
I am not sure, if this is really relevant in our case, but it seems that the exact definition is that only the control variable is not leaked into the outer scope, but I guess the only sane way to model this is exactly like you did by opening up a local scope for the comprehension.
There was a problem hiding this comment.
Interesting, from https://peps.python.org/pep-0709/ -> "Comprehensions are currently compiled as nested functions, which provides isolation of the comprehension’s iteration variable, but is inefficient at runtime."
There was a problem hiding this comment.
Hm, this sounds as if the current behavior would be appropriately modeled with the local scope but the intended/future behavior might be different.
Actually, we're only generating a VariableDeclaration inside this local scope for the variable of the comprehension expression and this is done inside the PythonAddDeclarationsPass. Leaving the scope earlier (i.e., right after generating the ComprehensionExpression) here won't impact on resolving variables, right?
There was a problem hiding this comment.
Hm, this sounds as if the current behavior would be appropriately modeled with the local scope but the intended/future behavior might be different.
Yes, although I am not sure if this "new" bevahiour will leak the control variable or not. Maybe our PEP master @maximiliankaul understands this better than us.
Actually, we're only generating a
VariableDeclarationinside this local scope for the variable of the comprehension expression and this is done inside thePythonAddDeclarationsPass. Leaving the scope earlier (i.e., right after generating theComprehensionExpression) here won't impact on resolving variables, right?
I would assume so
There was a problem hiding this comment.
From my understanding, the variable should be available only in the LocalScope tied to the ComprehensionExpression. Obviously, we can always enter this scope to incorrectly resolve a variable but this shouldn't be the case. I also added a test which simulates the test case he provided in the original comment.
Fixes #2005 by
LocalScopefor list, set, dict comprehensions and generatorsVariableDeclarationinside this scope in thePythonAddDeclarationsPassfor the variables.