Skip to content

Commit 9b3bd2d

Browse files
retailcoderretailcoder
authored
Merge pull request #5338 from MDoerner/FixFunctionReturnValueNotUsedInspection
Fix function return value not used inspection
2 parents 60f6985 + 681fc3d commit 9b3bd2d

32 files changed

+1241
-428
lines changed

‎Rubberduck.CodeAnalysis/Inspections/Abstract/DeclarationInspectionBase.cs‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
using System.Collections.Generic;
2+
using System.Linq;
3+
using Rubberduck.Inspections.Results;
4+
using Rubberduck.Parsing.Inspections.Abstract;
5+
using Rubberduck.Parsing.Symbols;
6+
using Rubberduck.Parsing.VBA;
7+
using Rubberduck.VBEditor;
8+
9+
namespace Rubberduck.Inspections.Abstract
10+
{
11+
public abstract class DeclarationInspectionBase : InspectionBase
12+
{
13+
protected readonly IDeclarationFinderProvider DeclarationFinderProvider;
14+
protected readonly DeclarationType[] RelevantDeclarationTypes;
15+
16+
protected DeclarationInspectionBase(RubberduckParserState state, params DeclarationType[] relevantDeclarationTypes)
17+
: base(state)
18+
{
19+
DeclarationFinderProvider = state;
20+
RelevantDeclarationTypes = relevantDeclarationTypes;
21+
}
22+
23+
protected abstract bool IsResultDeclaration(Declaration declaration);
24+
protected abstract string ResultDescription(Declaration declaration);
25+
26+
protected override IEnumerable<IInspectionResult> DoGetInspectionResults()
27+
{
28+
var results = new List<IInspectionResult>();
29+
foreach (var moduleDeclaration in State.DeclarationFinder.UserDeclarations(DeclarationType.Module))
30+
{
31+
if (moduleDeclaration == null)
32+
{
33+
continue;
34+
}
35+
36+
var module = moduleDeclaration.QualifiedModuleName;
37+
results.AddRange(DoGetInspectionResults(module));
38+
}
39+
40+
return results;
41+
}
42+
43+
private IEnumerable<IInspectionResult> DoGetInspectionResults(QualifiedModuleName module)
44+
{
45+
var objectionableDeclarations = RelevantDeclarationsInModule(module)
46+
.Where(IsResultDeclaration);
47+
48+
return objectionableDeclarations
49+
.Select(InspectionResult)
50+
.ToList();
51+
}
52+
53+
protected virtual IEnumerable<Declaration> RelevantDeclarationsInModule(QualifiedModuleName module)
54+
{
55+
return RelevantDeclarationTypes
56+
.SelectMany(declarationType => DeclarationFinderProvider.DeclarationFinder.Members(module, declarationType))
57+
.Distinct();
58+
}
59+
60+
protected virtual IInspectionResult InspectionResult(Declaration declaration)
61+
{
62+
return new DeclarationInspectionResult(
63+
this,
64+
ResultDescription(declaration),
65+
declaration);
66+
}
67+
}
68+
}

