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

Jump to bottom

Add support for destructured records #7

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add support for destructured records #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
99bdc8d
add support for destructured records
rfvgyhn Dec 7, 2023
ba051a6
add unit tests
rfvgyhn Dec 7, 2023
82b637b
make new constructors internal
rfvgyhn Jun 6, 2024
902d824
add negative test for matching against base type
rfvgyhn Jun 17, 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
48 changes: 44 additions & 4 deletions ClosedTypeHierarchyDiagnosticSuppressor.Tests/SwitchExpressionSuppressorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,16 @@ class SwitchExpressionSuppressorTests
"Microsoft.CodeAnalysis.CSharp.PopulateSwitch.CSharpPopulateSwitchExpressionDiagnosticAnalyzer")?.Unwrap()
?? throw new InvalidOperationException("could not instantiate populate switch expression analyzer for IDE0072"));

Task EnsureNotSuppressed(string code, NullableContextOptions nullableContextOptions) =>
Task EnsureNotSuppressed(string code, NullableContextOptions nullableContextOptions, bool allowRecords = false) =>
DiagnosticSuppressorAnalyer.EnsureNotSuppressed(
new SwitchExpressionSuppressor(),
new SwitchExpressionSuppressor(allowRecords),
code,
nullableContextOptions,
("IDE0072", IDE0072Analyzer));

Task EnsureSuppressed(string code, NullableContextOptions nullableContextOptions) =>
Task EnsureSuppressed(string code, NullableContextOptions nullableContextOptions, bool allowRecords = false) =>
DiagnosticSuppressorAnalyer.EnsureSuppressed(
new SwitchExpressionSuppressor(),
new SwitchExpressionSuppressor(allowRecords),
SwitchExpressionSuppressor.SuppressionDescriptorByDiagnosticId.Values,
code,
nullableContextOptions,
Expand Down Expand Up @@ -87,6 +87,46 @@ public static int DoSwitch(Root root)

return EnsureNotSuppressed(code, NullableContextOptions.Disable);
}

[Test]
public Task When_record_with_destructure_And_all_subtypes_match_Then_suppress()
{
var code = CodeHelper.WrapInNamespace(TypeHierarchies.ProtectedCopyConstructorOnly.PositionalParameter + @"
static class SwitchTest
{
public static int DoSwitch(Root root)
{
return root switch
{
Root.Leaf1(var v) => 0,
Root.Leaf2 => 1,
};
}
}
");

return EnsureSuppressed(code, NullableContextOptions.Enable, allowRecords: true);
}

[Test]
public Task When_record_with_destructure_And_only_base_type_is_matched_Then_do_not_suppress()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of the tests are the wrong way around. The test should define a hierarchy where Leaf1 has a object v and then test that a match agains Leaf1(string v) is not exhaustive.

The fact that the current wrong tests succeed is due to a missing feature implementation in the IsDeclarationPatternNonRestrictive method. The method currently expects containingSubpatternSyntax.NameColon to be present, whereas with deconstruction it is not.

Please fix the tests and add the implementation accordingly.

The positive tests (that assert suppression) happen to succeed correctly because they use var. Had you written them with object v, they would have failed as well and revealed the missing implementation.

{
var code = CodeHelper.WrapInNamespace(TypeHierarchies.ProtectedCopyConstructorOnly.PositionalParameter + @"
static class SwitchTest
{
public static int DoSwitch(Root root)
{
return root switch
{
Root.Leaf1(object v) => 0,
Root.Leaf2 => 1,
};
}
}
");

return EnsureNotSuppressed(code, NullableContextOptions.Enable, allowRecords: true);
}

[Test]
public Task When_nullable_is_disabled_And_null_is_matched_on_its_own_Then_suppress()
Expand Down
52 changes: 48 additions & 4 deletions ClosedTypeHierarchyDiagnosticSuppressor.Tests/SwitchStatementSuppressorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,16 @@ class SwitchStatementSuppressorTests
"Microsoft.CodeAnalysis.CSharp.PopulateSwitch.CSharpPopulateSwitchStatementDiagnosticAnalyzer")?.Unwrap()
?? throw new InvalidOperationException("could not instantiate populate switch statement analyzer for IDE0072"));

Task EnsureNotSuppressed(string code, NullableContextOptions nullableContextOptions) =>
Task EnsureNotSuppressed(string code, NullableContextOptions nullableContextOptions, bool allowRecords = false) =>
DiagnosticSuppressorAnalyer.EnsureNotSuppressed(
new SwitchStatementSuppressor(),
new SwitchStatementSuppressor(allowRecords),
code,
nullableContextOptions,
("IDE0010", IDE0010Analyzer));

Task EnsureSuppressed(string code, NullableContextOptions nullableContextOptions) =>
Task EnsureSuppressed(string code, NullableContextOptions nullableContextOptions, bool allowRecords = false) =>
DiagnosticSuppressorAnalyer.EnsureSuppressed(
new SwitchStatementSuppressor(),
new SwitchStatementSuppressor(allowRecords),
SwitchStatementSuppressor.SuppressionDescriptorByDiagnosticId.Values,
code,
nullableContextOptions,
Expand Down Expand Up @@ -94,6 +94,50 @@ public static void DoSwitch(Root root)
return EnsureNotSuppressed(code, NullableContextOptions.Disable);
}

