Unnecessary warnings for DiagnosticSourceEventSource

[eerhardt]

dotnet publish the following application:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <RuntimeIdentifier>win-x64</RuntimeIdentifier>
    <PublishTrimmed>true</PublishTrimmed>
  </PropertyGroup>

</Project>

using System;
using System.Xml;

class Program
{
    static void Main(string[] args)
    {
        XmlDocument doc = new();
        doc.LoadXml("<hello />");
        Console.WriteLine(doc.InnerXml);
    }
}

Expected results

There shouldn't be any ILLink warnings, since I am not using code that is unsafe.

Actual results

ILLink : Trim analysis warning IL2026: System.Diagnostics.DiagnosticSourceEventSource: Using method 'System.Diagnostics.DiagnosticSourceEventSource.TransformSpec.Morph(Object)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. The type of object being written to DiagnosticSource cannot be discovered statically. [C:\DotNetTest\HelloWorld\HelloWorld.csproj]
ILLink : Trim analysis warning IL2026: System.Diagnostics.DiagnosticSourceEventSource: Using method 'System.Diagnostics.DiagnosticSourceEventSource.TransformSpec.PropertySpec.Fetch(Object)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. The type of object being written to DiagnosticSource cannot be discovered statically. [C:\DotNetTest\HelloWorld\HelloWorld.csproj]
ILLink : Trim analysis warning IL2026: System.Diagnostics.DiagnosticSourceEventSource: Using method 'System.Diagnostics.DiagnosticSourceEventSource.FilterAndTransform.Morph(Object)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. The type of object being written to DiagnosticSource cannot be discovered statically. [C:\DotNetTest\HelloWorld\HelloWorld.csproj]
ILLink : Trim analysis warning IL2026: System.Diagnostics.DiagnosticSourceEventSource: Using method 'System.Diagnostics.DiagnosticSourceEventSource.FilterAndTransform.MakeImplicitTransforms(Type)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. The type of object being written to DiagnosticSource cannot be discovered statically. [C:\DotNetTest\HelloWorld\HelloWorld.csproj]
ILLink : Trim analysis warning IL2026: System.Diagnostics.DiagnosticSourceEventSource: Using method 'System.Diagnostics.DiagnosticSourceEventSource.TransformSpec.PropertySpec.PropertyFetch.FetcherForProperty(Type,String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. The type of object being written to DiagnosticSource cannot be discovered statically. [C:\DotNetTest\HelloWorld\HelloWorld.csproj]

Notes

The APIs that are getting warned about all either have RequiresUnreferencedCodeAttribute or UnconditionalSuppressMessage, so I don't understand why the linker is warning here. All those methods are internal implementation details of DiagnosticSourceEventSource. See the code here:

https://github.com/dotnet/runtime/blob/2be4fdc81f7f190ac35646ccc18799deb05da7dd/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs#L650-L661

As far as I can tell, DiagnosticSource is referenced by System.Net.Http, which is referenced by XmlDownloadManager. However, when I used HttpClient by itself in an app, I couldn't get the warnings to show up.

cc @vitek-karas @sbomer @marek-safar

[sbomer]

