fix: Diagnostic on empty regions (#715)

Co-authored-by: Brian Collamore <57269455+bcollamore@users.noreply.github.com>

Author: ynsehoornenborg

Documentation/Diagnostics/PH2141.md

@@ -0,0 +1,51 @@
+# PH2141: Avoid Empty Regions
+
+| Property | Value  |
+|--|--|
+| Package | [Philips.CodeAnalysis.MaintainabilityAnalyzers](https://www.nuget.org/packages/Philips.CodeAnalysis.MaintainabilityAnalyzers) |
+| Diagnostic ID | PH2141 |
+| Category  | [Readability](../Readability.md) |
+| Analyzer | [EnforceRegionsAnalyzer](https://github.com/philips-software/roslyn-analyzers/blob/main/Philips.CodeAnalysis.MaintainabilityAnalyzers/Readability/EnforceRegionsAnalyzer.cs)
+| CodeFix  | No |
+| Severity | Error |
+| Enabled By Default | Yes |
+
+## Introduction
+
+Avoid writing regions with no code inside.
+
+## How to solve
+
+Remove the empty region.
+
+## Example
+
+Code that triggers a diagnostic:
+``` cs
+class BadExample
+{
+    #region Put some stuff here later
+    #endregion
+}
+
+```
+
+And the replacement code:
+``` cs
+class GoodExample
+{
+    #region Public Interface
+
+    public void GoodMethod()
+    {
+        return;
+    }
+
+    #endregion
+}
+
+```
+
+## Configuration
+
+This analyzer does not offer any special configuration. The general ways of [suppressing](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/suppress-warnings) diagnostics apply.

Philips.CodeAnalysis.Common/DiagnosticId.cs

@@ -134,6 +134,7 @@ public enum DiagnosticId
 		AvoidVoidReturn = 2138,
 		EnableDocumentationCreation = 2139,
 		AvoidExcludeFromCodeCoverage = 2140,
+		AvoidEmptyRegions = 2141,
 		AvoidCastToString = 2142,
 		AvoidAssemblyGetEntryAssembly = 2143,
 	}

Philips.CodeAnalysis.MaintainabilityAnalyzers/LocationRangeModel.cs

@@ -1,15 +1,20 @@
 // © 2019 Koninklijke Philips N.V. See License.md in the project root for license information.
 
+using Microsoft.CodeAnalysis;
+
 namespace Philips.CodeAnalysis.MaintainabilityAnalyzers
 {
 	public class LocationRangeModel
 	{
-		public LocationRangeModel(int startLine, int endLine)
+		public LocationRangeModel(Location location, int startLine, int endLine)
 		{
+			Location = location;
 			StartLine = startLine;
 			EndLine = endLine;
 		}
 
+		public Location Location { get; }
+
 		public int StartLine { get; }
 
 		public int EndLine { get; set; }

Philips.CodeAnalysis.MaintainabilityAnalyzers/Philips.CodeAnalysis.MaintainabilityAnalyzers.csproj

@@ -123,6 +123,7 @@
 	    * Introduce PH2136: Avoid duplicate strings.
       * Introduce PH2139: Enable documentation creation.
       * Introduce PH2140: Avoid ExcludeFromCodeCoverage attribute.
+      * Introduce PH2141: Avoid Empty Regions.
       * Introduce PH2142: Avoid casting to String.
       * Introduce PH2143: Avoid Assembly.GetEntryAssembly().
     </PackageReleaseNotes>

Philips.CodeAnalysis.MaintainabilityAnalyzers/Philips.CodeAnalysis.MaintainabilityAnalyzers.md

@@ -90,5 +90,5 @@
 | [PH2138](../Documentation/Diagnostics/PH2138.md)  | Avoid void returns                           | Methods that return void are mysterious. |
 | [PH2139](../Documentation/Diagnostics/PH2139.md)  | Enable documentation creation                | Add XML documentation generation to the project file, to be able to see Diagnostics for XML documentation. |
 | [PH2140](../Documentation/Diagnostics/PH2140.md)  | Avoid ExcludeFromCodeCoverage attribute      | Avoid the usage of the 'ExcludeFromCodeCoverage' attribute. |
-| [PH2142](../Documentation/Diagnostics/PH2142.md)  | Avoid Casting to String                      | Avoid casting to int, use `object.ToString()` or a serialization solution instead. |
+| [PH2141](../Documentation/Diagnostics/PH2141.md)  | Avoid Empty Regions                          | Avoid writing regions that have no code inside. |
 | [PH2143](../Documentation/Diagnostics/PH2143.md)  | Avoid Assembly.GetEntryAssembly()            | Avoid using Assembly.GetEntryAssembly(), as it might give different results when your code runs under a TestRunner. |

Philips.CodeAnalysis.MaintainabilityAnalyzers/Readability/EnforceRegionsAnalyzer.cs

@@ -8,6 +8,7 @@
 using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.CodeAnalysis.Text;
 using Philips.CodeAnalysis.Common;
 
 namespace Philips.CodeAnalysis.MaintainabilityAnalyzers.Readability
@@ -28,13 +29,19 @@ public class EnforceRegionsAnalyzer : DiagnosticAnalyzerBase
 		private const string EnforceNonDuplicateRegionCategory = Categories.Readability;
 
 		private const string NonCheckedRegionMemberTitle = @"Members location relative to regions (Enforce Region Analyzer).";
-		public const string NonCheckedRegionMemberTitleMessageFormat = @"Member's location relative to region {0} should be verified in Enforce Regions Analyzer";
-		private const string NonCheckedRegionMemberTitleDescription = @"Member's location relative to region {0} should be verified. Implement the check in enforce region analyer";
-		private const string NonCheckedRegionMemberTitleCategory = Categories.Readability;
+		public const string NonCheckedRegionMemberMessageFormat = @"Member's location relative to region {0} should be verified in Enforce Regions Analyzer";
+		private const string NonCheckedRegionMemberDescription = @"Member's location relative to region {0} should be verified. Implement the check in enforce region analyer";
+		private const string NonCheckedRegionMemberCategory = Categories.Readability;
+
+		private const string AvoidEmptyRegionTitle = @"Avoid empty regions";
+		public const string AvoidEmptyRegionMessageFormat = @"Remove the empty region";
+		private const string AvoidEmptyRegionDescription = @"Remove the empty region";
+		private const string AvoidEmptyRegionCategory = Categories.Readability;
 
 		private const string NonPublicDataMembersRegion = "Non-Public Data Members";
 		private const string NonPublicPropertiesAndMethodsRegion = "Non-Public Properties/Methods";
 		private const string PublicInterfaceRegion = "Public Interface";
+		private const string EmptyRegion = "Empty region";
 
 		private static readonly Dictionary<string, Func<IReadOnlyList<MemberDeclarationSyntax>, SyntaxNodeAnalysisContext, bool>> RegionChecks = new()
 		{
@@ -49,38 +56,51 @@ public class EnforceRegionsAnalyzer : DiagnosticAnalyzerBase
 			EnforceNonDuplicateRegionTitle, EnforceNonDuplicateRegionMessageFormat, EnforceNonDuplicateRegionCategory,
 			DiagnosticSeverity.Error, isEnabledByDefault: true, description: EnforceNonDuplicateRegionDescription);
 		private static readonly DiagnosticDescriptor NonCheckedMember = new(DiagnosticId.NonCheckedRegionMember.ToId(),
-			NonCheckedRegionMemberTitle, NonCheckedRegionMemberTitleMessageFormat, NonCheckedRegionMemberTitleCategory,
-			DiagnosticSeverity.Info, isEnabledByDefault: true, description: NonCheckedRegionMemberTitleDescription);
+			NonCheckedRegionMemberTitle, NonCheckedRegionMemberMessageFormat, NonCheckedRegionMemberCategory,
+			DiagnosticSeverity.Info, isEnabledByDefault: true, description: NonCheckedRegionMemberDescription);
+		private static readonly DiagnosticDescriptor AvoidEmpty = new(DiagnosticId.AvoidEmptyRegions.ToId(), AvoidEmptyRegionTitle,
+			AvoidEmptyRegionMessageFormat, AvoidEmptyRegionCategory, DiagnosticSeverity.Error, isEnabledByDefault: true, description: AvoidEmptyRegionDescription);
 
 
-		public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(EnforceMemberLocation, EnforceNonDupliateRegion, NonCheckedMember);
+		public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(EnforceMemberLocation, EnforceNonDupliateRegion, NonCheckedMember, AvoidEmpty);
 
 		protected override void InitializeCompilation(CompilationStartAnalysisContext context)
 		{
-			context.RegisterSyntaxNodeAction(Analyze, SyntaxKind.ClassDeclaration);
+			context.RegisterSyntaxNodeAction(Analyze, SyntaxKind.ClassDeclaration, SyntaxKind.StructDeclaration);
 		}
 
 		private static void Analyze(SyntaxNodeAnalysisContext context)
 		{
-			var classDeclaration = (ClassDeclarationSyntax)context.Node;
+			var typeDeclaration = (TypeDeclarationSyntax)context.Node;
 
 			RegionVisitor visitor = new();
 
-			visitor.Visit(classDeclaration);
+			visitor.Visit(typeDeclaration);
 
 			IReadOnlyList<DirectiveTriviaSyntax> regions = visitor.Regions;
 
+			// Region directives should come in pairs
+			if (regions.Count == 1)
+			{
+				return;
+			}
+
 			Dictionary<string, LocationRangeModel> regionLocations = PopulateRegionLocations(regions, context);
 
 			if (regionLocations.Count == 0)
 			{
 				return;
 			}
 
-			SyntaxList<MemberDeclarationSyntax> members = classDeclaration.Members;
+			SyntaxList<MemberDeclarationSyntax> members = typeDeclaration.Members;
 
 			foreach (KeyValuePair<string, LocationRangeModel> pair in regionLocations)
 			{
+				if (!MemberPresentInRegion(typeDeclaration, pair.Value))
+				{
+					CheckEmptyRegion(pair.Value, members, context);
+				}
+
 				if (RegionChecks.TryGetValue(pair.Key, out Func<IReadOnlyList<MemberDeclarationSyntax>, SyntaxNodeAnalysisContext, bool> functionToCall))
 				{
 					IReadOnlyList<MemberDeclarationSyntax> membersOfRegion = GetMembersOfRegion(members, pair.Value);
@@ -94,7 +114,7 @@ private static string GetRegionName(DirectiveTriviaSyntax region)
 		{
 			var regionName = string.Empty;
 
-			Microsoft.CodeAnalysis.Text.TextLineCollection lines = region.GetText().Lines;
+			TextLineCollection lines = region.GetText().Lines;
 
 			if (lines.Count > 0)
 			{
@@ -119,21 +139,19 @@ private static void PopulateRegionLocation(ref string regionStartName, Dictionar
 					return;
 				}
 
-				if (RegionChecks.ContainsKey(regionName))
+				if (regionLocations.Remove(regionName))
 				{
-					if (regionLocations.Remove(regionName))
-					{
-						Location memberLocation = region.DirectiveNameToken.GetLocation();
-						CreateDiagnostic(memberLocation, context, regionName, EnforceNonDupliateRegion);
-					}
-					else
-					{
-						Location location = region.GetLocation();
-						var lineNumber = GetMemberLineNumber(location);
-
-						regionLocations.Add(regionName, new LocationRangeModel(lineNumber, lineNumber));
-						regionStartName = regionName;
-					}
+					Location memberLocation = region.DirectiveNameToken.GetLocation();
+					CreateDiagnostic(memberLocation, context, regionName, EnforceNonDupliateRegion);
+				}
+				else
+				{
+					Location location = region.GetLocation();
+					Location memberLocation = region.DirectiveNameToken.GetLocation();
+					var lineNumber = GetMemberLineNumber(location);
+
+					regionLocations.Add(regionName, new LocationRangeModel(memberLocation, lineNumber, lineNumber));
+					regionStartName = regionName;
 				}
 			}
 			else
@@ -162,7 +180,7 @@ private static void PopulateRegionLocation(ref string regionStartName, Dictionar
 		private static Dictionary<string, LocationRangeModel> PopulateRegionLocations(IReadOnlyList<DirectiveTriviaSyntax> regions, SyntaxNodeAnalysisContext context)
 		{
 			Dictionary<string, LocationRangeModel> regionLocations = [];
-			var regionStartName = "";
+			var regionStartName = string.Empty;
 			for (var i = 0; i < regions.Count; i++)
 			{
 				DirectiveTriviaSyntax region = regions[i];
@@ -188,11 +206,11 @@ private static List<MemberDeclarationSyntax> GetMembersOfRegion(SyntaxList<Membe
 		/// <param name="member"></param>
 		/// <param name="locationRange"></param>
 		/// <returns>true if member is inside the given region, else false</returns>
-		private static bool MemberPresentInRegion(MemberDeclarationSyntax member, LocationRangeModel locationRange)
+		private static bool MemberPresentInRegion(SyntaxNode member, LocationRangeModel locationRange)
 		{
 			Location location = member.GetLocation();
-			var memberLocation = GetMemberLineNumber(location);
-			return memberLocation > locationRange.StartLine && memberLocation < locationRange.EndLine;
+			var memberLine = GetMemberLineNumber(location);
+			return memberLine > locationRange.StartLine && memberLine < locationRange.EndLine;
 		}
 
 		/// <summary>
@@ -217,13 +235,25 @@ private static bool CheckMembersOfPublicInterfaceRegion(IReadOnlyList<MemberDecl
 			return true;
 		}
 
+		/// <summary>
+		/// Checks whether the regions are empty, contain no statements.
+		/// </summary>
+		private static void CheckEmptyRegion(LocationRangeModel locationRange, IReadOnlyList<MemberDeclarationSyntax> members, SyntaxNodeAnalysisContext context)
+		{
+			var foundMemberInside = members.Any(m => MemberPresentInRegion(m, locationRange));
+
+			if (!foundMemberInside)
+			{
+				// Empty region
+				CreateDiagnostic(locationRange.Location, context, EmptyRegion, AvoidEmpty);
+			}
+		}
+
 		/// <summary>
 		/// Verify whether the given member belongs to the public interface region
 		/// If the member is of type field, then throw an error
 		/// If the member is of type method, then check if its non-public
 		/// </summary>
-		/// <param name="member"></param>
-		/// <param name="context"></param>
 		private static void VerifyMemberForPublicInterfaceRegion(MemberDeclarationSyntax member, SyntaxNodeAnalysisContext context)
 		{
 			if (TryGetModifiers(member, true, out SyntaxTokenList modifiers))

Philips.CodeAnalysis.Test/Maintainability/Maintainability/ProhibitDynamicKeywordAnalyzerTest.cs

@@ -13,10 +13,6 @@ namespace Philips.CodeAnalysis.Test.Maintainability.Maintainability
 	[TestClass]
 	public class ProhibitDynamicKeywordAnalyzerTest : DiagnosticVerifier
 	{
-		#region Non-Public Data Members
-
-		#endregion
-
 		#region Non-Public Properties/Methods
 
 		protected override DiagnosticAnalyzer GetDiagnosticAnalyzer()

Philips.CodeAnalysis.Test/Maintainability/Readability/EnforceRegionsTest.cs

@@ -195,6 +195,37 @@ class Foo
 			await VerifyError(baseline, given, isError, 6, 2).ConfigureAwait(false);
 		}
 
+		[TestMethod]
+		[TestCategory(TestDefinitions.UnitTests)]
+		public async Task AvoidEmptyRegion()
+		{
+			var givenText = @"
+class C {{
+  #region Some Small Region
+
+  #endregion
+}}
+";
+			await VerifyDiagnostic(givenText, DiagnosticId.AvoidEmptyRegions, regex: EnforceRegionsAnalyzer.AvoidEmptyRegionMessageFormat, line: 3, column: 4).ConfigureAwait(false);
+		}
+
+		[TestMethod]
+		[TestCategory(TestDefinitions.UnitTests)]
+		public async Task AvoidRegionInsideMethod()
+		{
+			var givenText = @"
+class C {{
+  public void LongMethod() {{
+    private int i = 0;
+    #region Inside Method
+    i++;
+    #endregion
+  }}
+}}
+";
+			await VerifySuccessfulCompilation(givenText);
+		}
+
 		[DataTestMethod]
 		[DataRow(@"#region Constants", false)]
 		[DataRow(@"#region Public Properties/Methods", false)]

Philips.CodeAnalysis.Test/Moq/MockObjectsMustCallExistingConstructorsAnalyzerTest.cs

@@ -16,10 +16,6 @@ namespace Philips.CodeAnalysis.Test.Moq
 	[TestClass]
 	public class MockObjectsMustCallExistingConstructorsAnalyzerTest : DiagnosticVerifier
 	{
-		#region Non-Public Data Members
-
-		#endregion
-
 		#region Non-Public Properties/Methods
 		protected override DiagnosticAnalyzer GetDiagnosticAnalyzer()
 		{

Philips.CodeAnalysis.Test/MsTest/AssertAreEqualLiteralAnalyzerTest.cs

@@ -14,10 +14,6 @@ namespace Philips.CodeAnalysis.Test.MsTest
 	[TestClass]
 	public class AssertAreEqualLiteralAnalyzerTest : AssertCodeFixVerifier
 	{
-		#region Non-Public Data Members
-
-		#endregion
-
 		#region Non-Public Properties/Methods
 
 		protected override DiagnosticResult GetExpectedDiagnostic(int expectedLineNumberErrorOffset = 0, int expectedColumnErrorOffset = 0)