Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not offer to add default switch case when it handles null and an underlying value #74018

Merged
merged 9 commits into from
Jun 16, 2024
Merged

Do not offer to add default switch case when it handles null and an underlying value #74018

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a2535d0
Do not offer to add default switch case when it handles `null` and an…
DoctorKrolic Jun 15, 2024
68828ce
Use helper
DoctorKrolic Jun 15, 2024
3b950f2
More tests
DoctorKrolic Jun 15, 2024
b27deb4
Move check in loop
CyrusNajmabadi Jun 15, 2024
0e9e49f
rename and add guard tests
CyrusNajmabadi Jun 15, 2024
df30535
simplify checks
CyrusNajmabadi Jun 15, 2024
5b2d0a6
in progress
CyrusNajmabadi Jun 15, 2024
b83a526
in progress
CyrusNajmabadi Jun 15, 2024
e1a461a
Simplify
CyrusNajmabadi Jun 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 80 additions & 1 deletion src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchExpressionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
};
}
}
""");
}
}
164 changes: 164 additions & 0 deletions src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchStatementTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
""");
}
}
11 changes: 8 additions & 3 deletions src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -38,6 +37,7 @@ protected AbstractPopulateSwitchDiagnosticAnalyzer(string diagnosticId, EnforceO
protected abstract bool HasConstantCase(TSwitchOperation operation, object? value);
protected abstract ICollection<ISymbol> 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;
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions ...yzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchExpressionDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ protected sealed override ICollection<ISymbol> 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)
Expand Down
3 changes: 3 additions & 0 deletions ...lyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchStatementDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ protected sealed override ICollection<ISymbol> 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();

Expand Down
33 changes: 32 additions & 1 deletion src/Analyzers/Core/Analyzers/PopulateSwitch/PopulateSwitchExpressionHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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)
{
DoctorKrolic marked this conversation as resolved.
Show resolved Hide resolved
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;
DoctorKrolic marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading
Loading