C#: Improve db consistency by removing assembly id #3350
Conversation
|
Performance report here: https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/130/ |
calumgrant
force-pushed
the
cs/unqualify-trap-ids2
branch
from
230a6d6 to
a7a9d84
Compare
calumgrant
force-pushed
the
cs/unqualify-trap-ids2
branch
from
71521a0 to
813ed78
Compare
| @@ -999,5 +999,63 @@ class TypeMention extends @type_mention { | |||
| @type_or_ref getTypeRef(@type type) { | |||
There was a problem hiding this comment.
We should probably cache this predicate now that it relies on some non-trivial calculations.
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
A few comments and questions.
|
|
||
| string toString() { result = getName() } | ||
|
|
||
| Type getAType() { typeref_type(this, result) } |
There was a problem hiding this comment.
Refreshing my memory here: This is a one-to-many relation, right? I.e. count(TypeRef r | typeref_type(r, t)) <= 1 for all types t?
There was a problem hiding this comment.
Yes. As you observe, not all types have typerefs - only named types do.
calumgrant
force-pushed
the
cs/unqualify-trap-ids2
branch
from
42c623c to
2c6ee5e
Compare
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
Changes LGTM. Lets just wait for a final CSharp-Changes job to finish before merging (I have started on here: https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/174/console).
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
Blocking this, as performance is worsened significantly (see https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/174/).
calumgrant
force-pushed
the
cs/unqualify-trap-ids2
branch
from
2c6ee5e to
7438254
Compare
|
Changes job results are here: https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/232/ |
calumgrant
force-pushed
the
cs/unqualify-trap-ids2
branch
from
7438254 to
b4bc96a
Compare
|
Started differences job https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/260/ |
:-( |
…rwise, an entity with the wrong (cached) nullability could be created.
…er imports into alphabetical order.
…e` needed to be the same as `objectEntityCache` to fix nullability warnings.
calumgrant
force-pushed
the
cs/unqualify-trap-ids2
branch
from
a46cb79 to
e03971f
Compare
2 participants
Improves database consistency and fixes unstable results.
Summary of changes:
getTypeRef(that underlies predicates likeget_type_argument(n)) has just a single type. This prevents certain predicates from blowing up.In trap files, the identifiers for types are prefixed by their declaring assembly, in order to handle the same type name being used in different assemblies. This is because there can be several versions of the same type, or if the same type name is accidentally used in different assemblies, then it is useful to be able to distinguish them. This also prevents database errors if different versions of the same type name are incompatible.
However with .NET Core, the same type can be defined in different assemblies, for example
mscorlib,System.Private.CoreLib,netstandardandSystem.Runtime. This causes database instability if an assembly is extracted againstnetstandardin one build thread, andSystem.Runtimein another build thread. Depending upon which extraction happened first, the identifiers in the trap file will reference eitherSystem.Runtimeornetstandard.The fix is to remove the assembly-id qualified from the trap file, so that the build order does not matter.