Skip to content

Commit

Permalink
1: add option to treat record-based hierarchies as closed
Browse files Browse the repository at this point in the history
via .editorconfig dotnet_diagnostic.CTH001.suppress_on_record_hierarchies = true

could not make unit tests work for this
  • Loading branch information
shuebner committed Nov 13, 2022
1 parent c0f6258 commit 1b4e910
Show file tree
Hide file tree
Showing 7 changed files with 126 additions and 14 deletions.
36 changes: 33 additions & 3 deletions ClosedTypeHierarchyDiagnosticSuppressor.Tests/TypeHierarchies.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ public sealed class Leaf2 : Root<T> { public Leaf2(T value) : base(value) {} }
";
}

public static class ProtectedCopyConstructorOnly
{
public const string ImplicitProtectedCopyCtor = @"
abstract record Root
{
Root() {}
public sealed record Leaf1 : Root {}
public sealed record Leaf2 : Root {}
}
";
}

public static class NotClosed
{
public const string RootNotAbstract = @"
Expand All @@ -60,10 +72,10 @@ public sealed class Leaf2 : Root {}
}
";

public const string ImplicitProtectedCopyCtor = @"
abstract record Root
public const string ProtectedCtorOtherThanCopyConstructor = @"
abstract class Root
{
Root() {}
protected Root() {}
public sealed record Leaf1 : Root {}
public sealed record Leaf2 : Root {}
}
Expand All @@ -77,5 +89,23 @@ public class Leaf1 : Root {}
public sealed class Leaf2 : Root {}
}
";

public const string ExplicitPrivateProtectedCopyCtor = @"
abstract class Root
{
private protected Root(Root root) {}
public sealed record Leaf1 : Root {}
public sealed record Leaf2 : Root {}
}
";

public const string ExplicitProtectedCopyCtor = @"
abstract class Root
{
protected Root(Root root) {}
public sealed record Leaf1 : Root {}
public sealed record Leaf2 : Root {}
}
";
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using Microsoft.CodeAnalysis;
using NUnit.Framework;
using SvSoft.Analyzers.ClosedTypeHerarchyDiagnosticSuppression;
using System;
using System.Collections.Generic;
using System.Linq;

Expand Down Expand Up @@ -31,32 +32,65 @@ public void When_type_hierarchy_is_closed_Then_returns_leaf_types(string typeCod
{
INamedTypeSymbol type = GetRootTypeCandidate(typeCode, "Root");

var subtypes = TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(type);
var subtypes = TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(type, false);

Assert.That(subtypes, Is.Not.Null);
Assert.That(subtypes!.Select(s => s.Name), Is.EquivalentTo(expectedSubTypes));
}

[Test]
[TestCaseSource(nameof(NotClosedSamples))]
public void When_type_hierarchy_is_not_closed_Then_returns_null(string typeCode)
public void When_type_hierarchy_is_not_closed_Then_returns_null(string typeCode, bool allowProtectedCopyCtors)
{
INamedTypeSymbol type = GetRootTypeCandidate(typeCode, "Root");

var subtypes = TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(type);
var subtypes = TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(type, allowRecords: allowProtectedCopyCtors);

Assert.That(subtypes, Is.Null);
}

[Test]
[TestCaseSource(nameof(ProtectedCopyConstructorOnlySamples))]
public void When_type_hierarchy_is_closed_except_for_copy_ctor_And_copy_ctor_is_not_explicitly_allowed_Then_returns_null(string typeCode, string[] _)
{
INamedTypeSymbol type = GetRootTypeCandidate(typeCode, "Root");

var subtypes = TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(type, false);

Assert.That(subtypes, Is.Null);
}

[Test]
[TestCaseSource(nameof(ProtectedCopyConstructorOnlySamples))]
public void When_type_hierarchy_is_closed_except_for_copy_ctor_And_copy_ctor_is_explicitly_allowed_Then_returns_leaf_types(string typeCode, string[] expectedSubTypes)
{
INamedTypeSymbol type = GetRootTypeCandidate(typeCode, "Root");

var subtypes = TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(type, true);

Assert.That(subtypes, Is.Not.Null);
Assert.That(subtypes!.Select(s => s.Name), Is.EquivalentTo(expectedSubTypes));
}

public static readonly IEnumerable<TestCaseData> ClosedSamples = new (string SampleName, string TypeCode, string[] ExpectedTypeNames)[]
{
("Simple", TypeHierarchies.Closed.Simple, new[] { "Leaf1", "Leaf2" }),
("Nested", TypeHierarchies.Closed.Nested, new[] { "Leaf1", "Leaf2", "Leaf3" }),
("Generic", TypeHierarchies.Closed.Generic, new[] { "Leaf1", "Leaf2" })
}.Select(t => new TestCaseData(t.TypeCode, t.ExpectedTypeNames).SetArgDisplayNames(t.SampleName));

public static readonly IEnumerable<TestCaseData> ProtectedCopyConstructorOnlySamples = new (string SampleName, string TypeCode, string[] ExpectedTypeNames)[]
{
("ImplicitProtectedCopyCtor", TypeHierarchies.ProtectedCopyConstructorOnly.ImplicitProtectedCopyCtor, new[] { "Leaf1", "Leaf2" }),
}.Select(t => new TestCaseData(t.TypeCode, t.ExpectedTypeNames).SetArgDisplayNames(t.SampleName));

public static readonly IEnumerable<TestCaseData> NotClosedSamples = typeof(TypeHierarchies.NotClosed)
.GetFields()
.Select(f => (SampleName: f.Name, TypeCode: f.GetValue(null)))
.Select(t => new TestCaseData(t.TypeCode).SetArgDisplayNames(t.SampleName));
.SelectMany(t => new[] { true, false }.Select(allowCopyCtor => (allowCopyCtor, t))
.Select(t => new TestCaseData(t.t.TypeCode, t.allowCopyCtor).SetArgDisplayNames(t.t.SampleName, t.allowCopyCtor ? "allow copy ctor" : "disallow copy ctor")));

public static readonly IEnumerable<string> NotClosedCodeOnlySamples = typeof(TypeHierarchies.NotClosed)
.GetFields()
.Select(f => (string)f.GetValue(null)!);
}
16 changes: 16 additions & 0 deletions ClosedTypeHierarchyDiagnosticSuppressor/OptionsHelper.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;

