Merge pull request #6035 from BZngr/5730PublicShouldBePrivate

Introduce PublicImplementationShouldBePrivate Inspection

Rubberduck.CodeAnalysis/Inspections/Concrete/PublicImplementationShouldBePrivateInspection.cs

@@ -0,0 +1,155 @@
+using Rubberduck.CodeAnalysis.CodeMetrics;
+using Rubberduck.CodeAnalysis.Inspections.Abstract;
+using Rubberduck.Parsing.Symbols;
+using Rubberduck.Parsing.VBA;
+using Rubberduck.Parsing.VBA.DeclarationCaching;
+using Rubberduck.Refactorings.Common;
+using Rubberduck.Resources.Inspections;
+using Rubberduck.VBEditor;
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+
+namespace Rubberduck.CodeAnalysis.Inspections.Concrete
+{
+    /// <summary>
+    /// Flags Interface implementation members and EventHandlers with Public scope.
+    /// </summary>
+    /// <why>
+    /// The default (Public) interface of a class module should not expose the implementation of other interfaces or event handler procedures.
+    /// If the implementation of an interface member or event handler is useful for a class to expose, it should do so using
+    /// a dedicated Public member rather than changing the interface member or event handler scope from 'Private' to 'Public'.
+    /// </why>
+    /// <example hasResult="true">
+    /// <module name="MyClassModule" type="Class Module">
+    /// <![CDATA[
+    /// Implements ISomething   'ISomething defines procedure "DoSomething"
+    /// 
+    /// Public Sub ISomething_DoSomething(ByVal someValue As Long)
+    ///     Debug.Print someValue
+    /// End Sub
+    /// ]]>
+    /// </module>
+    /// </example>
+    /// <example hasResult="true">
+    /// <module name="MyClassModule" type="Class Module">
+    /// <![CDATA[
+    /// Private WithEvents abc As MyEvent   'MyEvent defines a "MyValueChanged" event
+    /// 
+    /// Public Sub abc_MyValueChanged(ByVal newVal As Long)
+    ///     Debug.Print newVal
+    /// End Sub
+    /// ]]>
+    /// </module>
+    /// </example>
+    /// <example hasResult="true">
+    /// <module name="MyClassModule" type="Class Module">
+    /// <![CDATA[
+    /// 'Code found in the "ThisWorkbook" Document
+    /// Public Sub Workbook_Open()
+    ///     Debug.Print "Workbook was opened"
+    /// End Sub
+    /// ]]>
+    /// </module>
+    /// </example>
+    /// <example hasResult="false">
+    /// <module name="MyClassModule" type="Class Module">
+    /// <![CDATA[
+    /// Implements ISomething   'ISomething defines procedure "DoSomething"
+    /// 
+    /// Private Sub ISomething_DoSomething(ByVal someValue As Long)
+    ///     Debug.Print someValue
+    /// End Sub
+    /// ]]>
+    /// </module>
+    /// </example>
+    /// <example hasResult="false">
+    /// <module name="MyClassModule" type="Class Module">
+    /// <![CDATA[
+    /// Public Sub Do_Something(ByVal someValue As Long)
+    ///     Debug.Print someValue
+    /// End Sub
+    /// ]]>
+    /// </module>
+
+    internal sealed class PublicImplementationShouldBePrivateInspection : DeclarationInspectionBase
+    {
+        public PublicImplementationShouldBePrivateInspection(IDeclarationFinderProvider declarationFinderProvider)
+            : base(declarationFinderProvider, DeclarationType.Member)
+        {}
+
+        //Overriding DoGetInspectionResults in order to dereference the DeclarationFinder FindXXX declaration 
+        //lists only once per inspections pass.
+        protected override IEnumerable<IInspectionResult> DoGetInspectionResults(DeclarationFinder finder)
+        {
+            var publicMembers = finder.UserDeclarations(DeclarationType.Member)
+                .Where(d => !d.HasPrivateAccessibility()
+                    && IsLikeAnImplementerOrHandlerName(d.IdentifierName));
+
+            if (!publicMembers.Any())
+            {
+                return Enumerable.Empty<IInspectionResult>();
+            }
+
+            var publicImplementersAndHandlers = finder.FindAllInterfaceImplementingMembers()
+                .Where(d => !d.HasPrivateAccessibility())
+                .Concat(finder.FindEventHandlers()
+                    .Where(d => !d.HasPrivateAccessibility()));
+
+            var publicDocumentEvents = FindDocumentEventHandlers(publicMembers);
+
+            return publicMembers.Intersect(publicImplementersAndHandlers)
+                .Concat(publicDocumentEvents)
+                .Select(InspectionResult)
+                .ToList();
+        }
+
+        private static IEnumerable<Declaration> FindDocumentEventHandlers(IEnumerable<Declaration> publicMembers)
+        {
+            //Excel and Word
+            var docEventPrefixes = new List<string>() 
+            { 
+                "Workbook", 
+                "Worksheet", 
+                "Document" 
+            };
+
+            //FindDocumentEventHandlers can be a source of False Positives if a Document's code
+            //contains Public procedure Identifiers (with a single underscore).
+            return publicMembers.Where(d => d.ParentDeclaration.DeclarationType.HasFlag(DeclarationType.Document)
+                && d.DeclarationType.Equals(DeclarationType.Procedure)
+                && docEventPrefixes.Any(dep => IsLikeADocumentEventHandlerName(d.IdentifierName, dep)));
+        }
+
+        protected override string ResultDescription(Declaration declaration)
+        {
+            return string.Format(Resources.Inspections.InspectionResults.PublicImplementationShouldBePrivateInspection, 
+                declaration.IdentifierName);
+        }
+
+        private static bool IsLikeAnImplementerOrHandlerName(string identifier)
+        {
+            var splitup = identifier.Split('_');
+            return splitup.Length == 2 && splitup[1].Length > 0;
+        }
+
+        private static bool IsLikeADocumentEventHandlerName(string procedureName, string docEventHandlerPrefix)
+        {
+            var splitup = procedureName.Split('_');
+
+            return splitup.Length == 2 
+                && splitup[0].Equals(docEventHandlerPrefix, StringComparison.InvariantCultureIgnoreCase)
+                && splitup[1].Length > 2 //Excel and Word document events all have at least 3 characters
+                && !splitup[1].Any(c => char.IsDigit(c)); //Excel and Word document event names do not contain numbers
+        }
+
+        //The 'DoGetInspectionResults' override excludes IsResultDeclaration from the execution path
+        protected override bool IsResultDeclaration(Declaration declaration, DeclarationFinder finder)
+        {
+            throw new NotImplementedException();
+        }
+
+    }
+}

Rubberduck.Resources/Inspections/InspectionInfo.resx

@@ -475,4 +475,7 @@ If the parameter can be null, ignore this inspection result; passing a null valu
   <data name="UDTMemberNotUsedInspection" xml:space="preserve">
     <value>A User Defined Type (UDT) member is declared but not used.  Consider removing the UDT member declaration.</value>
   </data>
+  <data name="PublicImplementationShouldBePrivateInspection" xml:space="preserve">
+    <value>The default (Public) interface of a class module should not expose the implementation of other interfaces or event handler procedures.</value>
+  </data>
 </root>

Rubberduck.Resources/Inspections/InspectionNames.resx

@@ -475,4 +475,7 @@
   <data name="UDTMemberNotUsedInspection" xml:space="preserve">
     <value>User Defined Type member is not used</value>
   </data>
+  <data name="PublicImplementationShouldBePrivateInspection" xml:space="preserve">
+    <value>Implementations of interfaces and event handlers should be Private</value>
+  </data>
 </root>

Rubberduck.Resources/Inspections/InspectionResults.resx

@@ -556,4 +556,8 @@ In memoriam, 1972-2018</value>
     <value>Control '{0}.{1}' is being accessed from outside its parent form.</value>
     <comment>{0} parent UserForm name; {1} control name</comment>
   </data>
+  <data name="PublicImplementationShouldBePrivateInspection" xml:space="preserve">
+    <value>Member '{0}' should be Private</value>
+    <comment>{0} Member Identifier</comment>
+  </data>
 </root>

RubberduckTests/Inspections/PublicImplementationShouldBePrivateInspectionTests.cs

@@ -0,0 +1,139 @@
+using NUnit.Framework;
+using Rubberduck.CodeAnalysis.Inspections;
+using Rubberduck.CodeAnalysis.Inspections.Concrete;
+using Rubberduck.Parsing.VBA;
+using Rubberduck.VBEditor.SafeComWrappers;
+using RubberduckTests.Mocks;
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+
+namespace RubberduckTests.Inspections
+{
+    [TestFixture]
+    public class PublicImplementationShouldBePrivateInspectionTests : InspectionTestsBase
+    {
+        [TestCase("Class_Initialize", "Private", 0)]
+        [TestCase("Class_Initialize", "Public", 1)]
+        [TestCase("Class_Terminate", "Friend", 1)]
+        [TestCase("Class_NamedPoorly", "Friend", 0)]
+        [TestCase("Class_Initialize_Again", "Friend", 0)]
+        [Category("Inspections")]
+        [Category(nameof(PublicImplementationShouldBePrivateInspectionTests))]
+        public void LifecycleHandlers(string memberIdentifier, string accessibility, long expected)
+        {
+            var inputCode = 
+$@"Option Explicit
+
+Private mVal As Long
+
+{accessibility} Sub {memberIdentifier}()
+    mVal = 5
+End Sub
+";
+
+            var inspectionResults = InspectionResultsForModules(
+                (MockVbeBuilder.TestModuleName, inputCode, ComponentType.ClassModule)); ;
+
+            var actual = inspectionResults.Count();
+
+            Assert.AreEqual(expected, actual);
+        }
+
+        [TestCase("Public", 1)]
+        [TestCase("Private", 0)]
+        [Category("Inspections")]
+        [Category(nameof(PublicImplementationShouldBePrivateInspectionTests))]
+        public void UserDefinedEventHandlers(string accessibility, long expected)
+        {
+            var eventDeclaringClassName = "EventClass";
+            var eventDeclarationCode =
+$@"
+    Option Explicit
+
+    Public Event MyEvent(ByVal arg1 As Integer, ByVal arg2 As String)
+";
+
+            var inputCode =
+$@"
+    Option Explicit
+
+    Private WithEvents abc As {eventDeclaringClassName}
+
+    {accessibility} Sub abc_MyEvent(ByVal i As Integer, ByVal s As String)
+    End Sub
+";
+
+            var inspectionResults = InspectionResultsForModules(
+                (eventDeclaringClassName, eventDeclarationCode, ComponentType.ClassModule),
+                (MockVbeBuilder.TestModuleName, inputCode, ComponentType.ClassModule));
+
+            var actual = inspectionResults.Count();
+
+            Assert.AreEqual(expected, actual);
+        }
+
+        [TestCase("Public", 1)]
+        [TestCase("Private", 0)]
+        [Category("Inspections")]
+        [Category(nameof(PublicImplementationShouldBePrivateInspectionTests))]
+        public void InterfaceImplementingMembers(string accessibility, long expected)
+        {
+            var interfaceDeclarationClass = "ITestClass";
+            var interfaceDeclarationCode =
+$@"
+    Option Explicit
+
+    Public Sub ImplementMe(ByVal arg1 As Integer, ByVal arg2 As String)
+    End Sub
+";
+
+            var inputCode =
+$@"
+    Option Explicit
+
+    Implements {interfaceDeclarationClass}
+
+    {accessibility} Sub {interfaceDeclarationClass}_ImplementMe(ByVal i As Integer, ByVal s As String)
+    End Sub
+";
+
+            var inspectionResults = InspectionResultsForModules(
+                (interfaceDeclarationClass, interfaceDeclarationCode, ComponentType.ClassModule),
+                (MockVbeBuilder.TestModuleName, inputCode, ComponentType.ClassModule));
+
+            var actual = inspectionResults.Count();
+
+            Assert.AreEqual(expected, actual);
+        }
+
+        [TestCase("Workbook_Open", "ThisWorkbook", 1)]
+        [TestCase("Worksheet_SelectionChange", "Sheet1", 1)]
+        [TestCase("Document_Open", "ThisDocument", 1)]
+        [TestCase("Document_Open_Again", "ThisDocument", 0)]
+        [Category("Inspections")]
+        [Category(nameof(PublicImplementationShouldBePrivateInspectionTests))]
+        public void DocumentEventHandlers(string subroutineName, string objectName, long expected)
+        {
+            var inputCode =
+$@"
+Public Sub {subroutineName}()    
+    Range(""A1"").Value = ""Test""
+End Sub";
+
+            var inspectionResults = InspectionResultsForModules(
+                (objectName, inputCode, ComponentType.Document)); ;
+
+            var actual = inspectionResults.Count();
+
+            Assert.AreEqual(expected, actual);
+        }
+
+        protected override IInspection InspectionUnderTest(RubberduckParserState state)
+        {
+            return new PublicImplementationShouldBePrivateInspection(state);
+        }
+    }
+}