From 2d5a62f1d0c335c5436366e5a38464a2d3d57eec Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Fri, 3 Nov 2023 14:30:35 +0100 Subject: [PATCH 01/18] Allow inheritdoc for constructors with base types --- .../DocumentationRules/SA1648UnitTests.cs | 19 ++++++++++++++++++- ...InheritDocMustBeUsedWithInheritingClass.cs | 5 +++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs index a4566480b..5d87450b8 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs @@ -1,7 +1,7 @@ // Copyright (c) Tunnel Vision Laboratories, LLC. All Rights Reserved. // Licensed under the MIT License. See LICENSE in the project root for license information. -#nullable disable +#nullable enable namespace StyleCop.Analyzers.Test.DocumentationRules { @@ -18,6 +18,23 @@ namespace StyleCop.Analyzers.Test.DocumentationRules /// public class SA1648UnitTests { + [Fact] + public async Task TestConstructorInheritsFromParentAsync() + { + var testCode = @"class Base +{ + public Base() { } +} + +class Test : Base +{ + /// + public Test() { } +}"; + + await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); + } + [Fact] public async Task TestClassOverridesClassAsync() { diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs index 339508867..a6f46069f 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs @@ -159,6 +159,11 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) } } + if (memberSyntax is ConstructorDeclarationSyntax && declaredSymbol is IMethodSymbol methodSymbol && methodSymbol.ContainingType?.BaseType != null) + { + return; + } + if (documentation.Content.GetFirstXmlElement(XmlCommentHelper.IncludeXmlTag) is XmlEmptyElementSyntax includeElement) { if (declaredSymbol == null) From 7cd2b05736997af9269892c4a3f221653e7f1960 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Fri, 3 Nov 2023 19:58:58 +0100 Subject: [PATCH 02/18] Fix --- .../SA1648InheritDocMustBeUsedWithInheritingClass.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs index a6f46069f..69c10475d 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs @@ -159,7 +159,9 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) } } - if (memberSyntax is ConstructorDeclarationSyntax && declaredSymbol is IMethodSymbol methodSymbol && methodSymbol.ContainingType?.BaseType != null) + // "class Test {}" has assigne System.Object as its base type. + if (memberSyntax is ConstructorDeclarationSyntax && declaredSymbol is IMethodSymbol methodSymbol && methodSymbol.ContainingType != null + && methodSymbol.ContainingType.BaseType?.SpecialType != SpecialType.System_Object) { return; } From ef223ab8ac25e91ec50a99e129c186fdff02a0bc Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Tue, 7 Nov 2023 15:04:34 +0100 Subject: [PATCH 03/18] Another approach --- ...InheritDocMustBeUsedWithInheritingClass.cs | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs index 69c10475d..daf71906c 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs @@ -159,11 +159,28 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) } } - // "class Test {}" has assigne System.Object as its base type. - if (memberSyntax is ConstructorDeclarationSyntax && declaredSymbol is IMethodSymbol methodSymbol && methodSymbol.ContainingType != null - && methodSymbol.ContainingType.BaseType?.SpecialType != SpecialType.System_Object) + if (memberSyntax is ConstructorDeclarationSyntax constructorDeclarationSyntax) { - return; + // ConstructorInitializerSyntax initializer = constructorDeclarationSyntax.Initializer; + ISymbol symbol = context.SemanticModel.GetDeclaredSymbol(constructorDeclarationSyntax, context.CancellationToken); + + if (symbol is IMethodSymbol constructorMethodSymbol && constructorMethodSymbol.ContainingType is INamedTypeSymbol enclosingNamedTypeSymbol) + { + INamedTypeSymbol baseType = enclosingNamedTypeSymbol.BaseType; + + if (baseType.SpecialType != SpecialType.System_Object) + { + foreach (IMethodSymbol baseConstructorMethod in baseType.Constructors) + { + // TODO: SymbolEqualityComparer? + if (constructorMethodSymbol.Parameters.SequenceEqual(baseConstructorMethod.Parameters)) + { + // Matching constructor was found. + return; + } + } + } + } } if (documentation.Content.GetFirstXmlElement(XmlCommentHelper.IncludeXmlTag) is XmlEmptyElementSyntax includeElement) From d58dedeacea1d94e5842bf2f62bc8762a536b8e0 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Tue, 7 Nov 2023 19:51:17 +0100 Subject: [PATCH 04/18] Improve implementation --- .../DocumentationRules/SA1648UnitTests.cs | 17 +++++++++ ...InheritDocMustBeUsedWithInheritingClass.cs | 36 ++++++++++++++++--- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs index 5d87450b8..fbbeb7003 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs @@ -23,6 +23,7 @@ public async Task TestConstructorInheritsFromParentAsync() { var testCode = @"class Base { + /// Base constructor. public Base() { } } @@ -33,6 +34,22 @@ public Test() { } }"; await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); + + testCode = @"class Base +{ + /// Base constructor. + public Base(string s, int a) { } +} + +class Test : Base +{ + /// + public Test(string s, int b) + : base(s, b) { } +} +"; + + await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } [Fact] diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs index daf71906c..dd682107d 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs @@ -161,7 +161,6 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) if (memberSyntax is ConstructorDeclarationSyntax constructorDeclarationSyntax) { - // ConstructorInitializerSyntax initializer = constructorDeclarationSyntax.Initializer; ISymbol symbol = context.SemanticModel.GetDeclaredSymbol(constructorDeclarationSyntax, context.CancellationToken); if (symbol is IMethodSymbol constructorMethodSymbol && constructorMethodSymbol.ContainingType is INamedTypeSymbol enclosingNamedTypeSymbol) @@ -170,14 +169,41 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) if (baseType.SpecialType != SpecialType.System_Object) { + bool foundMatchingConstructorsToInheritFrom = false; + foreach (IMethodSymbol baseConstructorMethod in baseType.Constructors) { - // TODO: SymbolEqualityComparer? - if (constructorMethodSymbol.Parameters.SequenceEqual(baseConstructorMethod.Parameters)) + // Constructors must have the same number of parameters. + if (constructorMethodSymbol.Parameters.Count() != baseConstructorMethod.Parameters.Count()) + { + continue; + } + + // Our constructor and the base constructor must have the same signature. But variable names can be different. + bool success = true; + + for (int i = 0; i < constructorMethodSymbol.Parameters.Length; i++) { - // Matching constructor was found. - return; + IParameterSymbol constructorParameter = constructorMethodSymbol.Parameters[i]; + IParameterSymbol baseParameter = baseConstructorMethod.Parameters[i]; + + if (!constructorParameter.Type.Equals(baseParameter.Type)) + { + success = false; + break; + } } + + if (success) + { + foundMatchingConstructorsToInheritFrom = true; + break; + } + } + + if (foundMatchingConstructorsToInheritFrom) + { + return; } } } From 836540c8ec36820c42acaaa18e7727ccac5f2753 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Sat, 11 Nov 2023 11:14:09 +0100 Subject: [PATCH 05/18] Remove #nullable enable from the test --- .../DocumentationRules/SA1648UnitTests.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs index fbbeb7003..1743c57f8 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs @@ -1,8 +1,6 @@ // Copyright (c) Tunnel Vision Laboratories, LLC. All Rights Reserved. // Licensed under the MIT License. See LICENSE in the project root for license information. -#nullable enable - namespace StyleCop.Analyzers.Test.DocumentationRules { using System.Threading; From 62237f37d15d405bcb725e8f3928e0f529430e32 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Sat, 11 Nov 2023 11:14:55 +0100 Subject: [PATCH 06/18] SA1648InheritDocMustBeUsedWithInheritingClass: Enable NRT --- .../SA1648InheritDocMustBeUsedWithInheritingClass.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs index dd682107d..24f38303a 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs @@ -1,8 +1,6 @@ // Copyright (c) Tunnel Vision Laboratories, LLC. All Rights Reserved. // Licensed under the MIT License. See LICENSE in the project root for license information. -#nullable disable - namespace StyleCop.Analyzers.DocumentationRules { using System; @@ -70,7 +68,7 @@ public override void Initialize(AnalysisContext context) private static void HandleBaseTypeLikeDeclaration(SyntaxNodeAnalysisContext context) { - BaseTypeDeclarationSyntax baseType = context.Node as BaseTypeDeclarationSyntax; + BaseTypeDeclarationSyntax? baseType = context.Node as BaseTypeDeclarationSyntax; // baseType can be null here if we are looking at a delegate declaration if (baseType != null && baseType.BaseList != null && baseType.BaseList.Types.Any()) @@ -258,13 +256,13 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) private static bool HasXmlCrefAttribute(XmlNodeSyntax inheritDocElement) { - XmlElementSyntax xmlElementSyntax = inheritDocElement as XmlElementSyntax; + XmlElementSyntax? xmlElementSyntax = inheritDocElement as XmlElementSyntax; if (xmlElementSyntax?.StartTag?.Attributes.Any(SyntaxKind.XmlCrefAttribute) ?? false) { return true; } - XmlEmptyElementSyntax xmlEmptyElementSyntax = inheritDocElement as XmlEmptyElementSyntax; + XmlEmptyElementSyntax? xmlEmptyElementSyntax = inheritDocElement as XmlEmptyElementSyntax; if (xmlEmptyElementSyntax?.Attributes.Any(SyntaxKind.XmlCrefAttribute) ?? false) { return true; From 49d59ced5e7e29ba8a613b3e83d9bc173619ebb0 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Sat, 11 Nov 2023 11:15:23 +0100 Subject: [PATCH 07/18] Move up and use `declaredSymbol` --- ...InheritDocMustBeUsedWithInheritingClass.cs | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs index 24f38303a..5c6943a60 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs @@ -147,21 +147,10 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) Location location; ISymbol declaredSymbol = context.SemanticModel.GetDeclaredSymbol(memberSyntax, context.CancellationToken); - if (declaredSymbol == null && memberSyntax.IsKind(SyntaxKind.EventFieldDeclaration)) - { - var eventFieldDeclarationSyntax = (EventFieldDeclarationSyntax)memberSyntax; - VariableDeclaratorSyntax firstVariable = eventFieldDeclarationSyntax.Declaration?.Variables.FirstOrDefault(); - if (firstVariable != null) - { - declaredSymbol = context.SemanticModel.GetDeclaredSymbol(firstVariable, context.CancellationToken); - } - } if (memberSyntax is ConstructorDeclarationSyntax constructorDeclarationSyntax) { - ISymbol symbol = context.SemanticModel.GetDeclaredSymbol(constructorDeclarationSyntax, context.CancellationToken); - - if (symbol is IMethodSymbol constructorMethodSymbol && constructorMethodSymbol.ContainingType is INamedTypeSymbol enclosingNamedTypeSymbol) + if (declaredSymbol is IMethodSymbol constructorMethodSymbol && constructorMethodSymbol.ContainingType is INamedTypeSymbol enclosingNamedTypeSymbol) { INamedTypeSymbol baseType = enclosingNamedTypeSymbol.BaseType; @@ -207,6 +196,16 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) } } + if (declaredSymbol == null && memberSyntax.IsKind(SyntaxKind.EventFieldDeclaration)) + { + var eventFieldDeclarationSyntax = (EventFieldDeclarationSyntax)memberSyntax; + VariableDeclaratorSyntax? firstVariable = eventFieldDeclarationSyntax.Declaration?.Variables.FirstOrDefault(); + if (firstVariable != null) + { + declaredSymbol = context.SemanticModel.GetDeclaredSymbol(firstVariable, context.CancellationToken); + } + } + if (documentation.Content.GetFirstXmlElement(XmlCommentHelper.IncludeXmlTag) is XmlEmptyElementSyntax includeElement) { if (declaredSymbol == null) From b7d10a83305b5d949642e6d59364b0ccf4216e3d Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Sat, 11 Nov 2023 13:18:52 +0100 Subject: [PATCH 08/18] Split tests --- .../DocumentationRules/SA1648UnitTests.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs index 1743c57f8..326948347 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs @@ -17,7 +17,7 @@ namespace StyleCop.Analyzers.Test.DocumentationRules public class SA1648UnitTests { [Fact] - public async Task TestConstructorInheritsFromParentAsync() + public async Task TestConstructorWithNoParametersInheritsFromParentAsync() { var testCode = @"class Base { @@ -32,8 +32,12 @@ public Test() { } }"; await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); + } - testCode = @"class Base + [Fact] + public async Task TestConstructorWithParametersInheritsFromParentAsync() + { + var testCode = @"class Base { /// Base constructor. public Base(string s, int a) { } From fd402ad281705bbe75c026624b763d61afaab701 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Sat, 11 Nov 2023 13:42:27 +0100 Subject: [PATCH 09/18] Address review comments --- .../DocumentationRules/SA1648UnitTests.cs | 25 +++++- ...InheritDocMustBeUsedWithInheritingClass.cs | 87 +++++++++++-------- 2 files changed, 75 insertions(+), 37 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs index 326948347..0f6bc3e0c 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs @@ -54,6 +54,30 @@ public Test(string s, int b) await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } + [Fact] + public async Task TestConstructorInheritsImplicitlyFromSystemObjectAsync() + { + var testCode = @"class Test +{ + /// + public Test() { } +}"; + + await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); + } + + [Fact] + public async Task TestConstructorInheritsExplicitlyFromSystemObjectAsync() + { + var testCode = @"class Test : System.Object +{ + /// + public Test() { } +}"; + + await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); + } + [Fact] public async Task TestClassOverridesClassAsync() { @@ -126,7 +150,6 @@ public async Task TestTypeWithEmptyBaseListAndCrefAttributeAsync(string declarat } [Theory] - [InlineData("Test() { }")] [InlineData("void Foo() { }")] [InlineData("string foo;")] [InlineData("string Foo { get; set; }")] diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs index 5c6943a60..3916a08f6 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs @@ -148,51 +148,24 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) ISymbol declaredSymbol = context.SemanticModel.GetDeclaredSymbol(memberSyntax, context.CancellationToken); - if (memberSyntax is ConstructorDeclarationSyntax constructorDeclarationSyntax) + if (memberSyntax is ConstructorDeclarationSyntax constructorDeclarationSyntax && declaredSymbol is IMethodSymbol constructorMethodSymbol) { - if (declaredSymbol is IMethodSymbol constructorMethodSymbol && constructorMethodSymbol.ContainingType is INamedTypeSymbol enclosingNamedTypeSymbol) + if (constructorMethodSymbol.ContainingType is INamedTypeSymbol enclosingNamedTypeSymbol) { INamedTypeSymbol baseType = enclosingNamedTypeSymbol.BaseType; - if (baseType.SpecialType != SpecialType.System_Object) + if (baseType.SpecialType == SpecialType.System_Object) { - bool foundMatchingConstructorsToInheritFrom = false; - - foreach (IMethodSymbol baseConstructorMethod in baseType.Constructors) - { - // Constructors must have the same number of parameters. - if (constructorMethodSymbol.Parameters.Count() != baseConstructorMethod.Parameters.Count()) - { - continue; - } - - // Our constructor and the base constructor must have the same signature. But variable names can be different. - bool success = true; - - for (int i = 0; i < constructorMethodSymbol.Parameters.Length; i++) - { - IParameterSymbol constructorParameter = constructorMethodSymbol.Parameters[i]; - IParameterSymbol baseParameter = baseConstructorMethod.Parameters[i]; - - if (!constructorParameter.Type.Equals(baseParameter.Type)) - { - success = false; - break; - } - } - - if (success) - { - foundMatchingConstructorsToInheritFrom = true; - break; - } - } - - if (foundMatchingConstructorsToInheritFrom) + // Exception: If the base type is System.Object, then we can use if our constructor has zero parameters. + if (constructorMethodSymbol.Parameters.Length == 0) { return; } } + else if (HasMatchingSignature(baseType.Constructors, constructorMethodSymbol)) + { + return; + } } } @@ -253,6 +226,48 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) } } + /// + /// Method compares a constructor method signature against its + /// base type constructors to find if there is a method signature match. + /// + /// if any base type constructor's signature matches the signature of , otherwise. + private static bool HasMatchingSignature(ImmutableArray baseConstructorSymbols, IMethodSymbol constructorMethodSymbol) + { + bool found = false; + + foreach (IMethodSymbol baseConstructorMethod in baseConstructorSymbols) + { + // Constructors must have the same number of parameters. + if (constructorMethodSymbol.Parameters.Count() != baseConstructorMethod.Parameters.Count()) + { + continue; + } + + // Our constructor and the base constructor must have the same signature. But variable names can be different. + bool success = true; + + for (int i = 0; i < constructorMethodSymbol.Parameters.Length; i++) + { + IParameterSymbol constructorParameter = constructorMethodSymbol.Parameters[i]; + IParameterSymbol baseParameter = baseConstructorMethod.Parameters[i]; + + if (!constructorParameter.Type.Equals(baseParameter.Type)) + { + success = false; + break; + } + } + + if (success) + { + found = true; + break; + } + } + + return found; + } + private static bool HasXmlCrefAttribute(XmlNodeSyntax inheritDocElement) { XmlElementSyntax? xmlElementSyntax = inheritDocElement as XmlElementSyntax; From 5ecef986274211f01265fd9708bff9226b9708b5 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Wed, 29 Nov 2023 08:23:57 +0100 Subject: [PATCH 10/18] Add a test for ArgumentException (type from a different assembly) --- .../DocumentationRules/SA1648UnitTests.cs | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs index 0f6bc3e0c..3eeaa7d4b 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs @@ -27,7 +27,7 @@ public Base() { } class Test : Base { - /// + /// public Test() { } }"; @@ -45,7 +45,7 @@ public Base(string s, int a) { } class Test : Base { - /// + /// public Test(string s, int b) : base(s, b) { } } @@ -59,7 +59,7 @@ public async Task TestConstructorInheritsImplicitlyFromSystemObjectAsync() { var testCode = @"class Test { - /// + /// public Test() { } }"; @@ -71,13 +71,25 @@ public async Task TestConstructorInheritsExplicitlyFromSystemObjectAsync() { var testCode = @"class Test : System.Object { - /// + /// public Test() { } }"; await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } + [Fact] + public async Task TestConstructorInheritsExplicitlyFromTypeInDifferentAssemblyAsync() + { + var testCode = @"class MyArgumentException : System.ArgumentException +{ + /// + public MyArgumentException() { } +}"; + + await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); + } + [Fact] public async Task TestClassOverridesClassAsync() { From 31643cf68f5e12410eb745235c9e859a2542782c Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Wed, 29 Nov 2023 08:56:08 +0100 Subject: [PATCH 11/18] Test records as well --- .../DocumentationRules/SA1648UnitTests.cs | 44 +++++++++++-------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs index 3eeaa7d4b..2d95da795 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs @@ -7,6 +7,7 @@ namespace StyleCop.Analyzers.Test.DocumentationRules using System.Threading.Tasks; using Microsoft.CodeAnalysis.Testing; using StyleCop.Analyzers.DocumentationRules; + using StyleCop.Analyzers.Test.Helpers; using StyleCop.Analyzers.Test.Verifiers; using Xunit; using static StyleCop.Analyzers.Test.Verifiers.CustomDiagnosticVerifier; @@ -16,34 +17,36 @@ namespace StyleCop.Analyzers.Test.DocumentationRules /// public class SA1648UnitTests { - [Fact] - public async Task TestConstructorWithNoParametersInheritsFromParentAsync() + [Theory] + [MemberData(nameof(CommonMemberData.ReferenceTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))] + public async Task TestConstructorWithNoParametersInheritsFromParentAsync(string keyword) { - var testCode = @"class Base + var testCode = @"$KEYWORD$ Base { /// Base constructor. public Base() { } } -class Test : Base +$KEYWORD$ Test : Base { /// public Test() { } }"; - await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode.Replace("$KEYWORD$", keyword), DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } - [Fact] - public async Task TestConstructorWithParametersInheritsFromParentAsync() + [Theory] + [MemberData(nameof(CommonMemberData.ReferenceTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))] + public async Task TestConstructorWithParametersInheritsFromParentAsync(string keyword) { - var testCode = @"class Base + var testCode = @"$KEYWORD$ Base { /// Base constructor. public Base(string s, int a) { } } -class Test : Base +$KEYWORD$ Test : Base { /// public Test(string s, int b) @@ -51,31 +54,33 @@ public Test(string s, int b) } "; - await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode.Replace("$KEYWORD$", keyword), DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } - [Fact] - public async Task TestConstructorInheritsImplicitlyFromSystemObjectAsync() + [Theory] + [MemberData(nameof(CommonMemberData.ReferenceTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))] + public async Task TestConstructorInheritsImplicitlyFromSystemObjectAsync(string keyword) { - var testCode = @"class Test + var testCode = @"$KEYWORD$ Test { /// public Test() { } }"; - await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode.Replace("$KEYWORD$", keyword), DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } - [Fact] - public async Task TestConstructorInheritsExplicitlyFromSystemObjectAsync() + [Theory] + [MemberData(nameof(CommonMemberData.ReferenceTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))] + public async Task TestConstructorInheritsExplicitlyFromSystemObjectAsync(string keyword) { - var testCode = @"class Test : System.Object + var testCode = @"$KEYWORD$ Test : System.Object { /// public Test() { } }"; - await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode.Replace("$KEYWORD$", keyword), DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } [Fact] @@ -85,6 +90,9 @@ public async Task TestConstructorInheritsExplicitlyFromTypeInDifferentAssemblyAs { /// public MyArgumentException() { } + + /// + public MyArgumentException(string message) : base(message) { } }"; await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); From 50a5fbfe469d1adee347dc8f54579e651aeead51 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Wed, 29 Nov 2023 08:59:43 +0100 Subject: [PATCH 12/18] Struct case --- .../DocumentationRules/SA1648UnitTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs index 2d95da795..aed107934 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs @@ -58,7 +58,7 @@ public Test(string s, int b) } [Theory] - [MemberData(nameof(CommonMemberData.ReferenceTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))] + [MemberData(nameof(CommonMemberData.DataTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))] public async Task TestConstructorInheritsImplicitlyFromSystemObjectAsync(string keyword) { var testCode = @"$KEYWORD$ Test From 182e0a77dc949d3386f8dfd40aa2cfecb7386ce5 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Wed, 29 Nov 2023 10:09:47 +0100 Subject: [PATCH 13/18] Revert "Struct case" This reverts commit 50a5fbfe469d1adee347dc8f54579e651aeead51. --- .../DocumentationRules/SA1648UnitTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs index 55def9eea..0c7a520bc 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs @@ -58,7 +58,7 @@ public Test(string s, int b) } [Theory] - [MemberData(nameof(CommonMemberData.DataTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))] + [MemberData(nameof(CommonMemberData.ReferenceTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))] public async Task TestConstructorInheritsImplicitlyFromSystemObjectAsync(string keyword) { var testCode = @"$KEYWORD$ Test From 0ebb9902a618398dd8dac1ef51ef480d5a9a5921 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Sat, 9 Dec 2023 22:04:20 +0100 Subject: [PATCH 14/18] Simplify --- .../SA1648InheritDocMustBeUsedWithInheritingClass.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs index 3916a08f6..3a2df2365 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs @@ -150,9 +150,9 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) if (memberSyntax is ConstructorDeclarationSyntax constructorDeclarationSyntax && declaredSymbol is IMethodSymbol constructorMethodSymbol) { - if (constructorMethodSymbol.ContainingType is INamedTypeSymbol enclosingNamedTypeSymbol) + if (constructorMethodSymbol.ContainingType != null) { - INamedTypeSymbol baseType = enclosingNamedTypeSymbol.BaseType; + INamedTypeSymbol baseType = constructorMethodSymbol.ContainingType.BaseType; if (baseType.SpecialType == SpecialType.System_Object) { From 7183ec0923d917148e81a9cb4d8ff0dffbc38163 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Sat, 9 Dec 2023 22:14:38 +0100 Subject: [PATCH 15/18] Simplify? --- .../SA1648InheritDocMustBeUsedWithInheritingClass.cs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs index 3a2df2365..0ae796e54 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs @@ -154,15 +154,7 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) { INamedTypeSymbol baseType = constructorMethodSymbol.ContainingType.BaseType; - if (baseType.SpecialType == SpecialType.System_Object) - { - // Exception: If the base type is System.Object, then we can use if our constructor has zero parameters. - if (constructorMethodSymbol.Parameters.Length == 0) - { - return; - } - } - else if (HasMatchingSignature(baseType.Constructors, constructorMethodSymbol)) + if (HasMatchingSignature(baseType.Constructors, constructorMethodSymbol)) { return; } From db794b076a596eb87498f2c80537a77b394bb282 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Sat, 9 Dec 2023 22:26:45 +0100 Subject: [PATCH 16/18] more tests --- .../DocumentationRules/SA1648UnitTests.cs | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs index 0c7a520bc..dd009bc38 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs @@ -6,6 +6,7 @@ namespace StyleCop.Analyzers.Test.DocumentationRules using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Testing; + using Microsoft.VisualBasic.Devices; using StyleCop.Analyzers.DocumentationRules; using StyleCop.Analyzers.Test.Helpers; using StyleCop.Analyzers.Test.Verifiers; @@ -98,6 +99,50 @@ public MyArgumentException(string message) : base(message) { } await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } + [Theory] + [MemberData(nameof(CommonMemberData.ReferenceTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))] + public async Task TestConstructorInheritsButBaseCtorHasTheSameNumberOfParametersButNotMatchingSignaturesAsync(string keyword) + { + var testCode = @"$KEYWORD$ Base +{ + /// Base constructor. + public Base(string s, string a) { } +} + +$KEYWORD$ Test : Base +{ + /// + public Test(string s, int b) + : base(s, b.ToString()) { } +} +"; + + var expected = Diagnostic().WithLocation(9, 9); + await VerifyCSharpDiagnosticAsync(testCode.Replace("$KEYWORD$", keyword), expected, CancellationToken.None).ConfigureAwait(false); + } + + [Theory] + [MemberData(nameof(CommonMemberData.ReferenceTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))] + public async Task TestConstructorInheritsButBaseCtorHasDifferentNumberOfParametersAsync(string keyword) + { + var testCode = @"$KEYWORD$ Base +{ + /// Base constructor. + public Base(string s) { } +} + +$KEYWORD$ Test : Base +{ + /// + public Test(string s, int b) + : base(s) { } +} +"; + + var expected = Diagnostic().WithLocation(9, 9); + await VerifyCSharpDiagnosticAsync(testCode.Replace("$KEYWORD$", keyword), expected, CancellationToken.None).ConfigureAwait(false); + } + [Fact] public async Task TestClassOverridesClassAsync() { From 80804ab021c86914772e84bc067a649d6c024bbf Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Tue, 5 Dec 2023 07:58:39 -0600 Subject: [PATCH 17/18] Increase test timeout to 80 minutes --- azure-pipelines.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index bb3d4f15f..314ca8aed 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -19,6 +19,7 @@ jobs: Release: BuildConfiguration: Release _debugArg: '' + timeoutInMinutes: 90 steps: - powershell: .\init.ps1 -NoRestore displayName: Install .NET Core SDK @@ -45,6 +46,7 @@ jobs: - task: PowerShell@2 displayName: Run Tests + timeoutInMinutes: 80 inputs: workingDirectory: '$(Build.SourcesDirectory)/build' filePath: build/opencover-report.ps1 From 6e752bb4afb6f031e5345ab77798961cf5073f74 Mon Sep 17 00:00:00 2001 From: MartyIX <203266+MartyIX@users.noreply.github.com> Date: Tue, 19 Dec 2023 20:52:36 +0100 Subject: [PATCH 18/18] Feedback --- .../DocumentationRules/SA1648UnitTests.cs | 2 +- .../SA1648InheritDocMustBeUsedWithInheritingClass.cs | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs index dd009bc38..68e36c227 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1648UnitTests.cs @@ -6,7 +6,6 @@ namespace StyleCop.Analyzers.Test.DocumentationRules using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Testing; - using Microsoft.VisualBasic.Devices; using StyleCop.Analyzers.DocumentationRules; using StyleCop.Analyzers.Test.Helpers; using StyleCop.Analyzers.Test.Verifiers; @@ -215,6 +214,7 @@ public async Task TestTypeWithEmptyBaseListAndCrefAttributeAsync(string declarat } [Theory] + [InlineData("Test(int ignored) { }")] [InlineData("void Foo() { }")] [InlineData("string foo;")] [InlineData("string Foo { get; set; }")] diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs index 0ae796e54..8c6747100 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1648InheritDocMustBeUsedWithInheritingClass.cs @@ -225,12 +225,10 @@ private static void HandleMemberDeclaration(SyntaxNodeAnalysisContext context) /// if any base type constructor's signature matches the signature of , otherwise. private static bool HasMatchingSignature(ImmutableArray baseConstructorSymbols, IMethodSymbol constructorMethodSymbol) { - bool found = false; - foreach (IMethodSymbol baseConstructorMethod in baseConstructorSymbols) { // Constructors must have the same number of parameters. - if (constructorMethodSymbol.Parameters.Count() != baseConstructorMethod.Parameters.Count()) + if (constructorMethodSymbol.Parameters.Length != baseConstructorMethod.Parameters.Length) { continue; } @@ -252,12 +250,11 @@ private static bool HasMatchingSignature(ImmutableArray baseConst if (success) { - found = true; - break; + return true; } } - return found; + return false; } private static bool HasXmlCrefAttribute(XmlNodeSyntax inheritDocElement)