Code Cleanup | null != x => x != null

[benrr101]

Description: In the second of a series of code cleanup PRs, this PR removes the Yoda conditions null != x and replaces it with x != null. This is the most requested PR from my second PR, so hopefully it will be appreciated :)

As with the previous PR, I have opted to only flip the operands and not replace the condition with x is not null. This is to make the changes as safe as possible, since is not null will potentially behave differently than != null.

AI Analysis: I ran the patch file through a large language model to assess safety but uhhhh, it didn't like me. and it seems to approve of this PR

Testing: projecs build andif CI goes through we should be good

[Tornhoof]

AI Analysis: I ran the patch file through a large language model to assess safety but uhhhh, it didn't like me. and it seems to approve of this PR

Your AI Analysis is not really correct afaik, technically they are different:

x != null calls the inequality operator != with x on the left hand and null != x with x on the right hand.
If there are maniacs who implement != that wrong, i.e. with different null behavior for left hand vs. right hand the result will be different. You can run https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEUCuA7AHwAEAmARgFgAoIgZgAJT6Bhegb2vq8ZJPc+6CAsgE9mAGwCGAZ2n0E9ALz08MAO71REmdLYA1SeJwwl9MgF8A3AMFcAlgDMAFHhzjx9AITKEAShu2HFS2IYxkAJxOAESu7lG+1sGh5gGCjk4K3ipu4v5JIUGhtkQR0bHi8YlFKfm2qdzpCnZyeBAY2e55RYVF3CWRMTmV9Vw1gjU11HQ8mmJSstQ9XNN2eO0GRiZsAOYwGJbSe5Zj3AHTJUj0wBAQHhAADjBQkhjQSopOWvNyCGiz2rJ6CIutwlsUAOzyAB0G2M7yBMMMxiq3BOywYFyuNzuj2erygXg+Xx08j+xMBwICYMEI3ojTpLRyIIKtME/WiTUZcQStLRdVqaWcIgZHVytOpoXZUWFzVFwwFqNptKIkPkAH5EZtCUCNbCYCjRtRzEA=

for an example, to see the call difference.

That's why the whole is not null syntax is superior, it bypasses the inequality operator alltogether.

Having said that, the only way I know of, how x != null or null != x behaves differently than x is not null is, if someone overloads the inequality operator != in the source code, a short look through the code via GitHub code search only shows one instance in a test and that one looks fine. I assume here that types in external libraries, e.g. BCL are fine.

Personally I'd recommend normalizing x != null null != x to is not null as it is either equally wrong as your changes or equally correct, but infinitely more readable.

Note: The same applies to the previously merged PR with ==, I was just not fast enough there to comment on it.

[benrr101]

@Tornhoof I won't disagree too hard - you're right that it is possible someone wrote a ==/!= overload that treats left and right nulls differently, and you're right that a person doing that would be a maniac. I slightly disagree that replacing with x is not null would be equally wrong vs x != null. The way I see it, there is three scenarios

• != operator was overloaded to treat left and right null differently - maniacal, and would be broken with both replacements
• != operator was overloaded to treat null differently - not ideal, but is not null would break it, != null would not
• != operator is not overloaded - != null and is not null behave the same

It all likelihood, the last case is probably 90+% of the cases in the code, and in that case I would indeed advocate for using is not null. But, I want to take these changes one step at a time and try to break as little in the process as possible. I have a slew more of these cleanup PRs ready to go, and I plan to do one that addresses == null to is null at the same time as using conditional access and pattern matching (to some extent, not the x is { foo: bar } kind)

[codecov]

Codecov Report

Attention: Patch coverage is 75.31250% with 158 lines in your changes missing coverage. Please review.

Project coverage is 71.76%. Comparing base (6f177e3) to head (117ed98).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...etfx/src/Microsoft/Data/SqlClient/SqlDataReader.cs	34.88%	28 Missing ⚠️
...crosoft/Data/SqlClient/SqlInternalConnectionSmi.cs	0.00%	26 Missing ⚠️
...t/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs	72.72%	15 Missing ⚠️
...rc/Microsoft/Data/SqlClient/SqlClientPermission.cs	18.75%	13 Missing ⚠️
...n/src/Microsoft/Data/Common/NameValuePermission.cs	47.36%	10 Missing ⚠️
...fx/src/Microsoft/Data/Common/DBConnectionString.cs	0.00%	8 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs	82.35%	6 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs	85.71%	5 Missing ⚠️
...soft/Data/SqlClient/Server/SmiEventSink_Default.cs	28.57%	5 Missing ⚠️
...etfx/src/Microsoft/Data/SqlClient/SqlConnection.cs	84.00%	4 Missing ⚠️
... and 26 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2751      +/-   ##
==========================================
+ Coverage   71.74%   71.76%   +0.01%     
==========================================
  Files         308      308              
  Lines       62301    62314      +13     
==========================================
+ Hits        44700    44717      +17     
+ Misses      17601    17597       -4     

Flag	Coverage Δ
addons	92.90% <ø> (+0.02%)	⬆️
netcore	76.03% <89.00%> (+0.03%)	⬆️
netfx	69.71% <68.26%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JRahnama]

I think, we can also have RoslynAnalyzer to enforce some coding standards to prevent this in future as this cannot be done in editorconfig file.