Python: Add Node::getPreUpdateNode#6079
Conversation
This is mostly as a convenience, as `foo.(PostUpdateNode).getPreUpdateNode()` can be a bit of a mouthful. This does not change the external API in any meaningful way, so I don't think a change note is necessary.
I had forgotten that `DataFlowImplCommon.qll` is shared across languages, and so any change would have to be mirrored there. But that would also mean that other languages would have to add the same setup as us for these nodes, which I think is going to be a hard sell. I'm not really sure where this leaves this proposal... :/
Turns out I had only looked for `(PostUpdateNode).get...` and not `(DataFlow::PostUpdateNode).get...` which had a bunch more instances that could be cleaned up.
There was a problem hiding this comment.
I'm slightly split about this (even after thinking it over for some days). On one hand, I really like foo.(DataFlow::PostUpdateNode).getPreUpdateNode() since it so explicitly says that foo is in fact a PostUpdateNode. On the other hand, for foo.getPreUpdateNode() I have to remember that this implies that foo is now PostUpdateNode, which is slightly subtle, and doing this mental gymnastics feels draining to me.
foo.(PostUpdateNode).getPreUpdateNode() is a bit of a mouthful, but from my point of view, this explicitness makes it much easier to figure out the pre- and post update node setup. So I think overall I'm not in favor of this change 😐
EDIT: I see this might seem a bit strange to oppose when I'm suggesting to introduce .getPostUpdateNode in #5926... But I think it has to do with the fact that in bar.getPostUpdateNode(), I can still think of bar as a "normal" data-flow node, and in foo.getPreUpdateNode() I have to remember that foo is in fact a "special" data-flow node, namely a PostUpdateNode.
|
I also feel split, and I have not really found a principle to guide me. |
|
I think whatever we decide, it should be consistent: either we have both, or we have neither. Otherwise, we end up in a situation where one direction is nice to express, and the other one is just... seemingly missing (and requires a "magic" cast to work). Also, my desire for adding these "convenience" functions is to help combat that "magic". If the interface behind which we access pre- and post-update nodes requires casting, then it's basically impossible to encounter by chance. No one sits down and goes "hey I wonder what happens if I cast to Seeing as I'm still very tempted by the idea of being able to access the pre- and post-update node for any node (and these nodes always existing), but having these be equal to the node in question if no synthesised node exists. That way, I don't ever have to remember that in some cases it's the I believe this would be as simple as defining these methods as the identity relation on |
This is mostly as a convenience, as
foo.(PostUpdateNode).getPreUpdateNode()can be a bit of a mouthful.This does not change the external API in any meaningful way, so I don't
think a change note is necessary.