Warning Waves plan of record

[gafter]

• Implement compiler flag /warnversion #45702: Compiler will implement a /warnversion:version extend the /warn:level command-line switch
  1. The version is going to start at 5 and increase monotonically with new compiler releases. It is an integral value.
     Open Question: do we bump the number for .NET releases or language releases? Today they will be the same but what if that changes?
  2. For each warning, the compiler knows which warning level it is associated with.
• roslyn-analyzers#3822: Projects targeting .NET 5 will pass the /warn:5 flag to the compiler. Newer target frameworks will pass a newer number to the compiler (dependent on the resolution to the question in 1.)
• roslyn-analyzers#3823: There will be a central MSBuild property that can be used to configure what /warn is passed to the compiler.
  1. This will need to be plumbed through VS tooling including property pages
• Enable existing "strict" warnings under /warnversion #45704: Tie initial warning level to the expression of rules currently behind the strict flag
  • CS8073: warn when expression is always false or true
  • CS0185: do not allow locks on non-reference types
  • CS7023: do not allow as or is on static types
  • CS0165: track definite assignment of structs across assemblies
  • CS0019: additional error for type conversions
  • CS0149: Do not accept refs to delegate expressions
  • CS0177: Do not ignore private instance fields of structs imported through metadata
• Warnings at level 5 and above will be documented at https://github.com/dotnet/roslyn/blob/master/docs/compilers/CSharp/Warnversion%20Warning%20Waves.md
  We will create separate issues to track these work items and link from here.

This issue replaces #1580

This will target C#. If we want to add warnings to VB under waves in the future we can extend the work to VB.

[alrz]

Compiler will implement a /warnversion:version command-line switch. The version is going to start at 5 and increase monotonically with new compiler releases.

This is strangely compatible with -warn (defaults to level four). Any particular reason we don't use that existing option?

[gafter]

We met and discussed what to do about the command-line for warning waves.

Options:

1. /warn and an integer
2. /warnversion and a language version
3. /warnversion and a decimal
4. /warn and a string

We selected option 1.

Note that /warn is an existing command-line option that has wanrning “levels” from 0 to 4. We’re just adding new “levels” going forward.

• The first new warning wave is 5. We’ll increment it for each new warning wave.
• Default is 4 (like it always was)
• The option and command-line accept any value >= 0.

We’re not going to have a new command-line flag or a new property of CSharpCommandLine, we’re just going to extend the meaning of the existing flag.

When producing warnings in the compiler, we will just produce them unconditionally like we do everywhere else today. They are filtered in a post-pass based on the warning level, pragmas, editorconfig, and other mechanisms.

[roji]

Possibly allow an asterisk to float to the latest wave?

[PathogenDavid]

Staying on the latest wave strikes me as a bad practice potentially dangerous if done without understanding the consequences. (Edited to be less inflammatory.) One could easily (unknowingly) combine it with warn as error, causing code to fail to build with future compiler updates, which is what warning waves is meant to fix.

If latest-wave is added, then I think warnaserror would need to be modified to indicate up to which warning levels should be errors (and not allow latest-wave.)

[roji]

@PathogenDavid that's an opt-in decision for users though, isn't it? The same concern applies to users floating nuget dependency versions, for example. Note also that new warnings don't appear without an update of the compiler/SDK, which is already (typically) an action on the user's part.

Some users are probably going to want to just always be on the cutting edge of compiler warnings, and would be fine with the build suddenly breaking because some have been added...

[PathogenDavid]

The problem is people do things without understanding the implications. I'm all about "play stupid games win stupid prizes", but I don't think it'll be immediately obvious to most why latest-wave with warn-as-error is a bad idea.

I also don't feel like that logic scales when you have multiple developers working on a project. You get one developer enabling latest-wave because it seems like a good idea to always have the latest and greatest compiler diagnostics. You get a different developer enabling warn-as-error because we don't have warnings today and we don't want any in the future.

It'd also be a great way to get "Works fine on my machine."-type issues which are always a huge pain.

Being on the latest compiler/SDK is a whole different ball game because they strive for backwards compatibility, warning waves aren't backwards-compatible by design.

[alrz]

We're in the same situation with /langversion:latest. You could turn it on and older compilers will happily accept it but it might cause syntax errors or semantic changes in some cases.

[jnm2]

...and with just /langversion:8 and /nullable:enable, we've had our build broken by three consecutive non-preview releases of VS.

[PathogenDavid]

You could turn it on and older compilers will happily accept it

Except that's an issue in the other direction, it's a forward-compatibility issue.