[Test]
public Task When_record_with_destructure_And_all_subtypes_match_Then_suppress()
{
var code = CodeHelper.WrapInNamespace(TypeHierarchies.ProtectedCopyConstructorOnly.PositionalParameter + @"
static class SwitchTest
{
public static void DoSwitch(Root root)
{
switch(root)
{
case Root.Leaf1(var v):
rfvgyhn marked this conversation as resolved.
Show resolved Hide resolved
break;
case Root.Leaf2:
break;
}
}
}
");

return EnsureSuppressed(code, NullableContextOptions.Enable, allowRecords: true);
}

[Test]
public Task When_record_with_destructure_And_only_base_type_is_matched_Then_do_not_suppress()
{
var code = CodeHelper.WrapInNamespace(TypeHierarchies.ProtectedCopyConstructorOnly.PositionalParameter + @"
static class SwitchTest
{
public static void DoSwitch(Root root)
{
switch(root)
{
case Root.Leaf1(object v):
break;
case Root.Leaf2:
break;
};
}
}
");

return EnsureNotSuppressed(code, NullableContextOptions.Enable, allowRecords: true);
}

[Test]
public Task When_nullable_is_disabled_And_null_is_matched_on_its_own_Then_suppress()
{
Expand Down
8 changes: 8 additions & 0 deletions ClosedTypeHierarchyDiagnosticSuppressor.Tests/TypeHierarchies.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ abstract record Root
public sealed record Leaf1 : Root {}
public sealed record Leaf2 : Root {}
}
";
public const string PositionalParameter = @"
abstract record Root
{
Root() {}
public sealed record Leaf1(string Value) : Root {}
public sealed record Leaf2 : Root {}
}
";
}

Expand Down
4 changes: 4 additions & 0 deletions ClosedTypeHierarchyDiagnosticSuppressor/ClosedTypeHierarchyDiagnosticSuppressor.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
<Description>Suppresses exhaustiveness warnings of switch statement and switch expressions when all cases of a "closed type hierarchy" are covered. See project URL for details. It suppresses IDE0010, IDE0072 and CS8509. It is NRT-aware.</Description>
</PropertyGroup>

<ItemGroup>
<InternalsVisibleTo Include="ClosedTypeHierarchyDiagnosticSuppressor.Tests" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.1.1" PrivateAssets="All" />
</ItemGroup>
Expand Down
3 changes: 2 additions & 1 deletion ClosedTypeHierarchyDiagnosticSuppressor/PatternHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ bool IsSubpatternNonRestrictive(SubpatternSyntax subpatternSyntax) =>
};

bool IsRecursivePatternNonRestrictive(RecursivePatternSyntax recursivePatternSyntax) =>
recursivePatternSyntax.PropertyPatternClause?.Subpatterns.All(IsSubpatternNonRestrictive) ?? false;
(recursivePatternSyntax.PropertyPatternClause?.Subpatterns ?? recursivePatternSyntax.PositionalPatternClause?.Subpatterns)
?.All(IsSubpatternNonRestrictive) ?? false;

bool IsDeclarationPatternNonRestrictive(DeclarationPatternSyntax declarationPatternSyntax, SubpatternSyntax containingSubpatternSyntax)
{
Expand Down
12 changes: 11 additions & 1 deletion ClosedTypeHierarchyDiagnosticSuppressor/SwitchExpressionSuppressor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ namespace SvSoft.Analyzers.ClosedTypeHerarchyDiagnosticSuppression;
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class SwitchExpressionSuppressor : DiagnosticSuppressor
{
readonly bool _forceAllowRecords;

public SwitchExpressionSuppressor() : this(false) { }

/// <summary>Constructor to facilitate unit testing</summary>
internal SwitchExpressionSuppressor(bool forceAllowRecords)
{
_forceAllowRecords = forceAllowRecords;
}

static readonly string[] SuppressedDiagnosticIds = { "CS8509", "IDE0072" };

public static readonly IReadOnlyDictionary<string, SuppressionDescriptor> SuppressionDescriptorByDiagnosticId = SuppressedDiagnosticIds.ToDictionary(
Expand Down Expand Up @@ -42,7 +52,7 @@ void HandleDiagnostic(Diagnostic diagnostic)
return;
}

bool allowRecords = context.AreRecordHierarchiesAllowed(node.SyntaxTree);
bool allowRecords = _forceAllowRecords || context.AreRecordHierarchiesAllowed(node.SyntaxTree);

var switchExpression = node.DescendantNodesAndSelf().OfType<SwitchExpressionSyntax>().FirstOrDefault();

Expand Down
12 changes: 11 additions & 1 deletion ClosedTypeHierarchyDiagnosticSuppressor/SwitchStatementSuppressor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@ namespace SvSoft.Analyzers.ClosedTypeHerarchyDiagnosticSuppression;
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class SwitchStatementSuppressor : DiagnosticSuppressor
{
readonly bool _forceAllowRecords;

public SwitchStatementSuppressor() : this(false) { }

/// <summary>Constructor to facilitate unit testing</summary>
internal SwitchStatementSuppressor(bool forceAllowRecords)
{
_forceAllowRecords = forceAllowRecords;
}

static readonly string[] SuppressedDiagnosticIds = { "IDE0010" };

public static readonly IReadOnlyDictionary<string, SuppressionDescriptor> SuppressionDescriptorByDiagnosticId = SuppressedDiagnosticIds.ToDictionary(
Expand Down Expand Up @@ -40,7 +50,7 @@ void HandleDiagnostic(Diagnostic diagnostic)
return;
}

bool allowRecords = context.AreRecordHierarchiesAllowed(node.SyntaxTree);
bool allowRecords = _forceAllowRecords || context.AreRecordHierarchiesAllowed(node.SyntaxTree);

ExpressionSyntax switchee = switchStatement.Expression;
var switcheeModel = context.GetSemanticModel(switchee.SyntaxTree);
Expand Down