Detect correctly multitargeted apps for trim/AOT/single-file warnings

[sbomer]

The warnings introduced by #34077 and described in https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/trimming-unsupported-targetframework are unnecessary noise for projects that multitarget to include a TargetFramework that is compatible with trimming/AOT/single-file, and is a low enough version to ensure that it will be picked over any other TFM's assets when the library is consumed in a trimmed/AOT'd/single-file app.

This fixes the issue by detecting correctly multi-targeted libraries and suppressing the warnings in these cases.

Note that prior to the .NET 8 SDK, netstandard libraries would still get the IsTrimmable attribute embedded in the assembly when setting IsTrimmable, and could use trim analysis despite incomplete warnings due to unannotated ref assemblies. With the .NET 8 SDK this is no longer the case. Even for correctly multi-targeted libraries, the netstandard output assembly will no longer contain the IsTrimmable attribute. We debated whether it was worth warning in this case, but decided that the negative impact of the warning on library developers following the "golden path" was too large to justify keeping this as a warning.

Fixes #35528

[dsplaisted]

Unfortunately, we want to avoid parsing other TargetFrameworks values for multi-targeted projects. The reason is we want to support TargetFramework aliases (see also here), and this kind of check would not correctly handle the aliases.

You could disable the warning when the project is multi-targeted at all without trying to parse the other TargetFramework values. I think we've done something similar in other situations.

[sbomer]

Interesting, thanks for the pointers. Still trying to understand this - doesn't the pattern of using IsTargetFrameworkCompatible with $(TargetFramework) already have the problem, since it (I assume) wouldn't handle TargetFramework values that are just aliases?

[vitek-karas]

Personally I'd be fine if this didn't work with the aliases. This is not a correctness feature, it's a convenience feature. So if it works in majority of scenarios, it's good enough. If there's a typical "golden path" way which works, I think that would be good enough for this.

[dsplaisted]

Interesting, thanks for the pointers. Still trying to understand this - doesn't the pattern of using IsTargetFrameworkCompatible with $(TargetFramework) already have the problem, since it (I assume) wouldn't handle TargetFramework values that are just aliases?

Yes, we shouldn't be using IsTargetFrameworkCompatible in generic code. We should be using the TargetFrameworkIdentifier and TargetFrameworkVersion properties instead, for example: '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0)). IsTargetFrameworkCompatible is OK to use in project files where you're in control of the list of TargetFrameworks and you know you're not using aliases.

Personally I'd be fine if this didn't work with the aliases. This is not a correctness feature, it's a convenience feature. So if it works in majority of scenarios, it's good enough. If there's a typical "golden path" way which works, I think that would be good enough for this.

In projects where it doesn't work due to aliasing, wouldn't you always get the warnings?

[sbomer]

Yes, we shouldn't be using IsTargetFrameworkCompatible in generic code.

Makes sense, I understand the reasoning. We'll need to decide somehow whether this use is justified given that it won't work for aliases.

In projects where it doesn't work due to aliasing, wouldn't you always get the warnings?

I believe this change doesn't make the behavior any worse for projects with aliases. For unsupported TFMs, you'd get the normal warning (the new logic won't detect aliases and so won't silence the warnings). If the TFMs are are all supported (as detected through TargetFrameworkIdentifier/TargetFrameworkVersion, there will be no warnings to silence in the first place.

I'll try to confirm with some new testcases.

[dsplaisted]

I believe this change doesn't make the behavior any worse for projects with aliases. For unsupported TFMs, you'd get the normal warning (the new logic won't detect aliases and so won't silence the warnings). If the TFMs are are all supported (as detected through TargetFrameworkIdentifier/TargetFrameworkVersion, there will be no warnings to silence in the first place.

OK, that makes sense.

[vitek-karas]

Looks good to me.

[sbomer]

@dsplaisted I think this is ready to merge if you are on board with the approach. PTAL.

[dsplaisted]

I'm OK with this in general, but see my comment about the complexity of understanding it and consider whether it's worth simplifying.

[dsplaisted]

I think I understand this now but it took me forever to wrap my head around. Is this complexity going to surface to developers? Is it worth doing so when it may be confusing, or would it be better to just ignore the "gap" and suppress the warning if any of the target frameworks supports the functionality?

[sbomer]

I share your concern about the complexity - it would be super confusing to a user who is trying to figure out under which circumstances exactly the warning is produced. The intention is that users following our guidance won't need to worry about this and the warning should just go away.

Our team discussed at length whether this was worth doing before landing on this solution. The complexity solves a correctness problem that we wanted to avoid (the correctness problem: allowing an inadvertently non-"trimmable" netstandard asset to be consumed in a trimmed app that uses a supported TFM).

It's still possible to get into that situation in an app that explicitly references the netstandard assembly, or that targets an EOL TFM (causing it to consume the netstandard asset from a library that multitargets netstandard2.0;net6.0 for example).