Merge pull request #2258 from bjornhellander/Issue2253_CorrectSa1623DiagnosticAndCodeFix

Corrected analyzer for SA1623 and SA1624 to report the correct diagnostic

Author: vweijsters

StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1623UnitTests.cs

@@ -177,47 +177,54 @@ public int Property
         }
 
         /// <summary>
-        /// Verifies that an incomplete summary tag (with only get or set, when get and set are needed) will be fixed
-        /// correctly. Regression test for #2098.
+        /// Verifies that an incorrect summary tag with a known prefix will be fixed correctly.
         /// </summary>
+        /// <param name="accessibility">The accessibility of the property.</param>
+        /// <param name="type">The type to use for the property.</param>
+        /// <param name="accessors">The accessors for the property.</param>
+        /// <param name="summaryPrefix">The summary prefix used in the test code.</param>
+        /// <param name="expectedArgument">The expected argument for the diagnostic message.</param>
         /// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
-        [Fact]
-        public async Task IncompleteSummaryTagShouldBeFixedCorrectlyAsync()
+        [Theory]
+        [InlineData("public", "int", "{ get; set; }", "Gets", "Gets or sets")] // Regression test for #2098
+        [InlineData("public", "int", "{ get; set; }", "Sets", "Gets or sets")] // Regression test for #2098
+        [InlineData("public", "int", "{ get; }", "Sets", "Gets")] // Regression test for #2253
+        [InlineData("public", "int", "{ get; }", "Gets or sets", "Gets")] // Regression test for #2253
+        [InlineData("public", "int", "=> 0;", "Sets", "Gets")] // Regression test for #2253
+        [InlineData("public", "int", "=> 0;", "Gets or sets", "Gets")] // Regression test for #2253
+        [InlineData("public", "bool", "=> false;", "Gets or sets a value indicating whether", "Gets a value indicating whether")] // Regression test for #2253
+        [InlineData("protected", "int", "=> 0;", "Gets or sets", "Gets")] // Regression test for #2253
+        [InlineData("protected internal", "int", "=> 0;", "Gets or sets", "Gets")] // Regression test for #2253
+        [InlineData("internal", "int", "=> 0;", "Gets or sets", "Gets")] // Regression test for #2253
+        [InlineData("public", "int", "{ set {} }", "Gets", "Sets")] // Regression test for #2253
+        [InlineData("public", "int", "{ set {} }", "Gets or sets", "Sets")] // Regression test for #2253
+        [InlineData("public", "int", "{ get; private set; }", "Sets", "Gets")] // Regression test for #2253
+        [InlineData("public", "int", "{ private get; set; }", "Gets", "Sets")] // Regression test for #2253
+        public async Task IncorrectSummaryTagWithKnownPrefixShouldBeFixedCorrectlyAsync(string accessibility, string type, string accessors, string summaryPrefix, string expectedArgument)
         {
-            var testCode = @"
+            var testCode = $@"
 public class TestClass
-{
-    /// <summary>
-    /// Gets the first test value.
-    /// </summary>
-    public int TestProperty1 { get; set; }
-
+{{
     /// <summary>
-    /// Sets the seconds test value.
+    /// {summaryPrefix} the value.
     /// </summary>
-    public int TestProperty2 { get; set; }
-}
+    {accessibility} {type} TestProperty {accessors}
+}}
 ";
 
-            var fixedTestCode = @"
+            var fixedTestCode = $@"
 public class TestClass
-{
-    /// <summary>
-    /// Gets or sets the first test value.
-    /// </summary>
-    public int TestProperty1 { get; set; }
-
+{{
     /// <summary>
-    /// Gets or sets the seconds test value.
+    /// {expectedArgument} the value.
     /// </summary>
-    public int TestProperty2 { get; set; }
-}
+    {accessibility} {type} TestProperty {accessors}
+}}
 ";
 
             DiagnosticResult[] expected =
             {
-                this.CSharpDiagnostic(PropertySummaryDocumentationAnalyzer.SA1623Descriptor).WithLocation(7, 16).WithArguments("Gets or sets"),
-                this.CSharpDiagnostic(PropertySummaryDocumentationAnalyzer.SA1623Descriptor).WithLocation(12, 16).WithArguments("Gets or sets"),
+                this.CSharpDiagnostic(PropertySummaryDocumentationAnalyzer.SA1623Descriptor).WithLocation(7, 7 + accessibility.Length + type.Length).WithArguments(expectedArgument),
             };
 
             await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);

StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1624UnitTests.cs

@@ -78,7 +78,7 @@ public class TestClass
         }
 
         /// <summary>
-        /// Verifies that documentation that starts with the proper text for multiple accessors will not produce this
+        /// Verifies that documentation that starts with the proper text for a lone getter will not produce this
         /// diagnostic when the property has an expression body.
         /// </summary>
         /// <param name="accessibility">The accessibility of the property.</param>
@@ -107,52 +107,6 @@ public class TestClass
             await this.VerifyCSharpDiagnosticAsync(testCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
         }
 
-        /// <summary>
-        /// Verifies that documentation that starts with the proper text for multiple accessors will produce a diagnostic for expression body properties.
-        /// </summary>
-        /// <param name="accessibility">The accessibility of the property.</param>
-        /// <param name="type">The type to use for the property.</param>
-        /// <param name="summaryPrefix">The prefix to use in the summary text.</param>
-        /// <param name="expectedArgument1">The first expected argument for the diagnostic.</param>
-        /// <param name="expectedArgument2">The second expected argument for the diagnostic.</param>
-        /// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
-        [Theory(DisplayName = "ExpressionBody Gets&Sets")]
-        [InlineData("public", "int", "Gets or sets", "get", "Gets")]
-        [InlineData("public", "bool", "Gets or sets a value indicating whether", "get", "Gets a value indicating whether")]
-        [InlineData("protected", "int", "Gets or sets", "get", "Gets")]
-        [InlineData("protected internal", "int", "Gets or sets", "get", "Gets")]
-        [InlineData("internal", "int", "Gets or sets", "get", "Gets")]
-        public async Task VerifyThatInvalidDocumentationWillReportDiagnosticForExpressionBodyAsync(string accessibility, string type, string summaryPrefix, string expectedArgument1, string expectedArgument2)
-        {
-            var testCode = $@"
-public class TestClass
-{{
-    /// <summary>
-    /// {summaryPrefix} the test property.
-    /// </summary>
-    {accessibility} {type} TestProperty =>
-        default({type});
-}}
-";
-
-            var fixedTestCode = $@"
-public class TestClass
-{{
-    /// <summary>
-    /// {expectedArgument2} the test property.
-    /// </summary>
-    {accessibility} {type} TestProperty =>
-        default({type});
-}}
-";
-
-            var expected = this.CSharpDiagnostic(PropertySummaryDocumentationAnalyzer.SA1624Descriptor).WithLocation(7, 7 + accessibility.Length + type.Length).WithArguments(expectedArgument1, expectedArgument2);
-
-            await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);
-            await this.VerifyCSharpDiagnosticAsync(fixedTestCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
-            await this.VerifyCSharpFixAsync(testCode, fixedTestCode).ConfigureAwait(false);
-        }
-
         /// <summary>
         /// Verify that no diagnostic will be reported when a public and a private accessor are present within a property that is defined in a contained class of a private class.
         /// </summary>

StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/PropertySummaryDocumentationAnalyzer.cs

