Fix NullConditionalAssertion false positive (#170)

* Fix null conditional assertion tip

* Specify more testcases

* Ignore null conditional inside ArgumentList, but current implementation creates false negatives.

* Add boolean value for each scope level where relevant.

* Ordering

* Remove unused

* Visit ParenthesizedExpressionSyntax not necessary as ConditionalAccessExpressionSyntax already includes this.

* Rename variable

* Rename again

Author: renzo-baasdam

src/FluentAssertions.Analyzers.Tests/Tips/NullConditionalAssertionTests.cs

@@ -1,7 +1,7 @@
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using System.Text;
 
-namespace FluentAssertions.Analyzers.Tests
+namespace FluentAssertions.Analyzers.Tests.Tips
 {
     [TestClass]
     public class NullConditionalAssertionTests
@@ -10,13 +10,25 @@ public class NullConditionalAssertionTests
         [AssertionDiagnostic("actual?.Should().Be(expected{0});")]
         [AssertionDiagnostic("actual?.MyProperty.Should().Be(\"test\"{0});")]
         [AssertionDiagnostic("actual.MyProperty?.Should().Be(\"test\"{0});")]
+        [AssertionDiagnostic("(actual.MyProperty)?.Should().Be(\"test\"{0});")]
+        [AssertionDiagnostic("(actual?.MyProperty)?.Should().Be(\"test\"{0});")]
+        [AssertionDiagnostic("actual?.MyProperty.Should().Be(actual?.MyProperty{0});")]
+        [AssertionDiagnostic("actual.MyList?.Where(obj => obj?.ToString() == null).Count().Should().Be(0{0});")]
         [Implemented]
-        public void TestAnalyzer(string assertion) => VerifyCSharpDiagnostic(assertion);
+        public void NullConditionalMayNotExecuteTest(string assertion) => VerifyCSharpDiagnostic(assertion);
 
-        private void VerifyCSharpDiagnostic(string assertion)
-        {
-            var code = new StringBuilder()
+        [AssertionDataTestMethod]
+        [AssertionDiagnostic("(actual?.MyProperty).Should().Be(\"test\"{0});")]
+        [AssertionDiagnostic("actual.MyProperty.Should().Be(actual?.MyProperty{0});")]
+        [AssertionDiagnostic("actual.MyList.Where(obj => obj?.ToString() == null).Count().Should().Be(0{0});")]
+        [Implemented]
+        public void NullConditionalWillStillExecuteTest(string assertion) => VerifyCSharpDiagnosticPass(assertion);
+
+        private static string Code(string assertion) =>
+            new StringBuilder()
                 .AppendLine("using System;")
+                .AppendLine("using System.Collections.Generic;")
+                .AppendLine("using System.Linq;")
                 .AppendLine("using FluentAssertions;using FluentAssertions.Extensions;")
                 .AppendLine("namespace TestNamespace")
                 .AppendLine("{")
@@ -30,6 +42,7 @@ private void VerifyCSharpDiagnostic(string assertion)
                 .AppendLine("    class MyClass")
                 .AppendLine("    {")
                 .AppendLine("        public string MyProperty { get; set; }")
+                .AppendLine("        public List<object> MyList { get; set; }")
                 .AppendLine("    }")
                 .AppendLine("    class Program")
                 .AppendLine("    {")
@@ -40,16 +53,19 @@ private void VerifyCSharpDiagnostic(string assertion)
                 .AppendLine("}")
                 .ToString();
 
-            DiagnosticVerifier.VerifyCSharpDiagnostic<NullConditionalAssertionAnalyzer>(code, new DiagnosticResult
+        private static void VerifyCSharpDiagnosticPass(string assertion)
+            => DiagnosticVerifier.VerifyCSharpDiagnostic<NullConditionalAssertionAnalyzer>(Code(assertion));
+
+        private static void VerifyCSharpDiagnostic(string assertion)
+            => DiagnosticVerifier.VerifyCSharpDiagnostic<NullConditionalAssertionAnalyzer>(Code(assertion), new DiagnosticResult
             {
                 Id = NullConditionalAssertionAnalyzer.DiagnosticId,
                 Message = NullConditionalAssertionAnalyzer.Message,
                 Severity = Microsoft.CodeAnalysis.DiagnosticSeverity.Warning,
                 Locations = new DiagnosticResultLocation[]
                 {
-                    new DiagnosticResultLocation("Test0.cs", 9, 13)
+                    new DiagnosticResultLocation("Test0.cs", 11, 13)
                 }
             });
-        }
     }
 }

src/FluentAssertions.Analyzers/Tips/NullConditionalAssertionAnalyzer.cs

@@ -2,6 +2,7 @@
 using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Diagnostics;
+using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.Linq;
 
@@ -62,25 +63,33 @@ protected virtual Diagnostic AnalyzeExpression(ExpressionSyntax expression)
 
         private class ConditionalAccessExpressionVisitor : CSharpSyntaxWalker
         {
-            private bool _foundConditionalAccess;
-            private bool _foundShouldMethod;
+            private readonly Stack<bool> _foundConditionalAccess = new();
+            private bool _foundShouldMethodAfterConditionalAccessInSameScope;
 
-            public bool CodeSmells => _foundShouldMethod && _foundConditionalAccess;
-            public Location ConditionalAccess { get; private set; }
+            private bool FoundConditionalAccessInCurrentScope => _foundConditionalAccess.Any() && _foundConditionalAccess.Peek();
+
+            public bool CodeSmells => _foundShouldMethodAfterConditionalAccessInSameScope;
 
             public override void VisitConditionalAccessExpression(ConditionalAccessExpressionSyntax node)
             {
-                if (!_foundConditionalAccess)
-                {
-                    _foundConditionalAccess = true;
-                }
-                base.VisitConditionalAccessExpression(node);
+                Visit(node.Expression);
+                _foundConditionalAccess.Push(true);
+                Visit(node.WhenNotNull);
+                _foundConditionalAccess.Pop();
             }
+
+            public override void VisitArgumentList(ArgumentListSyntax node)
+            {
+                _foundConditionalAccess.Push(false);
+                base.DefaultVisit(node);
+                _foundConditionalAccess.Pop();
+            }
+
             public override void VisitIdentifierName(IdentifierNameSyntax node)
             {
-                if(_foundConditionalAccess && node.Identifier.ValueText == "Should")
+                if (FoundConditionalAccessInCurrentScope && node.Identifier.ValueText == "Should")
                 {
-                    _foundShouldMethod = true;
+                    _foundShouldMethodAfterConditionalAccessInSameScope = true;
                 }
             }
         }