From f395c71208b6a34d9eb4bad6183758b107d182fa Mon Sep 17 00:00:00 2001 From: "Collin.Alpert" Date: Wed, 19 Apr 2023 14:14:55 +0200 Subject: [PATCH 1/6] Don't emit SA1414 for interface implementations --- .../SA1414CSharp7UnitTests.cs | 18 ++++++++++++++++++ ...eTypesInSignaturesShouldHaveElementNames.cs | 15 +++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs index 4c9543c85..929d31982 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs @@ -117,5 +117,23 @@ public static explicit operator TestClass({typeExpression} p1) await VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false); } + + [Fact] + public Task ValidateTuplesFromInheritedTypeAsync() + { + const string testCode = @" +using System.Collections.Generic; + +namespace Test { + class StringTupleComparer : IEqualityComparer<(string, string)> + { + public bool Equals((string, string) x, (string, string) y) => throw null; + + public int GetHashCode((string, string) obj) => throw null; + } +} +"; + return VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, default); + } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs b/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs index 27afb2d93..c223b629b 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs @@ -7,6 +7,7 @@ namespace StyleCop.Analyzers.MaintainabilityRules { using System; using System.Collections.Immutable; + using System.Linq; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; @@ -62,6 +63,20 @@ private static void HandleMethodDeclaration(SyntaxNodeAnalysisContext context) } var methodDeclaration = (MethodDeclarationSyntax)context.Node; + var methodSymbol = context.SemanticModel.GetDeclaredSymbol(methodDeclaration); + var containingType = methodSymbol.ContainingType; + if (containingType == null) + { + return; + } + + foreach (var member in containingType.AllInterfaces.SelectMany(i => i.GetMembers(methodSymbol.Name).OfType())) + { + if (methodSymbol.Equals(containingType.FindImplementationForInterfaceMember(member))) + { + return; + } + } CheckType(context, methodDeclaration.ReturnType); CheckParameterList(context, methodDeclaration.ParameterList); From 22e8d6bea7ab2ffc9a58c6e6281981c590772a52 Mon Sep 17 00:00:00 2001 From: "Collin.Alpert" Date: Thu, 20 Apr 2023 09:24:12 +0200 Subject: [PATCH 2/6] Added tests for explicit interface implementations and base classes. --- .../SA1414CSharp7UnitTests.cs | 51 +++++++++++++++++-- ...TypesInSignaturesShouldHaveElementNames.cs | 7 ++- 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs index 929d31982..146e75cf9 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs @@ -5,6 +5,7 @@ namespace StyleCop.Analyzers.Test.CSharp7.MaintainabilityRules { + using System; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Testing; @@ -119,7 +120,7 @@ public static explicit operator TestClass({typeExpression} p1) } [Fact] - public Task ValidateTuplesFromInheritedTypeAsync() + public Task ValidateTuplesFromInterfaceAsync() { const string testCode = @" using System.Collections.Generic; @@ -131,9 +132,53 @@ class StringTupleComparer : IEqualityComparer<(string, string)> public int GetHashCode((string, string) obj) => throw null; } -} -"; +}"; return VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, default); } + + [Fact] + public Task ValidateTuplesFromExplicitInterfaceImplementationAsync() + { + const string testCode = @" +using System.Collections.Generic; + +namespace Test { + class StringTupleComparer : IEqualityComparer<(string, string)> + { + bool IEqualityComparer<(string, string)>.Equals((string, string) x, (string, string) y) => throw null; + + int IEqualityComparer<(string, string)>.GetHashCode((string, string) obj) => throw null; + } +}"; + return VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, default); + } + + [Fact] + public Task ValidateTuplesFromBaseClassAsync() + { + const string testCode = @" +namespace Test { + class A : B + { + public override void Run((string, string) x) + { + } + + public override void Run((int, int) y) + { + } + } + + abstract class B + { + public abstract void Run(([|string|], [|string|]) x); + + public virtual void Run(([|int|], [|int|]) y) + { + } + } +}"; + return VerifyCSharpDiagnosticAsync(testCode, Array.Empty(), default); + } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs b/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs index c223b629b..1b2b6688f 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs @@ -63,9 +63,14 @@ private static void HandleMethodDeclaration(SyntaxNodeAnalysisContext context) } var methodDeclaration = (MethodDeclarationSyntax)context.Node; + if (methodDeclaration.Modifiers.Any(SyntaxKind.OverrideKeyword)) + { + return; + } + var methodSymbol = context.SemanticModel.GetDeclaredSymbol(methodDeclaration); var containingType = methodSymbol.ContainingType; - if (containingType == null) + if (containingType is null || methodSymbol.ExplicitInterfaceImplementations.Length > 0) { return; } From 89e104f057b0f913a4e1995630ccffcb412e1b82 Mon Sep 17 00:00:00 2001 From: Collin Alpert Date: Thu, 20 Apr 2023 21:35:00 +0200 Subject: [PATCH 3/6] Use DiagnosticResult.EmptyDiagnosticResults --- .../MaintainabilityRules/SA1414CSharp7UnitTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs index 146e75cf9..9b52bf83f 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs @@ -178,7 +178,7 @@ public virtual void Run(([|int|], [|int|]) y) } } }"; - return VerifyCSharpDiagnosticAsync(testCode, Array.Empty(), default); + return VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, default); } } } From 339938bcde266192ab6e15e4ea3e04948d044e2a Mon Sep 17 00:00:00 2001 From: "Collin.Alpert" Date: Fri, 21 Apr 2023 10:44:27 +0200 Subject: [PATCH 4/6] Extend test cases to cover return types. --- .../MaintainabilityRules/SA1414CSharp7UnitTests.cs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs index 9b52bf83f..9e2dd5832 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs @@ -160,22 +160,16 @@ public Task ValidateTuplesFromBaseClassAsync() namespace Test { class A : B { - public override void Run((string, string) x) - { - } + public override (string, string) Run((string, string) x) => throw null; - public override void Run((int, int) y) - { - } + public override (int, int) Run((int, int) y) => throw null; } abstract class B { - public abstract void Run(([|string|], [|string|]) x); + public abstract ([|string|], [|string|]) Run(([|string|], [|string|]) x); - public virtual void Run(([|int|], [|int|]) y) - { - } + public virtual ([|int|], [|int|]) Run(([|int|], [|int|]) y) => throw null; } }"; return VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, default); From 2543742eb8dab2a1920abb4cf9bc9d761135b9c7 Mon Sep 17 00:00:00 2001 From: "Collin.Alpert" Date: Wed, 26 Apr 2023 11:51:32 +0200 Subject: [PATCH 5/6] Only query semantic model when absolutely necessary. --- ...leTypesInSignaturesShouldHaveElementNames.cs | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs b/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs index 1b2b6688f..0105c038a 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/MaintainabilityRules/SA1414TupleTypesInSignaturesShouldHaveElementNames.cs @@ -68,21 +68,6 @@ private static void HandleMethodDeclaration(SyntaxNodeAnalysisContext context) return; } - var methodSymbol = context.SemanticModel.GetDeclaredSymbol(methodDeclaration); - var containingType = methodSymbol.ContainingType; - if (containingType is null || methodSymbol.ExplicitInterfaceImplementations.Length > 0) - { - return; - } - - foreach (var member in containingType.AllInterfaces.SelectMany(i => i.GetMembers(methodSymbol.Name).OfType())) - { - if (methodSymbol.Equals(containingType.FindImplementationForInterfaceMember(member))) - { - return; - } - } - CheckType(context, methodDeclaration.ReturnType); CheckParameterList(context, methodDeclaration.ParameterList); } @@ -181,7 +166,7 @@ private static void CheckTupleType(SyntaxNodeAnalysisContext context, TupleTypeS { CheckType(context, tupleElementSyntax.Type); - if (tupleElementSyntax.Identifier.IsKind(SyntaxKind.None)) + if (tupleElementSyntax.Identifier.IsKind(SyntaxKind.None) && !NamedTypeHelpers.IsImplementingAnInterfaceMember(context.SemanticModel.GetDeclaredSymbol(context.Node))) { var location = tupleElementSyntax.SyntaxNode.GetLocation(); context.ReportDiagnostic(Diagnostic.Create(Descriptor, location)); From bda8c5a3008d3e09b000da9b4143ddc1adbf23a6 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 18 May 2023 08:24:26 -0500 Subject: [PATCH 6/6] Apply suggestions from code review --- .../MaintainabilityRules/SA1414CSharp7UnitTests.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs index 9e2dd5832..569492f1f 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp7/MaintainabilityRules/SA1414CSharp7UnitTests.cs @@ -120,7 +120,7 @@ public static explicit operator TestClass({typeExpression} p1) } [Fact] - public Task ValidateTuplesFromInterfaceAsync() + public async Task ValidateTuplesFromInterfaceAsync() { const string testCode = @" using System.Collections.Generic; @@ -133,11 +133,11 @@ class StringTupleComparer : IEqualityComparer<(string, string)> public int GetHashCode((string, string) obj) => throw null; } }"; - return VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, default); + await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } [Fact] - public Task ValidateTuplesFromExplicitInterfaceImplementationAsync() + public async Task ValidateTuplesFromExplicitInterfaceImplementationAsync() { const string testCode = @" using System.Collections.Generic; @@ -150,11 +150,11 @@ class StringTupleComparer : IEqualityComparer<(string, string)> int IEqualityComparer<(string, string)>.GetHashCode((string, string) obj) => throw null; } }"; - return VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, default); + await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } [Fact] - public Task ValidateTuplesFromBaseClassAsync() + public async Task ValidateTuplesFromBaseClassAsync() { const string testCode = @" namespace Test { @@ -172,7 +172,7 @@ abstract class B public virtual ([|int|], [|int|]) Run(([|int|], [|int|]) y) => throw null; } }"; - return VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, default); + await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } } }