Opaque warnings for unsupported features without managed callers

[sbomer]

By design, turning on startup hook support produces a warning. However, the warning shown by default is not helpful.

First, it is collapsed: (fixed by #2087)

dotnet publish -r linux-x64 -p:PublishTrimmed=true -p:StartupHookSupport=true

...System.Private.CoreLib.dll : warning IL2104: Assembly 'System.Private.CoreLib' produced trim warnings. For more information see https://aka.ms/dotnet-illink/libraries

If you uncollapse it, the warning mentions implementation details of the startup hook:

dotnet publish -r linux-x64 -p:PublishTrimmed=true -p:StartupHookSupport=true -p:TrimmerSingleWarn=false

ILLink : Trim analysis warning IL2026: System.StartupHookProvider.ProcessStartupHooks(): Using method 'System.StartupHookProvider.CallStartupHook(StartupHookProvider.StartupHookNameOrPath)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. The StartupHookSupport feature switch has been enabled for this app which is being trimmed. Startup hook code is not observable by the trimmer and so required assemblies, types and members may be removed.

Only the last part is useful to the app developer. Note that there is a dependency from the XML descriptor -> ProcessStartupHook -> [RUC] CallStartupHook, and the warning is produced for the call to CallStartupHook.

I believe we need a way to mark some APIs such that they:

• produce warnings that "blame" the feature switch settings, even if they are never called by any other methods (only rooted due to the feature setting)
• do not look like they are warning about implementation details of corelib
• don't get collapsed by default

For startup hook, I think it should only say "The StartupHookSupport feature switch has been enabled for this app which is being trimmed. Startup hook code is not observable by the trimmer and so required assemblies, types and members may be removed."

Startup hooks happen to not have public APIs, but the native hosting APIs will have a similar problem, and they do have public UnmanagedCallersOnly APIs. The current approach will show warnings like "public API calls internal method with RequiresUnreferencedCode" - it would be better not to mention the internal APIs.

@LakshanF

[vitek-karas]

This is related to a request from @eerhardt which is to allow RequiresUnreferencedCode to specify the warning code, so that features (as in groups of APIs) can use a new warning which can be suppressed globally. For example - I just want to suppress XML serialization warnings across my codebase. (I was there was an issue on this, but I can't find it now).

We could couple that with such warnings being visible "through" the single-warning mechanism. And we could also make them less about the "call" and more about the feature in the messages.

[sbomer]

Another problem with the current behavior is that the following program produces no trim warnings:

class Program {
    [RequiresUnreferencedCode("")]
    public static void Main() {
        // do something with Reflection
    }
}

I think this should warn, but at the same time we probably shouldn't warn when annotating public APIs of a library.

[vitek-karas]

Technically you're right, but I won't loose any sleep if we don't 😉

[eerhardt]

This is related to a request from @eerhardt

I don't think I ever officially wrote it up. Just discussed offline with you.

[MichalStrehovsky]

How would people feel if we were to disable warnings collapsing if the IsTrimmable metadata is set on a library? The reasoning is that if someone sets IsTrimmable, they reviewed/annotated the library. Any leftover warnings are either a library bug, or intentional places like this one.

Or have another attribute that says "collapsing warnings from this library would not serve the user".

[marek-safar]

Any leftover warnings are either a library bug, or intentional places like this one.

What about the case where the library is used with runtime/SDK/dependency version which had a fix that introduced a new warning?

[sbomer]

The way I think about collapsing, it is designed to prevent warnings which "come from" implementation details of a library. Startup hook logically isn't an implementation detail - it's a feature provided by the library with a user-facing option to enable it - and so the warning shouldn't "come from" the library internals. If we can find a way to express this, the no-collapse behavior would fall out of it.

One way would be to put the warning in the SDK - if PublishTrimmed == true and StartupHookSupport == true, produce a warning. Then it is individually suppressible, and isn't collapsed. But I don't like this solution because it isn't tied to the attribute at all.

Also, when I suggested tying warning collapsing to "IsTrimmable" a while ago, one concern was that trimming everything would produce undesired warnings. Collapsing them for IsTrimmable assembly metadata but not for the IsTrimmable MSBuild metadata would mean that the two have subtly different behavior.

[vitek-karas]

The SDK solution is problematic. For startup hook it might work, but what if it's something which the app doesn't use - then it should not get a warning (like ResourceManager warnings for example).

[sbomer]

For features which are activated by calls from managed code, this isn't a problem to begin with since we just "blame" the caller, if I'm not missing something. Bubbling up RUC or DAM to the public API will prevent warnings from the implementing library. If the app calls it the warning won't be collapsed, and if it doesn't, there won't be any warning.

[vitek-karas]

True - unless it's hidden behind some other API - for example:

• BinaryFormatter has a feature switch on it (and RUC attributes)
• ResourceManager uses BinaryFormater - but we mostly removed the RUC because of the feature switch

If you turn on BinaryFormatter - you will get a warning - but from inside ResourceManager - not from the public API surface.
And currently that warning will be collapsed as if it's an internal problem of the library. In this sense it's basically the same thing as startup hooks. The difference is, that if my app doesn't use ResourceManager - then it should not matter if BinaryFormatter is on or off - it should not be in the app at all (unlike startup hooks).

[eerhardt]

If you turn on BinaryFormatter - you will get a warning - but from inside ResourceManager - not from the public API surface

This is technically not true. We introduced a separate feature switch in ResourceManager (System.Resources.ResourceManager.AllowCustomResourceTypes), which if you enable it you'll get a warning. Same for TypeConverter's usage of BinaryFormatter. Just enabling BinaryFormatter won't give you a warning inside those libraries because of these extra feature switches.

BinaryFormatter's API is marked as RUC, so if we didn't have these extra feature switches, these libraries would still be warning, even though BinaryFormatter is turned off.

[eerhardt]

Coming back to this issue, I agree something needs to be changed with the current "collapsing" behavior. We have a few warnings left in the dotnet/runtime libraries (in files named ILLink.Suppressions.LibraryBuild.xml) that we expect user's to see if they are using the functionality. And some of them aren't just "feature switch" based either.

One example I just ran into is in System.Text.Json where it integrates with dynamic. The best approach I can come up with is to emit a warning telling users that using JsonObject with dynamic is unsafe for trimming. I want the user to see the warning and not be hidden. But there isn't a direct API I can mark as RUC because the developer doesn't directly call the methods, instead they use dynamic and the compiler generates code that uses Microsoft.CSharp Binder, which calls the System.Text.Json code.

So in this case, I don't want the warning to be collapsed and saying Assembly 'System.Text.Json' produced trim warnings.. I want it to say the message I put into the code to say that using JsonObject and dynamic is unsafe.

[MichalStrehovsky]

One example I just ran into is in System.Text.Json where it integrates with dynamic. The best approach I can come up with is to emit a warning telling users that using JsonObject with dynamic is unsafe for trimming. I want the user to see the warning and not be hidden.

I'm a bit worried about actionability of the warning - "hey, you're using JsonObject with dynamic somewhere and it could be a problem with trimming, but I can't tell you where".

I can't think of a good solution - modulo just annotating the API in Microsoft.CSharp, because all those dynamic-support APIs are dead to me.

[eerhardt]

I'm a bit worried about actionability of the warning - "hey, you're using JsonObject with dynamic somewhere and it could be a problem with trimming, but I can't tell you where".

That's (roughly) the same experience of the original warning in this issue - StartupHookProvider. The warning can't tell you where a startup hook might break with a trimmed app - just that using it with trimming doesn't always work.

I can't think of a good solution - modulo just annotating the API in Microsoft.CSharp

Agreed that the developer will get warnings about all dynamic usages in their app when they trim. But I don't want to rely on the Microsoft.CSharp warnings in the off chance that someone casts a JsonObject to a IDynamicMetaObjectProvider and starts calling methods directly on it. Even in that scenario, the user's app may break.

[MichalStrehovsky]

That's (roughly) the same experience of the original warning in this issue - StartupHookProvider. The warning can't tell you where a startup hook might break with a trimmed app - just that using it with trimming doesn't always work.

That one has a more natural place to issue the warning like Sven suggested above - the SDK. It's where we decide startup hook support should be disabled when trimming and where we could warn if someone still turns it on. The fix can be spelled out in the warning message (just don't enable it in your csproj).

[sbomer]

I took a closer look at how we handled this for resources and now I understand @vitek-karas' point that it's not so different from the StartupHook case. I personally find this mind-boggling and would like to solve it differently. 😄

Let's say we have a feature Feature which requires unreferenced code (and is disabled by default when trimming), and an API Library which optionally uses Feature in some code path. This is a bit like ResourceReader's use of BinaryFormatter (but not quite - I initially wrote down some more details about ResourceReader and feature patterns in case anyone's interested, but I'm trying to keep this short).

void Library() {
    LibraryImpl();
    if (Feature.IsSupported)
        UseFeature(); // warns
}

[RUC("Feature")]
void UseFeature() {}

Is it ok to suppress this warning with UnconditionalSuppressMessage? Branch elimination means the linker won't warn when Feature.IsSupported == false, but it will still warn when Feature.IsSupported == true.

I would argue yes - the author of Library has gone to the trouble to guard calls to UseFeature, and Library may do useful work even without Feature. If the app author enables Feature, that will cause Library to take the "unsafe" path for Feature, but we shouldn't blame Library for this; we should blame the app author for turning on Feature which is unsupported when trimming.

If we emit a warning that "blames" the library, it will look like the library author hasn't done the work to annotate their library, when in fact the library is perfectly "safe" with the default linker settings.

So I see two rough directions we could go for solving this:

1. Stick with the model we use today: recommend that Library doesn't suppress the warning, and keep it in the product in case Feature.IsSupported is true at link time.

   Then we need to design the warnings so they don't look like they "blame" Library internals. The issue isn't necessarily that Library hasn't been annotated - it's that with the current feature settings, library has some (maybe 100% correctly annotated) code which depends on a trim-unfriendly feature. This would need to look a little different than the warnings we produce while annotating user code.

   Maybe we could do this: Give RUC a "FeatureName" member or similar. When collapsing warnings, collect the warnings that are generated for calls to RUC methods. Discard the "origin" that's internal to the library, just remember the originating library and the RUC. Then deduplicate them so we have one per (library, feature name), and output warnings like this (when the feature is enabled):

   "warning: Library.dll: using feature "feature name" which requires unreferenced code.

   One issue with this is that the warning doesn't distinguish between properly annotated libraries that have optional dependencies on RUC features, and unannotated (or incorrectly unannotated) ones. Maybe longer-term we could treat the guarded calls differently than the unguarded ones - so that the unguarded calls produce different warnings, like "Library.dll produced trim analysis warnings".

   I would also question whether mentioning "Library.dll" here is actually useful. If multiple libraries use the same feature, you'd get a warning about each library, when really the only useful bit of information is that you should turn off the feature in your .csproj. We could go further and deduplicate them to have one warning per feature name.

2. Recommend that Library suppress the warning. (In the future we could automatically suppress warnings for calls that are guarded by feature checks.) This means that fully-annotated libraries will never produce linker warnings with any combination of feature settings (or lack thereof).

   Instead, we need to separately encode the information about which features are trim-incompatible, maybe in the SDK. Then this information is used to generate warnings when any trim-incompatible features are enabled while trimming.

   The downside is that for annotated libraries, we lose information about which ones had an optional dependency on the RUC feature. However, unannotated libraries would still produce the warnings about missing annotations (we could combine it with 1. when collapsing, to avoid mentioning implementation details).

I think going forward we will want to distinguish between guarded and unguarded calls to RUC methods, and it might be worth designing it so that it can be extended to support checking calls to any feature code (including features that don't require unreferenced code) as mentioned in #1723.

[eerhardt]

I don't really like option (2) above, mostly because:

1. The solution is limited to Feature switches. It can't be used for any other situation where a library needs to warn the app developer about something that may not work in a trimmed app.
2. It requires encoding this information into the SDK. But this kind of information feels like it better belongs in the library itself.

[sbomer]

The solution is limited to Feature switches. It can't be used for any other situation where a library needs to warn the app developer about something that may not work in a trimmed app.

I didn't think this was an issue unless you consider feature switches - it sounds like we are looking at the problem slightly differently. Is this an example of the situation you're concerned about?

class LibraryA {
    static void Method() {
        LibraryB.DoStuff();
    }
}

class LibraryB {
    [RUC("DoStuff is not compatible with trimming")]
    static void DoStuff() { }
}

When building an app that references LibraryA as a nuget package and calls Method, there will be a collapsed warning like

LibraryA.dll : warning IL2104: Assembly 'LibraryA' produced trim warnings...

I would consider that to be by design since we don't want to give detailed warnings for unannotated third-party libraries. If the app called LibraryB.DoStuff directly, it would not be collapsed.

It requires encoding this information into the SDK. But this kind of information feels like it better belongs in the library itself.

I agree it better belongs in the library. Thinking some more about it I actually don't think we need to put it in the SDK. We just need some way to tell which feature settings are trim-incompatible - all that's missing is a link between RUC and the feature switches. If we combine this with approach 1., say by giving RUC a FeatureName (and maybe which value is the "bad" setting), then the linker could:

• remember the set of features it encountered while marking RUC methods, and
• give a global warning for any that were enabled via feature settings
  • (or we could mention which libraries had a dependency on it if that seems useful - but in any case the warning shouldn't "blame" the library)

This way the warnings are also limited to the RUC code that's actually reachable from the app.

This would give the following behavior:

• guarded/annotated call to inactive feature:
  • no warning
• unguarded call to inactive feature:
  • warns because caller isn't annotated (feature would likely throw at runtime), or collapsed warning if caller is a third-party library
• guarded/annotated call to active feature:
  • warns that the RUC feature is active, but doesn't "blame" the library
• unguarded call to active feature:
  • warns that the RUC feature is active without "blaming" a particular library
  • additionally warns about the unannotated callsite, or collapsed warning if caller is a third-party library

[sbomer]

Note that this doesn't address the part of the issue about individually-suppressable warnings for different RUC attributes. It seems like that requires lifting the Roslyn restriction that warning codes are statically known (if we want to suppress them by warning code).

If we are ok with using a different suppression mechanism, we could add a linker-specific way to suppress warnings by feature name - or by subcategory as @mateoatr suggested - I just think the extra info we add to RUC attributes needs to specify the feature switch. This also wouldn't give libraries a way to suppress the feature switch warnings via attributes, but I think that makes sense because these warnings would be "global".

[eerhardt]

it sounds like we are looking at the problem slightly differently. Is this an example of the situation you're concerned about?

There are 2 concrete situations that I'm thinking about right now:

1. The Json situation I described above - which I'm implementing in Fully annotate JsonNode for trimmability runtime#53184.

2. There is one remaining ILLink warning in DependencyInjection, which we don't have a story for. The high-level scenario is that you can use DI to register an "open generic" service (services.AddSingleton<IMyService<>, MyService<>>()) where MyService<T> has a DynamicallyAccessedMembers attribute on the T, but IMyService<T> doesn't. When someone "closes" the generic (provider.GetService<IMyService<Customer>>()), the linker is unable to recognize that it needs to preserve the DynamicallyAccessedMembers members on Customer in order to use it with MyService<T>. We do get a warning deep in the DependencyInjection library. However, if we were to "bubble" this warning up as RequiresUnreferencedCode attributes on the public APIs, it basically means every usage of DependencyInjection will issue a warning.

   • The plan I was going with in my head for 6.0 was to leave this warning in the library, but with a really good message and probably a URL to a document that describes the situation and how to fix it (by ensuring the interface generic type's DAM matches all implementation type's DAM attributes). But this doesn't work if by default the linker makes the warning opaque and says "Assembly 'DependencyInjection' produced trim warnings."

[sbomer]

To distill the Json case, here's a simple example with the same issue (the warning will show up whenever IFoo.Method is kept and MyFoo is instantiated, even if these usages are unrelated to each other in the app):

public interface IFoo {
  void Method();
}

public class MyFoo : IFoo {
  public void Method() {
    RUC(); // warning
  }

  [RUC]
  void RUC() {}
}

There are a few options for dealing with the warning:

1. Annotate MyFoo's ctor and suppress the warning from Method

   Doesn't work when most uses of MyFoo don't hit the linker-unfriendly path (the case for JsonNode)

2. Annotate IFoo.Method and MyFoo.Method

   Bad because annotations on virtuals are "viral" (some implementations of IDynamicMetaObjectProvider.GetMetaObject are linker-friendly)

3. Leave the warning, give it a good message, and ensure it shows up

   Has the big (in my mind) problem that the attribute isn't present in the ref assembly - Roslyn analyzers won't be able to see it

4. Put the problematic behavior behind a feature switch (assuming MyFoo.Method does other useful work)

   Seems like it could work for the Json case. Why not do this? (This would then boil down to the cases I discussed above when the feature is enabled)

5. (?) Maybe another option is to put RUC directly on MyFoo.Method, and leave in the warning IL2046. This warns in the same situations as 3, but has the advantage that the attribute is visible in the ref assembly. We'd need to ensure that the message gets printed (maybe tweak the message for IsTrimmable assemblies?) and think about who to "blame". I have some reservations about this - I think we need a more principled approach to determine when the RUC message is shown.

What will we recommend to customers who hit this issue in their own code?

[MichalStrehovsky]

Thanks for writing these down Sven!

The problem is really that we don't have a good spot where to put the blame and what action should the user take if they hit the warning. There are some parallels with the ResourceReader support scenario, which we solved with approach 4 from Sven's list. That one was a piece of problematic code that is always included. We put it behind a feature switch and added RUC annotation that mentions the feature switch. The only problem with that approach is the warning collapsing if someone goes and turns the feature back on.

[vitek-karas]

The expectation for now is that this is relatively rare - meaning that there will be only a handful of RUC attributes for which we need "new behavior". So I don't see a big need to make this super easy to understand for somebody added the RUC attribute.
It should be relatively easy to understand for app developer seeing the warning, but that can be mitigated relatively easily by having a good warning message in the RUC itself.

I personally don't see it as a problem that these would not show up in Roslyn analyzer. All the cases I've seen so far as effectively global problems. Feature switches are by definition global (specified by the app, not any library) - even if the Roslyn analyzer for the app would see these, it would not have a good place to report the warning from. The case with the interfaces is also global by definition and Roslyn analyzer has no chance of detecting this (outside of the special case of all of it happening in one assembly).

Whichever solution we decide to use should:

• Never under-report from the linker - linker MUST show at least some warning (currently it does, just not a very good one)
• Can over-report from the linker - this is basically noise - we already have this (lot of our warnings are in the "may break the app" category, that's why they are warnings). The only requirement should be that the warning can be suppressed in such case - which is a bit tricky in this case.
• Can under-report from the analyzer - if the analyzer doesn't report the warning, it's OK - the linker will. There are other cases where the analyzer simply can't report the warning anyway.
• Never over-report from the analyzer - the analyzer on the other hand should not report warnings which the linker doesn't. That would lead to confusion.

My current favorite (but we would have to try it) is basically 5 from the above list. It is not a super robust solution, but it should work. I would rather try this with some reasonably simple solution and only when we see this becoming a bigger problem, try to design some robust, probably more explicit solution.

[eerhardt]

FYI for completeness - here's one more candidate where leaving a warning in the library might make sense:

dotnet/runtime#52046 (comment)

The scenario here is an implementation of ISerializable, so it is basically the same scenario as the JSON + dynamic scenario - we have a class that implements an interface in an trim-incompatible fashion, and don't want to mark the interface method as [RUC]. We also don't want to mark the ctors of the class as [RUC]. (Although, for this specific case, maybe marking ISerializable.GetObjectData as [RUC] may be the right path forward.)