From bd6ef1b7f3e7e7f2a41af7b6fd224c9be25023d0 Mon Sep 17 00:00:00 2001 From: Brian Zenger Date: Mon, 17 Oct 2022 05:34:11 -0700 Subject: [PATCH 1/5] Initial Inspection Tests --- ...mentationShouldBePrivateInspectionTests.cs | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 RubberduckTests/Inspections/PublicImplementationShouldBePrivateInspectionTests.cs diff --git a/RubberduckTests/Inspections/PublicImplementationShouldBePrivateInspectionTests.cs b/RubberduckTests/Inspections/PublicImplementationShouldBePrivateInspectionTests.cs new file mode 100644 index 0000000000..2f94e3f515 --- /dev/null +++ b/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); + } + } +} From 1cbc99b5ffece1cf28feb9c009031af5bb8a4cd8 Mon Sep 17 00:00:00 2001 From: Brian Zenger Date: Mon, 17 Oct 2022 12:02:31 -0700 Subject: [PATCH 2/5] Add PublicImplementationShouldBePrivateInspection resources --- .../Inspections/InspectionInfo.Designer.cs | 9 +++++++++ .../Inspections/InspectionInfo.resx | 3 +++ .../Inspections/InspectionNames.Designer.cs | 9 +++++++++ .../Inspections/InspectionNames.resx | 3 +++ .../Inspections/InspectionResults.Designer.cs | 20 +++++++++---------- .../Inspections/InspectionResults.resx | 4 ++++ 6 files changed, 38 insertions(+), 10 deletions(-) diff --git a/Rubberduck.Resources/Inspections/InspectionInfo.Designer.cs b/Rubberduck.Resources/Inspections/InspectionInfo.Designer.cs index 6cfa167ca4..07db96b0a3 100644 --- a/Rubberduck.Resources/Inspections/InspectionInfo.Designer.cs +++ b/Rubberduck.Resources/Inspections/InspectionInfo.Designer.cs @@ -861,6 +861,15 @@ public class InspectionInfo { } } + /// + /// Looks up a localized string similar to The default (Public) interface of a class module should not expose the implementation of other interfaces or event handler procedures.. + /// + public static string PublicImplementationShouldBePrivateInspection { + get { + return ResourceManager.GetString("PublicImplementationShouldBePrivateInspection", resourceCulture); + } + } + /// /// Looks up a localized string similar to In general, the VBE editor catches this type of error and will not compile. However, there are a few scenarios where the error is overlooked by the compiler and an error is generated at runtime. To avoid a runtime error, implement the missing Property or Subroutine. . /// diff --git a/Rubberduck.Resources/Inspections/InspectionInfo.resx b/Rubberduck.Resources/Inspections/InspectionInfo.resx index 79996f8523..e3bc92ff90 100644 --- a/Rubberduck.Resources/Inspections/InspectionInfo.resx +++ b/Rubberduck.Resources/Inspections/InspectionInfo.resx @@ -475,4 +475,7 @@ If the parameter can be null, ignore this inspection result; passing a null valu A User Defined Type (UDT) member is declared but not used. Consider removing the UDT member declaration. + + The default (Public) interface of a class module should not expose the implementation of other interfaces or event handler procedures. + \ No newline at end of file diff --git a/Rubberduck.Resources/Inspections/InspectionNames.Designer.cs b/Rubberduck.Resources/Inspections/InspectionNames.Designer.cs index 32e067bf28..821a1501d6 100644 --- a/Rubberduck.Resources/Inspections/InspectionNames.Designer.cs +++ b/Rubberduck.Resources/Inspections/InspectionNames.Designer.cs @@ -861,6 +861,15 @@ public class InspectionNames { } } + /// + /// Looks up a localized string similar to Public implementations of interfaces and event handlers should be Private. + /// + public static string PublicImplementationShouldBePrivateInspection { + get { + return ResourceManager.GetString("PublicImplementationShouldBePrivateInspection", resourceCulture); + } + } + /// /// Looks up a localized string similar to Read-Only Property assignment. /// diff --git a/Rubberduck.Resources/Inspections/InspectionNames.resx b/Rubberduck.Resources/Inspections/InspectionNames.resx index 442dffb9b6..cb0f7a649f 100644 --- a/Rubberduck.Resources/Inspections/InspectionNames.resx +++ b/Rubberduck.Resources/Inspections/InspectionNames.resx @@ -475,4 +475,7 @@ User Defined Type member is not used + + Public implementations of interfaces and event handlers should be Private + \ No newline at end of file diff --git a/Rubberduck.Resources/Inspections/InspectionResults.Designer.cs b/Rubberduck.Resources/Inspections/InspectionResults.Designer.cs index 5c3c3ef886..aeedd681d5 100644 --- a/Rubberduck.Resources/Inspections/InspectionResults.Designer.cs +++ b/Rubberduck.Resources/Inspections/InspectionResults.Designer.cs @@ -889,7 +889,7 @@ public class InspectionResults { } /// - /// Looks up a localized string similar to The public enumeration `{0}` should be declared within a Standard or Class module. + /// Looks up a localized string similar to The public enumeration '{0}' should be declared within a Standard or Class module. /// public static string PublicEnumerationDeclaredInWorksheetInspection { get { @@ -897,6 +897,15 @@ public class InspectionResults { } } + /// + /// Looks up a localized string similar to Member '{0}' should be Private. + /// + public static string PublicImplementationShouldBePrivateInspection { + get { + return ResourceManager.GetString("PublicImplementationShouldBePrivateInspection", resourceCulture); + } + } + /// /// Looks up a localized string similar to Attempt to assign Read-Only Property '{0}'. /// @@ -1026,15 +1035,6 @@ public class InspectionResults { } } - /// - /// Looks up a localized string similar to {0} '{1}' is not used. - /// - public static string UDTMemberNotUsedInspection { - get { - return ResourceManager.GetString("UDTMemberNotUsedInspection", resourceCulture); - } - } - /// /// Looks up a localized string similar to Variable '{0}' is used but not assigned.. /// diff --git a/Rubberduck.Resources/Inspections/InspectionResults.resx b/Rubberduck.Resources/Inspections/InspectionResults.resx index e619a92b0d..041022f111 100644 --- a/Rubberduck.Resources/Inspections/InspectionResults.resx +++ b/Rubberduck.Resources/Inspections/InspectionResults.resx @@ -556,4 +556,8 @@ In memoriam, 1972-2018 Control '{0}.{1}' is being accessed from outside its parent form. {0} parent UserForm name; {1} control name + + Member '{0}' should be Private + {0} Member Identifier + \ No newline at end of file From 13435fca89f76f4fcb3e2ef84f1075cfba513d91 Mon Sep 17 00:00:00 2001 From: Brian Zenger Date: Mon, 17 Oct 2022 05:33:40 -0700 Subject: [PATCH 3/5] Initial inspection implementation --- ...ImplementationShouldBePrivateInspection.cs | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 Rubberduck.CodeAnalysis/Inspections/Concrete/PublicImplementationShouldBePrivateInspection.cs diff --git a/Rubberduck.CodeAnalysis/Inspections/Concrete/PublicImplementationShouldBePrivateInspection.cs b/Rubberduck.CodeAnalysis/Inspections/Concrete/PublicImplementationShouldBePrivateInspection.cs new file mode 100644 index 0000000000..bbb7c25926 --- /dev/null +++ b/Rubberduck.CodeAnalysis/Inspections/Concrete/PublicImplementationShouldBePrivateInspection.cs @@ -0,0 +1,95 @@ +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 +{ + 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 DoGetInspectionResults(DeclarationFinder finder) + { + var publicMembers = finder.UserDeclarations(DeclarationType.Member) + .Where(d => !d.HasPrivateAccessibility() + && IsLikeAnImplementerOrHandlerName(d.IdentifierName)); + + if (!publicMembers.Any()) + { + return Enumerable.Empty(); + } + + 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 FindDocumentEventHandlers(IEnumerable publicMembers) + { + //Excel and Word + var docEventPrefixes = new List() + { + "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(); + } + + } +} From 2f5fae7c71a8932f527bda21431aa3ca173ba198 Mon Sep 17 00:00:00 2001 From: Brian Zenger Date: Tue, 18 Oct 2022 11:04:31 -0700 Subject: [PATCH 4/5] Add xmldocs --- ...ImplementationShouldBePrivateInspection.cs | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/Rubberduck.CodeAnalysis/Inspections/Concrete/PublicImplementationShouldBePrivateInspection.cs b/Rubberduck.CodeAnalysis/Inspections/Concrete/PublicImplementationShouldBePrivateInspection.cs index bbb7c25926..69e5166b39 100644 --- a/Rubberduck.CodeAnalysis/Inspections/Concrete/PublicImplementationShouldBePrivateInspection.cs +++ b/Rubberduck.CodeAnalysis/Inspections/Concrete/PublicImplementationShouldBePrivateInspection.cs @@ -14,6 +14,66 @@ namespace Rubberduck.CodeAnalysis.Inspections.Concrete { + /// + /// Flags Interface implementation members and EventHandlers with Public scope. + /// + /// + /// 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'. + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// + internal sealed class PublicImplementationShouldBePrivateInspection : DeclarationInspectionBase { public PublicImplementationShouldBePrivateInspection(IDeclarationFinderProvider declarationFinderProvider) From 6451f99bd2055ef8d92e72b65b4d9a6227f2b11b Mon Sep 17 00:00:00 2001 From: Brian Zenger Date: Tue, 18 Oct 2022 11:26:28 -0700 Subject: [PATCH 5/5] Edit InspectionNames resource InspectionNames resource for PublicImplementationsShouldBePrivateInspection --- Rubberduck.Resources/Inspections/InspectionNames.Designer.cs | 2 +- Rubberduck.Resources/Inspections/InspectionNames.resx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Rubberduck.Resources/Inspections/InspectionNames.Designer.cs b/Rubberduck.Resources/Inspections/InspectionNames.Designer.cs index 821a1501d6..7b24516203 100644 --- a/Rubberduck.Resources/Inspections/InspectionNames.Designer.cs +++ b/Rubberduck.Resources/Inspections/InspectionNames.Designer.cs @@ -862,7 +862,7 @@ public class InspectionNames { } /// - /// Looks up a localized string similar to Public implementations of interfaces and event handlers should be Private. + /// Looks up a localized string similar to Implementations of interfaces and event handlers should be Private. /// public static string PublicImplementationShouldBePrivateInspection { get { diff --git a/Rubberduck.Resources/Inspections/InspectionNames.resx b/Rubberduck.Resources/Inspections/InspectionNames.resx index cb0f7a649f..794b939ab1 100644 --- a/Rubberduck.Resources/Inspections/InspectionNames.resx +++ b/Rubberduck.Resources/Inspections/InspectionNames.resx @@ -476,6 +476,6 @@ User Defined Type member is not used - Public implementations of interfaces and event handlers should be Private + Implementations of interfaces and event handlers should be Private \ No newline at end of file