‎Rubberduck.CodeAnalysis/Inspections/Abstract/IdentifierReferenceInspectionBase.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
using System.Collections.Generic;
22
using System.Linq;
3-
using Rubberduck.Inspections.Inspections.Extensions;
43
using Rubberduck.Inspections.Results;
54
using Rubberduck.Parsing.Inspections.Abstract;
65
using Rubberduck.Parsing.Symbols;
@@ -13,7 +12,7 @@ public abstract class IdentifierReferenceInspectionBase : InspectionBase
1312
{
1413
protected readonly IDeclarationFinderProvider DeclarationFinderProvider;
1514

16-
public IdentifierReferenceInspectionBase(RubberduckParserState state)
15+
protected IdentifierReferenceInspectionBase(RubberduckParserState state)
1716
: base(state)
1817
{
1918
DeclarationFinderProvider = state;

‎Rubberduck.CodeAnalysis/Inspections/Concrete/FunctionReturnValueAlwaysDiscardedInspection.cs‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
using System.Collections.Generic;
2+
using System.Linq;
3+
using Antlr4.Runtime;
4+
using Rubberduck.Inspections.Abstract;
5+
using Rubberduck.JunkDrawer.Extensions;
6+
using Rubberduck.Parsing;
7+
using Rubberduck.Parsing.Grammar;
8+
using Rubberduck.Resources.Inspections;
9+
using Rubberduck.Parsing.Symbols;
10+
using Rubberduck.Parsing.VBA;
11+
using Rubberduck.Parsing.VBA.DeclarationCaching;
12+
13+
namespace Rubberduck.Inspections.Concrete
14+
{
15+
/// <summary>
16+
/// Warns when a user function's return value is discarded at all its call sites.
17+
/// </summary>
18+
/// <why>
19+
/// A 'Function' procedure normally means its return value to be captured and consumed by the calling code.
20+
/// It's possible that not all call sites need the return value, but if the value is systematically discarded then this
21+
/// means the function is side-effecting, and thus should probably be a 'Sub' procedure instead.
22+
/// </why>
23+
/// <example hasResults="true">
24+
/// <![CDATA[
25+
/// Public Sub DoSomething()
26+
/// GetFoo ' return value is not captured
27+
/// End Sub
28+
///
29+
/// Private Function GetFoo() As Long
30+
/// GetFoo = 42
31+
/// End Function
32+
/// ]]>
33+
/// </example>
34+
/// <example hasResults="false">
35+
/// <![CDATA[
36+
/// Public Sub DoSomething()
37+
/// GetFoo ' return value is discarded
38+
/// End Sub
39+
///
40+
/// Public Sub DoSomethingElse()
41+
/// Dim foo As Long
42+
/// foo = GetFoo ' return value is captured
43+
/// End Sub
44+
///
45+
/// Private Function GetFoo() As Long
46+
/// GetFoo = 42
47+
/// End Function
48+
/// ]]>
49+
/// </example>
50+
public sealed class FunctionReturnValueAlwaysDiscardedInspection : DeclarationInspectionBase
51+
{
52+
public FunctionReturnValueAlwaysDiscardedInspection(RubberduckParserState state)
53+
:base(state, DeclarationType.Function)
54+
{}
55+
56+
protected override bool IsResultDeclaration(Declaration declaration)
57+
{
58+
if (!(declaration is ModuleBodyElementDeclaration moduleBodyElementDeclaration))
59+
{
60+
return false;
61+
}
62+
63+
//We only report the interface itself.
64+
if (moduleBodyElementDeclaration.IsInterfaceImplementation)
65+
{
66+
return false;
67+
}
68+
69+
var finder = DeclarationFinderProvider.DeclarationFinder;
70+
71+
if (moduleBodyElementDeclaration.IsInterfaceMember)
72+
{
73+
return IsInterfaceIssue(moduleBodyElementDeclaration, finder);
74+
}
75+
76+
return IsIssueItself(moduleBodyElementDeclaration);
77+
}
78+
79+
private bool IsIssueItself(ModuleBodyElementDeclaration declaration)
80+
{
81+
var procedureCallReferences = ProcedureCallReferences(declaration).ToHashSet();
82+
if (!procedureCallReferences.Any())
83+
{
84+
return false;
85+
}
86+
87+
return declaration.References
88+
.All(reference => procedureCallReferences.Contains(reference)
89+
|| reference.IsAssignment && IsReturnStatement(declaration, reference));
90+
}
91+
92+
private bool IsReturnStatement(Declaration function, IdentifierReference assignment)
93+
{
94+
return assignment.ParentScoping.Equals(function) && assignment.Declaration.Equals(function);
95+
}
96+
97+
private bool IsInterfaceIssue(ModuleBodyElementDeclaration declaration, DeclarationFinder finder)
98+
{
99+
if (!IsIssueItself(declaration))
100+
{
101+
return false;
102+
}
103+
104+
var implementations = finder.FindInterfaceImplementationMembers(declaration);
105+
return implementations.All(implementation => IsIssueItself(implementation)
106+
|| implementation.References.All(reference =>
107+
reference.IsAssignment
108+
&& IsReturnStatement(implementation, reference)));
109+
}
110+
111+
private static IEnumerable<IdentifierReference> ProcedureCallReferences(Declaration declaration)
112+
{
113+
return declaration.References
114+
.Where(IsProcedureCallReference);
115+
}
116+
117+
private static bool IsProcedureCallReference(IdentifierReference reference)
118+
{
119+
return reference?.Declaration != null
120+
&& !reference.IsAssignment
121+
&& !reference.IsArrayAccess
122+
&& !reference.IsInnerRecursiveDefaultMemberAccess
123+
&& IsCalledAsProcedure(reference.Context);
124+
}
125+
126+
private static bool IsCalledAsProcedure(ParserRuleContext context)
127+
{
128+
var callStmt = context.GetAncestor<VBAParser.CallStmtContext>();
129+
if (callStmt == null)
130+
{
131+
return false;
132+
}
133+
134+
//If we are in an argument list, the value is used somewhere in defining the argument.
135+
var argumentListParent = context.GetAncestor<VBAParser.ArgumentListContext>();
136+
if (argumentListParent != null)
137+
{
138+
return false;
139+
}
140+
141+
//Member accesses are parsed right-to-left, e.g. 'foo.Bar' is the parent of 'foo'.
142+
//Thus, having a member access parent means that the return value is used somehow.
143+
var ownFunctionCallExpression = context.Parent is VBAParser.MemberAccessExprContext methodCall
144+
? methodCall
145+
: context;
146+
var memberAccessParent = ownFunctionCallExpression.GetAncestor<VBAParser.MemberAccessExprContext>();
147+
return memberAccessParent == null;
148+
}
149+
150+
protected override string ResultDescription(Declaration declaration)
151+
{
152+
var functionName = declaration.QualifiedName.ToString();
153+
return string.Format(InspectionResults.FunctionReturnValueAlwaysDiscardedInspection, functionName);
154+
}
155+
}
156+
}

‎Rubberduck.CodeAnalysis/Inspections/Concrete/FunctionReturnValueDiscardedInspection.cs‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
using Antlr4.Runtime;
2+
using Rubberduck.Inspections.Abstract;
3+
using Rubberduck.Parsing;
4+
using Rubberduck.Parsing.Grammar;
5+
using Rubberduck.Parsing.Inspections;
6+
using Rubberduck.Resources.Inspections;
7+
using Rubberduck.Parsing.Symbols;
8+
using Rubberduck.Parsing.VBA;
9+
10+
namespace Rubberduck.Inspections.Concrete
11+
{
12+
/// <summary>
13+
/// Warns when a user function's return value is not used at a call site.
14+
/// </summary>
15+
/// <why>
16+
/// A 'Function' procedure normally means its return value to be captured and consumed by the calling code.
17+
/// </why>
18+
/// <example hasResults="true">
19+
/// <![CDATA[
20+
/// Public Sub DoSomething()
21+
/// GetFoo ' return value is not captured
22+
/// End Sub
23+
///
24+
/// Private Function GetFoo() As Long
25+
/// GetFoo = 42
26+
/// End Function
27+
/// ]]>
28+
/// </example>
29+
/// <example hasResults="false">
30+
/// <![CDATA[
31+
/// Public Sub DoSomething()
32+
/// Dim foo As Long
33+
/// foo = GetFoo
34+
/// End Sub
35+
///
36+
/// Private Function GetFoo() As Long
37+
/// GetFoo = 42
38+
/// End Function
39+
/// ]]>
40+
/// </example>
41+
public sealed class FunctionReturnValueDiscardedInspection : IdentifierReferenceInspectionBase
42+
{
43+
public FunctionReturnValueDiscardedInspection(RubberduckParserState state)
44+
: base(state)
45+
{
46+
Severity = CodeInspectionSeverity.Suggestion;
47+
}
48+
49+
protected override bool IsResultReference(IdentifierReference reference)
50+
{
51+
return reference?.Declaration != null
52+
&& !reference.IsAssignment
53+
&& !reference.IsArrayAccess
54+
&& !reference.IsInnerRecursiveDefaultMemberAccess
55+
&& reference.Declaration.DeclarationType == DeclarationType.Function
56+
&& IsCalledAsProcedure(reference.Context);
57+
}
58+
59+
private static bool IsCalledAsProcedure(ParserRuleContext context)
60+
{
61+
var callStmt = context.GetAncestor<VBAParser.CallStmtContext>();
62+
if (callStmt == null)
63+
{
64+
return false;
65+
}
66+
67+
//If we are in an argument list, the value is used somewhere in defining the argument.
68+
var argumentListParent = context.GetAncestor<VBAParser.ArgumentListContext>();
69+
if (argumentListParent != null)
70+
{
71+
return false;
72+
}
73+
74+
//Member accesses are parsed right-to-left, e.g. 'foo.Bar' is the parent of 'foo'.
75+
//Thus, having a member access parent means that the return value is used somehow.
76+
var ownFunctionCallExpression = context.Parent is VBAParser.MemberAccessExprContext methodCall
77+
? methodCall
78+
: context;
79+
var memberAccessParent = ownFunctionCallExpression.GetAncestor<VBAParser.MemberAccessExprContext>();
80+
if (memberAccessParent != null)
81+
{
82+
return false;
83+
}
84+
85+
return true;
86+
}
87+
88+
protected override string ResultDescription(IdentifierReference reference)
89+
{
90+
var functionName = reference.Declaration.QualifiedName.ToString();
91+
return string.Format(InspectionResults.FunctionReturnValueDiscardedInspection, functionName);
92+
}
93+
}
94+
}

0 commit comments

Comments
 (0)