namespace SvSoft.Analyzers.ClosedTypeHierarchyDiagnosticSuppression;
static class OptionsHelper
{
public static bool AreRecordHierarchiesAllowed(this SuppressionAnalysisContext context, SyntaxTree syntaxTree)
{
AnalyzerConfigOptions options = context.Options.AnalyzerConfigOptionsProvider.GetOptions(syntaxTree);
bool allowRecords = options.TryGetValue("dotnet_diagnostic.CTH001.suppress_on_record_hierarchies", out string? allowRecordHierarchiesStr) &&
bool.TryParse(allowRecordHierarchiesStr, out bool allowRecordHierarchies)
&& allowRecordHierarchies;

return allowRecords;
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using SvSoft.Analyzers.ClosedTypeHierarchyDiagnosticSuppression;
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
Expand Down Expand Up @@ -41,6 +42,8 @@ void HandleDiagnostic(Diagnostic diagnostic)
return;
}

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

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

ExpressionSyntax switchee = switchExpression.GoverningExpression;
Expand All @@ -51,7 +54,7 @@ void HandleDiagnostic(Diagnostic diagnostic)
return;
}

if (TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(switcheeType) is not IEnumerable<INamedTypeSymbol> subtypes)
if (TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(switcheeType, allowRecords) is not IEnumerable<INamedTypeSymbol> subtypes)
{
return;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using SvSoft.Analyzers.ClosedTypeHierarchyDiagnosticSuppression;
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
Expand All @@ -18,7 +19,7 @@ public sealed class SwitchStatementSuppressor : DiagnosticSuppressor
id => new SuppressionDescriptor("CTH001", id, "every possible type of closed type hierarchy was matched without restrictions"));

public override ImmutableArray<SuppressionDescriptor> SupportedSuppressions { get; } = ImmutableArray.CreateRange(SuppressionDescriptorByDiagnosticId.Values);

public override void ReportSuppressions(SuppressionAnalysisContext context)
{
foreach (Diagnostic diagnostic in context.ReportedDiagnostics)
Expand All @@ -39,6 +40,8 @@ void HandleDiagnostic(Diagnostic diagnostic)
return;
}

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

ExpressionSyntax switchee = switchStatement.Expression;
var switcheeModel = context.GetSemanticModel(switchee.SyntaxTree);
var switcheeTypeInfo = switcheeModel.GetTypeInfo(switchee);
Expand All @@ -47,7 +50,7 @@ void HandleDiagnostic(Diagnostic diagnostic)
return;
}

if (TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(switcheeType) is not IEnumerable<INamedTypeSymbol> subtypes)
if (TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(switcheeType, allowRecords) is not IEnumerable<INamedTypeSymbol> subtypes)
{
return;
}
Expand Down
28 changes: 24 additions & 4 deletions ClosedTypeHierarchyDiagnosticSuppressor/TypeHierarchyHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace SvSoft.Analyzers.ClosedTypeHerarchyDiagnosticSuppression;

