C#: Refactor data-flow predicates defined by dispatch

[hvitved]

Several data-flow predicates, such as getEnclosingCallable() are defined by dispatch. This PR adds an intermediate implementation layer NodeImpl, whereby the rootdef getEnclosingCallableImpl() can be made abstract. This means that the QL compiler will check that subclasses implement the abstract implementation predicates, but it also allows us to apply the unique aggregate in the exposed predicates where appropriate.

CSharp-Differences job here: https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/208/.

[hvitved]

CSharp-Differences job here: https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/208/.

g/mono/mono | 10760 | 9532 | 0.88587

🎉

[calumgrant]

Hey @hvitved, this seems like a really nice idea! If I understand things correctly, the performance gain is due to unique rather than dispatch optimization? In that case, I wondered whether we could just fold class NodeImpl into class Node and make class Node abstract? This would avoid the extra classes and the additional downcast to NodeImpl, and I think make things a bit clearer.

[hvitved]

If I understand things correctly, the performance gain is due to unique rather than dispatch optimization?

Yes.

In that case, I wondered whether we could just fold class NodeImpl into class Node and make class Node abstract? This would avoid the extra classes and the additional downcast to NodeImpl, and I think make things a bit clearer.

This would expose the internal implementation predicate and make the exposed class abstract, both things are undesirable. I agree that the extra layer is annoying, but it is the best we can do with QL at the moment, I'm afraid.

[calumgrant]

Okay, makes sense. My slight reservation is that the cast to NodeImpl also hides the implementation from the maintainers of the library as well!