Add MisleadingByRefParameterInspection

Author: BZngr

Rubberduck.CodeAnalysis/Inspections/Concrete/MisleadingByRefParameterInspection.cs

@@ -0,0 +1,72 @@
+using Rubberduck.CodeAnalysis.Inspections.Abstract;
+using Rubberduck.Parsing.Symbols;
+using Rubberduck.Parsing.VBA;
+using Rubberduck.Parsing.VBA.DeclarationCaching;
+using Rubberduck.Resources.Inspections;
+using System.Linq;
+
+namespace Rubberduck.CodeAnalysis.Inspections.Concrete
+{
+    /// <summary>
+    /// Flags the value-parameter of a property mutators that are declared with an explict ByRef modifier.
+    /// </summary>
+    /// <why>
+    /// Regardless of the presence or absence of an explicit ByRef or ByVal modifier, the value-parameter
+    /// of a property mutator is always treated as though it had an explicit ByVal modifier.
+    /// Exception: UserDefinedType parameters are always passed by reference.
+    /// </why>
+    /// <example hasResult="true">
+    /// <module name="MyModule" type="Standard Module">
+    /// <![CDATA[
+    /// Private fizzField As Long
+    /// Public Property Get Fizz() As Long
+    ///     Fizz = fizzFiled
+    /// End Property
+    /// Public Property Let Fizz(ByRef arg As Long)
+    ///     fizzFiled = arg
+    /// End Property
+    /// ]]>
+    /// </module>
+    /// </example>
+    /// <example hasResult="false">
+    /// <module name="MyModule" type="Standard Module">
+    /// <![CDATA[
+    /// Private fizzField As Long
+    /// Public Property Get Fizz() As Long
+    ///     Fizz = fizzFiled
+    /// End Property
+    /// Public Property Let Fizz(arg As Long)
+    ///     fizzFiled = arg
+    /// End Property
+    /// ]]>
+    /// </module>
+    /// </example>
+    internal sealed class MisleadingByRefParameterInspection : DeclarationInspectionBase
+    {
+        public MisleadingByRefParameterInspection(IDeclarationFinderProvider declarationFinderProvider)
+            : base(declarationFinderProvider, DeclarationType.Parameter)
+        { }
+
+        protected override bool IsResultDeclaration(Declaration declaration, DeclarationFinder finder)
+        {
+            if ((declaration is ParameterDeclaration parameter)  
+                && parameter.ParentDeclaration is ModuleBodyElementDeclaration enclosingMethod
+                && (enclosingMethod.DeclarationType.HasFlag(DeclarationType.PropertyLet)
+                    || enclosingMethod.DeclarationType.HasFlag(DeclarationType.PropertySet))
+                && enclosingMethod.Parameters.Last() == parameter
+                && !(parameter.AsTypeDeclaration?.DeclarationType.HasFlag(DeclarationType.UserDefinedType) ?? false))
+            {
+                return parameter.IsByRef && !parameter.IsImplicitByRef;
+            }
+
+            return false;
+        }
+
+        protected override string ResultDescription(Declaration declaration)
+        {
+            return string.Format(
+                InspectionResults.MisleadingByRefParameterInspection,
+                declaration.IdentifierName);
+        }
+    }
+}

Rubberduck.CodeAnalysis/QuickFixes/Concrete/PassParameterByValueQuickFix.cs

@@ -38,7 +38,7 @@ internal sealed class PassParameterByValueQuickFix : QuickFixBase
         private readonly IDeclarationFinderProvider _declarationFinderProvider;
 
         public PassParameterByValueQuickFix(IDeclarationFinderProvider declarationFinderProvider)
-            : base(typeof(ParameterCanBeByValInspection))
+            : base(typeof(ParameterCanBeByValInspection), typeof(MisleadingByRefParameterInspection))
         {
             _declarationFinderProvider = declarationFinderProvider;
         }

Rubberduck.Resources/Inspections/InspectionInfo.Designer.cs

@@ -442,4 +442,7 @@ If the parameter can be null, ignore this inspection result; passing a null valu
   <data name="SuperfluousAnnotationArgumentInspection" xml:space="preserve">
     <value>An annotation has more arguments than allowed; superfluous arguments are ignored.</value>
   </data>
+  <data name="MisleadingByRefParameterInspection" xml:space="preserve">
+    <value>The last parameter (the 'Value' parameter) of property mutators are always passed by value.  This is true regardless of the presence or absence of a ByVal modifier.  Exception: UserDefinedType parameters must always be passed by reference in all cases.</value>
+  </data>
 </root>

Rubberduck.Resources/Inspections/InspectionInfo.resx

@@ -362,19 +362,15 @@
   </data>
   <data name="KeywordsUsedAsMemberInspection" xml:space="preserve">
     <value>Keyword used as member name</value>
-    
   </data>
   <data name="LineContinuationBetweenKeywordsInspection" xml:space="preserve">
     <value>Line continuation between keywords</value>
