Support --warn <version_int>

[sbomer]

We would like to turn on all of the new warnings by default from the command-line (without requiring --verbose). The current plan is to then disable them from MSBuild for .NET5, by setting NoWarn to contain the trim analysis warnings. For backward compatibility, ideally none of the new warnings would be emitted when targeting netcoreapp3.0.

We can do this via NoWarn, or we can use versioned warnings, passing a --warnversion flag to the linker, like Roslyn is doing. Back-compat may not be as essential in this release (since in 3.0 the trimmer was experimental, and tended to silently break apps), but we will have similar problems as we introduce new warnings in future releases.

dotnet/roslyn#45701 is a pretty thorough summary of the Roslyn plan, which we would likely follow. The biggest open question is:

do we bump the number for .NET releases or language releases? Today they will be the same but what if that changes?

This is more relevant for the linker because we are introducing disabled "future" warnings even in .NET5 (since the trim analysis warnings will be available, but off). Should we have three "versions" (no warnings, all but trim analysis, and all warnings)? Or do we want to keep the "experimental" trim analysis warnings under a flag of its own? It feels a bit strange to give them a version number if we are not confident that they're ready to be on by default in a specific release.

[sbomer]

Warning versions proposal

Roslyn behavior
The current Roslyn design has decimal version numbers. The default behavior is version 0 when not specified, 5 for .NET5, and supports 9999 for all optional warnings. The SDK will set this via an AnalysisLevel property.

It sounds like some of those details are subject to change (see for example dotnet/roslyn#45941).

Proposed mapping
We will follow the Roslyn design for the command-line. Assuming that it does not change from the above, here is a proposal for our version numbers:

• 0 means none of the new warnings (effectively, what we shipped in 3.0). Errors about invalid command-line options, for example, will still be logged.
• 5 means all of the new warnings except IL2006 (unrecognized reflection), IL2026 (RequiresUnreferencedCode), and IL2045 (attribute types kept when removing attribute instances). These exceptions are the only warnings that remain in a blazor template app, after:
  • Suppressions from Enable verbose linker output & add baseline warning suppressions for library builds runtime#39133
  • The fix for IL2029 (Improve warnings on internal attribute use #1361)
  • The fix for unused resources causing IL2008 (Unused resources in System.Threading.Overlapped runtime#39385)
• 9999 means all of the warnings.

Based on previous discussions, our command-line default will be all warnings (effectively 9999) - because this is what we are considering the "neutral" behavior.

Open questions
In the SDK, we probably want to have an additional property instead of just using AnalysisLevel directly, so that developers can set warning versions independently for Roslyn and ILLink. The defaults for this property can come from AnalysisLevel since we are planning on using the same version numbers, at least in this release.

@vitek-karas @mateoatr @agocke @eerhardt @marek-safar

[agocke]

I said the same in the internal Roslyn design doc, but decimal is a mistake. Version numbers may have dots in them, but that doesn't make them numbers. Consider 5.0.1. I think there are two correct representations: on the command line, the version is a string that gets parsed (obviously). The only data type there is string. The result, I think, should be an enum. This matches Roslyn's LangVersion, which accepts versions like 8.0, but also named values like Latest and Preview. I think it's the most flexible option that also helps to reduce confusion after parsing, since you get type safety in the value.

[vitek-karas]

About the data type: How will this work in the future? When .NET 6 comes around we will add the value 6 to the set. But what happens if I pass 6 to .NET 5 linker? Should it fail? Should it effectively mean 5?...

Even though I agree that using double as the data type for this is wrong - I think we should follow whatever Roslyn does in .NET 5 in the end - consistency beats everything else here.

As for the proposal above:
I think that 5 should mean enable all warnings we will support in .NET 5 linker. No exceptions. The trim analysis warnings and the fact that they will be too noisy to be on by default should be an orthogonal decision - which should be covered by whatever property we pick in dotnet/sdk#12388

I guess what I'm saying - warning version should not take into account default experiences - and even more so default experiences for one specific application type.

[agocke]

How will this work in the future? When .NET 6 comes around we will add the value 6 to the set. But what happens if I pass 6 to .NET 5 linker?

I guess the question here is what behavior we want to enable. If we want to allow command line strings to be portable back to older versions of the linker, that seems like a very big compat burden, and I'm not sure of the benefit. If we only want to allow warning versions to be compatible backwards, that's more doable, but it seems like it would have even less value.

If we want people to have a flag to enable "the most possible" warnings in their project file, I would suggest a specific warning version, like latest (which could map to WarnVersion.MaxValue, just like C#'s LangVersion) would be clearer and more effective.

Even though I agree that using double as the data type for this is wrong - I think we should follow whatever Roslyn does in .NET 5 in the end - consistency beats everything else here.

I strongly agree that consistency is the most important, but since this would be an internal data type representation (all the command options are inherently strings), what's the value in matching internal implementation details with Roslyn? The value of using an enum internally seems to be type safety, and I don't think that value is diminished, even if Roslyn doesn't decide to adopt it.

I guess what I'm saying - warning version should not take into account default experiences - and even more so default experiences for one specific application type.

This seems like a good design principle. We might even say: warnversion is orthogonal to the other feature switches. If we have a special classification of warnings, we might require a specific feature switch turned on. The warnversion would be orthogonal to that feature switch in the sense that if you turn it on, you get those warnings and the warnversion doesn't affect it. Then, if we decide to add more warnings to the feature switches in the new version, we can do so by using the warnversion. A user who still wants the feature switch on, but the warnings from the previous version can downgrade the warnversion as appropriate. This is a coarse, but effective method of filtering. More fine-grained control is available through nowarn.

[sbomer]

I guess what I'm saying - warning version should not take into account default experiences - and even more so default experiences for one specific application type.

I don't think of it as a default for just a specific app type - I thought we were going to disable them for all .NET5 apps. Blazor was just the primary test scenario we used to evaluate the readiness of the warnings on this version of the framework. Preventing some set of warnings from showing up for a target framework version seems like what the versions were designed to do. What's the reasoning for separating the .NET5 defaults from the version 5 warnings?

Whatever the mechanism (versions or NoWarn), do the proposed disabled warnings for .NET5 look reasonable?

[vitek-karas]

What's the reasoning for separating the .NET5 defaults from the version 5 warnings?

If nothing else it sets a bad precedent. Yes - it might end up working the same way in .NET 5, but that doesn't mean that if we have new feature in .NET 6 we should hide it behind warning version. And also see the counter example at the end of #1030 (comment) - that nicely describes why I think we should have properties which capture the intent, not the underlying mechanics.

[sbomer]

warnversion is orthogonal to the other feature switches

I agree with this (maybe avoid "feature switches" since that has an established meaning - perhaps "analysis kinds" or "categories"?). We've had some discussion (ongoing in #1030) about the categories and where they should be defined (SDK vs illink.dll vs task).

[sbomer]

I've opened a new issue to discuss the category - let's continue that part of the discussion at #1367. The new proposal for the version number to warning mapping is:

• 0: none of the new warnings (which will be the default for netcoreapp3.0)
• 5: all of the warnings currently defined
• 9999: same as 5, plus warnings we add in future versions

[sbomer]

Reflecting dotnet/roslyn#46148, the command-line option is planned to be --warn <integer>.