From 1ae8b5cfb56c11f2af50388b642ea19ec0439e30 Mon Sep 17 00:00:00 2001
From: Manish Vasani <mavasani@microsoft.com>
Date: Mon, 14 Jan 2019 12:26:31 -0800
Subject: [PATCH] Fix assert in
 UsePatternMatching.CSharpAsAndNullCheckDiagnosticAnalyzer

1. Bail out early if there is a reference to assigned local between the declaration and null check being replaced with pattern matching.
2. Remove assert for bail out in presence of errors/unhandled cases.

Fixes #31388
---
 .../CSharpAsAndNullCheckTests.cs              | 21 +++++++++
 ...AndNullCheckDiagnosticAnalyzer.Analyzer.cs |  4 +-
 .../CSharpAsAndNullCheckDiagnosticAnalyzer.cs | 43 ++++++++++++-------
 3 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/src/EditorFeatures/CSharpTest/UsePatternMatching/CSharpAsAndNullCheckTests.cs b/src/EditorFeatures/CSharpTest/UsePatternMatching/CSharpAsAndNullCheckTests.cs
index 864d77960c5..479fec3a952 100644
--- a/src/EditorFeatures/CSharpTest/UsePatternMatching/CSharpAsAndNullCheckTests.cs
+++ b/src/EditorFeatures/CSharpTest/UsePatternMatching/CSharpAsAndNullCheckTests.cs
@@ -1373,6 +1373,27 @@ C M(object e)
         System.Func<C> f = () => c == null ? null : c;
         return c;
     }
+}");
+        }
+
+        [WorkItem(31388, "https://github.com/dotnet/roslyn/issues/31388")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInlineTypeCheck)]
+        public async Task TestUseBetweenAssignmentAndIfCondition()
+        {
+            await TestMissingInRegularAndScriptAsync(
+@"class C
+{
+    void M(object o)
+    {
+        [|var|] c = o as C;
+        M2(c != null);
+        if (c == null)
+        {
+            return;
+        }
+    }
+
+    void M2(bool b) { }
 }");
         }
     }
diff --git a/src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.Analyzer.cs b/src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.Analyzer.cs
index c46b8a6d65a..3ebab23e941 100644
--- a/src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.Analyzer.cs
+++ b/src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.Analyzer.cs
@@ -227,9 +227,7 @@ private bool CanSafelyConvertToPatternMatching()
                             return CheckStatement(statement);
                     }
 
-                    // We shouldn't normally get here but if we do, it's
-                    // either an error in the code or an unhandled case.
-                    Debug.Assert(false, "unhandled case.");
+                    // Bail out for error cases and unhandled cases.
                     break;
                 }
 
diff --git a/src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs b/src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs
index 5ba85f77a90..3439cb2d32e 100644
--- a/src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs
+++ b/src/Features/CSharp/Portable/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs
@@ -145,30 +145,41 @@ private void SyntaxNodeAction(SyntaxNodeAnalysisContext syntaxContext)
             //      if (s != null) { ... }
             //
             // It's no longer safe to use pattern-matching because 'field is string s' would never be true.
+            // 
+            // Additionally, also bail out if the assigned local is referenced (read or write) up to the point of null check.
+            //      var s = field as string;
+            //      MethodCall(flag: s == null);
+            //      if (s != null) { ... }
+            //
             var asOperand = semanticModel.GetSymbolInfo(asExpression.Left, cancellationToken).Symbol;
-            var symbolName = asOperand?.Name;
-            if (symbolName != null)
+            var localStatementStart = localStatement.SpanStart;
+            var comparisonSpanStart = comparison.SpanStart;
+
+            foreach (var descendentNode in enclosingBlock.DescendantNodes())
             {
-                var localStatementStart = localStatement.SpanStart;
-                var comparisonSpanStart = comparison.SpanStart;
+                var descendentNodeSpanStart = descendentNode.SpanStart;
+                if (descendentNodeSpanStart <= localStatementStart)
+                {
+                    continue;
+                }
 
-                foreach (var descendentNode in enclosingBlock.DescendantNodes())
+                if (descendentNodeSpanStart >= comparisonSpanStart)
                 {
-                    var descendentNodeSpanStart = descendentNode.SpanStart;
-                    if (descendentNodeSpanStart <= localStatementStart)
-                    {
-                        continue;
-                    }
+                    break;
+                }
 
-                    if (descendentNodeSpanStart >= comparisonSpanStart)
+                if (descendentNode.IsKind(SyntaxKind.IdentifierName, out IdentifierNameSyntax identifierName))
+                {
+                    // Check if this is a 'write' to the asOperand.
+                    if (identifierName.Identifier.ValueText == asOperand?.Name &&
+                        asOperand.Equals(semanticModel.GetSymbolInfo(identifierName, cancellationToken).Symbol) &&
+                        identifierName.IsWrittenTo())
                     {
-                        break;
+                        return;
                     }
 
-                    if (descendentNode.IsKind(SyntaxKind.IdentifierName, out IdentifierNameSyntax identifierName) &&
-                        identifierName.Identifier.ValueText == symbolName &&
-                        asOperand.Equals(semanticModel.GetSymbolInfo(identifierName, cancellationToken).Symbol) &&
-                        identifierName.IsWrittenTo())
+                    // Check if this is a reference to the assigned local.
+                    if (identifierName.Identifier.ValueText == localSymbol.Name)
                     {
                         return;
                     }
-- 
GitLab