-    
   </data>
   <data name="NonBreakingSpaceIdentifierInspection" xml:space="preserve">
     <value>Identifier containing a non-breaking space</value>
-    
   </data>
   <data name="NegativeLineNumberInspection" xml:space="preserve">
     <value>Negative line number</value>
-    
   </data>
   <data name="OnErrorGoToMinusOneInspection" xml:space="preserve">
     <value>OnErrorGoto -1</value>
@@ -446,4 +442,7 @@
   <data name="SuperfluousAnnotationArgumentInspection" xml:space="preserve">
     <value>Superfluous annotation arguments</value>
   </data>
+  <data name="MisleadingByRefParameterInspection" xml:space="preserve">
+    <value>ByRef is a misleading modifier because the parameter will be passed ByVal.</value>
+  </data>
 </root>

Rubberduck.Resources/Inspections/InspectionNames.Designer.cs

@@ -513,4 +513,8 @@ In memoriam, 1972-2018</value>
     <value>The annotation '{0}' was expected to have less arguments.</value>
     <comment>{0} annotation name</comment>
   </data>
+  <data name="MisleadingByRefParameterInspection" xml:space="preserve">
+    <value>Misleading ByRef modifier used for Parameter '{0}'.</value>
+    <comment>{0} Parameter name</comment>
+  </data>
 </root>

Rubberduck.Resources/Inspections/InspectionNames.resx

@@ -0,0 +1,72 @@
+using NUnit.Framework;
+using Rubberduck.CodeAnalysis.Inspections;
+using Rubberduck.CodeAnalysis.Inspections.Concrete;
+using Rubberduck.Parsing.VBA;
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+
+namespace RubberduckTests.Inspections
+{
+    [TestFixture]
+    public class MisleadingByRefParameterInspectionTests : InspectionTestsBase
+    {
+        [TestCase("Property Let Fizz(ByRef arg1 As Integer)\r\nEnd Property", 1)]
+        [TestCase("Property Let Fizz(arg1 As Integer)\r\nEnd Property", 0)]
+        [TestCase("Property Let Fizz(ByVal arg1 As Integer)\r\nEnd Property", 0)]
+        [TestCase("Property Set Fizz(ByRef arg1 As Object)\r\nEnd Property", 1)]
+        [TestCase("Property Set Fizz(arg1 As Object)\r\nEnd Property", 0)]
+        [TestCase("Property Set Fizz(ByVal arg1 As Object)\r\nEnd Property", 0)]
+        [Category("QuickFixes")]
+        [Category(nameof(MisleadingByRefParameterInspection))]
+        public void AllParamMechanisms(string inputCode, int expectedCount)
+        {
+            Assert.AreEqual(expectedCount, InspectionResultsForStandardModule(inputCode).Count());
+        }
+
+        [TestCase("arg")]
+        [TestCase("ByRef arg")]
+        [Category("QuickFixes")]
+        [Category(nameof(MisleadingByRefParameterInspection))]
+        public void UserDefinedTypeEdgeCase(string parameterMechanismAndParam)
+        {
+            var inputCode =
+$@"
+Option Explicit
+
+Public Type TestType
+    FirstValue As Long
+End Type
+
+Private this As TestType
+
+Public Property Get UserDefinedType() As TestType
+    UserDefinedType = this
+End Property
+
+Public Property Let UserDefinedType({parameterMechanismAndParam} As TestType)
+    this = arg
+End Property
+";
+
+            Assert.AreEqual(0, InspectionResultsForStandardModule(inputCode).Count());
+        }
+
+        [Test]
+        [Category("QuickFixes")]
+        [Category(nameof(MisleadingByRefParameterInspection))]
+        public void InspectionName()
+        {
+            var inspection = new MisleadingByRefParameterInspection(null);
+
+            Assert.AreEqual(nameof(MisleadingByRefParameterInspection), inspection.Name);
+        }
+
+        protected override IInspection InspectionUnderTest(RubberduckParserState state)
+        {
+            return new MisleadingByRefParameterInspection(state);
+        }
+    }
+}

Rubberduck.Resources/Inspections/InspectionResults.Designer.cs

@@ -296,6 +296,28 @@ Debug.Print foo
             Assert.AreEqual(expectedCode, actualCode);
         }
 
+        [Test]
+        [Category("QuickFixes")]
+        [Category(nameof(MisleadingByRefParameterInspection))]
+        public void CorrectsMisleadingByRefPropertyMutatorParameter()
+        {
+            const string inputCode =
+@"
+Option Explicit
+
+Private fizzField As Long
+
+Public Property Get Fizz() As Long
+    Fizz = fizzField
+End Property
+
+Public Property Let Fizz(ByRef arg As Long)
+    fizzField = arg
+End Property
+";
+            var actualCode = ApplyQuickFixToFirstInspectionResult(inputCode, state => new MisleadingByRefParameterInspection(state));
+            StringAssert.Contains("Public Property Let Fizz(ByVal arg As Long)", actualCode);
+        }
 
         protected override IQuickFix QuickFix(RubberduckParserState state)
         {