@@ -130,25 +130,26 @@ private static void AnalyzeSummaryElement(SyntaxNodeAnalysisContext context, Xml
             var textElement = summaryElement.Content.FirstOrDefault() as XmlTextSyntax;
             var text = textElement == null ? string.Empty : XmlCommentHelper.GetText(textElement, true).TrimStart();
 
-            bool startsWithGetOrSet = text.StartsWith(startingTextGetsOrSets, StringComparison.OrdinalIgnoreCase);
+            bool prefixIsGetsOrSets = text.StartsWith(startingTextGetsOrSets, StringComparison.OrdinalIgnoreCase);
+            bool prefixIsGets = !prefixIsGetsOrSets && text.StartsWith(startingTextGets, StringComparison.OrdinalIgnoreCase);
+            bool prefixIsSets = text.StartsWith(startingTextSets, StringComparison.OrdinalIgnoreCase);
+
             bool getterVisible, setterVisible;
             if (getter != null && setter != null)
             {
                 if (!getter.Modifiers.Any() && !setter.Modifiers.Any())
                 {
-                    // Case 1: The getter and setter have the same declared accessibility
+                    // The getter and setter have the same declared accessibility
                     getterVisible = true;
                     setterVisible = true;
                 }
                 else if (getter.Modifiers.Any(SyntaxKind.PrivateKeyword))
                 {
-                    // Case 3
                     getterVisible = false;
                     setterVisible = true;
                 }
                 else if (setter.Modifiers.Any(SyntaxKind.PrivateKeyword))
                 {
-                    // Case 3
                     getterVisible = true;
                     setterVisible = false;
                 }
@@ -160,7 +161,7 @@ private static void AnalyzeSummaryElement(SyntaxNodeAnalysisContext context, Xml
                                                 || propertyAccessibility == Accessibility.Private;
                     if (propertyOnlyInternal)
                     {
-                        // Case 2: Property only internal and no accessor is explicitly private
+                        // Property only internal and no accessor is explicitly private
                         getterVisible = true;
                         setterVisible = true;
                     }
@@ -174,7 +175,6 @@ private static void AnalyzeSummaryElement(SyntaxNodeAnalysisContext context, Xml
                         case Accessibility.Public:
                         case Accessibility.ProtectedOrInternal:
                         case Accessibility.Protected:
-                            // Case 4
                             getterVisible = true;
                             break;
 
@@ -192,7 +192,6 @@ private static void AnalyzeSummaryElement(SyntaxNodeAnalysisContext context, Xml
                         case Accessibility.Public:
                         case Accessibility.ProtectedOrInternal:
                         case Accessibility.Protected:
-                            // Case 4
                             setterVisible = true;
                             break;
 
@@ -225,51 +224,85 @@ private static void AnalyzeSummaryElement(SyntaxNodeAnalysisContext context, Xml
             {
                 if (setterVisible)
                 {
-                    if (!startsWithGetOrSet)
+                    // Both getter and setter are visible.
+                    if (!prefixIsGetsOrSets)
                     {
-                        diagnosticProperties.Add(ExpectedTextKey, startingTextGetsOrSets);
-
-                        if (text.StartsWith(startingTextGets, StringComparison.OrdinalIgnoreCase))
+                        ReportSA1623(context, diagnosticLocation, diagnosticProperties, text, expectedStartingText: startingTextGetsOrSets, unexpectedStartingText1: startingTextGets, unexpectedStartingText2: startingTextSets);
+                    }
+                }
+                else if (setter != null)
+                {
+                    // Both getter and setter exist, but only getter is visible.
+                    if (!prefixIsGets)
+                    {
+                        if (prefixIsGetsOrSets)
                         {
-                            diagnosticProperties.Add(TextToRemoveKey, text.Substring(0, startingTextGets.Length));
+                            ReportSA1624(context, diagnosticLocation, diagnosticProperties, accessor: "get", expectedStartingText: startingTextGets, startingTextToRemove: startingTextGetsOrSets);
                         }
-                        else if (text.StartsWith(startingTextSets, StringComparison.OrdinalIgnoreCase))
+                        else
                         {
-                            diagnosticProperties.Add(TextToRemoveKey, text.Substring(0, startingTextSets.Length));
+                            ReportSA1623(context, diagnosticLocation, diagnosticProperties, text, expectedStartingText: startingTextGets, unexpectedStartingText1: startingTextSets);
                         }
-
-                        context.ReportDiagnostic(Diagnostic.Create(SA1623Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), startingTextGetsOrSets));
                     }
                 }
                 else
                 {
-                    if (startsWithGetOrSet)
+                    // Getter exists and is visible. Setter does not exist.
+                    if (!prefixIsGets)
                     {
-                        diagnosticProperties.Add(ExpectedTextKey, startingTextGets);
-                        diagnosticProperties.Add(TextToRemoveKey, startingTextGetsOrSets);
-                        context.ReportDiagnostic(Diagnostic.Create(SA1624Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), "get", startingTextGets));
-                    }
-                    else if (!text.StartsWith(startingTextGets, StringComparison.OrdinalIgnoreCase))
-                    {
-                        diagnosticProperties.Add(ExpectedTextKey, startingTextGets);
-                        context.ReportDiagnostic(Diagnostic.Create(SA1623Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), startingTextGets));
+                        ReportSA1623(context, diagnosticLocation, diagnosticProperties, text, expectedStartingText: startingTextGets, unexpectedStartingText1: startingTextSets, unexpectedStartingText2: startingTextGetsOrSets);
                     }
                 }
             }
             else if (setterVisible)
             {
-                if (startsWithGetOrSet)
+                if (getter != null)
                 {
-                    diagnosticProperties.Add(ExpectedTextKey, startingTextSets);
-                    diagnosticProperties.Add(TextToRemoveKey, startingTextGetsOrSets);
-                    context.ReportDiagnostic(Diagnostic.Create(SA1624Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), "set", startingTextSets));
+                    // Both getter and setter exist, but only setter is visible.
+                    if (!prefixIsSets)
+                    {
+                        if (prefixIsGetsOrSets)
+                        {
+                            ReportSA1624(context, diagnosticLocation, diagnosticProperties, accessor: "set", expectedStartingText: startingTextSets, startingTextToRemove: startingTextGetsOrSets);
+                        }
+                        else
+                        {
+                            ReportSA1623(context, diagnosticLocation, diagnosticProperties, text, expectedStartingText: startingTextSets, unexpectedStartingText1: startingTextGets);
+                        }
+                    }
                 }
-                else if (!text.StartsWith(startingTextSets, StringComparison.OrdinalIgnoreCase))
+                else
                 {
-                    diagnosticProperties.Add(ExpectedTextKey, startingTextSets);
-                    context.ReportDiagnostic(Diagnostic.Create(SA1623Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), startingTextSets));
+                    // Setter exists and is visible. Getter does not exist.
+                    if (!prefixIsSets)
+                    {
+                        ReportSA1623(context, diagnosticLocation, diagnosticProperties, text, expectedStartingText: startingTextSets, unexpectedStartingText1: startingTextGetsOrSets, unexpectedStartingText2: startingTextGets);
+                    }
                 }
             }
         }
