C#: Restrict DataFlowType to types belonging to Nodes
#4065
Conversation
| exists(Gvn::GvnType t | | ||
| Gvn::unifiable(t1, t) and | ||
| commonSubType(t, t2) | ||
| commonSubTypeGeneral(t, t2) |
There was a problem hiding this comment.
As a general comment here: a lot of these annotations seem to be working around actual/supposed compiler missteps. For example, here one might more naturally use commonSubType, but pragma[nomagic] suggests that there has been a compilation problem that is being worked around. This leads to the code being more verbose and split up in slightly unnatural ways. Is there a better way to document what's being worked around, or should I simply continue to "filter out" all of these pragmas in my mind?
There was a problem hiding this comment.
Actually, here we can't use commonSubType, because we need the more general variant where the first column is not restricted to DataFlowType (but the more general DataFlowTypeOrUnifiable type).
|
|
||
| predicate readStep = readStepImpl/3; | ||
|
|
||
| /** |
There was a problem hiding this comment.
I don't understand the difference between DataFlowType and Gvn::GvnType. Gvn::GvnType has the following comment on it, which is pretty much the same as the one on DataFlowType.
A global value number for a type. Two types have the same GVN when they
are structurally equal modulo type parameters and identity conversions.
For example, `Func<T, int>` and `Func<S, int>` have the same GVN, but
`Func<T, int>` and `Func<string, int>` do not.
There was a problem hiding this comment.
DataFlowType is simply a subset of Gvn::GvnType; namely those that are the result of NodeImpl::getDataFlowType(). I decided to make the QL doc for DataFlowType a bit more data-flow centric, compared to the QL doc for the more generally applicable Gvn::GvnType.
calumgrant
left a comment
calumgrant
left a comment
There was a problem hiding this comment.
Thanks Tom, this is a really nice speedup.
| exists(Gvn::GvnType t | | ||
| Gvn::unifiable(t1, t) and | ||
| commonSubType(t, t2) | ||
| commonSubTypeGeneral(t, t2) |
3 participants
This reduces the size of the
commonSubTypepredicates as we only compute them for relevantDataFlowTypes.The CSharp-Differences job is here, and it shows a significant performance improvement on
dotnet/efcore. Since the predicates affected by this change are cached, the impact of this PR can be seen by focusing on the performance numbers forAPI Abuse/DisposeNotCalledOnException.ql, as this happens to be the query that evaluates the cached stage.