Enable and refactor to fix IDE0031 Use null propagation

[elachlan]

Used the build in Analyzer's code fix.
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0031

The analyzer currently is broken and doesn't take into account #if DEBUG statements.
Issue: dotnet/roslyn#65880

Related: #7887

Microsoft Reviewers: Open in CodeFlow

[elachlan]

IDE0031 seems to not take into account #if DEBUG statements.

[RussKie]

Thank you. Just to confirm, are there any manual changes or all changes are done by the auto fixers?

[RussKie]

The build is failing.

[elachlan]

The build is failing.

The analyzer is broken and doesn't take into account #if DEBUG statements. I am looking for where to raise an issue for it.

[elachlan]

@RussKie could we remove the #if DEBUG statements here? Does Debug.Assert generate IL on release builds?

[elachlan]

@RussKie could we remove the #if DEBUG statements here? Does Debug.Assert generate IL on release builds?

I just checked on sharplab.io, release builds remove the call to Debug.Assert so #if DEBUG is redundant. Unless I am missing some sort of config difference.

[RussKie]

No, we can't simply remove #if DEBUG - some of those guard more than just Debug.Assert calls. The simplest thing to do is to suppress IDE0031 in these specific cases, e.g.

diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs
index 4d3f98914..6e4d8a2c5 100644
--- a/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs
+++ b/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs
@@ -10915,6 +10915,7 @@ namespace System.Windows.Forms
 #if DEBUG
             int suspendCount = -44371;      // Arbitrary negative prime to surface bugs.
 #endif
+#pragma warning disable IDE0031 // Use null propagation
             if (ParentInternal is not null)
             {
 #if DEBUG
@@ -10922,6 +10923,7 @@ namespace System.Windows.Forms
 #endif
                 ParentInternal.SuspendLayout();
             }
+#pragma warning restore IDE0031 // Use null propagation
 
             try
             {

[RussKie]

Does Debug.Assert generate IL on release builds?

I don't think so, https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcAMACOSB0AIgJZgDmAdgPYTDEDGEA3OunAMy4LYDC2A3umzDcnOABZsAWQAUAIypUANtgBmASgFCROwgFM5AVzL4AghAh6ATsBmqANNgBEcpWCfqWaHQF90PoA

[elachlan]

Does Debug.Assert generate IL on release builds?

I don't think so, https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcAMACOSB0AIgJZgDmAdgPYTDEDGEA3OunAMy4LYDC2A3umzDcnOABZsAWQAUAIypUANtgBmASgFCROwgFM5AVzL4AghAh6ATsBmqANNgBEcpWCfqWaHQF90PoA

In cases where an #if DEBUG is used to guard only Debug.Assert, can I remove the #if DEBUG?

[RussKie]

Yes, but you may find that there are variables that are defined in DEBUG only, e.g., in TransactionLayout class.

[elachlan]

Yes, but you may find that there are variables that are defined in DEBUG only, e.g., in TransactionLayout class.

Thanks. I will add the supressions and remove any #if DEBUG guards if it makes sense.

[RussKie]

If I may ask you to consider adding fixup commits or just additional commits to ease the reviews whenever the changeset are this large. F-push makes it very hard to appreciate what changed.
We generally squash merge changes in this repo.
Thanks

[elachlan]

If I may ask you to consider adding fixup commits or just additional commits to ease the reviews whenever the changeset are this large. F-push makes it very hard to appreciate what changed. We generally squash merge changes in this repo. Thanks

Not a problem! I will do so in the future, I wasn't sure if you did squash merges and didn't want to pollute the main branch.

[RussKie]

Thank you for the consideration 👍