Merge pull request #2431 from retailcoder/next

retailcoder · web-flow · commit 2cd87bf0b2b3 · 2016-12-16T14:03:09.000-05:00
fixed handling of recursive functions in FunctionReturnValueNotUsed a…
diff --git a/RetailCoder.VBE/Inspections/FunctionReturnValueNotUsedInspection.cs b/RetailCoder.VBE/Inspections/FunctionReturnValueNotUsedInspection.cs
@@ -38,7 +38,7 @@ public override IEnumerable<InspectionResultBase> GetInspectionResults()
 
         private IEnumerable<FunctionReturnValueNotUsedInspectionResult> GetInterfaceMemberIssues(IEnumerable<Declaration> interfaceMembers)
         {
-            return from interfaceMember in interfaceMembers
+            return (from interfaceMember in interfaceMembers
                    let implementationMembers =
                        UserDeclarations.FindInterfaceImplementationMembers(interfaceMember.IdentifierName).ToList()
                    where interfaceMember.DeclarationType == DeclarationType.Function &&
@@ -52,13 +52,14 @@ private IEnumerable<FunctionReturnValueNotUsedInspectionResult> GetInterfaceMemb
                                        implementationMember.Selection), implementationMember))
                    select
                        new FunctionReturnValueNotUsedInspectionResult(this, interfaceMember.Context,
-                           interfaceMember.QualifiedName, implementationMemberIssues, interfaceMember);
+                           interfaceMember.QualifiedName, implementationMemberIssues, interfaceMember)).ToList();
         }
 
         private IEnumerable<FunctionReturnValueNotUsedInspectionResult> GetNonInterfaceIssues(IEnumerable<Declaration> nonInterfaceFunctions)
         {
             var returnValueNotUsedFunctions = nonInterfaceFunctions.Where(function => function.DeclarationType == DeclarationType.Function && !IsReturnValueUsed(function));
             var nonInterfaceIssues = returnValueNotUsedFunctions
+                .Where(function => !IsRecursive(function))
                 .Select(function =>
                         new FunctionReturnValueNotUsedInspectionResult(
                             this,
@@ -68,14 +69,15 @@ private IEnumerable<FunctionReturnValueNotUsedInspectionResult> GetNonInterfaceI
             return nonInterfaceIssues;
         }
 
+        private bool IsRecursive(Declaration function)
+        {
+            return function.References.Any(usage => usage.ParentScoping.Equals(function) && IsIndexExprOrCallStmt(usage));
+        }
+
         private bool IsReturnValueUsed(Declaration function)
         {
             foreach (var usage in function.References)
             {
-                if (IsReturnStatement(function, usage))
-                {
-                    continue;
-                }
                 if (IsAddressOfCall(usage))
                 {
                     continue;
@@ -84,7 +86,7 @@ private bool IsReturnValueUsed(Declaration function)
                 {
                     continue;
                 }
-                if (IsCallStmt(usage))
+                if (IsCallStmt(usage)) // IsIndexExprOrCallStmt(usage))
                 {
                     continue;
                 }
@@ -96,6 +98,10 @@ private bool IsReturnValueUsed(Declaration function)
                 {
                     continue;
                 }
+                if (IsReturnStatement(function, usage))
+                {
+                    continue;
+                }
                 return true;
             }
             return false;
@@ -116,6 +122,11 @@ private bool IsReturnStatement(Declaration function, IdentifierReference assignm
             return assignment.ParentScoping.Equals(function) && assignment.Declaration.Equals(function);
         }
 
+        private bool IsIndexExprOrCallStmt(IdentifierReference usage)
+        {
+            return IsCallStmt(usage) || IsIndexExprContext(usage);
+        }
+
         private bool IsCallStmt(IdentifierReference usage)
         {
             var callStmt = ParserRuleContextHelper.GetParent<VBAParser.CallStmtContext>(usage.Context);
@@ -128,8 +139,22 @@ private bool IsCallStmt(IdentifierReference usage)
             {
                 return true;
             }
-            bool isUsedAsArgumentThusReturnValueIsUsed = ParserRuleContextHelper.HasParent(usage.Context, argumentList);
-            return !isUsedAsArgumentThusReturnValueIsUsed;
+            return !ParserRuleContextHelper.HasParent(usage.Context, argumentList);
+        }
+
+        private bool IsIndexExprContext(IdentifierReference usage)
+        {
+            var indexExpr = ParserRuleContextHelper.GetParent<VBAParser.IndexExprContext>(usage.Context);
+            if (indexExpr == null)
+            {
+                return false;
+            }
+            var argumentList = indexExpr.argumentList();
+            if (argumentList == null)
+            {
+                return true;
+            }
+            return !ParserRuleContextHelper.HasParent(usage.Context, argumentList);
         }
 
         private bool IsLet(IdentifierReference usage)
diff --git a/RetailCoder.VBE/Inspections/NonReturningFunctionInspection.cs b/RetailCoder.VBE/Inspections/NonReturningFunctionInspection.cs
@@ -49,8 +49,8 @@ public override IEnumerable<InspectionResultBase> GetInspectionResults()
 
             return unassigned
                 .Select(issue =>
-                    new NonReturningFunctionInspectionResult(this, new QualifiedContext<ParserRuleContext>(issue.QualifiedName, issue.Context),
-                            interfaceImplementationMembers.Select(m => m.Scope).Contains(issue.Scope), issue));
+                    new NonReturningFunctionInspectionResult(this, new QualifiedContext<ParserRuleContext>(issue.QualifiedName, issue.Context), issue, 
+                        canConvertToProcedure: !IsRecursive(issue) && !interfaceImplementationMembers.Select(m => m.Scope).Contains(issue.Scope)));
         }
 
         private bool IsReturningUserDefinedType(Declaration member)
@@ -70,6 +70,46 @@ private bool IsUserDefinedTypeAssigned(Declaration member)
             return result;
         }
 
+        private bool IsRecursive(Declaration function)
+        {
+            return function.References.Any(usage => usage.ParentScoping.Equals(function) && IsIndexExprOrCallStmt(usage));
+        }
+
+        private bool IsIndexExprOrCallStmt(IdentifierReference usage)
+        {
+            return IsCallStmt(usage) || IsIndexExprContext(usage);
+        }
+
+        private bool IsCallStmt(IdentifierReference usage)
+        {
+            var callStmt = ParserRuleContextHelper.GetParent<VBAParser.CallStmtContext>(usage.Context);
+            if (callStmt == null)
+            {
+                return false;
+            }
+            var argumentList = CallStatement.GetArgumentList(callStmt);
+            if (argumentList == null)
+            {
+                return true;
+            }
+            return !ParserRuleContextHelper.HasParent(usage.Context, argumentList);
+        }
+
+        private bool IsIndexExprContext(IdentifierReference usage)
+        {
+            var indexExpr = ParserRuleContextHelper.GetParent<VBAParser.IndexExprContext>(usage.Context);
+            if (indexExpr == null)
+            {
+                return false;
+            }
+            var argumentList = indexExpr.argumentList();
+            if (argumentList == null)
+            {
+                return true;
+            }
+            return !ParserRuleContextHelper.HasParent(usage.Context, argumentList);
+        }
+
         /// <summary>
         /// A visitor that visits a member's body and returns <c>true</c> if any <c>LET</c> statement (assignment) is assigning the specified <c>name</c>.
         /// </summary>
diff --git a/RetailCoder.VBE/Inspections/ProcedureNotUsedInspection.cs b/RetailCoder.VBE/Inspections/ProcedureNotUsedInspection.cs
@@ -77,7 +77,7 @@ private bool IsIgnoredDeclaration(IEnumerable<Declaration> declarations, Declara
         {
             var enumerable = classes as IList<Declaration> ?? classes.ToList();
             var result = !ProcedureTypes.Contains(declaration.DeclarationType)
-                || declaration.References.Any(r => !r.IsAssignment)
+                || declaration.References.Any(r => !r.IsAssignment && !r.ParentScoping.Equals(declaration)) // recursive calls don't count
                 || handlers.Contains(declaration)
                 || IsPublicModuleMember(modules, declaration)
                 || IsClassLifeCycleHandler(enumerable, declaration)
diff --git a/RetailCoder.VBE/Inspections/Resources/InspectionsUI.Designer.cs b/RetailCoder.VBE/Inspections/Resources/InspectionsUI.Designer.cs
diff --git a/RetailCoder.VBE/Inspections/Resources/InspectionsUI.fr.resx b/RetailCoder.VBE/Inspections/Resources/InspectionsUI.fr.resx
@@ -375,7 +375,7 @@
     <value>Le paramètre '{0}' passé par valeur est assigné</value>
   </data>
   <data name="FunctionReturnValueNotUsedInspectionMeta" xml:space="preserve">
-    <value>Un membre est implémenté comme une fonction, mais utilisé comme une procédure. Considérez convertir la fonction en procédure.</value>
+    <value>Un membre est implémenté comme une fonction, mais utilisé comme une procédure. À moins que la fonction soit récursive, considérez la convertir en procédure. Si la fonction est récursive, aucun appelant externe n'utilise sa valeur de retour.</value>
   </data>
   <data name="IdentifierNotUsedInspectionResultFormat" xml:space="preserve">
     <value>{0} '{1}' n'est pas utilisé(e).</value>
diff --git a/RetailCoder.VBE/Inspections/Resources/InspectionsUI.resx b/RetailCoder.VBE/Inspections/Resources/InspectionsUI.resx
@@ -379,7 +379,7 @@ If the parameter can be null, ignore this inspection result; passing a null valu
     <value>Parameter '{0}' is passed ByVal and assigned a value</value>
   </data>
   <data name="FunctionReturnValueNotUsedInspectionMeta" xml:space="preserve">
-    <value>A member is written as a function, but used as a procedure. Consider converting the 'Function' into a 'Sub'.</value>
+    <value>A member is written as a function, but used as a procedure. Unless the function is recursive, consider converting the 'Function' into a 'Sub'. If the function is recursive, none of its external callers are using the returned value.</value>
   </data>
   <data name="IdentifierNotUsedInspectionResultFormat" xml:space="preserve">
     <value>{0} '{1}' is not used</value>
diff --git a/RetailCoder.VBE/Inspections/Results/FunctionReturnValueNotUsedInspectionResult.cs b/RetailCoder.VBE/Inspections/Results/FunctionReturnValueNotUsedInspectionResult.cs
@@ -12,6 +12,8 @@
 
 namespace Rubberduck.Inspections.Results
 {
+
+
     public class FunctionReturnValueNotUsedInspectionResult : InspectionResultBase
     {
         private readonly IEnumerable<QuickFixBase> _quickFixes;
@@ -20,8 +22,9 @@ public FunctionReturnValueNotUsedInspectionResult(
             IInspection inspection,
             ParserRuleContext context,
             QualifiedMemberName qualifiedName,
-            Declaration target)
-            : this(inspection, context, qualifiedName, new List<Tuple<ParserRuleContext, QualifiedSelection, Declaration>>(), target)
+            Declaration target,
+            bool allowConvertToProcedure = true)
+            : this(inspection, context, qualifiedName, new List<Tuple<ParserRuleContext, QualifiedSelection, Declaration>>(), target, allowConvertToProcedure)
         {
         }
 
@@ -30,17 +33,25 @@ public FunctionReturnValueNotUsedInspectionResult(
             ParserRuleContext context,
             QualifiedMemberName qualifiedName,
             IEnumerable<Tuple<ParserRuleContext, QualifiedSelection, Declaration>> children,
-            Declaration target)
+            Declaration target, bool allowConvertToProcedure = true)
             : base(inspection, qualifiedName.QualifiedModuleName, context, target)
         {
-            var root = new ConvertToProcedureQuickFix(context, QualifiedSelection, target);
-            var compositeFix = new CompositeCodeInspectionFix(root);
-            children.ToList().ForEach(child => compositeFix.AddChild(new ConvertToProcedureQuickFix(child.Item1, child.Item2, child.Item3)));
-            _quickFixes = new QuickFixBase[]
+            var ignoreOnce = new IgnoreOnceQuickFix(Context, QualifiedSelection, Inspection.AnnotationName);
+            if (allowConvertToProcedure)
             {
-                compositeFix,
-                new IgnoreOnceQuickFix(Context, QualifiedSelection, Inspection.AnnotationName)
-            };
+                var root = new ConvertToProcedureQuickFix(context, QualifiedSelection, target);
+                var compositeFix = new CompositeCodeInspectionFix(root);
+                children.ToList().ForEach(child => compositeFix.AddChild(new ConvertToProcedureQuickFix(child.Item1, child.Item2, child.Item3)));
+                _quickFixes = new QuickFixBase[]
+                {
+                    compositeFix,
+                    ignoreOnce
+                };
+            }
+            else
+            {
+                _quickFixes = new[] {ignoreOnce};
+            }
         }
 
         public override IEnumerable<QuickFixBase> QuickFixes { get { return _quickFixes; } }
diff --git a/RetailCoder.VBE/Inspections/Results/NonReturningFunctionInspectionResult.cs b/RetailCoder.VBE/Inspections/Results/NonReturningFunctionInspectionResult.cs
@@ -16,18 +16,17 @@ public sealed class NonReturningFunctionInspectionResult : InspectionResultBase
 
         public NonReturningFunctionInspectionResult(IInspection inspection,
             QualifiedContext<ParserRuleContext> qualifiedContext, 
-            bool isInterfaceImplementation,
-            Declaration target)
+            Declaration target, bool canConvertToProcedure)
             : base(inspection, qualifiedContext.ModuleName, qualifiedContext.Context, target)
         {
-            _quickFixes = isInterfaceImplementation 
+            _quickFixes = canConvertToProcedure
                 ? new QuickFixBase[] 
                 {
+                    new ConvertToProcedureQuickFix(Context, QualifiedSelection, target),
                     new IgnoreOnceQuickFix(Context, QualifiedSelection, Inspection.AnnotationName),
                 }
                 : new QuickFixBase[]
                 {
-                    new ConvertToProcedureQuickFix(Context, QualifiedSelection, target),
                     new IgnoreOnceQuickFix(Context, QualifiedSelection, Inspection.AnnotationName), 
                 };
         }
diff --git a/RubberduckTests/Inspections/FunctionReturnValueNotUsedInspectionTests.cs b/RubberduckTests/Inspections/FunctionReturnValueNotUsedInspectionTests.cs
@@ -332,6 +332,35 @@ Public Sub Baz()
             Assert.AreEqual(0, inspectionResults.Count());
         }
 
