F# diagnostics: create consistent WarningLevel behavior

[Martin521]

I propose to fix F# diagnostics so that the WarningLevel option does what the compiler reference says (see below) and thus aligns with the C# concept of WarningLevel

Every C# diagnostic CSxxxx has a well-defined warning level.
Errors have level 0, warnings have level 1 to 3 and informational warnings have level 4.
When building with <WarningLevel>n</WarningLevel> or --warn:n, all diagnostics up to level n are shown.

For F#, the compiler reference says

--warn:warning-level

Sets a warning level (0 to 5). The default level is 3. Each warning is given a level based on its severity. Level 5 gives more, but less severe, warnings than level 1.

This compiler option is equivalent to the C# compiler option of the same name. For more information, see /warn (C# Compiler Options).

However, the levels are not documented (except for the opt-in warnings).
Playing with the warning level (or diving deep into the compiler), you find

• Level 0: errors
• Level 1: info (informational warnings)
• Level 2: non-opt-in warnings
• Level 5: "opt-in" warnings 21, 22, 52, 1178
• No level (never shown): the remaining "opt-in" warnings

I propose to change this to

• Level 0: errors
• Level 2: warnings
• Level 3: info
• Level 5: opt-in warnings 21, 22, 52, 1178
• Level 6: the remaining opt-in warnings

While 4 for Info would be closer to C#, the above choice is for backwards compatibility.

Also for compatibility reasons, the proposal uses level 5 and 6 for opt-in warnings. (Note that C# uses levels above 4 for "warnings waves", which, I believe, we don't need. EDIT: see discussion below.)

All the nowarn/warnon/warnaserror functionality will of course continue to be supported.

The WarningLevel filter should work both for fsc and for editor support.

Pros and Cons

The new warning levels are more consistent, are extensible and can more easily be documented.
They align with the SDK and C# concept of "Warning level".
They will simplify the compiler code and make it more maintainable. (That's actually why I bring it up ;-))

Compatibility issues:

For WarningLevel 2 and lower, info warnings are no longer shown in the editor.
For WarningLevel 3 and above, fsc will print the info warnings.
For WarningLevel 6 and higher, opt-in warnings may appear that were not shown previously.
(I found on github two repositories with .fsproj files with WarningLevel 2; zero with WarningLevel 6, out of a total of 10k .fsproj with explicit WarningLevel.)

[baronfel]

Hey - warning level is a property that auto-increments for C# with each major version of the SDK. So the 8.x SDKs send a WarningLevel of 8 to the compiler for example. This lets diagnostics be created at a level that is below the currently WarningLevel max that will 'light up' automatically as the tooling advances.

A lot of this inference is actually handled in the SDK, so if the F# targets are hard-coding the behavior it should be simple to un-hardcode it and let the SDK take it from there.

[Martin521]

Hmm, I understand how this makes sense for the "warning waves", but does it mean there is no longer a way to do what the WarningLevel documentation says?

The WarningLevel option specifies the warning level for the compiler to display.

<WarningLevel>3</WarningLevel>

The element value is the warning level you want displayed for the compilation: Lower numbers show only high severity warnings. Higher numbers show more warnings. The value must be zero or a positive integer

[baronfel]

Those original values are special cased in the SDKs MSBuild inference logic, yes - but from 5 onwards the meaning I described (and the docs you linked to themselves link to) takes over.

[Martin521]

Ah ok.
That means we cannot use 5 or 6 for opt-in warnings.

[Martin521]

Or is that MSBuild logic in the language specific targets, and we could keep the traditional logic for F#?

[baronfel]

Yeah, the logic I'm speaking of is in a file that is currently only imported for C#/VB projects: https://github.com/dotnet/sdk/blob/05a82501a985078cf1d538b91da5cd1ad2f10373/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L1438

My main question is that there is this existing set of semantics that seem to be pretty well aligned with what you want - gradually-increasing strictness - and F# would have to do very little work to start piggybacking on this train, so why do additional work?

[Martin521]

The problem are the F# "opt-in warnings". They are not shown unless enabled by warnon. Most of them, currently, have no level (are never checked against WarningLevel). To fit them into the scheme in a backwards-compatible way, I wanted to give them level 6.

Something like warning waves would only be a next step, on top of a (then) consistent set of base levels.

[baronfel]

From a back-compat perspective, you could start this system with F# 10/.NET 10 and have Warning Level 10 be the first one that includes all of the current opt-in warnings.

[T-Gro]

I do like the idea of aligning this with SDK, but there are some warnings which we would likely never want to be turned ON by default for EVERYONE - they are too opinionated and obtrusive.

(e.g. warning about struct defensive copies)

Also, in the history, some PRs for warnings were only accepted on the premise of being optional, but enabling them for everyone (does not really matter if 1,2,3.. years in the future) is a different story - under that premise, they would likely never be accepted at all.

[Martin521]

I was actually wondering if we should not turn all these "default-off" warnings into infos.
Advantages:

• This fits better their nature,
• They can still be turned on in the same way as now,
• It would enable simpler and more consistent implementation (see PhasedDiagnostics.IsEnabled),
• It would enable the SDK mechanism that @baronfel pointed out.

I would volunteer to implement this if it makes sense for everyone.

[T-Gro]

I don't like flooding people's projects with infos that come from "opinionated" diagnostics. There is a reason they are opt in - they are not a good fit for everyone.

When we have an infrastructure for custom analyzers, I would happily encourage to move all the opinionated ones to custom analyzers, leaving only the warnings/infos where we can firmly say this is a good diagnostic for every kind of F# user. We just don't have the analyzer setup yet :(.

[Martin521]

Yeah, I forgot that F# always emits all infos.
Instead of doing that (like C#) depending on the warning level, i.e. not by default.