Conversation
Splits `ModuleVariableNode` away from `LocalSourceNode`, instead creating a class `TypeTrackingNode` that encapsulates both of these. This means we no longer have module variable nodes as part of `LocalSourceNode` (which is good, since they have no "local" aspect to them), and hence we can have `LocalSourceNode` inherit directly from `ExprNode` (which makes the API a bit nicer). Unfortunately these are breaking changes, so we can't actually fulfil the above two desiderata until the `track` and `backtrack` methods on `LocalSourceNode` have been fully deprecated. For this reason, we preserve the present implementation of `LocalSourceNode`, and instead lay the foundation for switching over in the future, by deprecating `track` and `backtrack` on `LocalSourceNode`.
|
I should probably run a performance test on this just in case. I don't expect it to change anything, but who knows how it may influence the compiler. |
There was a problem hiding this comment.
Overall looks good to me 👍 (as long as performance is good)
I think the major risk from this PR is the extra complexity of having another class of data-flow nodes that you need to know about, and a potential problem in that it's still possible to write type-tracking code using LocalSourceNode and not TypeTrackingNode (that just won't work properly).
Since we probably have some open PRs that use type-tracking right now, fixing this up is something we need to keep in mind for the near future (and possibly do a grep through our codebase on a regular cadence through the next few months)
For the grep part, I developed grep --files-with-match -Pazo 'LocalSourceNode[^({}]+\([^)]*(TypeTracker|TypeBackTracker)' **.{ql,qll} which handles predicate definition spanning multiple-lines... but obviously using CodeQL would be better 😉
|
Performance looks to be mostly unchanged: https://github.com/dsp-testing/tausbn-dca/issues/46 I'm not sure I understand your point about type-tracking using Just to verify my intuition above, I tried changing the I agree that this does add a bit more complexity, but my hope was that this would actually make things more clear (at least in the long run). To me, |
|
Oh, on that note, I must remember to update the documentation correspondingly. |
Very good point. The problem I'm talking about would actually never occur. The case I had in mind was when we were able to change the char-pred of I somehow didn't add 2 and 2 together on the fact that these are now deprecated, so we would start getting warnings if we merged anything that used these, which would in turn make our CI angry... so we probably won't need that |
|
Ah, good. 🙂 (I should also add that actually testing that things behaved the way I expected them to was probably a good idea. It would have been unfortunate if |
It seems I misremembered, and there is no Python-specific documentation of type trackers. While we should probably add this at some point, I don't think this needs to be in the present PR. |
Splits
ModuleVariableNodeaway fromLocalSourceNode, insteadcreating a class
TypeTrackingNodethat encapsulates both of these.This means we no longer have module variable nodes as part of
LocalSourceNode(which is good, since they have no "local" aspect tothem), and hence we can have
LocalSourceNodeinherit directly fromExprNode(which makes the API a bit nicer).Unfortunately these are breaking changes, so we can't actually fulfil
the above two desiderata until the
trackandbacktrackmethods onLocalSourceNodehave been fully deprecated. For this reason, wepreserve the present implementation of
LocalSourceNode, and insteadlay the foundation for switching over in the future, by deprecating
trackandbacktrackonLocalSourceNode.Deprecates two public methods, so needs a change note.