Open layomia opened 3 years ago
The only reference to the constructor is this LOC from the following snippet:
if (!_classes.TryGetValue(type, out JsonClassInfo? result))
{
if (JsonHelpers.DisableJsonSerializerDynamicFallback)
{
throw new NotSupportedException($"Metadata for type {type} not provided to serializer - will not go down reflection-based code path.");
}
else
{
result = _classes.GetOrAdd(type, new JsonClassInfo(type, this));
}
}
The JsonHelpers.DisableJsonSerializerDynamicFallback
boolean above is controlled by a linker feature switch (which is active in this repro). I mention this in case there is an issue with ILLinker only trimming source in the else
branch, rather than also trimming all code that it references.
I see that the type is annotated with DebuggerDisplay
attribute. We currently keep the entire type whenever the attribute's value is not understood.
https://github.com/mono/linker/blob/228dc42248bb56fc0897aa756236a6e79e2690a7/src/linker/Linker.Steps/MarkStep.cs#L1940
@layomia - can you try publishing with -p:DebuggerSupport=false
? That should trim the DebuggerDisplay
attribute.
Should this be a warning?
@mateoatr - at least we should put it in the linker dependencies file so someone can use the analyzer to see why the member was being preserved.
It currently doesn't show up in the trace because of this logic that skips tracing of custom attribute instances: https://github.com/mono/linker/blob/228dc42248bb56fc0897aa756236a6e79e2690a7/src/linker/Linker/XmlDependencyRecorder.cs#L109-L111
A quick fix would be to log the attribute type instead of just "CustomAttribute", but that has other problems. The trace in this case becomes:
--- Method:System.Void System.Text.Json.Serialization.Metadata.JsonClassInfo::.ctor(System.Type,System.Text.Json.JsonSerializerOptions) dependencies ---
Dependency #1
Method:System.Void System.Text.Json.Serialization.Metadata.JsonClassInfo::.ctor(System.Type,System.Text.Json.JsonSerializerOptions)
| Attribute:System.Diagnostics.DebuggerDisplayAttribute [43 deps]
| TypeDef:System.Text.Json.Serialization.Metadata.JsonClassInfo [106 deps]
| TypeDef:System.Text.Json.Serialization.Metadata.JsonTypeInfo`1 [14 deps]
| TypeSpec:System.Text.Json.Serialization.Metadata.JsonTypeInfo`1<Runner.MyClass0> [5 deps]
| Field:System.Text.Json.Serialization.Metadata.JsonTypeInfo`1<Runner.MyClass0> Runner.JsonSourceGeneration.JsonContext/MyClass0TypeInfo::<TypeInfo>k__BackingField [3 deps]
| TypeDef:Runner.JsonSourceGeneration.JsonContext/MyClass0TypeInfo:Runner.dll [9 deps]
| TypeDef:Runner.JsonSourceGeneration.JsonContext/MyClass0TypeInfo/<>c:Runner.dll [20 deps]
| Field:Runner.JsonSourceGeneration.JsonContext/MyClass0TypeInfo/<>c Runner.JsonSourceGeneration.JsonContext/MyClass0TypeInfo/<>c::<>9 [3 deps]
| Method:System.Void Runner.JsonSourceGeneration.JsonContext/MyClass0TypeInfo/<>c::.cctor() [2 deps]
warning: loop to TypeDef:Runner.JsonSourceGeneration.JsonContext/MyClass0TypeInfo/<>c:Runner.dll
Notice the DebuggerDisplayAttribute [43 deps]
- there were 43 things with DebuggerDisplayAttribute
and output just happened to pick the right one (JsonClassInfo).
To log attributes in general I think we'd need to figure out how to avoid unifying the same attribute instance on different members. For example:
[MyAttribute(typeof(C))]
class A {}
[MyAttribute(typeof(D))]
class B {}
class C {}
class D {}
This should produce a graph like:
A -> MyAttribute_instance1 -> C
B -> MyAttribute_instance2 -> D
Rather than:
A -> MyAttribute -> C
^ |
B --/ \-> D
@sbomer Related, why don't we add the reason why a member was marked in the trace (i.e., add DependencyKind
to the traced members)?
@layomia - can you try publishing with -p:DebuggerSupport=false? That should trim the DebuggerDisplay attribute
@eerhardt, yes - when publishing with -p:DebuggerSupport=false
the attribute was trimmed, along with the JsonClassInfo
constructor and other expected members. Looks like this property is used by default in a Blazor app which is helpful for size goals for JSON and Blazor in .NET 6.
It would be nice for DebuggerDisplay
to show up in the trace as a reason types are rooted.
Additionally, should DebuggerDisplay
even root the constructor? I don't think the contents of the DebuggerDisplay string would ever result in calling the constructor, just members: right?
Additionally, should DebuggerDisplay even root the constructor?
I think it's possible to call the ctor from DebuggerDisplay
, i.e.: DebuggerDisplay["Print whatever comes from {new Foo()}"]
(but then, you can actually call any method from within the attribute, so we'd actually need to keep everything whenever we don't understand the argument). I just opened #1873. I think we should stop keeping everything and also warn.
The vertex in question,
"Method:System.Void System.Text.Json.Serialization.Metadata.JsonClassInfo::.ctor(System.Type,System.Text.Json.JsonSerializerOptions)"
, is not used anywhere in the app. There's also no descriptor file that roots this ctor.I've attached the linker dependency zip.
Repro steps:
"https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental/nuget/v3/index.json"
as a NuGet feed, to get the updated System.Text.Json.dll and source generator.dotnet publish -c Release
illinkanalyzer -r "Method:System.Void System.Text.Json.Serialization.Metadata.JsonClassInfo::.ctor(System.Type,System.Text.Json.JsonSerializerOptions)" linker-dependencies.xml.gz
You'll find that the ctor is not a dependency of any other vertex, yet it is preserved after trimming.
Here's the linker command:
cc @sbomer @agocke @mateoatr @vitek-karas @eerhardt
linker-dependencies.xml.gz