The language is generally backwards-compatible. You can use /langversion:latest today and your code will still work tomorrow. You can't use /warn:* today and expect your code to work tomorrow. It's the future, it is unknowable.

I don't want to be dealing with futzing with the project files in 2030 to build some legacy code because someone in 2021 decided /warn:* /warnaserror was a good idea.

The same concern applies to users floating nuget dependency versions

@roji I kinda skipped over this, but I think it's more obvious why this might be a bad idea when you do it. Also I like to hope anyone using floating versions is also using a comitted lock file.

(Also personally your floating NuGet versions and /langversion:latest examples make me think those features were a mistake more than thinking this one should exist.)

[roji]

@PathogenDavid I don't generally use floating nuget versions (or even /langversion:latest), but some users do and there are valid reasons for doing so.

At the end of the day it's just an opt-in feature, you don't have to use it. I can see myself turning it on for an active project, to make sure I always have the best code quality. If that becomes a problem in 2030, it can just be pinned back again.

[PathogenDavid]

Sure, but it's an opt-in feature that has the potential to undo everything warning waves is meant to fix. Right now the compiler can't add warnings that can apply to previously valid code without breaking people because warn-as-error is common.

Come time for .NET 5+n they can't add new warning waves because using latest-wave with warn-as-error is common.

---

Also I'd like to reiterate this:

If latest-wave is added, then I think warnaserror would need to be modified to indicate up to which warning levels should be errors (and not allow latest-wave.)

I'm not opposed to latest-wave entirely (in fact I see uses for it myself), but I'm concerned it'd be a bad idea without additional guardrails to ensure it doesn't get us back into the same situation we're in today.

[CyrusNajmabadi]

Sure, but it's an opt-in feature that has the potential to undo everything warning waves is meant to fix.

I disagree. WArning waves are about allowing the user to express their preference on this. Specifically, it addresses the primary problem we have today where we cannot distinguish what a user prefers and have to default to taking a more conservative and less destabilizing approach for the entire audience. Warning waves allow users to let us know "yes, i am ok with breaking as i view the value of knowing about problems to be higher than the negative of being broken". Indeed, some people (myself included) view being broken as a good thing. i.e. it's worse to ship broken stuff silently, rather than know about it and have the issue blocked immediately.

With warning waves, users can now indicate what they want. And if someone wants to opt into a floating version exactly because they value being stopped when there are issues, that's completely reasonable to me.

[PathogenDavid]

"yes, i am ok with breaking as i view the value of knowing about problems to be higher than the negative of being broken"

There's another feature very similar to this, it's called warnaserror.

Jokes aside, this type of developer is the reason why warn-as-error exists, right? For teams and developers who view any warning as wholly unacceptable and won't allow any of them for any reason ever.

Indeed, some people (myself included) view being broken as a good thing.

Don't get me wrong, I love broken builds over broken software. That's why I've been frustrated that every time there's a proposal to add a new warning to the compiler, it gets shut down for compat reasons because it breaks people with warn-as-error.

I don't think it's unreasonable for me to be concerned that introducing a floating warning level threatens to bring back that problem.

I'm not even trying to say that a floating warning level is an objectively terrible idea (however I can see why my initial comment might've been interpreted that way.) I think it could be a dangerous idea that requires special consideration. I also don't think it's unreasonable for me to say I think warn-as-error should be aware of warning waves. Heck, you could even have it so TreatWarningsAsErrors doesn't apply to a floating warning level unless the user specifically opts-in as a "I'm very sure I want this code to break when there's new warnings."

---

All that being said,

I disagree. WArning waves are about allowing the user to express their preference on this.

If that's how the compiler team views this feature, then I guess I have nothing to be concerned about.

[CyrusNajmabadi]

I don't think it's unreasonable for me to be concerned that introducing a floating warning level threatens to bring back that problem.

It doesn't bring back the problem because a floating-warning is not a default, it's opt-in by the user. The user is stating: i explicitly want to be informed without an explicit future step about future issues.

The problem we have today is there is no way for users to indicate they are in the group. So we have to assume they aren't. With a flag, the user has given us that information and the issue is no longer there.

[PathogenDavid]

That makes sense, thanks for addressing my concerns.

I do still worry about how it might unintentionally interact with warn-as-error when people aren't thinking about the long-term big picture, but I'll live. I've made good money off of reviving ancient software made by those sorts of devs, so I suppose I shouldn't complain. 😅

[jaredpar]

Closing as the overall warning waves feature is now implemented and shipping with C# 9. There are a few warnings listed in this issue that didn't make it into 16.8 (time constraint). We will reconsider those in future warning wave iterations.