Add nowarn option

[mateoatr]

This turns off the analysis warnings by default and adds the ability to turn off all warnings or a subset of them by specifying a warning "subcategory superset", this can be done either using the command line or the ILLink task.

A warning subcategory superset is a set of subcategories. The only such superset added in this PR is analysis, which contains the following subcategories: UnrecognizedReflectionPattern, DynamicDependency, PreserveDependency UnreferencedCode. As an example, by specifying --nowarn analysis, the user would turn off all warnings that have their subcategory set to any of the subcategories aforementioned.

Usage examples:

Turning off all warnings

--nowarn all

Turning off warnings using a subcategory superset

--nowarn analysis

Here's a table that summarizes all warnings that specify a subcategory:

Code	Message	Subcategory
2003	Could not resolve 'assembly' assembly dependency specified in a 'PreserveDependency' attribute that targets method 'method'	PreserveDependency
2004	Could not resolve 'type' type dependency specified in a 'PreserveDependency' attribute that targets method 'method'	PreserveDependency
2005	Could not resolve dependency member 'member' declared in type 'type' specified in a 'PreserveDependency' attribute that targets method 'method'	PreserveDependency
2006	-	UnrecognizedReflectionPattern
2026	Calling method annotated with RequiresUnreferencedCodeAttribute	UnreferencedCode
2033	PreserveDependencyAttribute is deprecated. Use DynamicDependencyAttribute instead.	PreserveDependency
2034	Invalid DynamicDependencyAttribute on 'member'	DynamicDependency
2035	Unresolved assembly 'assemblyName' in DynamicDependencyAttribute on 'member'	DynamicDependency
2036	Unresolved type 'typeName' in DynamicDependencyAttribute on 'member'	DynamicDependency
2037	No members were resolved for 'memberSignature/memberTypes'.	DynamicDependency
2041	DynamicallyAccessedMembersAttribute is only allowed on method parameters, return value or generic parameters.	DynamicDependency
2042	Could not find a unique backing field for property 'property' to propagate DynamicallyAccessedMembersAttribute	DynamicDependency
2043	Trying to propagate DynamicallyAccessedMemberAttribute from property 'property' to its getter 'method', but it already has such attribute.	DynamicDependency

---

I'd like some comments on whether or not these new subcategories make sense (DynamicDependency, PreserveDependency, UnreferencedCode) and if all warnings that use these should indeed be categorized as analysis warnings.

---

This PR also changes the logging behavior so that warnings and errors are always logged, no matter if the --verbose option isn't used.

/cc @eerhardt

[agocke]

Do we need to worry about localization?

[mateoatr]

