New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warning Waves #1580

Open
gafter opened this Issue Mar 25, 2015 · 15 comments

Comments

@gafter
Member

gafter commented Mar 25, 2015

  • Compiler flag + Csc task + CoreCompile target
  • Project system change to display in IDE
  • 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.

@gafter gafter added this to the 1.1 milestone Mar 25, 2015

@gafter

This comment has been minimized.

Show comment
Hide comment
@AnthonyDGreen

This comment has been minimized.

Show comment
Hide comment
@AnthonyDGreen

AnthonyDGreen Sep 14, 2015

Contributor

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.
Contributor

AnthonyDGreen commented Sep 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@svick

svick Sep 14, 2015

Contributor

@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?

Contributor

svick commented Sep 14, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@gafter

gafter Sep 15, 2015

Member

Should that be ALLSEATBELTS 😉 ?

Member

gafter commented Sep 15, 2015

Should that be ALLSEATBELTS 😉 ?

@gafter

This comment has been minimized.

Show comment
Hide comment
@gafter

gafter Nov 16, 2016

Member

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

Member

gafter commented Nov 16, 2016

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

@alrz

This comment has been minimized.

Show comment
Hide comment
@alrz

alrz Dec 12, 2016

Contributor

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.

Contributor

alrz commented Dec 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Dec 12, 2016

Contributor

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.

Contributor

jnm2 commented Dec 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@eyalsk

eyalsk Dec 21, 2016

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"?

eyalsk commented Dec 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@DavidArno

DavidArno Dec 21, 2016

@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 commented Dec 21, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@DavidArno

DavidArno Dec 21, 2016

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 😁

DavidArno commented Dec 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@eyalsk

eyalsk Dec 21, 2016

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

eyalsk commented Dec 21, 2016

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

@gulshan

This comment has been minimized.

Show comment
Hide comment
@gulshan

gulshan May 9, 2017

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

gulshan commented May 9, 2017

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

@gafter

This comment has been minimized.

Show comment
Hide comment
@gafter

gafter May 9, 2017

Member

@gulshan There are no language changes proposed here.

Member

gafter commented May 9, 2017

@gulshan There are no language changes proposed here.

@jcouv

This comment has been minimized.

Show comment
Hide comment
@jcouv

jcouv Jun 12, 2017

Member

@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)

Update: I wonder if we should re-use the /strict flag. For instance: /strict:8.0 for 8.0 warning wave.

Member

jcouv commented Jun 12, 2017

@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)

Update: I wonder if we should re-use the /strict flag. For instance: /strict:8.0 for 8.0 warning wave.

@Pilchie

This comment has been minimized.

Show comment
Hide comment
@Pilchie

Pilchie Jun 12, 2017

Member

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.

Member

Pilchie commented Jun 12, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment