Roslyn: Warning Waves

[gafter]

• [ ] Compiler flag + Csc task + CoreCompile target
• [ ] Project system change to display in IDE (https://github.com/dotnet/project-system/issues/4725)
• [ ] Code fixer to help turn the feature on
• [ ] Select a few issues that we want in the first wave

Over the course of its few years of development, the Roslyn compiler team implemented a handful of valuable new diagnostics for situations in user code that almost certainly indicate errors, and which would be very expensive to implement outside the compiler. One typical example was the comparison of a struct value against null

    void M(..., CancellationToken token)
    {
        if (token == null) { ... }
    }

The test token == null must always fail (a struct value, when boxed into an object, is never null). This almost certainly indicates an error in the user's program. And indeed, this diagnostic alerted us to dozens of real bugs in the Roslyn code base. Other new warnings similarly alerted us to other problems.

Since compiler warnings are enabled by default, any project that upgrades to the new version of the compiler may be "broken" (e.g. if configured to treat warnings as errors) by the presence of newly implemented diagnostics at a time when changing the code or the build infrastructure is inconvenient. Due to this concern previous releases of the compiler have avoided adding diagnostics for existing code scenarios. We wanted to allow Roslyn, it its first release and in subsequent releases, to add value by adding additional warnings for likely code problems that are difficult to detect outside the compiler. But how could we do it without breaking compatibility?

A few years ago we considered adding the concept of "warning waves" to the compilers. The compiler would know in which compiler version support for each warning was added. The compiler command-line would specify a version for which warnings to allow; the compiler would then report only warnings that were implemented up until the specified "warning version". We held meetings and produced a detailed design.

Two years ago we dropped the idea of adding a specific "warning waves" flag to the compiler because we "realized" that the new mechanisms to control the diagnostics (such as ruleset files) are fine-grained enough to provide the same value.

Unfortunately we did not complete the work to address the scenario. Consequently all of the new warnings implemented in Roslyn have since been removed from the compilers due to compatibility concerns.

It is time for us to revisit the issue, to design and implement a solution that addresses the original problem. Users should be able to specify a version of the compiler that represents the set of warnings they are willing to have reported.

LDM notes:

• https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-09-26.md

[gafter]

/cc @eatdrinksleepcode @AnthonyDGreen @srivatsn @theoy @JohnHamby @joeduffy @MattGertz

[AnthonyDGreen]

We discussed this offline to come up with what we feel is a sensible design and some principles for this feature:

Definitions

• A _wave_ is defined to be a set of new diagnostic IDs and/or new circumstances for reporting existing diagnostics. i.e. either introducing a brand new warning or correctly reporting an existing warning in a new case that we hadn't reported it before due to a bug would be gated by this feature.

Principles

• Warning waves _are not_ intended to be a general mechanism that applies equally to Analyzers; this is a non-goal. Analyzers have NuGet semantic versioning as their primary vehicle for controlling the introduction of new warnings.
• Warning waves must be decoupled from language versions; customers must be able to opt into the goodness of new language features without also forcing upon themselves the pain of new diagnostics.

Design

• There shall be a flag which can be passed to the compiler which will enable any warnings introduced up to a specified warn-version or _wave_.
• The flag shall have the syntax /warnversion:
  
  
  
  • This flag may control errors as well but given that the vast majority of cases will be warnings it makes more sense to call it /warnversion rather than /warnorerrorversion or /diagnosticversion.
  
  
• The format of the version string shall match what /langversion accepts—the minor version will be incremented with each update, e.g. 6.1/14.1.
• The default in the absence of this flag shall be the latest wave.
• MSBuild will pass the value in the project file if specified; otherwise it will pass version 6/14 to maintain back compat for existing customers.
• There should be a NOSEATBELTS option that explicitly opts into the latest wave.

[svick]

@AnthonyDGreen Since project.json is already used to version analyzers and the framework (at least for some project types), could it make sense to also use it to version the compiler and warning waves?

[gafter]

Should that be ALLSEATBELTS :wink: ?

[gafter]

Moving to milestone 2.1 as that is when we'd like to consider the new warning on tuple name rearrangement #14217.

[alrz]

Do we need to retain backward compatibility in future versions of warning wave? Since this is an opt-in feature I think it should be possible to add useful warnings that did not exist in the first version.

[jnm2]

Yes. And before anyone brings them up, the warnings-as-errors folks by definition would want their build to break when new warnings are added.

[eyalsk]

Correct me if I'm wrong but does this feature mean we will have a _strict_ version of C#? where some code that currently gets compiled will no longer compile _iff_ the code violates the rules? I'm not sure if the comparison make sense but is it something like JavaScript "use strict"?

[DavidArno]

@eyalsk,

My reading of @AnthonyDGreen's description suggestions it wouldn't be a simple dichotomy like JS's not-strict/strict modes. Instead, v16.1 of the compiler might introduce warnings around unused variables (as suggested by @alrz in #15695), then v16.2 might introduce warnings when contextual keywords are used as type or variable names. Even when v17 of the compiler comes out, I could then choose /warnversion=16.1 to not use the keyword warnings or /warnversion=ALLSEATBELTS to automatically get the latest warnings as the compiler advances.

So in JS terms, this would be like having use strict, use stricter..., or use strictest to get the latest.

[DavidArno]

For the record, my vote for a warning in the first wave would be Don't write C# as if it were Java for when someone uses the K&R bracing style 😁

[eyalsk]

@DavidArno Thank you for explaining it to me! sounds good. 😄

[gulshan]

Shouldn't this be moved to the csharplang repo now?

[gafter]

@gulshan There are no language changes proposed here.

[jcouv]

@AnthonyDGreen Let's discuss when we have the chance.

The default in the absence of this flag shall be the latest wave.

I'm not sure that makes sense. It seems that the absence of /warnversion should mean "none" (that's functionally the same as warnversion 7, for C#).

My current understanding from discussion with Kevin and Cyrus:
We'd have the following options:

• none/missing: that means you are not opting into any additional diagnostics
• N: you dial additional diagnostics up to a certain version (for example, 7.0.1 or 7.1)
• latest/max: you dial additional diagnostics to the max supported by this compiler. (There is an open question whether we should offer this option)

With every release, we'll make sure to update templates for legacy project system to use the new N.
Msbuild would default to none/missing.
The templates for new SDK would not specify this option, and the SDK should translate that to mean "version of the SDK". (We need to clarify this last point, as version numbers of the SDK may not necessarily align with versions of the language. For example, C# 7.0.1 or VB 15.6.1)

[Pilchie]

BTW - If you want to start introducing new warnings for existing code and you want people to enable them in their existing projects, I would strongly suggest writing code fixes for as many of these as possible.

[alrz]

Consider: object[] o = new C[]{}, unconditional recursive methods or explicit values for caller info params.

Should they be included in warning waves vs IDE analyzers? and why? In other words, how a warning is decided to be a part of warning waves?

btw do we have a list of considered warnings here?

which would be very expensive to implement outside the compiler

(edit) That'd be the answer to the first question but I'd like to know if there's more to it.

[jcouv]

@alrz Issues marked with the "warning waves" labels are the candidates.
LDM still needs to triage them and set some guidance for what issues should qualify for warning waves. This will likely happen in the next couple weeks.

[jcouv]

Moving warning waves post 8.0 RTM.

[gafter]

This issue is replaced by https://github.com/dotnet/roslyn/issues/45701