Add `IsTrimmable` property for libraries

[agocke]

Two things we want to get out of this:

• Enable feature switch defaults
• Define IsTrimmable attribute in generated sources

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[sbomer]

I'm adding an IsTrimmable property in #17607.

For now this isn't setting any feature switches. There are two opposing arguments:

• For setting feature switches:

  • Analyzing a trimmable library shouldn't produce warnings for framework features that are normally disabled when trimming, like startup hook.

    This isn't yet a real issue since we don't analyze framework implementations during build - and for features like startup hook which are provided as an internal part of the framework, I'd argue that the results of library trim analysis should never depend on whether the feature is enabled, so there may be a better long-term solution (possibly relevant to dotnet/linker#2004).

• Against setting feature switches:

  • Building a trimmable library shouldn't assume certain values for feature settings since the library could be used in an app with or without those settings.

    This also isn't yet a real issue yet since the default feature switches (StartupHookSupport, CustomResourceTypesSupport, EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization, BuiltInComSupport) are all private or internal, so the library code won't be able to depend on the settings.

To decide, I would think about a hypothetical public switch that is set by default when trimming. It seems like a library shouldn't assume a certain value for such a switch, because an "IsTrimmable" library is still free to use trim-disabled features as long as it is properly annotated, and we want to check the annotations even in code that would be dead with the feature settings.

Anyway, I think there is more design work to be done around the build-time UX for feature switches, so in the meantime I'd like to add IsTrimmable without trying to fully solve this problem, and I don't think the decision is forced on us yet.

[sbomer]

Separately, @vitek-karas It looks like PublishTrimmed doesn't block building an app (not sure if something changed recently), so I am punting on defining a separate way to declare intent for apps without actually setting PublishTrimmed.

I would like to avoid subtly changing the meaning of IsTrimmable for apps/libs, and I am not sure the scenario justifies adding a new property. People who really need to set the feature switches during build can do so manually, or they could continue using PublishTrimmed, and just set it to false explicitly at publish time. We still have the option to add it later (add a new property, or change IsTrimmable so that it sets feature switches when building an app).

[vitek-karas]

Re feature switches: Properly designed and implemented feature switch should not cause more warnings in libraries. Let's assume there's an API DoSomethingWild which is annotated with RUC. And it has a feature switch which disables WildFeature in the framework. Calling DoSomethingWild is problematic regardless if the feature switch is on or off:

• If the feature switch is on - it will not work when trimmed -> produces a warning that trimming is problematic
• If the feature switch is off - the API should typically throw -> producing a warning when trimming is not ideal, but it doesn't hurt (the API won't do what it's supposed to do anyway).

This is for public feature switches. For internal feature switches (like startup hooks and so on), this is even simpler. There should be no way for the library to trigger the relevant code in the framework directly, so it should not get any warnings. Once built within an app, the behavior is entirely based on the app's config.

In the future the only place where this might cause trouble is if we provide the full library mode for linker - that is running linker on a library with framework implementation assemblies. That would require correctly setting feature switches. But we don't have that yet.

Re IsTrimmable on apps: I just tried with PublishTrimmed on an app - it works perfectly. So I don't have any problem at all with the proposed behavior. In the future we may decide to support IsTrimmable on the app as well, but it would effectively behave in the same way as on libs - add the attribute to the app's assembly.

[sbomer]

Properly designed and implemented feature switch should not cause more warnings in libraries.

I think I agree, if you don't consider constant propagation - whether features are enabled or disabled, there should be warnings for unannotated callsites to the RUC method. But I'm concerned about constant prop (thinking ahead to a world where either we have a linker full library mode, or the Roslyn analyzer matches the linker's analysis).

If the feature switch is on - it will not work when trimmed -> produces a warning that trimming is problematic

What I had in mind was something like this:

void DoLibraryStuff() {
  if (!WildFeature.IsEnabled)
    UseRUC(); // Linker wouldn't warn because it's in a removed branch
}

[RUC]
void UseRUC() {}

For the library to be correctly annotated it should warn (it may depend whether RUC on UseRUC is related to the same feature or a different one).

If the feature switch is off - the API should typically throw -> producing a warning when trimming is not ideal, but it doesn't hurt (the API won't do what it's supposed to do anyway).

I think warning is the right thing to do for unguarded calls to DoSomethingWild, in case the library doesn't handle the failure case (exception). For guarded calls, the linker wouldn't warn (if the feature switch is off):

if (WildFeature.IsEnabled)
  DoSomethingWild(); // Again no linker warning, this time it makes sense

I actually think not warning here is a good UX, but we shouldn't do it by setting the feature switches at build time if possible.

[agocke]

I don't really have an opinion here yet -- I thought about it a bit and it just sounded like a tough problem. 😊

But I do think it's important to think ahead to library trimming. I'd hate for us to make a choice which does the opposite of what a user would expect in those situations.