search

dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.9k stars 782 forks source link

F# diagnostics: create consistent WarningLevel behavior #17901

Open Martin521 opened 3 days ago

Martin521 commented 3 days ago

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

I propose to change this to

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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

Martin521 commented 3 days ago

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

baronfel commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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.