We currently don't localize any message outputted by the linker. I sincerely don't know how useful that would be, but might be a good next step after polishing the current messages that we have (#1275).

[vitek-karas]

Not for .NET 5 - linker is not localized (at all). If we think it's important it would be a relatively non-trivial feature on its own (mostly because there's nothing in that space in linker, so we would have to introduce everything from scratch). So .NET 6 (if we think it's important).

[samsp-msft]

Are other parts of the tool chain localized? What happens for Csc.exe?

[agocke]

Yeah, csc.exe is fully localized. Visual Studio requires localization in all core languages as a shipping criteria, so it was a hard requirement for us. That doesn't sound like the case for the linker, so let's come back to that in the 6.0 timeframe as Vitek suggests.

[vitek-karas]

This PR also changes the logging behavior so that warnings and errors are always logged, no matter if the --verbose option isn't used.

This should be a separate PR - mostly because of the dotnet/runtime impact it will have. I would like us to do a dry PR into runtime with these changes somehow hacked in, to validate we're not going to break dotnet/runtime.

[vitek-karas]

--nowarn

I think we need a different command line name - csc has exactly the same switch, but it accepts warning numbers. Since we also have a concept of warning numbers, if we do introduce --nowarn it should behave the same as for csc.

So we need a different command line switch name for this functionality.

As for the SubCategories - I don't like introducing a new grouping. That is "analysis" maps to "UnrecognizedReflectionPattern", "UnreferencedCode" and "DynamicDependency". The current desire is to have something with which we can enable/disable "linker analysis for trim correctness" and going forward similarly "linker analysis for single-file correctness" and so on. So I would probably group the warnings to subcategories which match this intent.

I don't see much value distinguishing between UnreferencedCode and DynamicDependency warnings - for end users these are all about the same thing.

So we should group all of them into one subcategory - name is tricky - "TrimCorrectness"? (Not sure I like it...) @samsp-msft for ideas.

[tlakollo]

Nit: Maybe use if (Array.IndexOf (MessageSubCategory.Analysis, message.SubCategory) > -1) to not have to add Linq to the LinkContext? Also, IndexOf does a faster lookup but not much of a difference. I do get that this is more readable, so I'm also ok if you don't want to change it

[sbomer]

Thanks for putting this together!

I think we want warnings about unrecognized reflection patterns and RequiresUnreferencedCode to be off by default (and in the same set). We then need an opt-in mechanism for these. To throw out an idea: maybe we could call the option --enable-warnings <set>, following --enable-opt and --disable-opt (without the per-assembly variants for now, though I could see them being useful in the future).

I don't think we need --nowarn or --disable-warnings <set> yet - maybe once we have more well-defined on-by-default warning sets (and then we might be able to combine it with --skip-unresolved). For now, the normal MSBuild suppression mechanism for specific warning codes seems like it would be enough.

I also think that warnings about invalid attributes or unresolved parameters in these attributes should be on by default (devs will want to get warnings if they add attributes with invalid signatures).

Regarding the set names: TrimCorrectness seems reasonable. If anything, the "Correctness" suffix might be redundant since all of the warnings are about various aspects of correctness. How about trimanalysis? (Lowercase for consistency with the optimizations). We could expose it at the MSBuild level as TrimmerAnalysisWarnings or similar.

[eerhardt]

Since we also have a concept of warning numbers, if we do introduce --nowarn it should behave the same as for csc.

So we need a different command line switch name for this functionality.

Why don't we just go with the same behavior as csc from the start? It feels like the right behavior IMO.

[vitek-karas]

Why don't we just go with the same behavior as csc from the start? It feels like the right behavior IMO.

Just to be clear - what behavior do you mean - the fact that -nowarn accepts warning numbers? I think this was already proposed, but there was some discussion that then there needs to be something outside of linker which knows which numbers map to analysis and to disable that by default.

[eerhardt]

but there was some discussion that then there needs to be something outside of linker which knows which numbers map to analysis and to disable that by default.

We can just default the LinkerNoWarn (or whatever name we come up with), just like we do for Csc:

https://github.com/dotnet/sdk/blob/bc5c514a45a1a900b71d39a3541196bc2cf1bb0d/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.CSharp.props#L16

So, in a .props file (which is imported before the .csproj)

<LinkerNoWarn Condition=" '$(LinkerNoWarn)' == '' ">2006;2026</LinkerNoWarn>

Then when a dev wants to enable all warnings:

<LinkerNoWarn></LinkerNoWarn>

in the .csproj would enable them all.

[sbomer]

My take on this is that we should't disable warnings by default in MSBuild - unless it is a workload-specific default.

<LinkerNoWarn></LinkerNoWarn>

also feels strange as the recommended way to opt into a new feature. I'm curious why csc does it this way. Those disabled warnings look like they have to do with assembly unification - maybe that was a workaround for existing behavior they couldn't change?

[samsp-msft]

--nowarn
I think we need a different command line name - csc has exactly the same switch, but it accepts warning numbers. Since we also have a concept of warning numbers, if we do introduce --nowarn it should behave the same as for csc.
...
So we should group all of them into one subcategory - name is tricky - "TrimCorrectness"? (Not sure I like it...) @samsp-msft for ideas.

I don't have a problem with --nowarn if it supports the same syntax as csc.exe, but would also add the named uber-category as depedendyAnalysis as a grouping.

--nowarn 2036,2037
or
--nowarn dependencyAnalysis

would be valid, where dependencyAnalysis maps to the respective collection of warning numbers.

If we want to have a default, it should be in msbuild files so it can be overridden in other rules, rather than as baked in behavior in the linker. msbuild rules today are/can be opinionated, but the exes should be neutral.

[sbomer]

If we want to have a default, it should be in msbuild files so it can be overridden in other rules
msbuild rules today are/can be opinionated, but the exes should be neutral.

I like this framing @samsp-msft. What should we consider to be the neutral behavior (analysis warnings on or off)?

My thinking so far has been that the neutral behavior is for the analysis warnings to be off, which is also the behavior we want for the .NET Core SDK (at least in .NET5 where they'll be pretty noisy) - then other msbuild files from developers or SDK components can override this to turn them on. I can see it going the other way too.

[samsp-msft]

If we want to have a default, it should be in msbuild files so it can be overridden in other rules
msbuild rules today are/can be opinionated, but the exes should be neutral.

I like this framing @samsp-msft. What should we consider to be the neutral behavior (analysis warnings on or off)?

My thinking so far has been that the neutral behavior is for the analysis warnings to be off, which is also the behavior we want for the .NET Core SDK (at least in .NET5 where they'll be pretty noisy) - then other msbuild files from developers or SDK components can override this to turn them on. I can see it going the other way too.

Following the compiler model - warnings are enabled by default - its showing something that we think is a problem.

[vitek-karas]

I agree with Sam on that the warnings should be produced by linker "by default" - we know they describe problems. It is somewhat noisy as the app might work regardless, but each one of them is potentially a problem.

The reason we want them off by default for now is not the linker (I think we've done a good job in the linker for these), it's the libraries - our own and external as well. Hopefully the reason will change in future versions - but versions of the framework, not the linker. So for example building .NET 5 app with .NET 6 SDK should hopefully still disable them by default (even if we enable them by default for .NET 6 apps), because the problem is in the libraries, not the linker.

Thanks a lot Sam for bringing this perspective to this discussion!

[MichalStrehovsky]

I think it would be fine to have the warnings off by default if the conservative trimming mode is enabled (the one that we shipped with in 3.0).

But if someone opts into more aggressive trimming, I would want them to see all the warnings by default.

[mateoatr]

I think it would be fine to have the warnings off by default if the conservative trimming mode is enabled (the one that we shipped with in 3.0).

Just to be sure, by this you mean all warnings or analysis warnings?

[mateoatr]

Summarizing a little bit the discussion:

1. Change parameter name (--nowarn should be used for warning codes, just as in csc.) Would --disable-warning-category be a good name for this new linker parameter?

2. Warnings under UnrecognizedReflectionPattern, UnreferencedCode and DynamicDependency should be recategorized as DependencyAnalysis/TrimmerAnalysis.

3. Warnings about invalid attributes or unresolved parameters should be printed regardless (in practical terms, these warnings shouldn't be categorized as DependencyAnalysis/TrimmerAnalysis.)

4. Expose option DisableWarningCategory (or however we name it) both in ILLinkTask and the SDK. Default its argument to DependencyAnalysis/TrimmerAnalysis, and whenever we are ready to enable these warnings by default we can do something like <DisableWarningCategory Condition="'$(TargetFrameworkVersion)' >= '6' and '$(DisableWarningCategory)' == ''"></DisableWarningCategory>.

Is this correct?

[agocke]

A couple notes about warning behavior in csc:

• I really regret that we used "codes" (numbers) in csc. They were preserved in Roslyn because of backwards compatibility, but it's a much better user experience to give full identifiers, like "IL_UnsupportedReflection" so the user can easily map a diagnostic ID to its purpose.

• We do have some categorization in Roslyn, but only for analyzers. Also, we don't allow it to be specified on the command line, only in editorconfig files (which we should probably think about for the future)

• It's true that warnings are on by default in Roslyn, but we also have never added a new warning to existing code, because it's considered a breaking change. That is something that is being reevaluated specifically based on upgrading the target framework, but Roslyn still plans to always have an "opt-in" for new warnings

[agocke]

Also, consider the benefit of using the same property to disable a warning ID in the linker and Roslyn -- if we moved any capabilities to be Roslyn analyzers, then Roslyn would see the suppression already and already have that warning silenced. Also, because of analyzers, Roslyn simply ignores suppressions it doesn't understand, it doesn't fail on them.

[sbomer]

@mateoatr great summary. Some thoughts:

• Enabling or disabling a warning category/set is like adding or removing an analyzer (we will likely have more, like singlefileanalysis) and then at a more granular level you should be able to use NoWarn for individual codes - which is the same property used for Roslyn, as @agocke suggests. You can do that today with MSBuildWarningsAsMessages, and once Support NoWarn as MSBuildWarningsAsMessages msbuild#4421 is done it will work with NoWarn. Do we also want to support --nowarn in the command-line, or is --disable-warning-category enough? (My vote would be just --disable-warning-category for now).

• --disable-warning-category seems reasonable. I originally suggested --disable-warnings to avoid ambiguity with the MSBuild "category". Now I am wondering if instead it should be called --disable-analysis. Whether we actually run the analysis is an implementation detail - to the user, it is essentially like turning on or off a roslyn analyzer, so the naming should reflect that. Any of these is fine with me, maybe @marek-safar or @vitek-karas has a preference.

We've effectively said that the defaults are:

• command-line has trimanalysis on
• msbuild disables trimanalysis for .NET5 (via NoWarn or DisableWarningCategory or similar), and hopefully leaves it enabled for .NET6

Since the disabled trimanalysis is specific to .NET5 I think @eerhardt's suggestion to disable them from MSBuild makes sense, with <NoWarn> or <DisableWarningCategory> instead of <LinkerNoWarn>. I am leaning towards using <NoWarn> so that we don't have to decide what the model is for enabling/disabling "illink analyzers" from MSBuild, until we have a better picture of what the other analyzers look like.

We still need a way to override the opinionated .NET5 default and opt into the warnings. <NoWarn></NoWarn> or <DisableWarningCategory></DisableWarningCategory> seem problematic since we don't want to enable other disabled warnings (or categories, once we define more of them). I'm not sure there is a user-friendly way to remove just part of the property string to do this. What do you think of a single-purpose <ExperimentalTrimAnalysis> or similar as a user knob for .NET5 only that determines whether we set <NoWarn> (or <DisableWarningCategory>)?

[vitek-karas]

I discussed some of this with @agocke offline - my opinions:

• We should go with --nowarn and emulate the behavior of Roslyn here.
• Disabling trim correctness warnings should be done in the SDK - by using the NoWarn property
  • This can be done behind another property (TrimmerCorrectnessAnalysisWarnings for example) - if the property is false/omitted we add the list of warning codes into the NoWarn, otherwise we leave it alone (as noted above, there's no good way to remove from NoWarn, but we should not need that)
• We can introduce categories for warnings, but purely as a documentation thing, I don't think linker itself should have a command line option to handle that - yet.
• I would strongly consider changing the warnings codes from numbers to string - @agocke mentioned that it's also considered for the new .NET 5 analyzers - we need to sync up with that work (we still have a chance to change it in the linker, so if there's an agreement on what things should look like moving forward, we should do that).
• @agocke also mentioned that there's a new notion of warning levels introduced in .NET 5 - (not the existing stuff) - they should act as "versions" in that warnings added in .NET 6 will get new (higher) warning level, so one can say "I only want warnings from .NET 5 and below". I'm not sure we need to do anything about that for linker, as this is effectively the first version of linker which has true warnings...

@agocke - would you know if/where is the discussion on using strings instead of numbers for warnings codes?

[agocke]

@agocke - would you know if/where is the discussion on using strings instead of numbers for warnings codes?

I'm not sure there was recorded discussion, just a few conversations, but I agree that we should have some stuff written down, especially since this is going to be a broader .NET-wide issue.

@terrajobst @jaredpar do know if we have any document outlining diagnostic ID recommendations? My opinion was that analyzers should generally pick descriptive diagnostic names, like CA_AvoidUnusedPrivateFields instead of opaque identifiers like CA1823. Does anyone disagree?

[jaredpar]

@mavasani

I'm not aware of any recommendations for diagnostic IDs. In general though a descriptive string does seem preferrable to an opaque identifier.

[mavasani]

I believe the core issue with using descriptive names instead of opaque diagnostic IDs is Localization story. We need to figure out whether this means IDs should be localizable. I am presuming they ought to be, otherwise you are trading one problem for another - a descriptive string that is only useful for english speaking users is not a good option. If they are localizable, that would mean DiagnosticDescriptor ctor should have a new overload that takes localizable diagnostic ID string, similar to title, message and description. Additionally, there are ton of places in the IDE where we show diagnostic IDs (error list, Analyzers node in sln explorer, ruleset editor, quickinfo, etc.), and ton of places in IDE code base where we store/pass it around as a non-localizable string. How about dotnet_diagnostic entries in editorconfig?

[samsp-msft]

I believe the core issue with using descriptive names instead of opaque diagnostic IDs is Localization story. We need to figure out whether this means IDs should be localizable.

We don't localize the argument names like "nowarn" so I don't think we need to localize the values.

[mavasani]

We don't localize the argument names like "nowarn" so I don't think we need to localize the values.

If not localized, then using strings like CA_AvoidUnusedPrivateFields as opposed to CA1823 adds no value. In fact one can argue that it seems like a bug as the ID now provides benefit to english speaking users at expense of non-english speaking ones, for whom it is an undecipherable string in a different langauge. I'd rather stick to opaque numbers until this issue is sorted out.

[agocke]

@mavasani

I'd rather stick to opaque numbers until this issue is sorted out.

I don't follow. First, I don't think there's an "until" -- we pick one and then have to stick with it forever. And I'm not following how we could possibly localize diagnostic IDs. The purpose of the diagnostic ID is to be fixed and immutable.

If we followed your approach I think it would imply that we should localize C# language keywords as well, since those are also "English-only" strings. But we've never considered that to be a goal and I don't see why we would special-case diagnostic identifiers here.

[mavasani]

My point is using "descriptive names/strings" for diagnostic IDs has lot of pitfalls, because we cannot localize the ID due to lot of features that will break. We should stay with using opaque number based identifiers.

[agocke]

Why is being more descriptive in one language worse than being less descriptive in all languages? Why is it OK for C# keywords to be English, and not random strings?

[mavasani]

If we followed your approach I think it would imply that we should localize C# language keywords as well, since those are also "English-only" strings. But we've never considered that to be a goal and I don't see why we would special-case diagnostic identifiers here.

That is not true or related. Language keywords are all documented, and parsed as a keyword by the compiler, hence not localized. On the other hand, the diagnostic ID shows up in lot of UX, such as error list, solution explorer, ruleset editor, etc., which is required to have localized strings for descriptions. For example, if I am a non-English speaking user, seeing error list entries with CA_RandomUnreableText as a column value for the primary column in error list would make it a very unpleasant experience.

[agocke]

How is that different from editorconfig, which requires the string dotnet_diagnostic.<ID>.suppress and is not localized?

[mavasani]

Additionally, I also don't understand why are we trying to push "descriptive text" into an "ID" field, while such a description is already part of the diagnostic title and message fields, which are already localized. Are we just missing surfacing these localized strings at a place where it is required?

[agocke]

This is the linker, not Roslyn. There is no diagnostic infrastructure or localization of any kind. We're trying to decide what format we should adopt.

[terrajobst]

@mavasani

If not localized, then using strings like CA_AvoidUnusedPrivateFields as opposed to CA1823 adds no value.

I don't agree with that:

1. We don't add types and methods like T6253.M1212()
2. We don't add language keywords like k1221
3. We don't have CLI commands like dotnet c1212

None of these are localized or even could be localized. It's a bit defeatist to say just because something won't be localized using a descriptive name in en-us has no value. That being said, I think using descriptive names has downsides as well:

1. The VS error list column won't work very well with variable length values b/c it will start conflicting with the description. But maybe that's OK.
2. We need to define a naming convention. (like two-letter code followed by underscore followed by PascalNamedRule and those won't be consistent with most diagnostics in the wild today (compiler, MSBuild, IDE, 3rd party analyzers)

[mavasani]

This is the linker, not Roslyn. There is no diagnostic infrastructure or localization of any kind. We're trying to decide what format we should adopt.

Sure, each analyzer author is free to use whatever format for the ID as they prefer, as long as it is a valid identifier. That does not mean we should recommend analyzer authors to use Prefix_DescriptiveText as a format for IDs, because this string will never get localized and is likely to lead to unpleasant experience for non-enu users who'd rather prefer an opaque number over an unreadable string in each of UX pieces, such as primary column of error list.

[agocke]

At the moment, the linker warnings don't even appear in the VS error list, so I agree that's unfortunate but also something we probably need to address later 😄

[agocke]

However, if the .NET 5 analyzers are being shipped through the existing roslyn-analyzers framework, then the .NET approach is to use two-letter prefix + number. I'd rather us be consistent with the rest of .NET than carve our own path

[agocke]

So "IL" + number sounds good. Any concerns? @terrajobst @mateoatr

[mavasani]

It's a bit defeatist to say just because something won't be localized using a descriptive name in en-us has no value.

How would we feel if we start seeing CA_避免使用未使用的字段 instead of CA1823 as error list ID column entries, ruleset files, dotnet_diagnostic entries in editorconfig files checked into our repos, etc. (assuming you are non-Chinese speaker)? Even though both these strings would be opaque, I think most of us would much rather prefer the latter version and would see such a change as an unpleasant experience regression.

[mateoatr]

So "IL" + number sounds good. Any concerns? @terrajobst @mateoatr

This sounds good. Let's stick with this in the linker for now.