Roll back warning for struct == null

[gafter]

Roslyn added a new warning compared to the native compiler: if you attempt to compare a struct value to null

  if (structValue == null) ...

Roslyn now gives you a warning (the value in this case is always false). It still typechecks because the comparison operator selected is operator==(Struct? left, Struct? right).

Unfortunately this is a breaking change for someone who instructs the compiler to treat warnings as errors. So the warning should be removed.

In the longer run we want a mechanism to add warnings in which users would be required to opt-in before receiving any new warnings. When that is done this warning can be restored as one you'd need to opt in to. But that is a separate issue.

[eatdrinksleepcode]

I don't agree with removing the warning, or with the planned "opt-in" mechanism, for the same reason: those who have set the compiler to treat warnings as errors have already indicated their strong desire to prevent obvious errors in their code base. They have already "opted in" by turning on that option in the first place; we shouldn't force them to do so again.

If a new warning is preventing the upgrade of an established code base to a new runtime/framework, that warning can be disabled. If for some reason the mechanisms for disabling warnings are not sufficient, then that is the issue that needs to be addressed. But not adding a warning about an obvious code error because it will break the build - when the owner of that build has already told you that is exactly what he wants - doesn't make any sense to me.

[gafter]

When upgrading to a new version of our tools breaks someone's build, it doesn't matter what options have been set - they are likely to reject the new version of the tools. Your logic only works if the person responsible for upgrading the tools, the person who controls the code, and the person who decided to use warnings-as-errors, are all the same person. They are usually not the same person in a larger organization.

[eatdrinksleepcode]

I am well aware of the absurdity of the decision making process in many organizations (many of them not so large at all). That has no bearing whatsoever on this decision. It is not up to us to decide policy at such organizations, nor should we be catering to those who have created such silos at the expense of those who haven't. It is entirely reasonable for an organization which does not have appropriate measures in place to deal with the consequences of a major framework upgrade to be unable to perform that upgrade. Why should we allow the rigidness of some to degrade the experience for all?

Think about it this way: in an organization like you describe, the person who had the authority to mandate "warnings as errors" (and also mandate that the warning could not be disabled) would also mandate opting-in to the new warnings as well. The only reason they wouldn't do that would be if they were unaware of the new mechanism, at which point you have essentially subverted the purpose of the "warnings as errors" option in the first place.

[gafter]

Having worked on compiler products at a half-dozen different companies, I have a pretty good feel based on hard-learned lessons what works well and what does not work well for the customers. What seems absurd to you is normal in the real world. Breaking compatibility, even in the most technical sense of adding new warnings that could be treated as errors, when the user does not change the command-line, is not good for adoption of the new versions of the tools. The decision to adopt the new set of tools should not automatically imply that customers also receive newly implemented diagnostics. On the other hand, it should be even easier for customers to ask for the newly implemented diagnostics than it is to upgrade to the new tools.

The mechanism to opt-in to new warnings that we envision would not have a mechanism that would allow you to opt in to future warnings and set warnings as errors at the same time. Or rather, if you do that we'd give you a warning that you are subjecting yourself to possible future compatibility issues when new warnings are implemented. And since you set warnings as errors, that warning will be an error. If you suppress that warning, then you've really asked for it.

[HaloFour]

I agree with both sides, but I think that I would probably lean towards @eatdrinksleepcode 's opinion mainly because any organization applying such strict build rules should also have a strict set of guidelines in terms of vetting any changes to the environment, including compiler/framework upgrades.

I don't think that this is any different than annotating an existing class or member with ObsoleteAttribute. That would also generate a compiler warnings and break builds where the compiler is set to treat warnings as errors.

Even with nothing else changing someone maintaining that code base would probably want to know that they're doing something improper so that they can fix it.

[eatdrinksleepcode]

What seems absurd to you is normal in the real world.

I am well aware of what happens in the "real world": I live here and work here every day. The fact that these situations exist does not make them any less absurd; nor does it mean that we should be catering to them.

The decision to adopt the new set of tools should not automatically imply that customers also receive newly implemented diagnostics. On the other hand, it should be even easier for customers to ask for the newly implemented diagnostics than it is to upgrade to the new tools.

The problem with this viewpoint is that it stands directly in contrast to the "pit of success". You are basically saying that an organization that has siloed decision making and refuses to respond to change should be prioritized over an organization that wants to do the right thing and respond to change. An organization that has claimed to want to know about errors, but in actuality doesn't want to know and isn't actually going to do anything about them, should be prioritized over the organization that wants to know and wants to fix them. I don't understand how you can justify that prioritization.

The mechanism to opt-in to new warnings that we envision would not have a mechanism that would allow you to opt in to future warnings and set warnings as errors at the same time. Or rather, if you do that we'd give you a warning that you are subjecting yourself to possible future compatibility issues when new warnings are implemented. And since you set warnings as errors, that warning will be an error. If you suppress that warning, then you've really asked for it.

...what??

Why on earth would you design this "feature" this way? Why would you make it so difficult for those who want to do the right thing? Of course I want both of these settings on at the same time; why wouldn't I? If for some reason there is a new warning that am not willing to fix before upgrading, I can always turn the setting off at that point; but of course I at least want to know about the warnings before making that decision!

If this feature were implemented this way, the first thing I would do every time I get to a new code base (which is often in my current job) would be to enable both of these settings. I cannot for the life of me understand why you would force me to jump through these hoops when all I want is to actively keep my code base clean and accurate.

[eatdrinksleepcode]

@HaloFour

I don't think that this is any different than annotating an existing class or member with ObsoleteAttribute.

You're absolutely right, it is very similar. Unfortunately, the .NET team has often refused to apply the ObsoleteAttribute for exactly the same reasons that have been described in this thread (and I have often raised the same objections to that reasoning).

[gafter]

@eatdrinksleepcode One person's "right" is another person's "wrong".

We need a solution so that those customers who want to upgrade to a new version of the toolchain (for example, because a bug in the previous toolchain affects them) should be able to do that without changing anything else (including their command line and their code).

Those customers, such as yourself, who want all the new warnings we are able to provide and are willing to change their code or command line when upgrading, should have a solution that enables them to do exactly that.

The solution we will provide addresses the needs of both of these groups. The solution you advocate addresses the need of only the second group.

[sharwell]

@gafter The warning in question here caught a real bug in some code I developed. It would be disappointing to see it completely removed. Looking at #738, it seems like there is still a way to have the compiler report this warning ("strict" mode?). Can you clarify the specific steps users would need to take to still have it reported?

[gafter]

To continue to see this warning and others, use the compiler flag /feature:strict. In the future we'll provide a more flexible and supported mechanism to control which warning "waves" you want to enable.

[gafter]

@agocke Can you please "Verify" that this is "Fixed"?