+        [TestMethod]
+        public void FunctionReturnValueNotUsed_DoesNotReturnResult_RecursiveFunction()
+        {
+            const string inputCode =
+@"Public Function Factorial(ByVal n As Long) As Long
+    If n <= 1 Then
+        Factorial = 1
+    Else
+        Factorial = Factorial(n - 1) * n
+    End If
+End Function";
+
+            //Arrange
+            var builder = new MockVbeBuilder();
+            IVBComponent component;
+            var vbe = builder.BuildFromSingleStandardModule(inputCode, out component);
+            var mockHost = new Mock<IHostApplication>();
+            mockHost.SetupAllProperties();
+            var parser = MockParser.Create(vbe.Object, new RubberduckParserState(new Mock<ISinks>().Object));
+
+            parser.Parse(new CancellationTokenSource());
+            if (parser.State.Status >= ParserState.Error) { Assert.Inconclusive("Parser Error"); }
+
+            var inspection = new FunctionReturnValueNotUsedInspection(parser.State);
+            var inspectionResults = inspection.GetInspectionResults();
+
+            Assert.AreEqual(0, inspectionResults.Count());
+        }
+
         [TestMethod]
         public void FunctionReturnValueNotUsed_DoesNotReturnResult_ArgumentFunctionCall()
         {

Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,7 @@ public override IEnumerable<InspectionResultBase> GetInspectionResults()`
`38`	`38`
`39`	`39`	`private IEnumerable<FunctionReturnValueNotUsedInspectionResult> GetInterfaceMemberIssues(IEnumerable<Declaration> interfaceMembers)`
`40`	`40`	`{`
`41`		`- return from interfaceMember in interfaceMembers`
	`41`	`+ return (from interfaceMember in interfaceMembers`
`42`	`42`	`let implementationMembers =`
`43`	`43`	`UserDeclarations.FindInterfaceImplementationMembers(interfaceMember.IdentifierName).ToList()`
`44`	`44`	`where interfaceMember.DeclarationType == DeclarationType.Function &&`
`@@ -52,13 +52,14 @@ private IEnumerable<FunctionReturnValueNotUsedInspectionResult> GetInterfaceMemb`
`52`	`52`	`implementationMember.Selection), implementationMember))`
`53`	`53`	`select`
`54`	`54`	`new FunctionReturnValueNotUsedInspectionResult(this, interfaceMember.Context,`
`55`		`- interfaceMember.QualifiedName, implementationMemberIssues, interfaceMember);`
	`55`	`+ interfaceMember.QualifiedName, implementationMemberIssues, interfaceMember)).ToList();`
`56`	`56`	`}`
`57`	`57`
`58`	`58`	`private IEnumerable<FunctionReturnValueNotUsedInspectionResult> GetNonInterfaceIssues(IEnumerable<Declaration> nonInterfaceFunctions)`
`59`	`59`	`{`
`60`	`60`	`var returnValueNotUsedFunctions = nonInterfaceFunctions.Where(function => function.DeclarationType == DeclarationType.Function && !IsReturnValueUsed(function));`
`61`	`61`	`var nonInterfaceIssues = returnValueNotUsedFunctions`
	`62`	`+ .Where(function => !IsRecursive(function))`
`62`	`63`	`.Select(function =>`
`63`	`64`	`new FunctionReturnValueNotUsedInspectionResult(`
`64`	`65`	`this,`
`@@ -68,14 +69,15 @@ private IEnumerable<FunctionReturnValueNotUsedInspectionResult> GetNonInterfaceI`
`68`	`69`	`return nonInterfaceIssues;`
`69`	`70`	`}`
`70`	`71`
	`72`	`+ private bool IsRecursive(Declaration function)`
	`73`	`+ {`
	`74`	`+ return function.References.Any(usage => usage.ParentScoping.Equals(function) && IsIndexExprOrCallStmt(usage));`
	`75`	`+ }`
	`76`	`+`
`71`	`77`	`private bool IsReturnValueUsed(Declaration function)`
`72`	`78`	`{`
`73`	`79`	`foreach (var usage in function.References)`
`74`	`80`	`{`
`75`		`- if (IsReturnStatement(function, usage))`
`76`		`- {`
`77`		`- continue;`
`78`		`- }`
`79`	`81`	`if (IsAddressOfCall(usage))`
`80`	`82`	`{`
`81`	`83`	`continue;`
`@@ -84,7 +86,7 @@ private bool IsReturnValueUsed(Declaration function)`
`84`	`86`	`{`
`85`	`87`	`continue;`
`86`	`88`	`}`
`87`		`- if (IsCallStmt(usage))`
	`89`	`+ if (IsCallStmt(usage)) // IsIndexExprOrCallStmt(usage))`
`88`	`90`	`{`
`89`	`91`	`continue;`
`90`	`92`	`}`
`@@ -96,6 +98,10 @@ private bool IsReturnValueUsed(Declaration function)`
`96`	`98`	`{`
`97`	`99`	`continue;`
`98`	`100`	`}`
	`101`	`+ if (IsReturnStatement(function, usage))`
	`102`	`+ {`
	`103`	`+ continue;`
	`104`	`+ }`
`99`	`105`	`return true;`
`100`	`106`	`}`
`101`	`107`	`return false;`
`@@ -116,6 +122,11 @@ private bool IsReturnStatement(Declaration function, IdentifierReference assignm`
`116`	`122`	`return assignment.ParentScoping.Equals(function) && assignment.Declaration.Equals(function);`
`117`	`123`	`}`
`118`	`124`
	`125`	`+ private bool IsIndexExprOrCallStmt(IdentifierReference usage)`
	`126`	`+ {`
	`127`	`+ return IsCallStmt(usage) \|\| IsIndexExprContext(usage);`
	`128`	`+ }`
	`129`	`+`
`119`	`130`	`private bool IsCallStmt(IdentifierReference usage)`
`120`	`131`	`{`
`121`	`132`	`var callStmt = ParserRuleContextHelper.GetParent<VBAParser.CallStmtContext>(usage.Context);`
`@@ -128,8 +139,22 @@ private bool IsCallStmt(IdentifierReference usage)`
`128`	`139`	`{`
`129`	`140`	`return true;`
`130`	`141`	`}`
`131`		`- bool isUsedAsArgumentThusReturnValueIsUsed = ParserRuleContextHelper.HasParent(usage.Context, argumentList);`
`132`		`- return !isUsedAsArgumentThusReturnValueIsUsed;`
	`142`	`+ return !ParserRuleContextHelper.HasParent(usage.Context, argumentList);`
	`143`	`+ }`
	`144`	`+`
	`145`	`+ private bool IsIndexExprContext(IdentifierReference usage)`
	`146`	`+ {`
	`147`	`+ var indexExpr = ParserRuleContextHelper.GetParent<VBAParser.IndexExprContext>(usage.Context);`
	`148`	`+ if (indexExpr == null)`
	`149`	`+ {`
	`150`	`+ return false;`
	`151`	`+ }`
	`152`	`+ var argumentList = indexExpr.argumentList();`
	`153`	`+ if (argumentList == null)`
	`154`	`+ {`
	`155`	`+ return true;`
	`156`	`+ }`
	`157`	`+ return !ParserRuleContextHelper.HasParent(usage.Context, argumentList);`
`133`	`158`	`}`
`134`	`159`
`135`	`160`	`private bool IsLet(IdentifierReference usage)`
Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ private bool IsIgnoredDeclaration(IEnumerable<Declaration> declarations, Declara`
`77`	`77`	`{`
`78`	`78`	`var enumerable = classes as IList<Declaration> ?? classes.ToList();`
`79`	`79`	`var result = !ProcedureTypes.Contains(declaration.DeclarationType)`
`80`		`- \|\| declaration.References.Any(r => !r.IsAssignment)`
	`80`	`+ \|\| declaration.References.Any(r => !r.IsAssignment && !r.ParentScoping.Equals(declaration)) // recursive calls don't count`
`81`	`81`	`\|\| handlers.Contains(declaration)`
`82`	`82`	`\|\| IsPublicModuleMember(modules, declaration)`
`83`	`83`	`\|\| IsClassLifeCycleHandler(enumerable, declaration)`
Original file line number	Diff line number	Diff line change
`@@ -16,18 +16,17 @@ public sealed class NonReturningFunctionInspectionResult : InspectionResultBase`
`16`	`16`
`17`	`17`	`public NonReturningFunctionInspectionResult(IInspection inspection,`
`18`	`18`	`QualifiedContext<ParserRuleContext> qualifiedContext,`
`19`		`- bool isInterfaceImplementation,`
`20`		`- Declaration target)`
	`19`	`+ Declaration target, bool canConvertToProcedure)`
`21`	`20`	`: base(inspection, qualifiedContext.ModuleName, qualifiedContext.Context, target)`
`22`	`21`	`{`
`23`		`- _quickFixes = isInterfaceImplementation`
	`22`	`+ _quickFixes = canConvertToProcedure`
`24`	`23`	`? new QuickFixBase[]`
`25`	`24`	`{`
	`25`	`+ new ConvertToProcedureQuickFix(Context, QualifiedSelection, target),`
`26`	`26`	`new IgnoreOnceQuickFix(Context, QualifiedSelection, Inspection.AnnotationName),`
`27`	`27`	`}`
`28`	`28`	`: new QuickFixBase[]`
`29`	`29`	`{`
`30`		`- new ConvertToProcedureQuickFix(Context, QualifiedSelection, target),`
`31`	`30`	`new IgnoreOnceQuickFix(Context, QualifiedSelection, Inspection.AnnotationName),`
`32`	`31`	`};`
`33`	`32`	`}`