-
Notifications
You must be signed in to change notification settings - Fork 66
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
Create a LocalScope
for CollectionComprehensions
and generate a Declaration
#2019
base: main
Are you sure you want to change the base?
Conversation
...anguage-python/src/main/kotlin/de/fraunhofer/aisec/cpg/frontends/python/ExpressionHandler.kt
Outdated
Show resolved
Hide resolved
...anguage-python/src/main/kotlin/de/fraunhofer/aisec/cpg/frontends/python/ExpressionHandler.kt
Show resolved
Hide resolved
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.
Looks good in general. However, there is still an issue with AssignExpressions leaking to the containing scope. See the added tests. I'm in favor of this PR once these issues are resolved.
...anguage-python/src/main/kotlin/de/fraunhofer/aisec/cpg/frontends/python/ExpressionHandler.kt
Outdated
Show resolved
Hide resolved
...anguage-python/src/main/kotlin/de/fraunhofer/aisec/cpg/frontends/python/ExpressionHandler.kt
Show resolved
Hide resolved
...age-python/src/test/kotlin/de/fraunhofer/aisec/cpg/frontends/python/ExpressionHandlerTest.kt
Outdated
Show resolved
Hide resolved
…side them using the := operator
...ge-python/src/main/kotlin/de/fraunhofer/aisec/cpg/frontends/python/PythonLanguageFrontend.kt
Outdated
Show resolved
Hide resolved
...ge-python/src/main/kotlin/de/fraunhofer/aisec/cpg/frontends/python/PythonLanguageFrontend.kt
Outdated
Show resolved
Hide resolved
bDeclaration, | ||
"We expect that the reference \"b\" in the tuple refers to the VariableDeclaration of \"b\" which is added outside the list comprehension (in statement 0).", | ||
) | ||
assertRefersTo( |
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 should discuss what we expect in this case. My version of Python3.13 crashes with the error "UnboundLocalError: cannot access local variable 'a' where it is not associated with a value".
We may interpret this by setting the refersTo
to same as the reference a
in the second item and later, an analysis must figure out that this variable is not initialized yet (we should check the EOG for this too).
We may also interpret this that refersTo
should be null
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 would say that refersTo
should be null. I think we did that in other cases where we have an unbound error as well
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, since the SymbolResolver
is in charge of this, I added issue #2035 which blocks this PR. I also added the asserts again.
Fixes #2005 by
LocalScope
for list, set, dict comprehensions and generators. This somewhat simulates PEP 709 where the loop variable is available only inside the loop.VariableDeclaration
inside this scope in thePythonAddDeclarationsPass
for the variables.refersTo
edges are setAssignExpressions
with the:=
operator inside a list/set/dict comprehension, the variable is looked up or created outside of the comprehension'sLocalScope
to address PEP 572