Skip to content

Commit 728ab6c

Browse files
CyrusNajmabadiCyrusNajmabadi
authored
Fix issue where PopulateSwitch would suggest populating already exhaustive switch expressions (#79850)
2 parents f616d20 + 0ceea19 commit 728ab6c

File tree

4 files changed

+194
-43
lines changed

4 files changed

+194
-43
lines changed

‎src/Analyzers/CSharp/Tests/PopulateSwitch/PopulateSwitchExpressionTests.cs‎

Lines changed: 154 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,9 @@
1414
namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics.PopulateSwitch;
1515

1616
[Trait(Traits.Feature, Traits.Features.CodeActionsPopulateSwitch)]
17-
public sealed partial class PopulateSwitchExpressionTests : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor
17+
public sealed partial class PopulateSwitchExpressionTests(ITestOutputHelper logger)
18+
: AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor(logger)
1819
{
19-
public PopulateSwitchExpressionTests(ITestOutputHelper logger)
20-
: base(logger)
21-
{
22-
}
23-
2420
internal override (DiagnosticAnalyzer, CodeFixProvider) CreateDiagnosticProviderAndFixer(Workspace workspace)
2521
=> (new CSharpPopulateSwitchExpressionDiagnosticAnalyzer(), new CSharpPopulateSwitchExpressionCodeFixProvider());
2622

@@ -1747,4 +1743,156 @@ int M(string? x)
17471743
}
17481744
}
17491745
""");
1746+
1747+
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/74977")]
1748+
public Task TestMissingWithExhaustiveSwitchExpression1()
1749+
=> TestMissingInRegularAndScriptAsync($$"""
1750+
class C
1751+
{
1752+
void M(bool showDiagnosticMessages, string assemblyDisplayName, bool noColor)
1753+
{
1754+
var prefix = (showDiagnosticMessages, assemblyDisplayName, noColor) [||]switch
1755+
{
1756+
(false, _, _) => null,
1757+
(true, null, false) => "",
1758+
(true, null, true) => "[D]",
1759+
(true, _, false) => "No color output",
1760+
(true, _, true) => "Color output enabled",
1761+
};
1762+
}
1763+
}
1764+
""");
1765+
1766+
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/74977")]
1767+
public Task TestMissingWithExhaustiveSwitchExpression2()
1768+
=> TestMissingInRegularAndScriptAsync($$"""
1769+
class C
1770+
{
1771+
uint[] GetUnlockableRows(uint row1, uint row2)
1772+
{
1773+
return (row1, row2) [||]switch
1774+
{
1775+
(0, _) => [],
1776+
(uint i, 0) => [i],
1777+
(uint i, uint j) => [i, j],
1778+
};
1779+
}
1780+
}
1781+
""");
1782+
1783+
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/60784")]
1784+
public Task TestMissingWithExhaustiveSwitchExpression3()
1785+
=> TestMissingInRegularAndScriptAsync($$"""
1786+
class C
1787+
{
1788+
void GetUnlockableRows(bool FirstBoolean, bool SecondBoolean)
1789+
{
1790+
var v = (FirstBoolean, SecondBoolean) [||]switch
1791+
{
1792+
(true, true) => 1,
1793+
(true, false) => 2,
1794+
(false, true) => 3,
1795+
(false, false) => 4
1796+
};
1797+
}
1798+
}
1799+
""");
1800+
1801+
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/66433")]
1802+
public Task TestMissingWithExhaustiveSwitchExpression4()
1803+
=> TestMissingInRegularAndScriptAsync($$"""
1804+
class C
1805+
{
1806+
static string TestSwitch(int goo)
1807+
=> goo [||]switch
1808+
{
1809+
0 => "zero",
1810+
< 0 => "negative",
1811+
> 0 => "positive",
1812+
};
1813+
}
1814+
""");
1815+
1816+
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/76080")]
1817+
public Task TestMissingWithExhaustiveSwitchExpression5()
1818+
=> TestMissingInRegularAndScriptAsync($$"""
1819+
class C
1820+
{
1821+
static void M()
1822+
{
1823+
char c = 'x';
1824+
bool i = false;
1825+
1826+
var x = (c, i) [||]switch
1827+
{
1828+
('L', _) => '#',
1829+
('#', true) => 'L',
1830+
('#', false) => 'M',
1831+
(var z, _) => z,
1832+
};
1833+
}
1834+
}
1835+
""");
1836+
1837+
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/76080")]
1838+
public Task TestMissingWithExhaustiveSwitchExpression6()
1839+
=> TestMissingInRegularAndScriptAsync($$"""
1840+
class C
1841+
{
1842+
static void M()
1843+
{
1844+
char c = 'x';
1845+
bool i = false;
1846+
1847+
var y = (c, i) [||]switch
1848+
{
1849+
('L', _) => '#',
1850+
('#', true) => 'L',
1851+
('#', false) => 'M',
1852+
(var z, _) => z,
1853+
_ => ' ',
1854+
};
1855+
}
1856+
}
1857+
""");
1858+
1859+
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/57523")]
1860+
public Task TestMissingWithExhaustiveSwitchExpression7()
1861+
=> TestMissingInRegularAndScriptAsync($$"""
1862+
class C
1863+
{
1864+
public static string ToApiString(this bool? value)
1865+
=> value [||]switch
1866+
{
1867+
{ } val => val.ToApiString(),
1868+
null => "Unknown",
1869+
};
1870+
}
1871+
""");
1872+
1873+
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/57523")]
1874+
public Task TestMissingWithExhaustiveSwitchExpression8()
1875+
=> TestMissingInRegularAndScriptAsync($$"""
1876+
public class Program
1877+
{
1878+
public static int Main() => GetCuts(Shape.Circle) is null ? -1 : 0;
1879+
1880+
static int? GetCuts(Shape? shape)
1881+
=> shape [||]switch
1882+
{
1883+
Shape.Triangle => 3,
1884+
Shape.Square or Shape.Rectangle => 4,
1885+
Shape.Circle => 0,
1886+
_ => throw new System.NotSupportedException(),
1887+
};
1888+
}
1889+
1890+
public enum Shape
1891+
{
1892+
Circle,
1893+
Square,
1894+
Rectangle,
1895+
Triangle,
1896+
}
1897+
""");
17501898
}

‎src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchDiagnosticAnalyzer.cs‎

Lines changed: 26 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -10,34 +10,34 @@
1010

1111
namespace Microsoft.CodeAnalysis.PopulateSwitch;
1212

13-
internal abstract class AbstractPopulateSwitchDiagnosticAnalyzer<TSwitchOperation, TSwitchSyntax> :
14-
AbstractBuiltInCodeStyleDiagnosticAnalyzer
13+
internal abstract class AbstractPopulateSwitchDiagnosticAnalyzer<TSwitchOperation, TSwitchSyntax>(
14+
string diagnosticId,
15+
EnforceOnBuild enforceOnBuild)
16+
: AbstractBuiltInCodeStyleDiagnosticAnalyzer(diagnosticId,
17+
enforceOnBuild,
18+
option: null,
19+
s_localizableTitle, s_localizableMessage)
1520
where TSwitchOperation : IOperation
1621
where TSwitchSyntax : SyntaxNode
1722
{
1823
private static readonly LocalizableString s_localizableTitle = new LocalizableResourceString(nameof(AnalyzersResources.Add_missing_cases), AnalyzersResources.ResourceManager, typeof(AnalyzersResources));
1924
private static readonly LocalizableString s_localizableMessage = new LocalizableResourceString(nameof(AnalyzersResources.Populate_switch), AnalyzersResources.ResourceManager, typeof(AnalyzersResources));
2025

21-
protected AbstractPopulateSwitchDiagnosticAnalyzer(string diagnosticId, EnforceOnBuild enforceOnBuild)
22-
: base(diagnosticId,
23-
enforceOnBuild,
24-
option: null,
25-
s_localizableTitle, s_localizableMessage)
26-
{
27-
}
28-
2926
protected abstract OperationKind OperationKind { get; }
3027

3128
protected abstract bool IsSwitchTypeUnknown(TSwitchOperation operation);
3229
protected abstract IOperation GetValueOfSwitchOperation(TSwitchOperation operation);
3330

31+
protected abstract bool IsKnownToBeExhaustive(TSwitchOperation switchOperation);
32+
3433
protected abstract bool HasConstantCase(TSwitchOperation operation, object? value);
3534
protected abstract ICollection<ISymbol> GetMissingEnumMembers(TSwitchOperation operation);
3635
protected abstract bool HasDefaultCase(TSwitchOperation operation);
3736
protected abstract bool HasExhaustiveNullAndTypeCheckCases(TSwitchOperation operation);
3837
protected abstract Location GetDiagnosticLocation(TSwitchSyntax switchBlock);
3938

40-
public sealed override DiagnosticAnalyzerCategory GetAnalyzerCategory() => DiagnosticAnalyzerCategory.SemanticSpanAnalysis;
39+
public sealed override DiagnosticAnalyzerCategory GetAnalyzerCategory()
40+
=> DiagnosticAnalyzerCategory.SemanticSpanAnalysis;
4141

4242
protected sealed override void InitializeWorker(AnalysisContext context)
4343
=> context.RegisterOperationAction(AnalyzeOperation, OperationKind);
@@ -81,18 +81,23 @@ private void AnalyzeOperation(OperationAnalysisContext context)
8181
{
8282
var typeWithoutNullable = type.RemoveNullableIfPresent();
8383

84+
// We treat enum switches specially. Specifically, even if exhaustive (because of a 'default' case),
85+
// we still want to let users use the feature to fill in missing enum members. That way if they add
86+
// new enum members, they can quickly find and fix the switches that aren't explicitly handling those cases.
87+
//
88+
// Note: this should likely be a refactoring instead of an analyzer. However, for historical reasons,
89+
// we shipped in this fashion.
90+
if (typeWithoutNullable.TypeKind == TypeKind.Enum)
91+
return AnalyzeEnumSwitch(switchOperation, type);
92+
93+
// For all other types, we don't want to offer the user anything if the switch is already exhaustive.
94+
if (this.IsKnownToBeExhaustive(switchOperation))
95+
return default;
96+
8497
if (typeWithoutNullable.SpecialType == SpecialType.System_Boolean)
85-
{
8698
return AnalyzeBooleanSwitch(switchOperation, type);
87-
}
88-
else if (typeWithoutNullable.TypeKind == TypeKind.Enum)
89-
{
90-
return AnalyzeEnumSwitch(switchOperation, type);
91-
}
92-
else
93-
{
94-
return (missingCases: false, missingDefaultCase: !HasDefaultCase(switchOperation));
95-
}
99+
100+
return (missingCases: false, missingDefaultCase: !HasDefaultCase(switchOperation));
96101
}
97102

98103
private (bool missingCases, bool missingDefaultCase) AnalyzeBooleanSwitch(TSwitchOperation operation, ITypeSymbol type)

‎src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchExpressionDiagnosticAnalyzer.cs‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,18 +8,17 @@
88

99
namespace Microsoft.CodeAnalysis.PopulateSwitch;
1010

11-
internal abstract class AbstractPopulateSwitchExpressionDiagnosticAnalyzer<TSwitchSyntax> :
12-
AbstractPopulateSwitchDiagnosticAnalyzer<ISwitchExpressionOperation, TSwitchSyntax>
11+
internal abstract class AbstractPopulateSwitchExpressionDiagnosticAnalyzer<TSwitchSyntax>()
12+
: AbstractPopulateSwitchDiagnosticAnalyzer<ISwitchExpressionOperation, TSwitchSyntax>(
13+
IDEDiagnosticIds.PopulateSwitchExpressionDiagnosticId,
14+
EnforceOnBuildValues.PopulateSwitchExpression)
1315
where TSwitchSyntax : SyntaxNode
1416
{
15-
protected AbstractPopulateSwitchExpressionDiagnosticAnalyzer()
16-
: base(IDEDiagnosticIds.PopulateSwitchExpressionDiagnosticId,
17-
EnforceOnBuildValues.PopulateSwitchExpression)
18-
{
19-
}
20-
2117
protected sealed override OperationKind OperationKind => OperationKind.SwitchExpression;
2218

19+
protected override bool IsKnownToBeExhaustive(ISwitchExpressionOperation switchOperation)
20+
=> switchOperation.IsExhaustive;
21+
2322
protected override IOperation GetValueOfSwitchOperation(ISwitchExpressionOperation operation)
2423
=> operation.Value;
2524

‎src/Analyzers/Core/Analyzers/PopulateSwitch/AbstractPopulateSwitchStatementDiagnosticAnalyzer.cs‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,18 +8,17 @@
88

99
namespace Microsoft.CodeAnalysis.PopulateSwitch;
1010

11-
internal abstract class AbstractPopulateSwitchStatementDiagnosticAnalyzer<TSwitchSyntax> :
12-
AbstractPopulateSwitchDiagnosticAnalyzer<ISwitchOperation, TSwitchSyntax>
11+
internal abstract class AbstractPopulateSwitchStatementDiagnosticAnalyzer<TSwitchSyntax>()
12+
: AbstractPopulateSwitchDiagnosticAnalyzer<ISwitchOperation, TSwitchSyntax>(
13+
IDEDiagnosticIds.PopulateSwitchStatementDiagnosticId,
14+
EnforceOnBuildValues.PopulateSwitchStatement)
1315
where TSwitchSyntax : SyntaxNode
1416
{
15-
protected AbstractPopulateSwitchStatementDiagnosticAnalyzer()
16-
: base(IDEDiagnosticIds.PopulateSwitchStatementDiagnosticId,
17-
EnforceOnBuildValues.PopulateSwitchStatement)
18-
{
19-
}
20-
2117
protected sealed override OperationKind OperationKind => OperationKind.Switch;
2218

19+
protected override bool IsKnownToBeExhaustive(ISwitchOperation switchOperation)
20+
=> false;
21+
2322
protected override IOperation GetValueOfSwitchOperation(ISwitchOperation operation)
2423
=> operation.Value;
2524

0 commit comments

Comments
 (0)