From 1b4e910b377e9643b8bc7a6fea9c549b4680ef56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sven=20H=C3=BCbner?= Date: Sun, 13 Nov 2022 14:00:50 +0100 Subject: [PATCH] 1: add option to treat record-based hierarchies as closed via .editorconfig dotnet_diagnostic.CTH001.suppress_on_record_hierarchies = true could not make unit tests work for this --- .../TypeHierarchies.cs | 36 ++++++++++++++-- .../TypeHierarchyHelperTests.cs | 42 +++++++++++++++++-- .../OptionsHelper.cs | 16 +++++++ .../SwitchExpressionSuppressor.cs | 5 ++- .../SwitchStatementSuppressor.cs | 7 +++- .../TypeHierarchyHelper.cs | 28 +++++++++++-- README.md | 6 +++ 7 files changed, 126 insertions(+), 14 deletions(-) create mode 100644 ClosedTypeHierarchyDiagnosticSuppressor/OptionsHelper.cs diff --git a/ClosedTypeHierarchyDiagnosticSuppressor.Tests/TypeHierarchies.cs b/ClosedTypeHierarchyDiagnosticSuppressor.Tests/TypeHierarchies.cs index d255146..371b9bb 100644 --- a/ClosedTypeHierarchyDiagnosticSuppressor.Tests/TypeHierarchies.cs +++ b/ClosedTypeHierarchyDiagnosticSuppressor.Tests/TypeHierarchies.cs @@ -40,6 +40,18 @@ public sealed class Leaf2 : Root { 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 = @" @@ -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 {} } @@ -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 {} + } + "; } } diff --git a/ClosedTypeHierarchyDiagnosticSuppressor.Tests/TypeHierarchyHelperTests.cs b/ClosedTypeHierarchyDiagnosticSuppressor.Tests/TypeHierarchyHelperTests.cs index 4b534a9..06ba12e 100644 --- a/ClosedTypeHierarchyDiagnosticSuppressor.Tests/TypeHierarchyHelperTests.cs +++ b/ClosedTypeHierarchyDiagnosticSuppressor.Tests/TypeHierarchyHelperTests.cs @@ -1,6 +1,7 @@ using Microsoft.CodeAnalysis; using NUnit.Framework; using SvSoft.Analyzers.ClosedTypeHerarchyDiagnosticSuppression; +using System; using System.Collections.Generic; using System.Linq; @@ -31,7 +32,7 @@ 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)); @@ -39,15 +40,38 @@ public void When_type_hierarchy_is_closed_Then_returns_leaf_types(string typeCod [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 ClosedSamples = new (string SampleName, string TypeCode, string[] ExpectedTypeNames)[] { ("Simple", TypeHierarchies.Closed.Simple, new[] { "Leaf1", "Leaf2" }), @@ -55,8 +79,18 @@ public void When_type_hierarchy_is_not_closed_Then_returns_null(string typeCode) ("Generic", TypeHierarchies.Closed.Generic, new[] { "Leaf1", "Leaf2" }) }.Select(t => new TestCaseData(t.TypeCode, t.ExpectedTypeNames).SetArgDisplayNames(t.SampleName)); + public static readonly IEnumerable 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 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 NotClosedCodeOnlySamples = typeof(TypeHierarchies.NotClosed) + .GetFields() + .Select(f => (string)f.GetValue(null)!); } diff --git a/ClosedTypeHierarchyDiagnosticSuppressor/OptionsHelper.cs b/ClosedTypeHierarchyDiagnosticSuppressor/OptionsHelper.cs new file mode 100644 index 0000000..b880a54 --- /dev/null +++ b/ClosedTypeHierarchyDiagnosticSuppressor/OptionsHelper.cs @@ -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; + } +} diff --git a/ClosedTypeHierarchyDiagnosticSuppressor/SwitchExpressionSuppressor.cs b/ClosedTypeHierarchyDiagnosticSuppressor/SwitchExpressionSuppressor.cs index e207a41..8cc8ed7 100644 --- a/ClosedTypeHierarchyDiagnosticSuppressor/SwitchExpressionSuppressor.cs +++ b/ClosedTypeHierarchyDiagnosticSuppressor/SwitchExpressionSuppressor.cs @@ -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; @@ -41,6 +42,8 @@ void HandleDiagnostic(Diagnostic diagnostic) return; } + bool allowRecords = context.AreRecordHierarchiesAllowed(node.SyntaxTree); + var switchExpression = node.DescendantNodesAndSelf().OfType().FirstOrDefault(); ExpressionSyntax switchee = switchExpression.GoverningExpression; @@ -51,7 +54,7 @@ void HandleDiagnostic(Diagnostic diagnostic) return; } - if (TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(switcheeType) is not IEnumerable subtypes) + if (TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(switcheeType, allowRecords) is not IEnumerable subtypes) { return; } diff --git a/ClosedTypeHierarchyDiagnosticSuppressor/SwitchStatementSuppressor.cs b/ClosedTypeHierarchyDiagnosticSuppressor/SwitchStatementSuppressor.cs index 1a8c4bc..6a02d24 100644 --- a/ClosedTypeHierarchyDiagnosticSuppressor/SwitchStatementSuppressor.cs +++ b/ClosedTypeHierarchyDiagnosticSuppressor/SwitchStatementSuppressor.cs @@ -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; @@ -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 SupportedSuppressions { get; } = ImmutableArray.CreateRange(SuppressionDescriptorByDiagnosticId.Values); - + public override void ReportSuppressions(SuppressionAnalysisContext context) { foreach (Diagnostic diagnostic in context.ReportedDiagnostics) @@ -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); @@ -47,7 +50,7 @@ void HandleDiagnostic(Diagnostic diagnostic) return; } - if (TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(switcheeType) is not IEnumerable subtypes) + if (TypeHierarchyHelper.InterpretAsClosedTypeHierarchy(switcheeType, allowRecords) is not IEnumerable subtypes) { return; } diff --git a/ClosedTypeHierarchyDiagnosticSuppressor/TypeHierarchyHelper.cs b/ClosedTypeHierarchyDiagnosticSuppressor/TypeHierarchyHelper.cs index 166f215..e9c524a 100644 --- a/ClosedTypeHierarchyDiagnosticSuppressor/TypeHierarchyHelper.cs +++ b/ClosedTypeHierarchyDiagnosticSuppressor/TypeHierarchyHelper.cs @@ -6,7 +6,7 @@ namespace SvSoft.Analyzers.ClosedTypeHerarchyDiagnosticSuppression; public static class TypeHierarchyHelper { - public static IEnumerable? InterpretAsClosedTypeHierarchy(INamedTypeSymbol typeSymbol) + public static IEnumerable? InterpretAsClosedTypeHierarchy(INamedTypeSymbol typeSymbol, bool allowRecords) { if (!IsPartOfClosedHierarchy(typeSymbol)) { @@ -15,7 +15,7 @@ public static class TypeHierarchyHelper return GetConcreteSubtypes(typeSymbol); - static bool IsPartOfClosedHierarchy(INamedTypeSymbol typeSymbol) + bool IsPartOfClosedHierarchy(INamedTypeSymbol typeSymbol) { if (CanBeClosedHierarchyRoot(typeSymbol)) { @@ -28,7 +28,7 @@ static bool IsPartOfClosedHierarchy(INamedTypeSymbol typeSymbol) return CanBeClosedHierarchyLeaf(typeSymbol); } - static IEnumerable GetConcreteSubtypes(INamedTypeSymbol typeSymbol) + IEnumerable GetConcreteSubtypes(INamedTypeSymbol typeSymbol) { if (CanBeClosedHierarchyRoot(typeSymbol)) { @@ -51,10 +51,30 @@ static IEnumerable 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 = "$"; + + 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; } } diff --git a/README.md b/README.md index 2e3fb9c..ab26ff3 100644 --- a/README.md +++ b/README.md @@ -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.