From a2535d085f9b928bfb01cde1be8f1894a015ac92 Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Sat, 15 Jun 2024 20:27:45 +0300 Subject: [PATCH 1/9] Do not offer to add default switch case when it handles `null` and an underlying value --- .../PopulateSwitchExpressionTests.cs | 81 ++++++++- .../PopulateSwitchStatementTests.cs | 164 ++++++++++++++++++ ...bstractPopulateSwitchDiagnosticAnalyzer.cs | 11 +- ...ulateSwitchExpressionDiagnosticAnalyzer.cs | 3 + ...pulateSwitchStatementDiagnosticAnalyzer.cs | 3 + .../PopulateSwitchExpressionHelpers.cs | 33 +++- .../PopulateSwitchStatementHelpers.cs | 36 +++- 7 files changed, 325 insertions(+), 6 deletions(-) diff --git a/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchExpressionTests.cs b/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchExpressionTests.cs index d2bb6dddf6e42..5f920acc5d201 100644 --- a/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchExpressionTests.cs +++ b/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchExpressionTests.cs @@ -4,7 +4,6 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis.CodeFixes; -using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.PopulateSwitch; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Test.Utilities; @@ -1716,4 +1715,84 @@ public string Method(bool? boolean) } """); } + + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("int")] + [InlineData("int i")] + public async Task NullableValueTypeWithNullAndUnderlyingValueArms1(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + int M(int? x) + { + return x [||]switch + { + null => -1, + {{underlyingTypePattern}} => 0, + }; + } + } + """); + } + + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("int")] + [InlineData("int i")] + public async Task NullableValueTypeWithNullAndUnderlyingValueArms2(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + int M(int? x) + { + return x [||]switch + { + {{underlyingTypePattern}} => 0, + null => -1, + }; + } + } + """); + } + + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("string")] + [InlineData("string s")] + public async Task NullableReferenceTypeWithNullAndUnderlyingValueArms1(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + int M(string? x) + { + return x [||]switch + { + null => -1, + {{underlyingTypePattern}} => 0, + }; + } + } + """); + } + + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("string")] + [InlineData("string s")] + public async Task NullableReferenceTypeWithNullAndUnderlyingValueArms2(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + int M(string? x) + { + return x [||]switch + { + {{underlyingTypePattern}} => 0, + null => -1, + }; + } + } + """); + } } diff --git a/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchStatementTests.cs b/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchStatementTests.cs index 736560e85795a..0937692354f8b 100644 --- a/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchStatementTests.cs +++ b/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchStatementTests.cs @@ -1758,4 +1758,168 @@ void Method() } """); } + + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("int")] + [InlineData("int i")] + public async Task NullableValueTypeWithNullAndUnderlyingValueCases1(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + void M(int? x) + { + [||]switch (x) + { + case null: + break; + case {{underlyingTypePattern}}: + break; + } + } + } + """); + } + + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("int")] + [InlineData("int i")] + public async Task NullableValueTypeWithNullAndUnderlyingValueCases2(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + int M(int? x) + { + [||]switch (x) + { + case {{underlyingTypePattern}}: + break; + case null: + break; + } + } + } + """); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + public async Task NullableValueTypeWithNullAndUnderlyingValueCases3() + { + await TestMissingInRegularAndScriptAsync(""" + class C + { + int M(int? x) + { + [||]switch (x) + { + case null: + case int: + break; + } + } + } + """); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + public async Task NullableValueTypeWithNullAndUnderlyingValueCases4() + { + await TestMissingInRegularAndScriptAsync(""" + class C + { + int M(int? x) + { + [||]switch (x) + { + case int: + case null: + break; + } + } + } + """); + } + + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("string")] + [InlineData("string s")] + public async Task NullableReferenceTypeWithNullAndUnderlyingValueCases1(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + void M(string? x) + { + [||]switch (x) + { + case null: + break; + case {{underlyingTypePattern}}: + break; + } + } + } + """); + } + + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("string")] + [InlineData("string s")] + public async Task NullableReferenceTypeWithNullAndUnderlyingValueCases2(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + int M(string? x) + { + [||]switch (x) + { + case {{underlyingTypePattern}}: + break; + case null: + break; + } + } + } + """); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + public async Task NullableReferenceTypeWithNullAndUnderlyingValueCases3() + { + await TestMissingInRegularAndScriptAsync(""" + class C + { + int M(string? x) + { + [||]switch (x) + { + case null: + case string: + break; + } + } + } + """); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + public async Task NullableReferenceTypeWithNullAndUnderlyingValueCases4() + { + await TestMissingInRegularAndScriptAsync(""" + class C + { + int M(string? x) + { + [||]switch (x) + { + case string: + case null: + break; + } + } + } + """); + } } diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs index f7a0eb28c621a..88609fb7d4a30 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs @@ -2,7 +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. -using System; using System.Collections.Generic; using System.Collections.Immutable; using System.Diagnostics; @@ -38,6 +37,7 @@ protected AbstractPopulateSwitchDiagnosticAnalyzer(string diagnosticId, EnforceO protected abstract bool HasConstantCase(TSwitchOperation operation, object? value); protected abstract ICollection GetMissingEnumMembers(TSwitchOperation operation); protected abstract bool HasDefaultCase(TSwitchOperation operation); + protected abstract bool HasBothNullAndUnderlyingValueCases(TSwitchOperation operation); protected abstract Location GetDiagnosticLocation(TSwitchSyntax switchBlock); public sealed override DiagnosticAnalyzerCategory GetAnalyzerCategory() => DiagnosticAnalyzerCategory.SemanticSpanAnalysis; @@ -86,8 +86,13 @@ private bool SwitchIsIncomplete( missingDefaultCase = !HasDefaultCase(operation); } - // The switch is incomplete if we're missing any cases or we're missing a default case. - return missingDefaultCase || missingCases; + if (missingCases) + return true; + + if (HasBothNullAndUnderlyingValueCases(operation)) + return false; + + return missingDefaultCase; } private bool IsBooleanSwitch(TSwitchOperation operation, out bool missingCases, out bool missingDefaultCase) diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchExpressionDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchExpressionDiagnosticAnalyzer.cs index 01d63183dba51..c9278de7bd992 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchExpressionDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchExpressionDiagnosticAnalyzer.cs @@ -32,6 +32,9 @@ protected sealed override ICollection GetMissingEnumMembers(ISwitchExpr protected sealed override bool HasDefaultCase(ISwitchExpressionOperation operation) => PopulateSwitchExpressionHelpers.HasDefaultCase(operation); + protected override bool HasBothNullAndUnderlyingValueCases(ISwitchExpressionOperation operation) + => PopulateSwitchExpressionHelpers.HasBothNullAndUnderlyingValueCases(operation); + protected override bool HasConstantCase(ISwitchExpressionOperation operation, object? value) { foreach (var arm in operation.Arms) diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchStatementDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchStatementDiagnosticAnalyzer.cs index 7dbaaf1bbef63..633806d87593c 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchStatementDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchStatementDiagnosticAnalyzer.cs @@ -32,6 +32,9 @@ protected sealed override ICollection GetMissingEnumMembers(ISwitchOper protected sealed override bool HasDefaultCase(ISwitchOperation operation) => PopulateSwitchStatementHelpers.HasDefaultCase(operation); + protected override bool HasBothNullAndUnderlyingValueCases(ISwitchOperation operation) + => PopulateSwitchStatementHelpers.HasBothNullAndUnderlyingValueCases(operation); + protected sealed override Location GetDiagnosticLocation(TSwitchSyntax switchBlock) => switchBlock.GetFirstToken().GetLocation(); diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs index 828eb3fa49939..49b138d9ff5ee 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs @@ -8,7 +8,6 @@ using Microsoft.CodeAnalysis.Operations; using Microsoft.CodeAnalysis.Shared.Extensions; using Microsoft.CodeAnalysis.Shared.Utilities; -using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.PopulateSwitch; @@ -103,4 +102,36 @@ public static bool IsDefault(IPatternOperation pattern) }, _ => false }; + + public static bool HasBothNullAndUnderlyingValueCases(ISwitchExpressionOperation operation) + { + var type = operation.Value.Type; + var underlyingType = type.IsNullable(out var underlying) ? underlying : type; + + var hasNullArm = false; + var hasUnderlyingTypeArm = false; + + foreach (var arm in operation.Arms) + { + var pattern = arm.Pattern; + + if (pattern is IConstantPatternOperation { Value: IConversionOperation { ConstantValue: { HasValue: true, Value: null } } }) + { + hasNullArm = true; + continue; + } + + var matchedType = pattern switch + { + ITypePatternOperation typePattern => typePattern.MatchedType, + IDeclarationPatternOperation declarationPattern => declarationPattern.MatchedType, + _ => null + }; + + if (SymbolEqualityComparer.Default.Equals(matchedType, underlyingType)) + hasUnderlyingTypeArm = true; + } + + return hasNullArm && hasUnderlyingTypeArm; + } } diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs index 17ccd7460211a..8cd81dc98706f 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs @@ -6,7 +6,6 @@ using Microsoft.CodeAnalysis.Operations; using Microsoft.CodeAnalysis.Shared.Extensions; using Microsoft.CodeAnalysis.Shared.Utilities; -using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.PopulateSwitch; @@ -180,4 +179,39 @@ public static bool TryGetAllEnumMembers( return true; } + + public static bool HasBothNullAndUnderlyingValueCases(ISwitchOperation operation) + { + var type = operation.Value.Type; + var underlyingType = type.IsNullable(out var underlying) ? underlying : type; + + var hasNullCase = false; + var hasUnderlyingTypeCase = false; + + foreach (var @case in operation.Cases) + { + foreach (var clause in @case.Clauses) + { + switch (clause) + { + case ISingleValueCaseClauseOperation { Value: IConversionOperation { ConstantValue: { HasValue: true, Value: null } } }: + hasNullCase = true; + break; + case IPatternCaseClauseOperation { Pattern: var pattern }: + var matchedType = pattern switch + { + ITypePatternOperation typePattern => typePattern.MatchedType, + IDeclarationPatternOperation declarationPattern => declarationPattern.MatchedType, + _ => null + }; + + if (SymbolEqualityComparer.Default.Equals(matchedType, underlyingType)) + hasUnderlyingTypeCase = true; + break; + } + } + } + + return hasNullCase && hasUnderlyingTypeCase; + } } From 68828cec9e6afd81dea8cc899a1d857bd0be706c Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Sat, 15 Jun 2024 22:48:33 +0300 Subject: [PATCH 2/9] Use helper --- .../PopulateSwitch/PopulateSwitchExpressionHelpers.cs | 2 +- .../PopulateSwitch/PopulateSwitchStatementHelpers.cs | 2 +- .../Compiler/Core/Extensions/ITypeSymbolExtensions.cs | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs index 49b138d9ff5ee..9ad59cd20b469 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs @@ -106,7 +106,7 @@ public static bool IsDefault(IPatternOperation pattern) public static bool HasBothNullAndUnderlyingValueCases(ISwitchExpressionOperation operation) { var type = operation.Value.Type; - var underlyingType = type.IsNullable(out var underlying) ? underlying : type; + var underlyingType = type.RemoveNullableIfPresent(); var hasNullArm = false; var hasUnderlyingTypeArm = false; diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs index 8cd81dc98706f..34fa765cbdcc4 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs @@ -183,7 +183,7 @@ public static bool TryGetAllEnumMembers( public static bool HasBothNullAndUnderlyingValueCases(ISwitchOperation operation) { var type = operation.Value.Type; - var underlyingType = type.IsNullable(out var underlying) ? underlying : type; + var underlyingType = type.RemoveNullableIfPresent(); var hasNullCase = false; var hasUnderlyingTypeCase = false; diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs index 8732bd70f0b9c..47fea31801a29 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs @@ -732,9 +732,9 @@ public static ITypeSymbol WithNullableAnnotationFrom(this ITypeSymbol type, ITyp [return: NotNullIfNotNull(parameterName: nameof(symbol))] public static ITypeSymbol? RemoveNullableIfPresent(this ITypeSymbol? symbol) { - if (symbol.IsNullable()) + if (symbol.IsNullable(out var underlyingType)) { - return symbol.GetTypeArguments().Single(); + return underlyingType; } return symbol; From 3b950f247a750a195999bac1f745cf6bb8ab8f8a Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Sat, 15 Jun 2024 23:33:09 +0300 Subject: [PATCH 3/9] More tests --- .../PopulateSwitchExpressionTests.cs | 42 ++++++++++++++ .../PopulateSwitchStatementTests.cs | 56 +++++++++++++++++-- 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchExpressionTests.cs b/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchExpressionTests.cs index 5f920acc5d201..d05a7a8ef1544 100644 --- a/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchExpressionTests.cs +++ b/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchExpressionTests.cs @@ -1756,6 +1756,27 @@ int M(int? x) """); } + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("int")] + [InlineData("int i")] + public async Task NullableValueTypeWithNullAndUnderlyingValueArms3(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + int M(int? x) + { + return x [||]switch + { + null => -1, + 0 => 0, + {{underlyingTypePattern}} => 1, + }; + } + } + """); + } + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] [InlineData("string")] [InlineData("string s")] @@ -1795,4 +1816,25 @@ int M(string? x) } """); } + + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("string")] + [InlineData("string s")] + public async Task NullableReferenceTypeWithNullAndUnderlyingValueArms3(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + int M(string? x) + { + return x [||]switch + { + null => -1, + "" => 0, + {{underlyingTypePattern}} => 1, + }; + } + } + """); + } } diff --git a/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchStatementTests.cs b/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchStatementTests.cs index 0937692354f8b..024a572ae29b7 100644 --- a/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchStatementTests.cs +++ b/src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchStatementTests.cs @@ -1803,8 +1803,32 @@ int M(int? x) """); } + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("int")] + [InlineData("int i")] + public async Task NullableValueTypeWithNullAndUnderlyingValueCases3(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + void M(int? x) + { + [||]switch (x) + { + case null: + break; + case 0: + break; + case {{underlyingTypePattern}}: + break; + } + } + } + """); + } + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] - public async Task NullableValueTypeWithNullAndUnderlyingValueCases3() + public async Task NullableValueTypeWithNullAndUnderlyingValueCases4() { await TestMissingInRegularAndScriptAsync(""" class C @@ -1823,7 +1847,7 @@ int M(int? x) } [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] - public async Task NullableValueTypeWithNullAndUnderlyingValueCases4() + public async Task NullableValueTypeWithNullAndUnderlyingValueCases5() { await TestMissingInRegularAndScriptAsync(""" class C @@ -1885,8 +1909,32 @@ int M(string? x) """); } + [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] + [InlineData("string")] + [InlineData("string s")] + public async Task NullableReferenceTypeWithNullAndUnderlyingValueCases3(string underlyingTypePattern) + { + await TestMissingInRegularAndScriptAsync($$""" + class C + { + void M(string? x) + { + [||]switch (x) + { + case null: + break; + case "": + break; + case {{underlyingTypePattern}}: + break; + } + } + } + """); + } + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] - public async Task NullableReferenceTypeWithNullAndUnderlyingValueCases3() + public async Task NullableReferenceTypeWithNullAndUnderlyingValueCases4() { await TestMissingInRegularAndScriptAsync(""" class C @@ -1905,7 +1953,7 @@ int M(string? x) } [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/50983")] - public async Task NullableReferenceTypeWithNullAndUnderlyingValueCases4() + public async Task NullableReferenceTypeWithNullAndUnderlyingValueCases5() { await TestMissingInRegularAndScriptAsync(""" class C From b27deb4871f274ff54a175a94706f0e0a137d8ee Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sat, 15 Jun 2024 14:48:29 -0700 Subject: [PATCH 4/9] Move check in loop --- .../PopulateSwitchExpressionHelpers.cs | 23 +++++++++++-------- .../PopulateSwitchStatementHelpers.cs | 22 ++++++++++-------- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs index 9ad59cd20b469..3793c411ce45d 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs @@ -118,20 +118,23 @@ public static bool HasBothNullAndUnderlyingValueCases(ISwitchExpressionOperation if (pattern is IConstantPatternOperation { Value: IConversionOperation { ConstantValue: { HasValue: true, Value: null } } }) { hasNullArm = true; - continue; } - - var matchedType = pattern switch + else { - ITypePatternOperation typePattern => typePattern.MatchedType, - IDeclarationPatternOperation declarationPattern => declarationPattern.MatchedType, - _ => null - }; + hasUnderlyingTypeArm |= SymbolEqualityComparer.Default.Equals( + underlyingType, + pattern switch + { + ITypePatternOperation typePattern => typePattern.MatchedType, + IDeclarationPatternOperation declarationPattern => declarationPattern.MatchedType, + _ => null + }); + } - if (SymbolEqualityComparer.Default.Equals(matchedType, underlyingType)) - hasUnderlyingTypeArm = true; + if (hasNullArm && hasUnderlyingTypeArm) + return true; } - return hasNullArm && hasUnderlyingTypeArm; + return false; } } diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs index 34fa765cbdcc4..fa9bea171333e 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs @@ -198,20 +198,22 @@ public static bool HasBothNullAndUnderlyingValueCases(ISwitchOperation operation hasNullCase = true; break; case IPatternCaseClauseOperation { Pattern: var pattern }: - var matchedType = pattern switch - { - ITypePatternOperation typePattern => typePattern.MatchedType, - IDeclarationPatternOperation declarationPattern => declarationPattern.MatchedType, - _ => null - }; - - if (SymbolEqualityComparer.Default.Equals(matchedType, underlyingType)) - hasUnderlyingTypeCase = true; + hasUnderlyingTypeCase |= SymbolEqualityComparer.Default.Equals( + underlyingType, + pattern switch + { + ITypePatternOperation typePattern => typePattern.MatchedType, + IDeclarationPatternOperation declarationPattern => declarationPattern.MatchedType, + _ => null + }); break; } + + if (hasNullCase && hasUnderlyingTypeCase) + return true; } } - return hasNullCase && hasUnderlyingTypeCase; + return false; } } From 0e9e49f257aa934545c19b1fa4affbfabe5bcc68 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sat, 15 Jun 2024 14:51:56 -0700 Subject: [PATCH 5/9] rename and add guard tests --- .../AbstractPopulateSwitchDiagnosticAnalyzer.cs | 4 ++-- .../AbstractPopulateSwitchExpressionDiagnosticAnalyzer.cs | 4 ++-- .../AbstractPopulateSwitchStatementDiagnosticAnalyzer.cs | 4 ++-- .../PopulateSwitch/PopulateSwitchExpressionHelpers.cs | 4 +++- .../PopulateSwitch/PopulateSwitchStatementHelpers.cs | 4 ++-- 5 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs index 88609fb7d4a30..0839a41486e90 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs @@ -37,7 +37,7 @@ protected AbstractPopulateSwitchDiagnosticAnalyzer(string diagnosticId, EnforceO protected abstract bool HasConstantCase(TSwitchOperation operation, object? value); protected abstract ICollection GetMissingEnumMembers(TSwitchOperation operation); protected abstract bool HasDefaultCase(TSwitchOperation operation); - protected abstract bool HasBothNullAndUnderlyingValueCases(TSwitchOperation operation); + protected abstract bool HasExhaustiveNullAndTypeCheckCases(TSwitchOperation operation); protected abstract Location GetDiagnosticLocation(TSwitchSyntax switchBlock); public sealed override DiagnosticAnalyzerCategory GetAnalyzerCategory() => DiagnosticAnalyzerCategory.SemanticSpanAnalysis; @@ -89,7 +89,7 @@ private bool SwitchIsIncomplete( if (missingCases) return true; - if (HasBothNullAndUnderlyingValueCases(operation)) + if (HasExhaustiveNullAndTypeCheckCases(operation)) return false; return missingDefaultCase; diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchExpressionDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchExpressionDiagnosticAnalyzer.cs index c9278de7bd992..f7e3584ad4deb 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchExpressionDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchExpressionDiagnosticAnalyzer.cs @@ -32,8 +32,8 @@ protected sealed override ICollection GetMissingEnumMembers(ISwitchExpr protected sealed override bool HasDefaultCase(ISwitchExpressionOperation operation) => PopulateSwitchExpressionHelpers.HasDefaultCase(operation); - protected override bool HasBothNullAndUnderlyingValueCases(ISwitchExpressionOperation operation) - => PopulateSwitchExpressionHelpers.HasBothNullAndUnderlyingValueCases(operation); + protected override bool HasExhaustiveNullAndTypeCheckCases(ISwitchExpressionOperation operation) + => PopulateSwitchExpressionHelpers.HasExhaustiveNullAndTypeCheckCases(operation); protected override bool HasConstantCase(ISwitchExpressionOperation operation, object? value) { diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchStatementDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchStatementDiagnosticAnalyzer.cs index 633806d87593c..f6491334870fe 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchStatementDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchStatementDiagnosticAnalyzer.cs @@ -32,8 +32,8 @@ protected sealed override ICollection GetMissingEnumMembers(ISwitchOper protected sealed override bool HasDefaultCase(ISwitchOperation operation) => PopulateSwitchStatementHelpers.HasDefaultCase(operation); - protected override bool HasBothNullAndUnderlyingValueCases(ISwitchOperation operation) - => PopulateSwitchStatementHelpers.HasBothNullAndUnderlyingValueCases(operation); + protected override bool HasExhaustiveNullAndTypeCheckCases(ISwitchOperation operation) + => PopulateSwitchStatementHelpers.HasExhaustiveNullAndTypeCheckCases(operation); protected sealed override Location GetDiagnosticLocation(TSwitchSyntax switchBlock) => switchBlock.GetFirstToken().GetLocation(); diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs index 3793c411ce45d..836cf0f7c4e33 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs @@ -103,7 +103,7 @@ public static bool IsDefault(IPatternOperation pattern) _ => false }; - public static bool HasBothNullAndUnderlyingValueCases(ISwitchExpressionOperation operation) + public static bool HasExhaustiveNullAndTypeCheckCases(ISwitchExpressionOperation operation) { var type = operation.Value.Type; var underlyingType = type.RemoveNullableIfPresent(); @@ -114,6 +114,8 @@ public static bool HasBothNullAndUnderlyingValueCases(ISwitchExpressionOperation foreach (var arm in operation.Arms) { var pattern = arm.Pattern; + if (arm.Guard != null) + continue; if (pattern is IConstantPatternOperation { Value: IConversionOperation { ConstantValue: { HasValue: true, Value: null } } }) { diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs index fa9bea171333e..99913f5c2c2b1 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchStatementHelpers.cs @@ -180,7 +180,7 @@ public static bool TryGetAllEnumMembers( return true; } - public static bool HasBothNullAndUnderlyingValueCases(ISwitchOperation operation) + public static bool HasExhaustiveNullAndTypeCheckCases(ISwitchOperation operation) { var type = operation.Value.Type; var underlyingType = type.RemoveNullableIfPresent(); @@ -197,7 +197,7 @@ public static bool HasBothNullAndUnderlyingValueCases(ISwitchOperation operation case ISingleValueCaseClauseOperation { Value: IConversionOperation { ConstantValue: { HasValue: true, Value: null } } }: hasNullCase = true; break; - case IPatternCaseClauseOperation { Pattern: var pattern }: + case IPatternCaseClauseOperation { Pattern: var pattern, Guard: null }: hasUnderlyingTypeCase |= SymbolEqualityComparer.Default.Equals( underlyingType, pattern switch From df30535975f708b6bcd91066b87c9b1dd1477297 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sat, 15 Jun 2024 14:53:42 -0700 Subject: [PATCH 6/9] simplify checks --- .../AbstractPopulateSwitchDiagnosticAnalyzer.cs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs index 0839a41486e90..7dc6585af58e6 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs @@ -76,8 +76,15 @@ private void AnalyzeOperation(OperationAnalysisContext context) private bool SwitchIsIncomplete( TSwitchOperation operation, - out bool missingCases, out bool missingDefaultCase) + out bool missingCases, + out bool missingDefaultCase) { + missingCases = false; + missingDefaultCase = false; + + if (HasExhaustiveNullAndTypeCheckCases(operation)) + return false; + if (!IsBooleanSwitch(operation, out missingCases, out missingDefaultCase)) { var missingEnumMembers = GetMissingEnumMembers(operation); @@ -86,13 +93,7 @@ private bool SwitchIsIncomplete( missingDefaultCase = !HasDefaultCase(operation); } - if (missingCases) - return true; - - if (HasExhaustiveNullAndTypeCheckCases(operation)) - return false; - - return missingDefaultCase; + return missingCases || missingDefaultCase; } private bool IsBooleanSwitch(TSwitchOperation operation, out bool missingCases, out bool missingDefaultCase) From 5b2d0a6fd3ddea453f4d34bf0092897932e49e0b Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sat, 15 Jun 2024 15:11:50 -0700 Subject: [PATCH 7/9] in progress --- ...bstractPopulateSwitchDiagnosticAnalyzer.cs | 112 +++++++++--------- 1 file changed, 57 insertions(+), 55 deletions(-) diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs index 7dc6585af58e6..53d5c1dd8dad7 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System; using System.Collections.Generic; using System.Collections.Immutable; using System.Diagnostics; @@ -27,8 +28,6 @@ protected AbstractPopulateSwitchDiagnosticAnalyzer(string diagnosticId, EnforceO { } - #region Interface methods - protected abstract OperationKind OperationKind { get; } protected abstract bool IsSwitchTypeUnknown(TSwitchOperation operation); @@ -54,74 +53,77 @@ private void AnalyzeOperation(OperationAnalysisContext context) if (switchOperation.Syntax is not TSwitchSyntax switchBlock || IsSwitchTypeUnknown(switchOperation)) return; - var tree = switchBlock.SyntaxTree; + if (HasExhaustiveNullAndTypeCheckCases(switchOperation)) + return; - if (SwitchIsIncomplete(switchOperation, out var missingCases, out var missingDefaultCase) && - !tree.OverlapsHiddenPosition(switchBlock.Span, context.CancellationToken)) - { - Debug.Assert(missingCases || missingDefaultCase); - var properties = ImmutableDictionary.Empty - .Add(PopulateSwitchStatementHelpers.MissingCases, missingCases.ToString()) - .Add(PopulateSwitchStatementHelpers.MissingDefaultCase, missingDefaultCase.ToString()); - var diagnostic = Diagnostic.Create( - Descriptor, - GetDiagnosticLocation(switchBlock), - properties: properties, - additionalLocations: [switchBlock.GetLocation()]); - context.ReportDiagnostic(diagnostic); - } - } + var value = GetValueOfSwitchOperation(switchOperation); + var type = value.Type; + if (type is null) + return; - #endregion + var (missingCases, missingDefaultCase) = AnalyzeSwitch(switchOperation, type); + if (!missingCases && !missingDefaultCase) + return; - private bool SwitchIsIncomplete( - TSwitchOperation operation, - out bool missingCases, - out bool missingDefaultCase) - { - missingCases = false; - missingDefaultCase = false; + if (switchBlock.SyntaxTree.OverlapsHiddenPosition(switchBlock.Span, context.CancellationToken)) + return; - if (HasExhaustiveNullAndTypeCheckCases(operation)) - return false; + var properties = ImmutableDictionary.Empty + .Add(PopulateSwitchStatementHelpers.MissingCases, missingCases.ToString()) + .Add(PopulateSwitchStatementHelpers.MissingDefaultCase, missingDefaultCase.ToString()); + var diagnostic = Diagnostic.Create( + Descriptor, + GetDiagnosticLocation(switchBlock), + properties: properties, + additionalLocations: [switchBlock.GetLocation()]); + context.ReportDiagnostic(diagnostic); + } - if (!IsBooleanSwitch(operation, out missingCases, out missingDefaultCase)) - { - var missingEnumMembers = GetMissingEnumMembers(operation); + private (bool missingCases, bool missingDefaultCase) AnalyzeSwitch(TSwitchOperation switchOperation, ITypeSymbol type) + { + var typeWithoutNullable = type.RemoveNullableIfPresent(); - missingCases = missingEnumMembers.Count > 0; - missingDefaultCase = !HasDefaultCase(operation); + if (typeWithoutNullable.SpecialType == SpecialType.System_Boolean) + { + return AnalyzeBooleanSwitch(switchOperation, type); + } + else if (typeWithoutNullable.TypeKind == TypeKind.Enum) + { + return AnalyzeEnumSwitch(switchOperation, type); + } + else + { + return (missingCases: false, missingDefaultCase: !HasDefaultCase(switchOperation)); } - - return missingCases || missingDefaultCase; } - private bool IsBooleanSwitch(TSwitchOperation operation, out bool missingCases, out bool missingDefaultCase) + private (bool missingCases, bool missingDefaultCase) AnalyzeBooleanSwitch(TSwitchOperation operation, ITypeSymbol type) { - missingCases = false; - missingDefaultCase = false; - - var value = GetValueOfSwitchOperation(operation); - var type = value.Type.RemoveNullableIfPresent(); - if (type is not { SpecialType: SpecialType.System_Boolean }) - return false; + if (type.RemoveNullableIfPresent() is not { SpecialType: SpecialType.System_Boolean }) + return default; // If the switch already has a default case, then we don't have to offer the user anything. if (HasDefaultCase(operation)) - { - missingDefaultCase = false; - } - else - { - // Doesn't have a default. We don't want to offer that if they're already complete. - var hasAllCases = HasConstantCase(operation, true) && HasConstantCase(operation, false); - if (value.Type.IsNullable()) - hasAllCases = hasAllCases && HasConstantCase(operation, null); + return default; - missingDefaultCase = !hasAllCases; - } + // Doesn't have a default. We don't want to offer that if they're already complete. + var hasAllCases = HasConstantCase(operation, true) && HasConstantCase(operation, false); + if (type.IsNullable()) + hasAllCases = hasAllCases && HasConstantCase(operation, null); + + return (missingCases: !hasAllCases, missingDefaultCase: false); + } + + private (bool missingCases, bool missingDefaultCase) AnalyzeEnumSwitch(TSwitchOperation operation, ITypeSymbol type) + { + if (type.RemoveNullableIfPresent()?.TypeKind != TypeKind.Enum) + return default; + + var missingEnumMembers = GetMissingEnumMembers(operation); - return true; + var missingCases = missingEnumMembers.Count > 0; + var missingDefaultCase = !HasDefaultCase(operation); + return (missingCases, missingDefaultCase); } protected static bool ConstantValueEquals(Optional constantValue, object? value) From b83a526bdf3c8f224f8787de64241a3ee0e65a59 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sat, 15 Jun 2024 15:17:13 -0700 Subject: [PATCH 8/9] in progress --- .../AbstractPopulateSwitchDiagnosticAnalyzer.cs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs index 53d5c1dd8dad7..b749986c1dd8e 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs @@ -102,16 +102,16 @@ private void AnalyzeOperation(OperationAnalysisContext context) if (type.RemoveNullableIfPresent() is not { SpecialType: SpecialType.System_Boolean }) return default; - // If the switch already has a default case, then we don't have to offer the user anything. - if (HasDefaultCase(operation)) - return default; - // Doesn't have a default. We don't want to offer that if they're already complete. var hasAllCases = HasConstantCase(operation, true) && HasConstantCase(operation, false); if (type.IsNullable()) hasAllCases = hasAllCases && HasConstantCase(operation, null); - return (missingCases: !hasAllCases, missingDefaultCase: false); + // If the switch already has a default case or already has all cases, then we don't have to offer the user anything. + if (HasDefaultCase(operation) || hasAllCases) + return default; + + return (missingCases: true, missingDefaultCase: true); } private (bool missingCases, bool missingDefaultCase) AnalyzeEnumSwitch(TSwitchOperation operation, ITypeSymbol type) @@ -121,9 +121,7 @@ private void AnalyzeOperation(OperationAnalysisContext context) var missingEnumMembers = GetMissingEnumMembers(operation); - var missingCases = missingEnumMembers.Count > 0; - var missingDefaultCase = !HasDefaultCase(operation); - return (missingCases, missingDefaultCase); + return (missingCases: missingEnumMembers.Count > 0, missingDefaultCase: !HasDefaultCase(operation)); } protected static bool ConstantValueEquals(Optional constantValue, object? value) From e1a461ac649c155bdfed24558dbb27f5da595df3 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sat, 15 Jun 2024 15:21:00 -0700 Subject: [PATCH 9/9] Simplify --- .../PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs index b749986c1dd8e..0e68fc6b49fdc 100644 --- a/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs @@ -111,7 +111,7 @@ private void AnalyzeOperation(OperationAnalysisContext context) if (HasDefaultCase(operation) || hasAllCases) return default; - return (missingCases: true, missingDefaultCase: true); + return (missingCases: false, missingDefaultCase: true); } private (bool missingCases, bool missingDefaultCase) AnalyzeEnumSwitch(TSwitchOperation operation, ITypeSymbol type)