public static class TypeHierarchyHelper
{
public static IEnumerable<INamedTypeSymbol>? InterpretAsClosedTypeHierarchy(INamedTypeSymbol typeSymbol)
public static IEnumerable<INamedTypeSymbol>? InterpretAsClosedTypeHierarchy(INamedTypeSymbol typeSymbol, bool allowRecords)
{
if (!IsPartOfClosedHierarchy(typeSymbol))
{
Expand All @@ -15,7 +15,7 @@ public static class TypeHierarchyHelper

return GetConcreteSubtypes(typeSymbol);

static bool IsPartOfClosedHierarchy(INamedTypeSymbol typeSymbol)
bool IsPartOfClosedHierarchy(INamedTypeSymbol typeSymbol)
{
if (CanBeClosedHierarchyRoot(typeSymbol))
{
Expand All @@ -28,7 +28,7 @@ static bool IsPartOfClosedHierarchy(INamedTypeSymbol typeSymbol)
return CanBeClosedHierarchyLeaf(typeSymbol);
}

static IEnumerable<INamedTypeSymbol> GetConcreteSubtypes(INamedTypeSymbol typeSymbol)
IEnumerable<INamedTypeSymbol> GetConcreteSubtypes(INamedTypeSymbol typeSymbol)
{
if (CanBeClosedHierarchyRoot(typeSymbol))
{
Expand All @@ -51,10 +51,30 @@ static IEnumerable<INamedTypeSymbol> GetConcreteSubtypes(INamedTypeSymbol typeSy
}
}

static bool CanBeClosedHierarchyRoot(INamedTypeSymbol rootCandidate) =>
bool CanBeClosedHierarchyRoot(INamedTypeSymbol rootCandidate) =>
rootCandidate.IsAbstract &&
(allowRecords
? HasOnlyPrivateConstructorsAndProtectedCopyCtors(rootCandidate)
: HasOnlyPrivateConstructors(rootCandidate));

static bool HasOnlyPrivateConstructors(INamedTypeSymbol rootCandidate) =>
rootCandidate.Constructors.All(c => c.DeclaredAccessibility == Accessibility.Private);

static bool HasOnlyPrivateConstructorsAndProtectedCopyCtors(INamedTypeSymbol rootCandidate) =>
HasOnlyPrivateConstructors(rootCandidate) ||
(IsRecord(rootCandidate) &&
rootCandidate.Constructors.All(c => c.DeclaredAccessibility == Accessibility.Private || MatchesImplicitlyCreatedRecordCopyCtor(rootCandidate, c)));

const string CompilerCreatedCloneMethodNameOnRecordTypes = "<Clone>$";

static bool IsRecord(INamedTypeSymbol recordCandidate) =>
recordCandidate.MemberNames.Contains(CompilerCreatedCloneMethodNameOnRecordTypes);

static bool MatchesImplicitlyCreatedRecordCopyCtor(INamedTypeSymbol constructedType, IMethodSymbol ctor) =>
ctor.DeclaredAccessibility == Accessibility.Protected &&
ctor.Parameters.Length == 1 &&
ctor.Parameters[0].Type.Equals(constructedType, SymbolEqualityComparer.Default);

static bool CanBeClosedHierarchyLeaf(INamedTypeSymbol typeSymbol) => typeSymbol.IsSealed;
}
}
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ Get on [nuget.org](https://www.nuget.org/packages/SvSoft.Analyzers.ClosedTypeHie
There are no attributes and no configuration.
It will just do The Right Thing™.

The only exception is the configuration for opting into treating type hierarchies based on `record` types as closed, even though [they are not because of their implicit protected copy constructor](https://svenhuebner-it.com/closed-type-hierarchies-with-records-not/).

```
dotnet_diagnostic.CTH001.suppress_on_record_hierarchies
```

See the test samples for [switch statement](https://github.com/shuebner/ClosedTypeHierarchyDiagnosticSuppressor/blob/main/ClosedTypeHierarchyDiagnosticSuppressor.Tests/SwitchStatementSuppressorTests.cs) and [switch expression](https://github.com/shuebner/ClosedTypeHierarchyDiagnosticSuppressor/blob/main/ClosedTypeHierarchyDiagnosticSuppressor.Tests/SwitchExpressionSuppressorTests.cs) to see what is supported.
I may add more documentation and examples in this README soon.

Expand Down

0 comments on commit 1b4e910

Please sign in to comment.