Do not offer 'use pattern matching' when it would change semantics

src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.Analyzer.cs

@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System.Diagnostics;
+using System.Linq;
 using System.Threading;
 using Microsoft.CodeAnalysis.CSharp.Extensions;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
@@ -19,7 +20,8 @@ private readonly struct Analyzer
         private readonly ILocalSymbol _localSymbol;
         private readonly ExpressionSyntax _comparison;
         private readonly ExpressionSyntax _operand;
-        private readonly SyntaxNode _localStatement;
+        private readonly LocalDeclarationStatementSyntax _localStatement;
+        private readonly VariableDeclaratorSyntax _declarator;
         private readonly SyntaxNode _enclosingBlock;
         private readonly CancellationToken _cancellationToken;
 
@@ -28,7 +30,8 @@ private Analyzer(
             ILocalSymbol localSymbol,
             ExpressionSyntax comparison,
             ExpressionSyntax operand,
-            SyntaxNode localStatement,
+            LocalDeclarationStatementSyntax localStatement,
+            VariableDeclaratorSyntax declarator,
             SyntaxNode enclosingBlock,
             CancellationToken cancellationToken)
         {
@@ -44,6 +47,7 @@ private Analyzer(
             _localSymbol = localSymbol;
             _operand = operand;
             _localStatement = localStatement;
+            _declarator = declarator;
             _enclosingBlock = enclosingBlock;
             _cancellationToken = cancellationToken;
         }
@@ -53,14 +57,20 @@ public static bool CanSafelyConvertToPatternMatching(
             ILocalSymbol localSymbol,
             ExpressionSyntax comparison,
             ExpressionSyntax operand,
-            SyntaxNode localStatement,
+            LocalDeclarationStatementSyntax localStatement,
+            VariableDeclaratorSyntax declarator,
             SyntaxNode enclosingBlock,
             CancellationToken cancellationToken)
         {
-            var analyzer = new Analyzer(semanticModel, localSymbol, comparison, operand, localStatement, enclosingBlock, cancellationToken);
+            var analyzer = new Analyzer(semanticModel, localSymbol, comparison, operand, localStatement, declarator, enclosingBlock, cancellationToken);
             return analyzer.CanSafelyConvertToPatternMatching();
         }
 
+        private bool CanSafelyConvertToPatternMatching()
+        {
+            return CheckDefiniteAssignment() && CheckNoInterveningMutations();
+        }
+
         // To convert a null-check to pattern-matching, we should make sure of a few things:
         //
         //      (1) The pattern variable may not be used before the point of declaration.
@@ -88,7 +98,7 @@ public static bool CanSafelyConvertToPatternMatching(
         //          }
         //
         // We walk up the tree from the point of null-check and see if any of the above is violated.
-        private bool CanSafelyConvertToPatternMatching()
+        private bool CheckDefiniteAssignment()
         {
             // Keep track of whether the pattern variable is definitely assigned when false/true.
             // We start by the null-check itself, if it's compared with '==', the pattern variable
@@ -237,6 +247,36 @@ private bool CanSafelyConvertToPatternMatching()
             return false;
         }
 
+        private bool CheckNoInterveningMutations()
+        {
+            var semanticModel = _semanticModel;
+            foreach (var node in _enclosingBlock.DescendantNodesAndSelf())
+            {
+                // Skip until we're after the initial `var v = x as y; statement.
+                if (node.SpanStart < _localStatement.Span.End)
+                    continue;
+
+                // analyze up to the `x != null` check.
+                if (node.Span.End >= _comparison.SpanStart)
+                    break;
+
+                if (node is ExpressionSyntax currentExpression &&
+                    currentExpression.IsWrittenTo(_semanticModel, _cancellationToken))
+                {
+                    // Ok, we have a write of some value in between.  See if the initial statement used that value as
+                    // well.  For example, if we have `var v = x[current] as y;` then if we see a `current++` in between
+                    // us and the null check, then we don't want to move things as that could change semantics.
+                    if (_declarator.DescendantNodesAndSelf().OfType<ExpressionSyntax>().Any(
+                            e => SemanticEquivalence.AreEquivalent(semanticModel, e, currentExpression)))
+                    {
+                        return false;
+                    }
+                }
+            }
+
+            return true;
+        }
+
         private bool CheckLoop(SyntaxNode statement, StatementSyntax body, bool defAssignedWhenTrue)
         {
             if (_operand.Kind() == SyntaxKind.IdentifierName)

src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs

@@ -27,17 +27,14 @@ namespace Microsoft.CodeAnalysis.CSharp.UsePatternMatching;
 /// </code>
 /// </summary>
 [DiagnosticAnalyzer(LanguageNames.CSharp)]
-internal sealed partial class CSharpAsAndNullCheckDiagnosticAnalyzer : AbstractBuiltInCodeStyleDiagnosticAnalyzer
+internal sealed partial class CSharpAsAndNullCheckDiagnosticAnalyzer()
+    : AbstractBuiltInCodeStyleDiagnosticAnalyzer(
+        IDEDiagnosticIds.InlineAsTypeCheckId,
+        EnforceOnBuildValues.InlineAsType,
+        CSharpCodeStyleOptions.PreferPatternMatchingOverAsWithNullCheck,
+        new LocalizableResourceString(
+            nameof(CSharpAnalyzersResources.Use_pattern_matching), CSharpAnalyzersResources.ResourceManager, typeof(CSharpAnalyzersResources)))
 {
-    public CSharpAsAndNullCheckDiagnosticAnalyzer()
-        : base(IDEDiagnosticIds.InlineAsTypeCheckId,
-               EnforceOnBuildValues.InlineAsType,
-               CSharpCodeStyleOptions.PreferPatternMatchingOverAsWithNullCheck,
-               new LocalizableResourceString(
-                    nameof(CSharpAnalyzersResources.Use_pattern_matching), CSharpAnalyzersResources.ResourceManager, typeof(CSharpAnalyzersResources)))
-    {
-    }
-
     public override DiagnosticAnalyzerCategory GetAnalyzerCategory()
         => DiagnosticAnalyzerCategory.SemanticSpanAnalysis;
 
@@ -104,26 +101,20 @@ private void SyntaxNodeAction(SyntaxNodeAnalysisContext syntaxContext)
             return;
         }
 
-        var localStatement = declarator.Parent?.Parent;
-        var enclosingBlock = localStatement?.Parent;
-        if (localStatement == null ||
-            enclosingBlock == null)
-        {
+        if (declarator is not { Parent.Parent: LocalDeclarationStatementSyntax localStatement })
             return;
-        }
 
-        // Bail out if the potential diagnostic location is outside the analysis span.
-        if (!syntaxContext.ShouldAnalyzeSpan(localStatement.Span))
+        // Don't convert if the as is part of a local declaration with a using keyword
+        // eg using var x = y as MyObject;
+        if (localStatement is LocalDeclarationStatementSyntax localDecl && localDecl.UsingKeyword != default)
             return;
 
-        // Don't convert if the as is part of a using statement
-        // eg using (var x = y as MyObject) { }
-        if (localStatement is UsingStatementSyntax)
+        var enclosingBlock = localStatement.Parent;
+        if (enclosingBlock is not BlockSyntax and not SwitchSectionSyntax)
             return;
 
-        // Don't convert if the as is part of a local declaration with a using keyword
-        // eg using var x = y as MyObject;
-        if (localStatement is LocalDeclarationStatementSyntax localDecl && localDecl.UsingKeyword != default)
+        // Bail out if the potential diagnostic location is outside the analysis span.
+        if (!syntaxContext.ShouldAnalyzeSpan(localStatement.Span))
             return;
 
         var typeNode = asExpression.Right;
@@ -202,7 +193,7 @@ private void SyntaxNodeAction(SyntaxNodeAnalysisContext syntaxContext)
 
         if (!Analyzer.CanSafelyConvertToPatternMatching(
                 semanticModel, localSymbol, comparison, operand,
-                localStatement, enclosingBlock, cancellationToken))
+                localStatement, declarator, enclosingBlock, cancellationToken))
         {
             return;
         }

src/Analyzers/CSharp/Tests/UsePatternMatching/CSharpAsAndNullCheckTests.cs

@@ -16,13 +16,9 @@
 namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UsePatternMatching;
 
 [Trait(Traits.Feature, Traits.Features.CodeActionsInlineTypeCheck)]
-public partial class CSharpAsAndNullCheckTests : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor
+public partial class CSharpAsAndNullCheckTests(ITestOutputHelper logger)
+    : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor(logger)
 {
-    public CSharpAsAndNullCheckTests(ITestOutputHelper logger)
-      : base(logger)
-    {
-    }
-
     internal override (DiagnosticAnalyzer, CodeFixProvider) CreateDiagnosticProviderAndFixer(Workspace workspace)
         => (new CSharpAsAndNullCheckDiagnosticAnalyzer(), new CSharpAsAndNullCheckCodeFixProvider());
 
@@ -2138,4 +2134,26 @@ static void Goo(object o1, object o2)
             }
             """);
     }
+
+    [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/39600")]
+    public async Task TestNotWithInterveningMutation()
+    {
+        await TestMissingInRegularAndScriptAsync("""
+            using System;
+
+            class Program
+            {
+                void Goo(object[] values, int index)
+                {
+                    [|var|] v1 = values[index++] as string;
+                    index++;
+
+                    if (v1 != null)
+                    {
+                        Console.WriteLine(v1);
+                    }
+                }
+            }
+            """);
+    }
 }

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/SemanticEquivalence.cs

@@ -2,8 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-#nullable disable
-
 using System;
 using System.Collections.Generic;
 using Microsoft.CodeAnalysis.Operations;
@@ -20,9 +18,9 @@ public static bool AreEquivalent(SemanticModel semanticModel, SyntaxNode node1,
     public static bool AreEquivalent(
         SemanticModel semanticModel1,
         SemanticModel semanticModel2,
-        SyntaxNode node1,
-        SyntaxNode node2,
-        Func<SyntaxNode, bool> predicate = null)
+        SyntaxNode? node1,
+        SyntaxNode? node2,
+        Func<SyntaxNode, bool>? predicate = null)
     {
         // First check for syntactic equivalency.  If two nodes aren't structurally equivalent,
         // then they're not semantically equivalent.
@@ -52,7 +50,7 @@ private static bool AreSemanticallyEquivalentWorker(
         SemanticModel semanticModel2,
         SyntaxNode node1,
         SyntaxNode node2,
-        Func<SyntaxNode, bool> predicate)
+        Func<SyntaxNode, bool>? predicate)
     {
         if (node1 == node2)
         {
@@ -115,9 +113,9 @@ private static bool AreSemanticallyEquivalentWorker(
             var c1 = e1.Current;
             var c2 = e2.Current;
 
-            if (c1.IsNode && c2.IsNode)
+            if (c1.AsNode(out var c1Node) && c2.AsNode(out var c2Node))
             {
-                if (!AreSemanticallyEquivalentWorker(semanticModel1, semanticModel2, c1.AsNode(), c2.AsNode(), predicate))
+                if (!AreSemanticallyEquivalentWorker(semanticModel1, semanticModel2, c1Node, c2Node, predicate))
                 {
                     return false;
                 }
@@ -134,13 +132,8 @@ private static bool AreEquals(
         SymbolInfo info1,
         SymbolInfo info2)
     {
-        if (semanticModel1 == semanticModel2)
-        {
-            return EqualityComparer<ISymbol>.Default.Equals(info1.Symbol, info2.Symbol);
-        }
-        else
-        {
-            return SymbolEquivalenceComparer.Instance.Equals(info1.Symbol, info2.Symbol);
-        }
+        return semanticModel1 == semanticModel2
+            ? EqualityComparer<ISymbol?>.Default.Equals(info1.Symbol, info2.Symbol)
+            : SymbolEquivalenceComparer.Instance.Equals(info1.Symbol, info2.Symbol);
     }
 }