+
+        private static void ReportSA1623(SyntaxNodeAnalysisContext context, Location diagnosticLocation, ImmutableDictionary<string, string>.Builder diagnosticProperties, string text, string expectedStartingText, string unexpectedStartingText1, string unexpectedStartingText2 = null)
+        {
+            diagnosticProperties.Add(ExpectedTextKey, expectedStartingText);
+
+            if (text.StartsWith(unexpectedStartingText1, StringComparison.OrdinalIgnoreCase))
+            {
+                diagnosticProperties.Add(TextToRemoveKey, text.Substring(0, unexpectedStartingText1.Length));
+            }
+            else if ((unexpectedStartingText2 != null) && text.StartsWith(unexpectedStartingText2, StringComparison.OrdinalIgnoreCase))
+            {
+                diagnosticProperties.Add(TextToRemoveKey, text.Substring(0, unexpectedStartingText2.Length));
+            }
+
+            context.ReportDiagnostic(Diagnostic.Create(SA1623Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), expectedStartingText));
+        }
+
+        private static void ReportSA1624(SyntaxNodeAnalysisContext context, Location diagnosticLocation, ImmutableDictionary<string, string>.Builder diagnosticProperties, string accessor, string expectedStartingText, string startingTextToRemove)
+        {
+            diagnosticProperties.Add(ExpectedTextKey, expectedStartingText);
+            diagnosticProperties.Add(TextToRemoveKey, startingTextToRemove);
+            context.ReportDiagnostic(Diagnostic.Create(SA1624Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), accessor, expectedStartingText));
+        }
     }
 }