Python: Use more general definitions #15080
Conversation
We now formulate local flow steps based on `DefinitionNode`s, instead of just `AssignmentDefinition`s. This collects all logic around definitions into `DefinitionNode` and avoids extra cases in the flow relations. It also allows a few more definitions needed for variable capture. These are currently excluded from `AssignmentDefinition` because they are not `TEssaNodeDefinition`s because they are not considered live. (`SsaComputeImpl::variableDefine` holds, but `Liveness::liveAtExit` does not). `DefinitionNode` includes parameter definitions from default values, which should be jump steps. So we split the definitions into local and jumps by looking at the enclosing callables. `DefinitionNode` did not include `with`-definitions, so that has been added. Like parameter definitions, this case of `DefinitionNode` must be excluded from `assignment_definition`. Finally, we incur a dataflow inconsistency for a non-sensical assignment in test code. This we simply accept. The code reads ```python len(unknown()) = "foo" ``` which makes the target of the flow edge a post-update node.
yoff
force-pushed
the
python/local-flow-general-definitions
branch
from
b0d5366 to
515e8fd
Compare
yoff
added
Awaiting evaluation
|
Performance looks fine (unchanged), but there is a small number of alert differences. Edit: Sorry, I looked at the earlier experiment. The new one has some failures related to the internet that I have asked it to retry. Apart from that, performance is unchanged, alerts are unchanged, and the meta alerts show that we have a richer flow relation. |
There was a problem hiding this comment.
Apart from the two comment NITS, overall LGTM 👍
I think there's something slightly strange about DefinitionNode containing with, since when you look into the semantics it's not just a straight assignment (but rather, the value of __enter__()/await __aenter__()). However, since DefinitionNode includes for at the moment, I think it's fair game to also include with 🤷♂️
It also allows a few more definitions needed for variable
capture. These are currently excluded from AssignmentDefinition
because they are not TEssaNodeDefinitions because
they are not considered live.
(SsaComputeImpl::variableDefine holds, but
Liveness::liveAtExit does not).
This seems to be your intuition for the additional flow we achieve with this change. I was hoping to see this reflected in some of our tests. Can you add a test that illustrates this, that isn't about variable capture?
|
I'm a little undecided around whether a change-note is required... I guess it's fine without one |
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
|
Converting to draft until we know more accurately what is added here. |
|
We agreed offline to take a closer look at the new edges being added from this PR |
We now formulate local flow steps based on
DefinitionNodes, instead of justAssignmentDefinitions.This collects all logic around definitions into
DefinitionNodeand avoids extra cases in theflow relations.
It also allows a few more definitions needed for variable
capture. These are currently excluded from
AssignmentDefinitionbecause they are not
TEssaNodeDefinitions becausethey are not considered live.
(
SsaComputeImpl::variableDefineholds, butLiveness::liveAtExitdoes not).DefinitionNodeincludes parameter definitionsfrom default values, which should be jump steps.
So we split the definitions into local and jumps
by looking at the enclosing callables.
DefinitionNodedid not includewith-definitions,so that has been added. Like parameter definitions,
this case of
DefinitionNodemust be excluded fromassignment_definition.Finally, we incur a dataflow inconsistency for a
non-sensical assignment in test code. This we simply accept.
The code reads
which makes the target of the flow edge a post-update node.