Repurpose unused dependency tracing to track marking reasons #1228
Conversation
marek-safar
commented
Jun 1, 2020
marek-safar
commented
- Convert DepdencyKind to MarkingInfo and include more cases for tracking
- Include marking info data in the trace output
- Include all edges in the output data and not only first hits
- Add documentation for the command line options
- Add documentation for the data format
- Convert DepdencyKind to MarkingInfo and include more cases for tracking - Include marking info data in the trace output - Include all edges in the output data and not only first hits - Add documentation for the command line options - Add documentation for the data format
| </xs:schema> | ||
| ``` | ||
|
|
||
| Every edge has `b` and `e` attribute which represent vertices of the directed graph. They can have |
There was a problem hiding this comment.
Just curious - the names b, e and c don't make sense to me - maybe explain what they stand for. For example I would choose s = "source", t = "target", r = "reason".
| |---|---|---| | ||
| | AS | Assembly | fully-qualified-name | | ||
| | CA | Custom Attribute | attribute-type | | ||
| | EV | Event | declaring-type:property-name | |
There was a problem hiding this comment.
This should probably be declaring-type::property-name (double :). Also for the ones below.
| <dependencies version="1.3"> | ||
| <edge b="FI:System.Exception::_innerException" ba="System.Private.CoreLib" e="TD:System.Exception" c="FieldType" /> | ||
| <edge b="ME:System.Runtime.CompilerServices.NullableAttribute::.ctor(System.Byte)" ba="System.Private.CoreLib" e="TD:System.Byte" c="ParameterType" /> | ||
| <edge b="AS:sample, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null" e="ME:AppDelegate::Main(System.String[])" ea="sample" c="RootAssembly" /> |
There was a problem hiding this comment.
Nit: I think this example is slightly wrong - Assembly -> EntryPoint reason should be something like AssemblyAction
| // | ||
| // Entry points to the analysis | ||
| // | ||
| AssemblyAction = 1, |
There was a problem hiding this comment.
Can we keep the comments from the original enum? I like that those include what is the edge the reason is used on - it makes it easier to understand the reason.
| continue; | ||
|
|
||
| MarkInterfaceImplementations (type); | ||
| MarkInterfaceImplementations (type.Item1, type.Item2); |
There was a problem hiding this comment.
Can we please name the values in the tuple?
| @@ -844,9 +745,7 @@ protected virtual void MarkSecurityAttribute (SecurityAttribute sa, in Dependenc | |||
| return; | |||
| } | |||
|
|
|||
| // Security attributes participate in inference logic without being marked. | |||
| Tracer.AddDirectDependency (sa, reason, marked: false); | |||
There was a problem hiding this comment.
This will introduce a "hole" in the graph, right?
That is we will see that the type of the attribute is marked due to the security attribute, but there won't be any edge describing what marked the security attribute. I find this weird.
| if (!_methodReasons.Contains (reason.Kind)) | ||
| throw new ArgumentOutOfRangeException ($"Internal error: unsupported method dependency {reason.Kind}"); | ||
| #endif | ||
| /* |
There was a problem hiding this comment.
I assume that the "comment out" here is to be removed... ?
| @@ -2423,26 +2269,23 @@ protected virtual bool ShouldParseMethodBody (MethodDefinition method) | |||
| } | |||
| } | |||
|
|
|||
| internal protected void MarkProperty (PropertyDefinition prop, in DependencyInfo reason) | |||
| internal protected void MarkProperty (PropertyDefinition prop) | |||
There was a problem hiding this comment.
Similar comment as above - won't this introduce a "hole" in the graph? That is we will mark the custom attribute on the property as "reason - property", but there won't be any record of why the property itself was marked...
|
|
||
| // Blame the field definition that we will resolve on the field reference. | ||
| reason = new DependencyInfo (DependencyKind.FieldOnGenericInstance, reference); | ||
| MarkType (reference.DeclaringType, new MarkingInfo (MarkingReason.GenericInstance, reference)); |
There was a problem hiding this comment.
public class G<T> {
public T field;
}
public class Foo {}
public class C {
public void M() {
var g = new G<Foo> ();
_ = g.field;
}
}Before the changes here and in GetOriginalType, this creates a dependency graph like:
M- FieldAccess ->
G<Foo>.field- DeclaringType (here) ->
G<Foo>- ElementType (in GetOriginalType) ->
G<T> - GenericArgumentType ->
Foo
- ElementType (in GetOriginalType) ->
- FieldOnGenericInstance ->
G<T>.field- DeclaringType (in MarkField) ->
G<T>
- DeclaringType (in MarkField) ->
- DeclaringType (here) ->
- FieldAccess ->
With the new logic, it looks like this:
M- FieldAccessInstr ->
G<T>.field- DeclaringType (in MarkField) ->
G<T>
- DeclaringType (in MarkField) ->
- FieldAccessInstr ->
G<Foo>.field- GenericInstance ->
G<T>
- GenericInstance ->
G<Foo>- GenericArgument ->
Foo
- GenericArgument ->
which includes G<Foo>.field or G<Foo> in the dependency graph without saying why, and also seems to record G<T> as a generic instance of G<Foo>.field which doesn't make sense to me. What is the reason for the change?
