From 79ed574dfeda6bf61b469c676574d6b79aff8bf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=A3o=20Almada?= Date: Sun, 19 Apr 2020 17:02:02 +0100 Subject: [PATCH] HLQ006: GetEnumerator() or GetAsyncEnumerator() should not return an interface type (#22) * Add GetEnumeratorReturnTypeAnalyzer * Add documentation --- .../GetEnumeratorReturnTypeAnalyzerTests.cs | 182 ++++++++++++++++++ NetFabric.Hyperlinq.Analyzer/DiagnosticIds.cs | 1 + .../GetEnumeratorReturnTypeAnalyzer.cs | 81 ++++++++ .../Resources.Designer.cs | 32 ++- NetFabric.Hyperlinq.Analyzer/Resources.resx | 11 +- .../HLQ006_GetEnumeratorReturnType.md | 77 ++++++++ 6 files changed, 380 insertions(+), 4 deletions(-) create mode 100644 NetFabric.Hyperlinq.Analyzer.UnitTests/GetEnumeratorReturnTypeAnalyzerTests.cs create mode 100644 NetFabric.Hyperlinq.Analyzer/GetEnumeratorReturnTypeAnalyzer.cs create mode 100644 docs/reference/HLQ006_GetEnumeratorReturnType.md diff --git a/NetFabric.Hyperlinq.Analyzer.UnitTests/GetEnumeratorReturnTypeAnalyzerTests.cs b/NetFabric.Hyperlinq.Analyzer.UnitTests/GetEnumeratorReturnTypeAnalyzerTests.cs new file mode 100644 index 0000000..c47bf32 --- /dev/null +++ b/NetFabric.Hyperlinq.Analyzer.UnitTests/GetEnumeratorReturnTypeAnalyzerTests.cs @@ -0,0 +1,182 @@ +using System; +using Xunit; +using TestHelper; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.CodeFixes; + +namespace NetFabric.Hyperlinq.Analyzer.UnitTests +{ + public class GetEnumeratorReturnTypeAnalyzerTests : CodeFixVerifier + { + protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() => + new GetEnumeratorReturnTypeAnalyzer(); + + [Fact] + public void Verify_NoDiagnostics() + { + var test = @" +using System; +using System.Collections; +using System.Collections.Generic; + +interface ICustomEnumerable +{ + IEnumerator GetEnumerator(); +} + +interface IValueEnumerable : IEnumerable + where TEnumerator : struct, IEnumerator +{ + new Enumerator GetEnumerator(); +} + +readonly struct Enumerable +{ + public Enumerator GetEnumerator() => new Enumerator(); + + public struct Enumerator + { + public int Current => 0; + + public bool MoveNext() => false; + } +} + +readonly struct Enumerable2 : IEnumerable +{ + public Enumerator GetEnumerator() => new Enumerator(); + IEnumerator IEnumerable.GetEnumerator() => new Enumerator(); + IEnumerator IEnumerable.GetEnumerator() => new Enumerator(); + + public struct Enumerator : IEnumerator + { + public int Current => 0; + + public bool MoveNext() => false; + } +} + +readonly struct AsyncEnumerable +{ + public Enumerator GetAsyncEnumerator() => new Enumerator(); + + public struct Enumerator + { + public int Current => 0; + + public ValueTask MoveNextAsync() => new ValueTask(false); + } +} + +readonly struct AsyncEnumerable2 +{ + public Enumerator GetAsyncEnumerator(CancellationToken cancellationToken = default) => new Enumerator(); + + public struct Enumerator + { + public int Current => 0; + + public ValueTask MoveNextAsync() => new ValueTask(false); + } +} + +readonly struct AsyncEnumerable3 : IAsyncEnumerable +{ + public Enumerator GetAsyncEnumerator(CancellationToken cancellationToken = default) => new Enumerator(); + IAsyncEnumerator IAsyncEnumerable.GetAsyncEnumerator(CancellationToken cancellationToken = default) => new Enumerator(); + + public struct Enumerator : IAsyncEnumerator + { + public int Current => 0; + + public ValueTask MoveNextAsync() => new ValueTask(false); + } +} +"; + + VerifyCSharpDiagnostic(test); + } + + [Fact] + public void Verify_Enumerable() + { + var test = @" +using System; +using System.Collections; +using System.Collections.Generic; + +readonly struct Enumerable : IEnumerable +{ + public IEnumerator GetEnumerator() => new Enumerator(); + IEnumerator IEnumerable.GetEnumerator() => new Enumerator(); + + public struct Enumerator : IEnumerator + { + public int Current => 0; + + object IEnumerator.Current => throw new NotImplementedException(); + + public bool MoveNext() => false; + + public void Reset() => throw new NotImplementedException(); + + public void Dispose() { } + } +} +"; + + var expected = new DiagnosticResult + { + Id = "HLQ006", + Message = "'GetEnumerator' returns an interface. Consider returning a value-type enumerator.", + Severity = DiagnosticSeverity.Warning, + Locations = new[] { + new DiagnosticResultLocation("Test0.cs", 8, 12) + }, + }; + + VerifyCSharpDiagnostic(test, expected); + } + + + [Fact] + public void Verify_AsyncEnumerable() + { + var test = @" +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; + +readonly struct AsyncEnumerable : IAsyncEnumerable +{ + public IAsyncEnumerator GetAsyncEnumerator(CancellationToken cancellationToken = default) => new Enumerator(); + + public struct Enumerator : IAsyncEnumerator + { + public int Current => 0; + + public ValueTask MoveNextAsync() => new ValueTask(false); + + public ValueTask DisposeAsync() => new ValueTask(); + } +} +"; + + var expected = new DiagnosticResult + { + Id = "HLQ006", + Message = "'GetAsyncEnumerator' returns an interface. Consider returning a value-type enumerator.", + Severity = DiagnosticSeverity.Warning, + Locations = new[] { + new DiagnosticResultLocation("Test0.cs", 9, 12) + }, + }; + + VerifyCSharpDiagnostic(test, expected); + } + } +} diff --git a/NetFabric.Hyperlinq.Analyzer/DiagnosticIds.cs b/NetFabric.Hyperlinq.Analyzer/DiagnosticIds.cs index aeed28e..eb91189 100644 --- a/NetFabric.Hyperlinq.Analyzer/DiagnosticIds.cs +++ b/NetFabric.Hyperlinq.Analyzer/DiagnosticIds.cs @@ -7,5 +7,6 @@ static class DiagnosticIds public const string HighestLevelInterfaceId = "HLQ003"; public const string RefEnumerationVariableId = "HLQ004"; public const string AvoidSingleId = "HLQ005"; + public const string GetEnumeratorReturnTypeId = "HLQ006"; } } \ No newline at end of file diff --git a/NetFabric.Hyperlinq.Analyzer/GetEnumeratorReturnTypeAnalyzer.cs b/NetFabric.Hyperlinq.Analyzer/GetEnumeratorReturnTypeAnalyzer.cs new file mode 100644 index 0000000..ee0d654 --- /dev/null +++ b/NetFabric.Hyperlinq.Analyzer/GetEnumeratorReturnTypeAnalyzer.cs @@ -0,0 +1,81 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using NetFabric.CodeAnalysis; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; + +namespace NetFabric.Hyperlinq.Analyzer +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class GetEnumeratorReturnTypeAnalyzer : DiagnosticAnalyzer + { + const string DiagnosticId = DiagnosticIds.GetEnumeratorReturnTypeId; + + static readonly LocalizableString Title = + new LocalizableResourceString(nameof(Resources.GetEnumeratorReturnType_Title), Resources.ResourceManager, typeof(Resources)); + static readonly LocalizableString MessageFormat = + new LocalizableResourceString(nameof(Resources.GetEnumeratorReturnType_MessageFormat), Resources.ResourceManager, typeof(Resources)); + static readonly LocalizableString Description = + new LocalizableResourceString(nameof(Resources.GetEnumeratorReturnType_Description), Resources.ResourceManager, typeof(Resources)); + const string Category = "Performance"; + + static readonly DiagnosticDescriptor rule = + new DiagnosticDescriptor(DiagnosticId, Title, MessageFormat, Category, DiagnosticSeverity.Warning, + isEnabledByDefault: true, description: Description, + helpLinkUri: "https://github.com/NetFabric/NetFabric.Hyperlinq.Analyzer/tree/master/docs/reference/HLQ006_GetEnumeratorReturnType.md"); + + public override ImmutableArray SupportedDiagnostics => + ImmutableArray.Create(rule); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); + context.RegisterSyntaxNodeAction(AnalyzeMethodDeclaration, SyntaxKind.MethodDeclaration); + } + + static void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context) + { + if (!(context.Node is MethodDeclarationSyntax methodDeclarationSyntax)) + return; + + var semanticModel = context.SemanticModel; + + // check if it returns an interface + var returnType = methodDeclarationSyntax.ReturnType; + if (semanticModel.GetTypeInfo(returnType).Type.TypeKind != TypeKind.Interface) + return; + + // check if it's public + if (!methodDeclarationSyntax.Modifiers.Any(modifier => modifier.IsKind(SyntaxKind.PublicKeyword))) + return; + + // check if it's "GetEnumerator" or "GetAsyncEnumerator" + var identifier = methodDeclarationSyntax.Identifier.ValueText; + if (identifier == "GetEnumerator" && methodDeclarationSyntax.ParameterList.Parameters.Count == 0) + { + // check if it returns an enumerator + var type = semanticModel.GetTypeInfo(returnType).Type; + if (!type.IsEnumerator(semanticModel.Compilation, out var enumerableSymbols)) + return; + } + else if (identifier == "GetAsyncEnumerator" && methodDeclarationSyntax.ParameterList.Parameters.Count < 2) + { + // check if it returns an async enumerator + var type = semanticModel.GetTypeInfo(returnType).Type; + if (!type.IsAsyncEnumerator(semanticModel.Compilation, out var enumerableSymbols)) + return; + } + else + { + return; + } + + var diagnostic = Diagnostic.Create(rule, methodDeclarationSyntax.ReturnType.GetLocation(), identifier); + context.ReportDiagnostic(diagnostic); + } + } +} \ No newline at end of file diff --git a/NetFabric.Hyperlinq.Analyzer/Resources.Designer.cs b/NetFabric.Hyperlinq.Analyzer/Resources.Designer.cs index 2cb8ba6..327ca93 100644 --- a/NetFabric.Hyperlinq.Analyzer/Resources.Designer.cs +++ b/NetFabric.Hyperlinq.Analyzer/Resources.Designer.cs @@ -10,7 +10,6 @@ namespace NetFabric.Hyperlinq.Analyzer { using System; - using System.Reflection; /// @@ -40,7 +39,7 @@ internal Resources() { internal static global::System.Resources.ResourceManager ResourceManager { get { if (object.ReferenceEquals(resourceMan, null)) { - global::System.Resources.ResourceManager temp = new global::System.Resources.ResourceManager("NetFabric.Hyperlinq.Analyzer.Resources", typeof(Resources).GetTypeInfo().Assembly); + global::System.Resources.ResourceManager temp = new global::System.Resources.ResourceManager("NetFabric.Hyperlinq.Analyzer.Resources", typeof(Resources).Assembly); resourceMan = temp; } return resourceMan; @@ -62,7 +61,7 @@ internal Resources() { } /// - /// Looks up a localized string similar to This collection has a value type enumerator. Assigning it to an interface cause it to be boxed and method calls to be virtual, affecting peformance.. + /// Looks up a localized string similar to This collection has a value type enumerator. Assigning it to an interface cause it to be boxed and method calls to be virtual, affecting performance.. /// internal static string AssignmentBoxing_Description { get { @@ -115,6 +114,33 @@ internal static string AvoidSingle_Title { } } + /// + /// Looks up a localized string similar to Returning a value-type enumerator allows 'foreach' loops to perform better. Interfaces make method calls to be virtual.. + /// + internal static string GetEnumeratorReturnType_Description { + get { + return ResourceManager.GetString("GetEnumeratorReturnType_Description", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to '{0}' returns an interface. Consider returning a value-type enumerator.. + /// + internal static string GetEnumeratorReturnType_MessageFormat { + get { + return ResourceManager.GetString("GetEnumeratorReturnType_MessageFormat", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to GetEnumerator() or GetAsyncEnumerator() return an interface.. + /// + internal static string GetEnumeratorReturnType_Title { + get { + return ResourceManager.GetString("GetEnumeratorReturnType_Title", resourceCulture); + } + } + /// /// Looks up a localized string similar to Public methods should return highest admissible level interface.. /// diff --git a/NetFabric.Hyperlinq.Analyzer/Resources.resx b/NetFabric.Hyperlinq.Analyzer/Resources.resx index 28b5d05..d8a0a4c 100644 --- a/NetFabric.Hyperlinq.Analyzer/Resources.resx +++ b/NetFabric.Hyperlinq.Analyzer/Resources.resx @@ -118,7 +118,7 @@ System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 - This collection has a value type enumerator. Assigning it to an interface cause it to be boxed and method calls to be virtual, affecting peformance. + This collection has a value type enumerator. Assigning it to an interface cause it to be boxed and method calls to be virtual, affecting performance. '{0}' has a value type enumerator. Assigning it to '{1}' causes boxing of the enumerator. @@ -135,6 +135,15 @@ Avoid Single() and SingleOrDefault() + + Returning a value-type enumerator allows 'foreach' loops to perform better. Interfaces make method calls to be virtual. + + + '{0}' returns an interface. Consider returning a value-type enumerator. + + + GetEnumerator() or GetAsyncEnumerator() return an interface. + Public methods should return highest admissible level interface. diff --git a/docs/reference/HLQ006_GetEnumeratorReturnType.md b/docs/reference/HLQ006_GetEnumeratorReturnType.md new file mode 100644 index 0000000..adac7ae --- /dev/null +++ b/docs/reference/HLQ006_GetEnumeratorReturnType.md @@ -0,0 +1,77 @@ +# HLQ006: `GetEnumerator()` or `GetAsyncEnumerator()` should not return an interface type + +A `GetEnumerator()` or `GetAsyncEnumerator()` methods returns an interface type. + +## Rule description + +Collections can be implemented so that the enumerator type returned by `GetEnumerator()` or `GetAsyncEnumerator()` is a value -type. The advantage is that the enumerator is allocated on the stack and method calls are not virtual. + +Returning an interface type makes it always a reference type. + +## How to fix violations + +Change the return type of the method to return the enumerator type. This may require the addition of interface methods to be explicitly implemented. + +## When to suppress warnings + +When it's not feasible to create a value-type enumerator. + +## Example of a violation + +The following example shows implementations of `IEnumerable` and `IAsyncEnumerable` that are detected by this rule: + +```csharp +readonly struct Enumerable : IEnumerable +{ + public IEnumerator GetEnumerator() => new Enumerator(); + IEnumerator IEnumerable.GetEnumerator() => new Enumerator(); + + struct Enumerator : IEnumerator + { + ... + } +} + +readonly struct AsyncEnumerable : IAsyncEnumerable +{ + public IAsyncEnumerator GetAsyncEnumerator(CancellationToken cancellationToken = default) + => new Enumerator(); + + struct Enumerator : IAsyncEnumerator + { + ... + } +} +``` + +## Example of how to fix + +Change the enumerator type declaration to be `public` and change the method return type. Add the necessary interface explicit implementations: + +```csharp +readonly struct Enumerable : IEnumerable +{ + public Enumerator GetEnumerator() => new Enumerator(); + IEnumerator IEnumerable.GetEnumerator() => new Enumerator(); + IEnumerator IEnumerable.GetEnumerator() => new Enumerator(); + + public struct Enumerator : IEnumerator + { + ... + } +} + +readonly struct AsyncEnumerable : IAsyncEnumerable +{ + public Enumerator GetAsyncEnumerator(CancellationToken cancellationToken = default) + => new Enumerator(); + IAsyncEnumerator IAsyncEnumerable.GetAsyncEnumerator(CancellationToken cancellationToken) + => new Enumerator(); + + public struct Enumerator : IAsyncEnumerator + { + ... + } +} + +```