[release/7.0] Limit impact of global NoWarn

sbomer commented 1 year ago

Customer Impact

This addresses a reported customer scenario where the global NoWarn breaks projects that use older tools. https://github.com/dotnet/linker/issues/3118

Testing

Local testing to confirm that this doesn't break trimmed apps. The change has also flowed to SDK in main, where all of the SDK tested scenarios have been validated.

Risk

Low risk. This doesn't change existing functionality at all - it only avoids setting an unnecessary property in scenarios where it is unused.

Fixes https://github.com/dotnet/linker/issues/3118. This limits the impact of the global NoWarn to scenarios where PublishTrimmed is true. It should help with older tools that aren't able to ignore NoWarn settings.

I also looked into making the whole target file import conditional on trim scenarios, but this isn't straightforward:

There are comments in there which say that they need to be imported unconditionally: https://github.com/dotnet/linker/blob/a5cd06e1d38464b7b3aae0c4064ae726bf417de9/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets#L14-L15 We could probably clean this up since we have already been relying on PublishTrimmed being set early for the analyzer.
The targets have logic for PublishTrimmed, IsTrimmable, SuppressTrimAnalysisWarnings, and feature switches, so we would need to have a more complex condition for the import and/or move some of the logic elsewhere.

If the analyzer adds support for redundant suppression detection, the condition will need to be updated. I'm not trying to make the condition future-proof here because the redundant suppression feature isn't officially supported in the first place. If we add support for it, this should come with proper SDK testing.

marek-safar commented 1 year ago

Could you send tactics request for 7.0 and create PR targeting main.

agocke commented 1 year ago

@carlossanlop Is this clear to merge for 7.0.3?

dotnet / linker