Code Cleanup | `null == x` to `x == null` #2749

benrr101 · 2024-08-02T17:25:51Z

Description: This PR is part of a series that aims to remove "yoda conditions" from the codebase. In my opinion, these make the code harder to read due to the natural tendency to read an if condition like "x equals y". In this case "null equals x" is unnatural compared to "x equals null". In most if not all cases, there is no semantic difference between x == null and null == x, meaning these changes can be safely made without impact to the behavior.

AI Analysis: Since this PR is large and boring for people to review, I ran the patch through a large language model to ask it to assess the safety of it. Here's is its analysis:

Testing: The code builds, I will wait for CI to verify it more thoroughly.

Note, this is only part 1 of several parts

DavoudEshtehari

Thanks for cleaning them up; I always want to fix these conditions.

The != conditions can be addressed similarly. I started suggesting the code change, but it would be better to be done on your side. I'll push my suggestions so far. Also, using is or is not instead of == or != can be slightly more efficient because it avoids the potential overhead of an overloaded operator. The approach you're taken is the safest, but feel free to choose the other one if you're ready for the potential headache of verifying possible overloads.

DavoudEshtehari · 2024-08-02T17:43:22Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/Common/DbConnectionOptions.cs

@@ -43,7 +43,7 @@ internal static string ExpandDataDirectory(string keyword, string value)
                // find the replacement path
                object rootFolderObject = AppDomain.CurrentDomain.GetData("DataDirectory");
                var rootFolderPath = (rootFolderObject as string);
-                if ((null != rootFolderObject) && (null == rootFolderPath))
+                if ((null != rootFolderObject) && rootFolderPath == null)


Suggested change

if ((null != rootFolderObject) && rootFolderPath == null)

if ((rootFolderObject != null) && rootFolderPath == null)

DavoudEshtehari · 2024-08-02T17:44:03Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs

@@ -37,7 +37,7 @@ protected internal Transaction EnlistedTransaction
            set
            {
                Transaction currentEnlistedTransaction = _enlistedTransaction;
-                if (((null == currentEnlistedTransaction) && (null != value))
+                if ((currentEnlistedTransaction == null && (null != value))


Suggested change

if ((currentEnlistedTransaction == null && (null != value))

if ((currentEnlistedTransaction == null && (value != null))

DavoudEshtehari · 2024-08-02T17:45:50Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs

@@ -85,8 +85,7 @@ override protected DbConnectionInternal CreateConnection(DbConnectionOptions opt
                redirectedUserInstance = true;
                string instanceName;

-                if ((null == pool) ||
-                     (null != pool && pool.Count <= 0))
+                if (pool == null || (null != pool && pool.Count <= 0))


Suggested change

if (pool == null || (null != pool && pool.Count <= 0))

if (pool == null || (pool != null && pool.Count <= 0))

DavoudEshtehari · 2024-08-02T17:46:58Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs

-                          || null == _currentTransaction
-                          || null == value
+                          || _currentTransaction == null
+                          || value == null
                          || (null != _currentTransaction && !_currentTransaction.IsLocal), "attempting to change current transaction?");


Suggested change

|| (null != _currentTransaction && !_currentTransaction.IsLocal), "attempting to change current transaction?");

|| (_currentTransaction != null && !_currentTransaction.IsLocal), "attempting to change current transaction?");

DavoudEshtehari · 2024-08-02T17:47:15Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs

-                if ((null == _currentTransaction && null != value)
-                  || (null != _currentTransaction && null == value))
+                if ((_currentTransaction == null && null != value)
+                    || (null != _currentTransaction && value == null))


Suggested change

|| (null != _currentTransaction && value == null))

|| (_currentTransaction != null && value == null))

DavoudEshtehari · 2024-08-02T17:51:21Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/DBConnectionString.cs

@@ -160,7 +160,7 @@ internal string Restrictions
            get
            {
                string restrictions = _restrictions;
-                if (null == restrictions)
+                if (restrictions == null)
                {
                    string[] restrictionValues = _restrictionValues;
                    if ((null != restrictionValues) && (0 < restrictionValues.Length))


Suggested change

if ((null != restrictionValues) && (0 < restrictionValues.Length))

if ((restrictionValues != null) && (0 < restrictionValues.Length))

DavoudEshtehari · 2024-08-02T17:51:42Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/Common/DbConnectionOptions.cs

                {
                    // find the replacement path
                    object rootFolderObject = AppDomain.CurrentDomain.GetData("DataDirectory");
                    rootFolderPath = (rootFolderObject as string);
-                    if ((null != rootFolderObject) && (null == rootFolderPath))
+                    if ((null != rootFolderObject) && rootFolderPath == null)


Suggested change

if ((null != rootFolderObject) && rootFolderPath == null)

if ((rootFolderObject != null) && rootFolderPath == null)

DavoudEshtehari · 2024-08-02T17:52:08Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionFactory.cs

@@ -449,7 +449,7 @@ internal DbConnectionPoolGroup GetConnectionPoolGroup(DbConnectionPoolKey key, D
                }

                // We don't support connection pooling on Win9x; it lacks too many of the APIs we require.
-                if ((null == poolOptions) && ADP.s_isWindowsNT)
+                if (poolOptions == null && ADP.s_isWindowsNT)
                {
                    if (null != connectionPoolGroup)


Suggested change

if (null != connectionPoolGroup)

if (connectionPoolGroup != null)

DavoudEshtehari · 2024-08-02T17:52:24Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionFactory.cs

@@ -492,7 +492,7 @@ internal DbConnectionPoolGroup GetConnectionPoolGroup(DbConnectionPoolKey key, D
                Debug.Assert(null != connectionPoolGroup, "how did we not create a pool entry?");


Suggested change

Debug.Assert(null != connectionPoolGroup, "how did we not create a pool entry?");

Debug.Assert(connectionPoolGroup != null, "how did we not create a pool entry?");

DavoudEshtehari · 2024-08-02T17:52:39Z

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionFactory.cs

@@ -492,7 +492,7 @@ internal DbConnectionPoolGroup GetConnectionPoolGroup(DbConnectionPoolKey key, D
                Debug.Assert(null != connectionPoolGroup, "how did we not create a pool entry?");
                Debug.Assert(null != userConnectionOptions, "how did we not have user connection options?");


Suggested change

Debug.Assert(null != userConnectionOptions, "how did we not have user connection options?");

Debug.Assert(userConnectionOptions != null, "how did we not have user connection options?");

David-Engel

I also prefer this direction. 👍

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlClientPermission.cs

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/NameValuePair.cs

codecov · 2024-08-02T18:50:44Z

Codecov Report

Attention: Patch coverage is 76.27119% with 84 lines in your changes missing coverage. Please review.

Project coverage is 71.69%. Comparing base (6f177e3) to head (2d236c9).

Files	Patch %	Lines
...ata/SqlClient/Server/SmiEventSink_Default.netfx.cs	0.00%	17 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs	80.55%	7 Missing ⚠️
...fx/src/Microsoft/Data/Common/DBConnectionString.cs	0.00%	6 Missing ⚠️
...rc/Microsoft/Data/SqlClient/SqlClientPermission.cs	40.00%	6 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs	82.35%	6 Missing ⚠️
...crosoft/Data/SqlClient/Server/SmiContextFactory.cs	0.00%	5 Missing ⚠️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs	70.00%	3 Missing ⚠️
...n/src/Microsoft/Data/Common/NameValuePermission.cs	40.00%	3 Missing ⚠️
...t/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs	81.25%	3 Missing ⚠️
...etfx/src/Microsoft/Data/SqlClient/SqlConnection.cs	72.72%	3 Missing ⚠️
... and 20 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2749      +/-   ##
==========================================
- Coverage   71.74%   71.69%   -0.06%     
==========================================
  Files         308      308              
  Lines       62301    62303       +2     
==========================================
- Hits        44700    44668      -32     
- Misses      17601    17635      +34

Flag	Coverage Δ
addons	`92.90% <100.00%> (+0.02%)`	⬆️
netcore	`75.92% <86.88%> (-0.08%)`	⬇️
netfx	`69.70% <70.78%> (-0.03%)`	⬇️

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.

benrr101 · 2024-08-02T18:55:57Z

@DavoudEshtehari @David-Engel Once this PR goes through, I have all the code changes lined up for doing null != x to x != null as well as several others for numbers and other constants. I originally started with making the change to x is null but since there is that potential for it to call an overloaded == operator, I wanted to err on the side of caution for safest change rather than risk breaking something in a very hard to detect way.

…t/TdsParser.cs Co-authored-by: David Engel <[email protected]>

roji · 2024-08-02T21:27:43Z

Just noting that it's becoming increasingly common/standard to do x is null and x is not null rather than x == null and x != null; with the latter, operator overloading can change the meaning of the expression, whereas with the former you know you're always checking for null.

benrr101 · 2024-08-02T22:08:05Z

@roji Acknowledged - I think I'm being overly cautious here just because it wouldn't surprise me if buried somewhere in the code there's an == overload that treats null as 1 or something ridiculous. (I suppose it's possible too that the there could be an overload that makes == not commutative, but I hope that didn't happen) I didn't want to dive into each of the operators to see if they have unexpected behavior, so this is the fastest, safest option.

We can convert to is/is not in another PR.

Wraith2 · 2024-08-03T14:54:40Z

Did you make the changes manually or did you write an analyzer+codefixer to do it for you?

benrr101 · 2024-08-03T16:47:15Z

@Wraith2 definitely not the latter, but not entirely the former. I had some automated finding and making the changes, but I assessed each one (very quickly, that is) as I made the change.

commit ac1012a Author: dauinsight <[email protected]> Date: Fri Aug 16 13:22:55 2024 -0700 Hotfix v3.1.6 Release notes (dotnet#2767) commit 619fa74 Author: Javad Rahnama <[email protected]> Date: Thu Aug 15 13:48:15 2024 -0700 Fix | Fix the issue with Socke.Connect in managed SNI (dotnet#2777) commit d3658ed Author: DavoudEshtehari <[email protected]> Date: Wed Aug 14 17:18:51 2024 -0700 Update SNI version to 6.0.0-preview1.24226.4 (dotnet#2772) commit 7216e84 Author: Benjamin Russell <[email protected]> Date: Fri Aug 9 12:40:30 2024 -0500 `null != x` => `x != null` (dotnet#2751) commit 6fbbb78 Author: Benjamin Russell <[email protected]> Date: Sat Aug 3 11:47:53 2024 -0500 Code Cleanup | `null == x` to `x == null` (dotnet#2749) - With one approval and CI success, I think that's enough to move ahead on this one. * `null == x` to `x == null` * Update src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs Co-authored-by: David Engel <[email protected]> --------- Co-authored-by: David Engel <[email protected]> commit 6f177e3 Author: Aris Rellegue <[email protected]> Date: Tue Jul 30 19:52:38 2024 +0000 Fix | Fixed some Azure managed identity authentication unit test failures (dotnet#2652)

null == x to x == null

1e63611

benrr101 requested review from saurabh500, mdaigle, JRahnama, DavoudEshtehari and samsharma2700 August 2, 2024 17:25

DavoudEshtehari added the ➕ Code Health Changes related to source code improvements label Aug 2, 2024

DavoudEshtehari reviewed Aug 2, 2024

View reviewed changes

David-Engel reviewed Aug 2, 2024

View reviewed changes

Update src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClien…

2d236c9

…t/TdsParser.cs Co-authored-by: David Engel <[email protected]>

David-Engel approved these changes Aug 2, 2024

View reviewed changes

benrr101 merged commit 6fbbb78 into dotnet:main Aug 3, 2024
131 checks passed

benrr101 deleted the russellben/yoda-equals-null branch August 3, 2024 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Cleanup | `null == x` to `x == null` #2749

Code Cleanup | `null == x` to `x == null` #2749

benrr101 commented Aug 2, 2024

DavoudEshtehari left a comment

DavoudEshtehari Aug 2, 2024

DavoudEshtehari Aug 2, 2024

DavoudEshtehari Aug 2, 2024

DavoudEshtehari Aug 2, 2024

DavoudEshtehari Aug 2, 2024

DavoudEshtehari Aug 2, 2024

DavoudEshtehari Aug 2, 2024

DavoudEshtehari Aug 2, 2024

DavoudEshtehari Aug 2, 2024

DavoudEshtehari Aug 2, 2024

David-Engel left a comment

codecov bot commented Aug 2, 2024 •

edited

Loading

benrr101 commented Aug 2, 2024

roji commented Aug 2, 2024

benrr101 commented Aug 2, 2024

Wraith2 commented Aug 3, 2024

benrr101 commented Aug 3, 2024

	if ((null != rootFolderObject) && rootFolderPath == null)
	if ((rootFolderObject != null) && rootFolderPath == null)

	if ((currentEnlistedTransaction == null && (null != value))
	if ((currentEnlistedTransaction == null && (value != null))

	if (pool == null \|\| (null != pool && pool.Count <= 0))
	if (pool == null \|\| (pool != null && pool.Count <= 0))

	\|\| (null != _currentTransaction && !_currentTransaction.IsLocal), "attempting to change current transaction?");
	\|\| (_currentTransaction != null && !_currentTransaction.IsLocal), "attempting to change current transaction?");

	\|\| (null != _currentTransaction && value == null))
	\|\| (_currentTransaction != null && value == null))

	if ((null != restrictionValues) && (0 < restrictionValues.Length))
	if ((restrictionValues != null) && (0 < restrictionValues.Length))

	if (null != connectionPoolGroup)
	if (connectionPoolGroup != null)

		@@ -492,7 +492,7 @@ internal DbConnectionPoolGroup GetConnectionPoolGroup(DbConnectionPoolKey key, D
		Debug.Assert(null != connectionPoolGroup, "how did we not create a pool entry?");

	Debug.Assert(null != connectionPoolGroup, "how did we not create a pool entry?");
	Debug.Assert(connectionPoolGroup != null, "how did we not create a pool entry?");

		@@ -492,7 +492,7 @@ internal DbConnectionPoolGroup GetConnectionPoolGroup(DbConnectionPoolKey key, D
		Debug.Assert(null != connectionPoolGroup, "how did we not create a pool entry?");
		Debug.Assert(null != userConnectionOptions, "how did we not have user connection options?");

	Debug.Assert(null != userConnectionOptions, "how did we not have user connection options?");
	Debug.Assert(userConnectionOptions != null, "how did we not have user connection options?");

Code Cleanup | null == x to x == null #2749

Code Cleanup | null == x to x == null #2749

Conversation

benrr101 commented Aug 2, 2024

DavoudEshtehari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

David-Engel left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 2, 2024 • edited Loading

Codecov Report

benrr101 commented Aug 2, 2024

roji commented Aug 2, 2024

benrr101 commented Aug 2, 2024

Wraith2 commented Aug 3, 2024

benrr101 commented Aug 3, 2024

Code Cleanup | `null == x` to `x == null` #2749

Code Cleanup | `null == x` to `x == null` #2749

codecov bot commented Aug 2, 2024 •

edited

Loading