The warnings are being produced because we keep all members of DiagnosticSourceEventSource when we see a reference to DiagnosticSourceEventSource, because the base type EventSource has DynamicallyAccessedMemberTypes.All (and there was a this.GetType()). The reference to DiagnosticSourceEventSource seems to from the DiagnosticListener ctor which touches the Logger member:

	| Field:System.Diagnostics.DiagnosticSourceEventSource System.Diagnostics.DiagnosticSourceEventSource::Logger [6 deps]
	| Method:System.Void System.Diagnostics.DiagnosticListener::.ctor(System.String) [1 deps]
	| Method:System.Void System.Net.Http.DiagnosticsHandler::.cctor() [1 deps]
	| Method:System.Boolean System.Net.Http.DiagnosticsHandler::get_IsGloballyEnabled() [1 deps]
	| Method:System.Void System.Net.Http.HttpClientHandler::.ctor() [2 deps]
	| Method:System.Void System.Xml.XmlDownloadManager/<GetNonFileStreamAsync>d__2::MoveNext() [1 deps]
	| TypeDef:System.Xml.XmlDownloadManager/<GetNonFileStreamAsync>d__2:System.Private.Xml.dll [11 deps]
	| Method:System.Threading.Tasks.Task`1<System.IO.Stream> System.Xml.XmlDownloadManager::GetNonFileStreamAsync(System.Uri,System.Net.ICredentials,System.Net.IWebProxy) [2 deps]
	| Method:System.IO.Stream System.Xml.XmlDownloadManager::GetStream(System.Uri,System.Net.ICredentials,System.Net.IWebProxy) [1 deps]
	| Method:System.Object System.Xml.XmlUrlResolver::GetEntity(System.Uri,System.String,System.Type) [1 deps]
	| TypeDef:System.Xml.XmlUrlResolver [11 deps]
	| Method:System.Void System.Xml.XmlUrlResolver::.ctor() [2 deps]
	| Method:System.Xml.XmlResolver System.Xml.XmlTextReaderImpl::GetTempResolver() [1 deps]
	| Method:System.Void System.Xml.XmlTextReaderImpl::OpenUrl() [1 deps]
	| Method:System.Boolean System.Xml.XmlTextReaderImpl::Read() [1 deps]
	| TypeDef:System.Xml.XmlTextReaderImpl:System.Private.Xml.dll [323 deps]
	| Field:System.Xml.XmlTextReaderImpl System.Xml.XmlTextReader::_impl [29 deps]
	| Method:System.Void System.Xml.XmlTextReader::.ctor(System.IO.TextReader,System.Xml.XmlNameTable) [1 deps]
	| Method:System.Void System.Xml.XmlDocument::LoadXml(System.String) [2 deps]
	| Method:System.Void ds.Program::Main(System.String[]) [1 deps]

[vitek-karas]

@LakshanF for context - another issue with EventSource marking.

This might be fixed once we switch from using All to a bit more specific. It seems we will need something like PublicNestedTypes | PublicMethods | NonPublicMethods | PublicProperties | NonPublicProperties which will not cover the methods which are RUC in this case (as they are on a non-public nested type).

But honestly that's just luck. (That said, why does this class use so many nested types? It seems it would be cleaner to just make them into standalone classes... )

What I'm most worried about going forward is the fact that we will have to have NonPublicMethods - because for some reason EventSource allows private methods to define events... that will basically mark all methods on the type and if some of them has either RUC or DAM, it will produce a warning with no good way to suppress it (other than adding the suppression on the entire type).

[eerhardt]

@sbomer - shouldn't the warnings be suppressed by the UnconditionalSuppressMessage and RequiresUnreferencedCode attributes?

[eerhardt]

That said, why does this class use so many nested types?

Some people write code that way - all contained inside the class that uses it. It's a style/preference thing, I think.

[vitek-karas]

shouldn't the warnings be suppressed by the UnconditionalSuppressMessage and RequiresUnreferencedCode attributes?

The warnings are generated because the linker thinks that the RUC annotated methods are being accessed via reflection (due to the All annotation). And it's not that far off - if one of these RUC methods were on the main type directly, the code in EventSource will actually look at it (it would probably skip it, unless it was an actual event definition) via reflection.

The fact that the EventSource will never call any of these methods (actually any methods at all) via reflection is something linker currently doesn't track, so it has to assume the worst (which is that they will be called).

[vitek-karas]

I think there's another interesting question here which is why we didn't see these warnings in the runtime repo build. Not in the per-assembly runs (that's something we're working on currently and it's not supposed to work yet), but in the analysis run. That should have seen enough to raise these warnings...

[LakshanF]

@LakshanF for context - another issue with EventSource marking.

This might be fixed once we switch from using All to a bit more specific. It seems we will need something like PublicNestedTypes | PublicMethods | NonPublicMethods | PublicProperties | NonPublicProperties which will not cover the methods which are RUC in this case (as they are on a non-public nested type).

But honestly that's just luck. (That said, why does this class use so many nested types? It seems it would be cleaner to just make them into standalone classes... )

What I'm most worried about going forward is the fact that we will have to have NonPublicMethods - because for some reason EventSource allows private methods to define events... that will basically mark all methods on the type and if some of them has either RUC or DAM, it will produce a warning with no good way to suppress it (other than adding the suppression on the entire type).

Do we need the properties? Prior to the change to All to accommodate NestedTypes, we had only methods (and nested types) looking at the history.

[sbomer]

Interesting - the scope stack is different when we get to MarkEntireType for DiagnosticSourceEventSource.

• When linking runtime, the scope is

  System.Byte[] System.Diagnostics.Tracing.EventSource::<EnsureDescriptorsInitialized>g__GetCreateManifestAndDescriptorsViaLocalMethod|91_0(System.String)
  

  which has a GetType call and a suppression:
  https://github.com/dotnet/runtime/blob/fdbca22b23727ec640666d385b3c41ffe4b7b914/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs#L2817-L2821
• When linking the sample app, the scope is

  System.Diagnostics.DiagnosticSourceEventSource
  

  which has no suppression.

The issue seems to be that the DynamicallyAccessedMembers-on-type/GetType logic is one of those cases (similar to virtual overrides) where there are two "reasons" required for a thing to get marked - in this case the GetType call, and the derived type - so it's not clear who to "blame" for the warning. In this case it's order-dependent, which isn't good. The MarkEntireType logic will only run once for a type, so whatever reaches it first will determine whether a warning is shown.

Did we write down anywhere how DynamicallyAccessedMembers is supposed to behave on a type?

[eerhardt]

In this case it's order-dependent

That might solve another mystery for me. I tried paring down the repro to just using HttpClient directly in an app, and there I don't get any warnings, even though DiagnosticSourceEventSource is still in the app. So it must be evaluated in a different order for that app, and suppressing the warnings (just like during the dotnet/runtime build analysis).

        HttpClient client = new();
        Console.WriteLine(client.GetStringAsync("http://www.msn.com").Result);

That app produces no warnings.

[vitek-karas]

This is tricky :-(

We should definitely fix linker to be deterministic and order independent, but how is an interesting question.
Ideally we would raise these warnings on the object.GetType call, but that is impossible to do when in library mode. In library mode we need to mark these just based on the annotation alone - which is on a type.
(Note that this is not just library mode problem, if we wanted to support framework dependent apps one day, those will have exactly the same problem and will effectively run in something similar to library mode).

That means it would be better to raise the warning on the annotated type (in this case EventSource itself), but that is also weird since that type is in a different assembly (in this case it's a ref assembly). So maybe raising it on the affected type is the right thing to do (in this case that is the DiagnosticEventSource).

But that brings another problem which is that there's no good way to suppress the warning if necessary (like in this case). Putting the suppression on the type will work, but it will also suppress all such warnings in all methods of that type, it doesn't mean "just the type declaration".

This leads me to another possibility - currently we issue the same warning IL2026 when a RUC method is called as well as when it's accessed via reflection (either directly or indirectly through DAM annotations). If nothing else this is somewhat confusing since the warning message is rather vague (as it has to be). This is basically #2003. If we did change the warning for reflection-access to a new warning code (not just message), let's say IL2300 (as an example), it would make the broad type-level suppressions a bit easier to swallow. It would still potentially suppress the warning too much, but now the warning is much more targeted.

Note that for #1764 we will introduce a new warning (for example IL2301) which will be about reflection-accessed DAM annotated methods (and members). So having a new warning code for reflection-accessed RUC would make it consistent.

So in short, the idea is to:

• Change the warning scope when marking things due to type hierarchy annotations to always point to the type being marked (as is already the case in 50% of ordering scenarios).
• Change the warning code for reflection-accessed RUC methods to a new value and give a better warning message (fixes Use different warning code/message for "maybe-used" RequiresUnreferencedCode methods than "definitely used" RequiredUnreferencedCode methods #2003)
• In this case use type-level suppression if necessary - this is not ideal, but it's probably the best we can do.

The thinking behind this could also be that by deriving from EventSource the type needs to fulfill all requirements of EventSource - one of which is that methods on it will be accessed via reflection. In that way that type is to blame, not EventSource itself. This is similar to what happens with generics for example:

class Base<T> where T : new () {}
class Derived<T> : Base<T> {} // This warns since T does not have new() constraint - the derived type is blamed, since Base clearly stated its requirements.

Alternative:
Change the warning scope to the RUC annotated method itself - so in a way the method would warn about itself. This would obviously need a good warning message, but that's something we can work on. That would allow us to target the warning suppression better. It is still not ideal, since it would suppress the warning for the entire method body.
While better in terms of suppressions, I don't like this very much as the behavior would be very different for DAM annotation induced warnings and direct reflection warnings:

typeof(foo).GetMethod("RUCMethod");  // This should definitely warn on this line

typeof(foo).GetMethods(); // This could now warn on the foo.RUCMethod instead which is weird

foo a; a.GetType(); // This would warn on foo.RUCMethod;

[vitek-karas]

Actually one last alternative - have a special warning code just for this (type hierarchy marking induced warnings). That would make the suppression exact and would allow us to provide the best warning message. And having yet another warning code doesn't sound that bad.

[MichalStrehovsky]

Actually one last alternative - have a special warning code just for this (type hierarchy marking induced warnings). That would make the suppression exact and would allow us to provide the best warning message. And having yet another warning code doesn't sound that bad.

This sounds like a pretty clean solution!

[vitek-karas]

Based on suggestion from @sbomer - one other solution:

• New warning code
• Warning is raised on the RUC method itself

So the behavior would be something like:

[DynamicallyAccessedMembers(PublicMethods | PublicFields)]
class Base {}

class Derived : Base
{
    // IL2300: Type `Derived` or one of its base types is annotated with `DynamicallyAccessedMembers` attribute which
    // instructs the trimmer to keep member `DangerousMethod()`. This member is potentially incompatible with trimming 
    // and is annotated with `RequiresUnreferencedCode`: Dangerous.
    [RequiresUnreferencedCode("Dangerous")]
    public void DangerousMethod() {}

    // IL3201: Type `Derived` or one of its base types is annotated with `DynamicallyAccessedMembers` attribute which
    // instructs the trimmer to keep member `AnnotatedTypeField` as it might be accessed via reflection. The member is
    // itself annotated with `DynamicallyAccessedMembers` attribute and the trimmer can't guarantee all the requirements 
    // when the member is accessed via reflection.
    [DynamicallyAccessedMembers(PublicProperties)]
    public Type AnnotatedTypeField;
}

This looks pretty good from a UX perspective. It also makes suppressing these straightforward and very precise - just add the correct UnconditionalSuppressMessage onto the affected member.
Note: We will have to try if trimmer can correctly handle the attribute on a field (or property).

[vitek-karas]

In case it wasn't clear - I like this last solution the best - raise the warning on the RUC method itself with new warning code.

[sbomer]

Why should the second scenario warn (on AnnotatedTypeField)? I would have thought that keeping the field due to the PublicFields on the base type was fine - and if someone tried to violate the field's annotations dynamically we should already produce warnings elsewhere. For example:

class Derived {
  void M() {
    var f = this.GetType().GetField("AnnotatedTypeField"); // fine due to the annotation
    f.SetValue(null, typeof(Foo)); // dynamic assignment violates the annotation
    f.GetValue(null).GetMethod("SomeMethod"); // should already warn because we don't know the type
  }
}

edit: I guess the concern is something like this:

void M() {
  var m = this.GetType().GetMethod("MethodWithDAMTParameter");
  m.Invoke(null, new object[] {typeof(Foo)}); // violates the annotation
}

but I think we can hit that today without type hierarchy marking. To fix that, I think dynamically invoking a method should produce a warning for pretty much the same reason that MakeGenericType does - we can't validate annotations dynamically.

I see that we can get there for the field example too, if instead of f.GetValue.GetMethod the method does AnnotatedTypeField.GetMethod - so the SetValue should probably produce a warning.

[sbomer]

Never mind - sounds like the plan is to warn on the initial GetField or GetMethod if the input type has RUC or DAMT annotated members, dicussed in #1764.

[sbomer]

How do we want this to work for derived types which are annotated?

class Base {
  [RUC]
  public static void RUC() {}
}

[DAMT.PublicMethods]
class AnnotatedDerived : Base { }

Here if the warning originates on Base.RUC, that would let derived types introduce warnings in the code of base types which may be in a different library. The only thing that makes sense to me here is for the warning to originate on the derived type - or to disallow DAMT on derived types altogether. Thoughts?