-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Python: remove assignments handled by capture library #15255
base: main
Are you sure you want to change the base?
Conversation
Evaluation did not show any improvements, so this is a clean-up more than an optimisation... |
I don't understand the rationale behind this. Can you please highlight what problem this solves? Doing a quick eval of |
Now that we are using the variable capture library, these assignments are modelled twice:
this is at best unnecessary and at worst a performance problem (the DCA run indicates that it is mostly the former, but it could be the latter in pathological cases).
They should all be relevant and something we want to keep modelling. But once should be enough. |
ok, so for the code below you want to remove the local flow from Do we have cases where this also affects correctness of our analysis? that is, without this change I assume we would have flow from def outer():
x = 1
def inner():
nonlocal x
x = 2
print(x)
print(x) # <-- flow from 1 to this x is ok
inner()
print(x) # <-- flow from 1 to this x is not what happens in execution AHA! I had forgotten the detail that our simple local flow step is defined as: codeql/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll Lines 474 to 480 in 0f89f69
so there are "new" local flow steps added through the variable capture library; do these cover all the flow we had before? -- did you make any queries to verify that we did not lose any flow? (what I could see happening is that if we lose local flow, our type-tracking won't be as good, which will certainly be a problem) |
Addresses https://github.com/github/codeql-python-team/issues/764
The following quick-query, run on top 100 Python projects, consistently reported about 1% of assignments can be removed: