Ignore Property mutator Value param

Author: BZngr

Rubberduck.CodeAnalysis/Inspections/Concrete/ImplicitByRefModifierInspection.cs

@@ -3,15 +3,19 @@
 using Rubberduck.Parsing.VBA;
 using Rubberduck.Parsing.VBA.DeclarationCaching;
 using Rubberduck.Resources.Inspections;
+using System.Linq;
 
 namespace Rubberduck.CodeAnalysis.Inspections.Concrete
 {
     /// <summary>
     /// Highlights implicit ByRef modifiers in user code.
     /// </summary>
     /// <why>
-    /// In modern VB (VB.NET), the implicit modifier is ByVal, as it is in most other programming languages.
-    /// Making the ByRef modifiers explicit can help surface potentially unexpected language defaults.
+    /// VBA parameters are implicitly ByRef, which differs from modern VB (VB.NET) and most other programming languages which are implicitly ByVal.
+    /// So, explicitly identifing VBA parameter mechanisms (the ByRef and ByVal modifiers) can help surface potentially unexpected language results.
+    /// The inspection does not flag an implicit parameter mechanism for the last parameter of Property mutators (Let or Set).
+    /// VBA applies a ByVal parameter mechanism to the last parameter in the absence (or presence!) of a modifier. 
+    /// Exception: UserDefinedType parameters must always be passed as ByRef.
     /// </why>
     /// <example hasResult="true">
     /// <module name="MyModule" type="Standard Module">
@@ -31,6 +35,16 @@ namespace Rubberduck.CodeAnalysis.Inspections.Concrete
     /// ]]>
     /// </module>
     /// </example>
+    /// <example hasResult="false">
+    /// <module name="MyModule" type="Standard Module">
+    /// <![CDATA[
+    /// Private theLength As Long
+    /// Public Property Let Length(newLength As Long)
+    ///     theLength = newLength
+    /// End Sub
+    /// ]]>
+    /// </module>
+    /// </example>
     internal sealed class ImplicitByRefModifierInspection : DeclarationInspectionBase
     {
         public ImplicitByRefModifierInspection(IDeclarationFinderProvider declarationFinderProvider)
@@ -41,21 +55,23 @@ protected override bool IsResultDeclaration(Declaration declaration, Declaration
         {
             if (!(declaration is ParameterDeclaration parameter)
                 || !parameter.IsImplicitByRef
-                || parameter.IsParamArray)
+                || parameter.IsParamArray
+                //Exclude parameters of Declare statements
+                || !(parameter.ParentDeclaration is ModuleBodyElementDeclaration enclosingMethod))
             {
                 return false;
             }
 
-            var parentDeclaration = parameter.ParentDeclaration;
-
-            if (parentDeclaration is ModuleBodyElementDeclaration enclosingMethod)
-            {
-                return !enclosingMethod.IsInterfaceImplementation
-                       && !finder.FindEventHandlers().Contains(enclosingMethod);
-            }
+            return !IsPropertyMutatorRHSParameter(enclosingMethod, parameter)
+                    && !enclosingMethod.IsInterfaceImplementation
+                    && !finder.FindEventHandlers().Contains(enclosingMethod);
+        }
 
-            return parentDeclaration.DeclarationType != DeclarationType.LibraryFunction
-                   && parentDeclaration.DeclarationType != DeclarationType.LibraryProcedure;
+        private bool IsPropertyMutatorRHSParameter(ModuleBodyElementDeclaration enclosingMethod, ParameterDeclaration implicitByRefParameter)
+        {
+            return (enclosingMethod.DeclarationType.HasFlag(DeclarationType.PropertyLet)
+                    || enclosingMethod.DeclarationType.HasFlag(DeclarationType.PropertySet)) 
+                && enclosingMethod.Parameters.Last().Equals(implicitByRefParameter);
         }
 
         protected override string ResultDescription(Declaration declaration)

RubberduckTests/Inspections/ImplicitByRefModifierInspectionTests.cs

@@ -17,13 +17,26 @@ public class ImplicitByRefModifierInspectionTests : InspectionTestsBase
         [TestCase("Sub Foo(arg1 As Integer, ByRef arg2 As Date)\r\nEnd Sub", 1)]
         [TestCase("Sub Foo(ParamArray arg1 As Integer)\r\nEnd Sub", 0)]
         [Category("QuickFixes")]
+        [Category(nameof(ImplicitByRefModifierInspection))]
         public void ImplicitByRefModifier_SimpleScenarios(string inputCode, int expectedCount)
         {
             Assert.AreEqual(expectedCount, InspectionResultsForStandardModule(inputCode).Count());
         }
 
+        [TestCase("Property Let Fizz(RHS As Integer)\r\nEnd Property", 0)]
+        [TestCase("Property Set Fizz(RHS As Object)\r\nEnd Property", 0)]
+        [TestCase("Property Let Fizz(index As Integer, RHS As Integer)\r\nEnd Property", 1)]
+        [TestCase("Property Set Fizz(index As Integer, RHS As Object)\r\nEnd Property", 1)]
+        [Category("QuickFixes")]
+        [Category(nameof(ImplicitByRefModifierInspection))]
+        public void ImplicitByRefModifier_PropertyMutatorRHSParameter(string inputCode, int expectedCount)
+        {
+            Assert.AreEqual(expectedCount, InspectionResultsForStandardModule(inputCode).Count());
+        }
+
         [Test]
         [Category("QuickFixes")]
+        [Category(nameof(ImplicitByRefModifierInspection))]
         public void ImplicitByRefModifier_ReturnsResult_InterfaceImplementation()
         {
             const string inputCode1 =
@@ -47,6 +60,7 @@ Sub IClass1_Foo(arg1 As Integer)
 
         [Test]
         [Category("QuickFixes")]
+        [Category(nameof(ImplicitByRefModifierInspection))]
         public void ImplicitByRefModifier_ReturnsResult_MultipleInterfaceImplementations()
         {
             const string inputCode1 =
@@ -77,6 +91,7 @@ Sub IClass1_Foo(arg1 As Integer)
 
         [Test]
         [Category("QuickFixes")]
+        [Category(nameof(ImplicitByRefModifierInspection))]
         public void ImplicitByRefModifier_Ignored_DoesNotReturnResult()
         {
             const string inputCode =
@@ -86,8 +101,30 @@ Sub Foo(arg1 As Integer)
             Assert.AreEqual(0, InspectionResultsForStandardModule(inputCode).Count());
         }
 
+        [TestCase(@"Public Declare PtrSafe Sub LibProcedure Lib ""MyLib""(arg As Long)", "LibProcedure 2000")]
+        [TestCase(@"Public Declare PtrSafe Function LibProcedure Lib ""MyLib""(arg As Long) As Long", "test = LibProcedure(2000)")]
+        [Category("QuickFixes")]
+        [Category(nameof(ImplicitByRefModifierInspection))]
+        public void ImplicitByRefModifier_IgnoresDeclareStatement(string declareStatement, string libraryCall)
+        {
+            var inputCode =
+$@"
+Option Explicit
+
+Private test As Long
+
+{declareStatement}
+
+Public Sub CallTheLib()
+    {libraryCall}
+End Sub";
+
+            Assert.AreEqual(0, InspectionResultsForStandardModule(inputCode).Count());
+        }
+
         [Test]
         [Category("QuickFixes")]
+        [Category(nameof(ImplicitByRefModifierInspection))]
         public void InspectionName()
         {
             var inspection = new ImplicitByRefModifierInspection(null);