From 6d5d215f8a3101d869343f19ed4d88f2ed9c63f6 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 12 Dec 2024 13:16:15 -0800 Subject: [PATCH] Do not offer 'use pattern matching' when it would change semantics --- ...AndNullCheckDiagnosticAnalyzer.Analyzer.cs | 50 +++++++++++++++++-- .../CSharpAsAndNullCheckDiagnosticAnalyzer.cs | 41 ++++++--------- .../CSharpAsAndNullCheckTests.cs | 30 ++++++++--- .../Core/Extensions/SemanticEquivalence.cs | 25 ++++------ 4 files changed, 94 insertions(+), 52 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.Analyzer.cs b/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.Analyzer.cs index 6bdd8a54244d1..292645d15ffea 100644 --- a/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.Analyzer.cs +++ b/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().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) diff --git a/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs index 12ee4ba1d4d19..4c2dc933729d3 100644 --- a/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpAsAndNullCheckDiagnosticAnalyzer.cs @@ -27,17 +27,14 @@ namespace Microsoft.CodeAnalysis.CSharp.UsePatternMatching; /// /// [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; } diff --git a/src/Analyzers/CSharp/Tests/UsePatternMatching/CSharpAsAndNullCheckTests.cs b/src/Analyzers/CSharp/Tests/UsePatternMatching/CSharpAsAndNullCheckTests.cs index e564a53218da3..a975e549bcf84 100644 --- a/src/Analyzers/CSharp/Tests/UsePatternMatching/CSharpAsAndNullCheckTests.cs +++ b/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); + } + } + } + """); + } } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/SemanticEquivalence.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/SemanticEquivalence.cs index 3b65858127df7..b9805bdfe3e96 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/SemanticEquivalence.cs +++ b/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 predicate = null) + SyntaxNode? node1, + SyntaxNode? node2, + Func? 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 predicate) + Func? 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.Default.Equals(info1.Symbol, info2.Symbol); - } - else - { - return SymbolEquivalenceComparer.Instance.Equals(info1.Symbol, info2.Symbol); - } + return semanticModel1 == semanticModel2 + ? EqualityComparer.Default.Equals(info1.Symbol, info2.Symbol) + : SymbolEquivalenceComparer.Instance.Equals(info1.Symbol, info2.Symbol); } }