From 56d2ecc363e04249944319b061c7cdea59227ab8 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 6 Dec 2023 12:51:42 -0800 Subject: [PATCH 1/3] Fix 'use pattern matching' with nullable types --- .../CSharpUseNotPatternDiagnosticAnalyzer.cs | 148 +++---- .../CSharpUseNotPatternTests.cs | 365 ++++++++++-------- 2 files changed, 280 insertions(+), 233 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs index 7f06dd89109d3..ff70425babb1f 100644 --- a/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs @@ -10,92 +10,94 @@ using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Shared.Extensions; -namespace Microsoft.CodeAnalysis.CSharp.UsePatternMatching -{ - /// - /// Looks for code of the forms: - /// - /// var x = o as Type; - /// if (!(x is Y y)) ... - /// - /// and converts it to: - /// - /// if (x is not Y y) ... - /// - /// - [DiagnosticAnalyzer(LanguageNames.CSharp)] - internal sealed class CSharpUseNotPatternDiagnosticAnalyzer : AbstractBuiltInCodeStyleDiagnosticAnalyzer - { - public CSharpUseNotPatternDiagnosticAnalyzer() - : base(IDEDiagnosticIds.UseNotPatternDiagnosticId, - EnforceOnBuildValues.UseNotPattern, - CSharpCodeStyleOptions.PreferNotPattern, - new LocalizableResourceString( - nameof(CSharpAnalyzersResources.Use_pattern_matching), CSharpAnalyzersResources.ResourceManager, typeof(CSharpAnalyzersResources))) - { - } +namespace Microsoft.CodeAnalysis.CSharp.UsePatternMatching; - public override DiagnosticAnalyzerCategory GetAnalyzerCategory() - => DiagnosticAnalyzerCategory.SemanticSpanAnalysis; +/// +/// Looks for code of the forms: +/// +/// var x = o as Type; +/// if (!(x is Y y)) ... +/// +/// and converts it to: +/// +/// if (x is not Y y) ... +/// +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +internal sealed class CSharpUseNotPatternDiagnosticAnalyzer() + : AbstractBuiltInCodeStyleDiagnosticAnalyzer(IDEDiagnosticIds.UseNotPatternDiagnosticId, + EnforceOnBuildValues.UseNotPattern, + CSharpCodeStyleOptions.PreferNotPattern, + new LocalizableResourceString( + nameof(CSharpAnalyzersResources.Use_pattern_matching), CSharpAnalyzersResources.ResourceManager, typeof(CSharpAnalyzersResources))) +{ + public override DiagnosticAnalyzerCategory GetAnalyzerCategory() + => DiagnosticAnalyzerCategory.SemanticSpanAnalysis; - protected override void InitializeWorker(AnalysisContext context) + protected override void InitializeWorker(AnalysisContext context) + { + context.RegisterCompilationStartAction(context => { - context.RegisterCompilationStartAction(context => - { - // "x is not Type y" is only available in C# 9.0 and above. Don't offer this refactoring - // in projects targeting a lesser version. - if (context.Compilation.LanguageVersion() < LanguageVersion.CSharp9) - return; + // "x is not Type y" is only available in C# 9.0 and above. Don't offer this refactoring + // in projects targeting a lesser version. + if (context.Compilation.LanguageVersion() < LanguageVersion.CSharp9) + return; - var expressionOfTType = context.Compilation.ExpressionOfTType(); - context.RegisterSyntaxNodeAction(n => SyntaxNodeAction(n, expressionOfTType), SyntaxKind.LogicalNotExpression); - }); - } + var expressionOfTType = context.Compilation.ExpressionOfTType(); + context.RegisterSyntaxNodeAction(n => SyntaxNodeAction(n, expressionOfTType), SyntaxKind.LogicalNotExpression); + }); + } - private void SyntaxNodeAction( - SyntaxNodeAnalysisContext context, - INamedTypeSymbol? expressionOfTType) - { - var cancellationToken = context.CancellationToken; + private void SyntaxNodeAction( + SyntaxNodeAnalysisContext context, + INamedTypeSymbol? expressionOfTType) + { + var cancellationToken = context.CancellationToken; - // Bail immediately if the user has disabled this feature. - var styleOption = context.GetCSharpAnalyzerOptions().PreferNotPattern; - if (!styleOption.Value || ShouldSkipAnalysis(context, styleOption.Notification)) - return; + // Bail immediately if the user has disabled this feature. + var styleOption = context.GetCSharpAnalyzerOptions().PreferNotPattern; + if (!styleOption.Value || ShouldSkipAnalysis(context, styleOption.Notification)) + return; - // Look for the form: !(...) - var node = context.Node; - if (node is not PrefixUnaryExpressionSyntax(SyntaxKind.LogicalNotExpression) - { - Operand: ParenthesizedExpressionSyntax parenthesizedExpression - }) + // Look for the form: !(...) + var node = context.Node; + if (node is not PrefixUnaryExpressionSyntax(SyntaxKind.LogicalNotExpression) { - return; - } + Operand: ParenthesizedExpressionSyntax parenthesizedExpression + }) + { + return; + } - var isKeywordLocation = parenthesizedExpression.Expression switch - { - // Look for the form: !(x is Y y) and !(x is const) - IsPatternExpressionSyntax { Pattern: DeclarationPatternSyntax or ConstantPatternSyntax } isPattern => isPattern.IsKeyword.GetLocation(), + var semanticModel = context.SemanticModel; + var isKeywordLocation = parenthesizedExpression.Expression switch + { + // Look for the form: !(x is Y y) and !(x is const) + IsPatternExpressionSyntax { Pattern: DeclarationPatternSyntax or ConstantPatternSyntax } isPattern + => isPattern.IsKeyword.GetLocation(), - // Look for the form: !(x is Y) - BinaryExpressionSyntax(SyntaxKind.IsExpression) { Right: TypeSyntax } isExpression => isExpression.OperatorToken.GetLocation(), + // Look for the form: !(x is Y) + // + // Note: can't convert `!(x is Y?)` to `x is not Y?`. The latter is not legal. + BinaryExpressionSyntax(SyntaxKind.IsExpression) { Right: TypeSyntax type } isExpression + => semanticModel.GetType(type, cancellationToken).IsNullable() + ? null + : isExpression.OperatorToken.GetLocation(), - _ => null - }; + _ => null + }; - if (isKeywordLocation is null) - return; + if (isKeywordLocation is null) + return; - if (node.IsInExpressionTree(context.SemanticModel, expressionOfTType, cancellationToken)) - return; + if (node.IsInExpressionTree(context.SemanticModel, expressionOfTType, cancellationToken)) + return; - context.ReportDiagnostic(DiagnosticHelper.Create( - Descriptor, - isKeywordLocation, - styleOption.Notification, - ImmutableArray.Create(node.GetLocation()), - properties: null)); - } + context.ReportDiagnostic(DiagnosticHelper.Create( + Descriptor, + isKeywordLocation, + styleOption.Notification, + ImmutableArray.Create(node.GetLocation()), + properties: null)); } } diff --git a/src/Analyzers/CSharp/Tests/UsePatternMatching/CSharpUseNotPatternTests.cs b/src/Analyzers/CSharp/Tests/UsePatternMatching/CSharpUseNotPatternTests.cs index b46aa53f1fc68..5a97b2bf607b8 100644 --- a/src/Analyzers/CSharp/Tests/UsePatternMatching/CSharpUseNotPatternTests.cs +++ b/src/Analyzers/CSharp/Tests/UsePatternMatching/CSharpUseNotPatternTests.cs @@ -10,239 +10,284 @@ using Roslyn.Test.Utilities; using Xunit; -namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UsePatternMatching -{ - using VerifyCS = CSharpCodeFixVerifier< - CSharpUseNotPatternDiagnosticAnalyzer, - CSharpUseNotPatternCodeFixProvider>; +namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UsePatternMatching; + +using VerifyCS = CSharpCodeFixVerifier< + CSharpUseNotPatternDiagnosticAnalyzer, + CSharpUseNotPatternCodeFixProvider>; - [Trait(Traits.Feature, Traits.Features.CodeActionsUseNotPattern)] - public partial class CSharpUseNotPatternTests +[Trait(Traits.Feature, Traits.Features.CodeActionsUseNotPattern)] +public partial class CSharpUseNotPatternTests +{ + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50690")] + public async Task BinaryIsExpression() { - [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50690")] - public async Task BinaryIsExpression() + await new VerifyCS.Test { - await new VerifyCS.Test - { - TestCode = """ - class C + TestCode = """ + class C + { + void M(object x) { - void M(object x) + if (!(x [|is|] string)) { - if (!(x [|is|] string)) - { - } } } - """, - FixedCode = """ - class C + } + """, + FixedCode = """ + class C + { + void M(object x) { - void M(object x) + if (x is not string) { - if (x is not string) - { - } } } - """, - LanguageVersion = LanguageVersion.CSharp9, - }.RunAsync(); - } + } + """, + LanguageVersion = LanguageVersion.CSharp9, + }.RunAsync(); + } - [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50690")] - public async Task ConstantPattern() + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50690")] + public async Task ConstantPattern() + { + await new VerifyCS.Test { - await new VerifyCS.Test - { - TestCode = """ - class C + TestCode = """ + class C + { + void M(object x) { - void M(object x) + if (!(x [|is|] null)) { - if (!(x [|is|] null)) - { - } } } - """, - FixedCode = """ - class C + } + """, + FixedCode = """ + class C + { + void M(object x) { - void M(object x) + if (x is not null) { - if (x is not null) - { - } } } - """, - LanguageVersion = LanguageVersion.CSharp9, - }.RunAsync(); - } + } + """, + LanguageVersion = LanguageVersion.CSharp9, + }.RunAsync(); + } - [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/64292")] - public async Task BooleanValueConstantPattern() + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/64292")] + public async Task BooleanValueConstantPattern() + { + await new VerifyCS.Test { - await new VerifyCS.Test - { - TestCode = """ - class C + TestCode = """ + class C + { + void M(bool x) { - void M(bool x) + if (!(x [|is|] true)) { - if (!(x [|is|] true)) - { - } } } - """, - FixedCode = """ - class C + } + """, + FixedCode = """ + class C + { + void M(bool x) { - void M(bool x) + if (x is false) { - if (x is false) - { - } } } - """, - LanguageVersion = LanguageVersion.CSharp9, - }.RunAsync(); - } + } + """, + LanguageVersion = LanguageVersion.CSharp9, + }.RunAsync(); + } - [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/64292")] - public async Task NonBooleanValueConstantPattern() + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/64292")] + public async Task NonBooleanValueConstantPattern() + { + await new VerifyCS.Test { - await new VerifyCS.Test - { - TestCode = """ - class C + TestCode = """ + class C + { + void M(object x) { - void M(object x) + if (!(x [|is|] true)) { - if (!(x [|is|] true)) - { - } } } - """, - FixedCode = """ - class C + } + """, + FixedCode = """ + class C + { + void M(object x) { - void M(object x) + if (x is not true) { - if (x is not true) - { - } } } - """, - LanguageVersion = LanguageVersion.CSharp9, - }.RunAsync(); - } + } + """, + LanguageVersion = LanguageVersion.CSharp9, + }.RunAsync(); + } - [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/46699")] - public async Task UseNotPattern() + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/46699")] + public async Task UseNotPattern() + { + await new VerifyCS.Test { - await new VerifyCS.Test - { - TestCode = """ - class C + TestCode = """ + class C + { + void M(object x) { - void M(object x) + if (!(x [|is|] string s)) { - if (!(x [|is|] string s)) - { - } } } - """, - FixedCode = """ - class C + } + """, + FixedCode = """ + class C + { + void M(object x) { - void M(object x) + if (x is not string s) { - if (x is not string s) - { - } } } - """, - LanguageVersion = LanguageVersion.CSharp9, - }.RunAsync(); - } + } + """, + LanguageVersion = LanguageVersion.CSharp9, + }.RunAsync(); + } - [Fact] - public async Task UnavailableInCSharp8() + [Fact] + public async Task UnavailableInCSharp8() + { + await new VerifyCS.Test { - await new VerifyCS.Test - { - TestCode = """ - class C + TestCode = """ + class C + { + void M(object x) { - void M(object x) + if (!(x is string s)) { - if (!(x is string s)) - { - } } } - """, - LanguageVersion = LanguageVersion.CSharp8, - }.RunAsync(); - } + } + """, + LanguageVersion = LanguageVersion.CSharp8, + }.RunAsync(); + } - [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50690")] - public async Task BinaryIsObject() + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50690")] + public async Task BinaryIsObject() + { + await new VerifyCS.Test { - await new VerifyCS.Test - { - TestCode = """ - class C + TestCode = """ + class C + { + void M(object x) { - void M(object x) + if (!(x [|is|] object)) { - if (!(x [|is|] object)) - { - } } } - """, - FixedCode = """ - class C + } + """, + FixedCode = """ + class C + { + void M(object x) { - void M(object x) + if (x is null) { - if (x is null) - { - } } } - """, - LanguageVersion = LanguageVersion.CSharp9, - }.RunAsync(); - } + } + """, + LanguageVersion = LanguageVersion.CSharp9, + }.RunAsync(); + } - [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/68784")] - public async Task NotInExpressionTree() + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/68784")] + public async Task NotInExpressionTree() + { + await new VerifyCS.Test + { + TestCode = """ + using System.Linq; + + class C + { + IQueryable M(IQueryable query) + { + return query.Where(x => !(x is string)); + } + } + """, + LanguageVersion = LanguageVersion.CSharp9, + }.RunAsync(); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/70964")] + public async Task TestMissingOnNullable1() + { + await new VerifyCS.Test + { + TestCode = """ + using System; + + class C + { + void M() + { + object o = null; + if (!(o is bool?)) + { + } + } + } + """, + LanguageVersion = LanguageVersion.CSharp9, + }.RunAsync(); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/70964")] + public async Task TestMissingOnNullable2() + { + await new VerifyCS.Test { - await new VerifyCS.Test - { - TestCode = """ - using System.Linq; + TestCode = """ + using System; - class C + class C + { + void M() { - IQueryable M(IQueryable query) + object o = null; + if (!(o is Nullable)) { - return query.Where(x => !(x is string)); } } - """, - LanguageVersion = LanguageVersion.CSharp9, - }.RunAsync(); - } + } + """, + LanguageVersion = LanguageVersion.CSharp9, + }.RunAsync(); } } From fbc8f1c84ae8acb284baafcaa81dc3617800c149 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 6 Dec 2023 12:56:01 -0800 Subject: [PATCH 2/3] simplify --- .../UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs index ff70425babb1f..b6deb95ad45c9 100644 --- a/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs @@ -80,7 +80,7 @@ private void SyntaxNodeAction( // // Note: can't convert `!(x is Y?)` to `x is not Y?`. The latter is not legal. BinaryExpressionSyntax(SyntaxKind.IsExpression) { Right: TypeSyntax type } isExpression - => semanticModel.GetType(type, cancellationToken).IsNullable() + => semanticModel.GetTypeInfo(type, cancellationToken).Type.IsNullable() ? null : isExpression.OperatorToken.GetLocation(), From 0f84ca036f9b117bd1e9692144f7c1cd4d9d4c71 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 6 Dec 2023 12:57:14 -0800 Subject: [PATCH 3/3] simplify --- .../CSharpUseNotPatternDiagnosticAnalyzer.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs index b6deb95ad45c9..29d1d3fede004 100644 --- a/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UsePatternMatching/CSharpUseNotPatternDiagnosticAnalyzer.cs @@ -70,24 +70,24 @@ private void SyntaxNodeAction( } var semanticModel = context.SemanticModel; - var isKeywordLocation = parenthesizedExpression.Expression switch + var isKeyword = parenthesizedExpression.Expression switch { // Look for the form: !(x is Y y) and !(x is const) IsPatternExpressionSyntax { Pattern: DeclarationPatternSyntax or ConstantPatternSyntax } isPattern - => isPattern.IsKeyword.GetLocation(), + => isPattern.IsKeyword, // Look for the form: !(x is Y) // // Note: can't convert `!(x is Y?)` to `x is not Y?`. The latter is not legal. BinaryExpressionSyntax(SyntaxKind.IsExpression) { Right: TypeSyntax type } isExpression => semanticModel.GetTypeInfo(type, cancellationToken).Type.IsNullable() - ? null - : isExpression.OperatorToken.GetLocation(), + ? default + : isExpression.OperatorToken, - _ => null + _ => default }; - if (isKeywordLocation is null) + if (isKeyword == default) return; if (node.IsInExpressionTree(context.SemanticModel, expressionOfTType, cancellationToken)) @@ -95,7 +95,7 @@ private void SyntaxNodeAction( context.ReportDiagnostic(DiagnosticHelper.Create( Descriptor, - isKeywordLocation, + isKeyword.GetLocation(), styleOption.Notification, ImmutableArray.Create(node.GetLocation()), properties: null));