Skip to content

Commit c4e0fd8

Browse files
MDoernerMDoerner
committed
Turn ProcedureCanBeWrittenAsFunctionInspection into a declaration inspection
1 parent 15d6320 commit c4e0fd8

File tree

4 files changed

+228
-58
lines changed

4 files changed

+228
-58
lines changed

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

Lines changed: 62 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,11 @@
1-
using System.Collections.Generic;
2-
using System.Linq;
3-
using Antlr4.Runtime;
1+
using System.Linq;
42
using Rubberduck.Inspections.Abstract;
5-
using Rubberduck.Inspections.Results;
63
using Rubberduck.Parsing;
74
using Rubberduck.Parsing.Grammar;
8-
using Rubberduck.Parsing.Inspections.Abstract;
95
using Rubberduck.Resources.Inspections;
106
using Rubberduck.Parsing.Symbols;
117
using Rubberduck.Parsing.VBA;
12-
using Rubberduck.Parsing.VBA.Parsing;
8+
using Rubberduck.Parsing.VBA.DeclarationCaching;
139

1410
namespace Rubberduck.Inspections.Concrete
1511
{
@@ -39,75 +35,85 @@ namespace Rubberduck.Inspections.Concrete
3935
/// End Function
4036
/// ]]>
4137
/// </example>
42-
public sealed class ProcedureCanBeWrittenAsFunctionInspection : InspectionBase, IParseTreeInspection
38+
/// <example hasResults="false">
39+
/// <![CDATA[
40+
/// Option Explicit
41+
/// Public Function DoSomething(ByVal arg As Long) As Long
42+
/// ' ...
43+
/// DoSomething = 42
44+
/// End Function
45+
/// ]]>
46+
/// </example>
47+
public sealed class ProcedureCanBeWrittenAsFunctionInspection : DeclarationInspectionBase
4348
{
4449
public ProcedureCanBeWrittenAsFunctionInspection(IDeclarationFinderProvider declarationFinderProvider)
45-
: base(declarationFinderProvider)
46-
{
47-
Listener = new SingleByRefParamArgListListener();
48-
}
50+
: base(declarationFinderProvider, new []{DeclarationType.Procedure}, new []{DeclarationType.LibraryProcedure, DeclarationType.PropertyLet, DeclarationType.PropertySet})
51+
{}
4952

50-
public CodeKind TargetKindOfCode => CodeKind.CodePaneCode;
51-
public IInspectionListener Listener { get; }
52-
53-
//FIXME This should really be a declaration inspection.
54-
55-
protected override IEnumerable<IInspectionResult> DoGetInspectionResults()
53+
protected override bool IsResultDeclaration(Declaration declaration, DeclarationFinder finder)
5654
{
57-
if (!Listener.Contexts().Any())
55+
if (!(declaration is ModuleBodyElementDeclaration member)
56+
|| member.IsInterfaceImplementation
57+
|| member.IsInterfaceMember
58+
|| finder.FindEventHandlers().Contains(member)
59+
|| member.Parameters.Count(param => param.IsByRef && !param.IsParamArray) != 1)
5860
{
59-
return Enumerable.Empty<IInspectionResult>();
61+
return false;
6062
}
6163

62-
var finder = DeclarationFinderProvider.DeclarationFinder;
63-
64-
var userDeclarations = finder.AllUserDeclarations.ToList();
65-
var builtinHandlers = finder.FindEventHandlers().ToList();
64+
var parameter = member.Parameters.First(param => param.IsByRef && !param.IsParamArray);
65+
var parameterReferences = parameter.References.ToList();
6666

67-
var contextLookup = userDeclarations.Where(decl => decl.Context != null).ToDictionary(decl => decl.Context);
67+
return parameterReferences.Any(reference => IsAssignment(reference, finder));
68+
}
6869

69-
var ignored = new HashSet<Declaration>(finder.FindAllInterfaceMembers()
70-
.Concat(finder.FindAllInterfaceImplementingMembers())
71-
.Concat(builtinHandlers)
72-
.Concat(userDeclarations.Where(item => item.IsWithEvents)));
70+
private static bool IsAssignment(IdentifierReference reference, DeclarationFinder finder)
71+
{
72+
return reference.IsAssignment
73+
|| IsUsageAsAssignedToByRefArgument(reference, finder);
74+
}
7375

74-
return Listener.Contexts()
75-
.Where(context => context.Context.Parent is VBAParser.SubStmtContext
76-
&& HasArgumentReferencesWithIsAssignmentFlagged(context))
77-
.Select(GetSubStmtParentDeclaration)
78-
.Where(decl => decl != null &&
79-
!ignored.Contains(decl) &&
80-
userDeclarations.Where(item => item.IsWithEvents)
81-
.All(withEvents => !finder.FindHandlersForWithEventsField(withEvents).Any()) &&
82-
!builtinHandlers.Contains(decl))
83-
.Select(result => new DeclarationInspectionResult(this,
84-
string.Format(InspectionResults.ProcedureCanBeWrittenAsFunctionInspection, result.IdentifierName),
85-
result));
76+
private static bool IsUsageAsAssignedToByRefArgument(IdentifierReference reference, DeclarationFinder finder)
77+
{
78+
var argExpression = ImmediateArgumentExpressionContext(reference);
8679

87-
bool HasArgumentReferencesWithIsAssignmentFlagged(QualifiedContext<ParserRuleContext> context)
80+
if (argExpression == null)
8881
{
89-
return contextLookup.TryGetValue(context.Context.GetChild<VBAParser.ArgContext>(), out Declaration decl)
90-
&& decl.References.Any(rf => rf.IsAssignment);
82+
return false;
9183
}
9284

93-
Declaration GetSubStmtParentDeclaration(QualifiedContext<ParserRuleContext> context)
85+
var parameter = finder.FindParameterOfNonDefaultMemberFromSimpleArgumentNotPassedByValExplicitly(argExpression, reference.QualifiedModuleName);
86+
87+
if (parameter == null)
9488
{
95-
return contextLookup.TryGetValue(context.Context.Parent as VBAParser.SubStmtContext, out Declaration decl)
96-
? decl
97-
: null;
89+
//We have no idea what parameter it is passed to as argument. So, we have to err on the safe side and assume it is not passed by reference.
90+
return false;
9891
}
92+
93+
//We go only one level deep and make a conservative check to avoid a potentially costly recursion.
94+
return parameter.IsByRef
95+
&& parameter.References.Any(paramReference => paramReference.IsAssignment);
9996
}
10097

101-
public class SingleByRefParamArgListListener : InspectionListenerBase
98+
private static VBAParser.ArgumentExpressionContext ImmediateArgumentExpressionContext(IdentifierReference reference)
10299
{
103-
public override void ExitArgList(VBAParser.ArgListContext context)
104-
{
105-
var args = context.arg();
106-
if (args != null && args.All(a => a.PARAMARRAY() == null && a.LPAREN() == null) && args.Count(a => a.BYREF() != null || (a.BYREF() == null && a.BYVAL() == null)) == 1)
107-
{
108-
SaveContext(context);
109-
}
110-
}
100+
var context = reference.Context;
101+
//The context is either already a simpleNameExprContext or an IdentifierValueContext used in a sub-rule of some other lExpression alternative.
102+
var lExpressionNameContext = context is VBAParser.SimpleNameExprContext simpleName
103+
? simpleName
104+
: context.GetAncestor<VBAParser.LExpressionContext>();
105+
106+
//To be an immediate argument and, thus, assignable by ref, the structure must be argumentExpression -> expression -> lExpression.
107+
return lExpressionNameContext?
108+
.Parent?
109+
.Parent as VBAParser.ArgumentExpressionContext;
110+
}
111+
112+
protected override string ResultDescription(Declaration declaration)
113+
{
114+
return string.Format(
115+
InspectionResults.ProcedureCanBeWrittenAsFunctionInspection,
116+
declaration.IdentifierName);
111117
}
112118
}
113119
}

‎Rubberduck.CodeAnalysis/QuickFixes/ChangeProcedureToFunctionQuickFix.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
using System.Linq;
33
using Rubberduck.Inspections.Abstract;
44
using Rubberduck.Inspections.Concrete;
5+
using Rubberduck.JunkDrawer.Extensions;
56
using Rubberduck.Parsing;
67
using Rubberduck.Parsing.Grammar;
78
using Rubberduck.Parsing.Inspections.Abstract;
@@ -56,7 +57,7 @@ public override void Fix(IInspectionResult result, IRewriteSession rewriteSessio
5657
{
5758
var parameterizedDeclaration = (IParameterizedDeclaration) result.Target;
5859
var arg = parameterizedDeclaration.Parameters.First(p => p.IsByRef || p.IsImplicitByRef);
59-
var argIndex = parameterizedDeclaration.Parameters.ToList().IndexOf(arg);
60+
var argIndex = parameterizedDeclaration.Parameters.IndexOf(arg);
6061

6162
UpdateSignature(result.Target, arg, rewriteSession);
6263
foreach (var reference in result.Target.References.Where(reference => !reference.IsDefaultMemberAccess))

‎RubberduckTests/Inspections/ProcedureShouldBeFunctionInspectionTests.cs

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,90 @@ public void ProcedureShouldBeFunction_ReturnsResult()
2222
Assert.AreEqual(1, InspectionResultsForStandardModule(inputCode).Count());
2323
}
2424

25+
[Test]
26+
[Category("Inspections")]
27+
public void ProcedureShouldBeFunction_ReturnsResult_UsedByValAfterConditionalAssignment()
28+
{
29+
const string inputCode =
30+
@"Private Sub Foo(ByRef bar As Boolean, ByVal baz As Boolean)
31+
If baz Then
32+
bar = 42
33+
End If
34+
Goo bar
35+
End Sub
36+
37+
Private Sub Goo(ByVal arg As Boolean)
38+
End Sub";
39+
40+
Assert.AreEqual(1, InspectionResultsForStandardModule(inputCode).Count());
41+
}
42+
43+
[Test]
44+
[Category("Inspections")]
45+
public void ProcedureShouldBeFunction_ReturnsResult_UsedByValBeforeAssignment()
46+
{
47+
const string inputCode =
48+
@"Private Sub Foo(ByRef bar As Boolean)
49+
Goo bar
50+
bar = 42
51+
End Sub
52+
53+
Private Sub Goo(ByVal arg As Boolean)
54+
End Sub";
55+
56+
Assert.AreEqual(1, InspectionResultsForStandardModule(inputCode).Count());
57+
}
58+
59+
[Test]
60+
[Category("Inspections")]
61+
public void ProcedureShouldBeFunction_ReturnsResult_UsedByRef()
62+
{
63+
const string inputCode =
64+
@"Private Sub Foo(ByRef bar As Boolean)
65+
Goo bar, True
66+
End Sub
67+
68+
Private Sub Goo(ByRef arg1 As Boolean, ByRef arg2 As Boolean)
69+
arg1 = arg2
70+
End Sub";
71+
72+
Assert.AreEqual(1, InspectionResultsForStandardModule(inputCode).Count());
73+
}
74+
75+
[Test]
76+
[Category("Inspections")]
77+
public void ProcedureShouldBeFunction_DoesNotReturnResult_UsedInExpression()
78+
{
79+
const string inputCode =
80+
@"Private Sub Foo(ByRef bar As Boolean)
81+
Goo Not bar, True
82+
End Sub
83+
84+
Private Sub Goo(ByRef arg1 As Boolean, ByRef arg2 As Boolean)
85+
arg1 = arg2
86+
End Sub";
87+
88+
Assert.AreEqual(0, InspectionResultsForStandardModule(inputCode).Count());
89+
}
90+
91+
[Test]
92+
[Category("Inspections")]
93+
public void ProcedureShouldBeFunction_ReturnsResult_UsedByRefWithArgumentUsage()
94+
{
95+
const string inputCode =
96+
@"Private Sub Foo(ByRef bar As Boolean)
97+
Goo bar, True
98+
End Sub
99+
100+
Private Sub Goo(ByRef arg1 As Boolean, ByRef arg2 As Boolean)
101+
Dim baz As Variant
102+
baz = arg1
103+
arg1 = arg2
104+
End Sub";
105+
106+
Assert.AreEqual(1, InspectionResultsForStandardModule(inputCode).Count());
107+
}
108+
25109
[Test]
26110
[Category("Inspections")]
27111
public void ProcedureShouldBeFunction_ReturnsResult_MultipleSubs()
@@ -85,6 +169,19 @@ public void ProcedureShouldBeFunction_DoesNotReturnsResult_MultipleByRefParams()
85169
Assert.AreEqual(0, InspectionResultsForStandardModule(inputCode).Count());
86170
}
87171

172+
[Test]
173+
[Category("Inspections")]
174+
public void ProcedureShouldBeFunction_ReturnsResult_MultipleParamsOneByRef()
175+
{
176+
const string inputCode =
177+
@"Private Sub Foo(ByVal foo As Integer, ByRef goo As Integer, ByVal hoo As Variant)
178+
foo = 42
179+
goo = 42
180+
End Sub";
181+
182+
Assert.AreEqual(1, InspectionResultsForStandardModule(inputCode).Count());
183+
}
184+
88185
[Test]
89186
[Category("Inspections")]
90187
public void ProcedureShouldBeFunction_DoesNotReturnResult_InterfaceImplementation()
@@ -107,6 +204,44 @@ Private Sub IClass1_DoSomething(ByRef a As Integer)
107204
Assert.AreEqual(0, InspectionResultsForModules(modules).Count());
108205
}
109206

207+
[Test]
208+
[Category("Inspections")]
209+
public void ProcedureShouldBeFunction_ReturnsResult_Object()
210+
{
211+
const string inputCode1 =
212+
@"Public bar As Variant";
213+
const string inputCode2 =
214+
@"Private Sub DoSomething(ByRef a As Class1)
215+
Set a = New Class1
216+
End Sub";
217+
var modules = new (string, string, ComponentType)[]
218+
{
219+
("Class1", inputCode1, ComponentType.ClassModule),
220+
("Class2", inputCode2, ComponentType.ClassModule)
221+
};
222+
223+
Assert.AreEqual(1, InspectionResultsForModules(modules).Count());
224+
}
225+
226+
[Test]
227+
[Category("Inspections")]
228+
public void ProcedureShouldBeFunction_DoesNotReturnResult_ObjectMember()
229+
{
230+
const string inputCode1 =
231+
@"Public bar As Variant";
232+
const string inputCode2 =
233+
@"Private Sub DoSomething(ByRef a As Class1)
234+
Set a.bar = New Class1
235+
End Sub";
236+
var modules = new (string, string, ComponentType)[]
237+
{
238+
("Class1", inputCode1, ComponentType.ClassModule),
239+
("Class2", inputCode2, ComponentType.ClassModule)
240+
};
241+
242+
Assert.AreEqual(0, InspectionResultsForModules(modules).Count());
243+
}
244+
110245
[Test]
111246
[Category("Inspections")]
112247
public void ProcedureShouldBeFunction_DoesNotReturnResult_EventImplementation()
@@ -146,6 +281,8 @@ Private Sub Foo(ByRef foo As Boolean)
146281
public void InspectionName()
147282
{
148283
var inspection = new ProcedureCanBeWrittenAsFunctionInspection(null);
284+
285+
Assert.AreEqual(nameof(ProcedureCanBeWrittenAsFunctionInspection), inspection.Name);
149286
}
150287

151288
protected override IInspection InspectionUnderTest(RubberduckParserState state)

‎RubberduckTests/QuickFixes/ChangeProcedureToFunctionQuickFixTests.cs

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ namespace RubberduckTests.QuickFixes
99
[TestFixture]
1010
public class ChangeProcedureToFunctionQuickFixTests : QuickFixTestBase
1111
{
12-
1312
[Test]
1413
[Category("QuickFixes")]
1514
public void ProcedureShouldBeFunction_QuickFixWorks()
@@ -29,6 +28,33 @@ public void ProcedureShouldBeFunction_QuickFixWorks()
2928
Assert.AreEqual(expectedCode, actualCode);
3029
}
3130

31+
[Test]
32+
[Category("QuickFixes")]
33+
public void ProcedureShouldBeFunction_QuickFixWorks_AssignedByRef()
34+
{
35+
const string inputCode =
36+
@"Private Sub Foo(ByRef bar As Boolean)
37+
Goo bar, True
38+
End Sub
39+
40+
Private Sub Goo(ByRef arg1 As Boolean, ByRef arg2 As Boolean)
41+
arg1 = arg2
42+
End Sub";
43+
44+
const string expectedCode =
45+
@"Private Function Foo(ByVal bar As Boolean) As Boolean
46+
Goo bar, True
47+
Foo = bar
48+
End Function
49+
50+
Private Sub Goo(ByRef arg1 As Boolean, ByRef arg2 As Boolean)
51+
arg1 = arg2
52+
End Sub";
53+
54+
var actualCode = ApplyQuickFixToFirstInspectionResult(inputCode, state => new ProcedureCanBeWrittenAsFunctionInspection(state));
55+
Assert.AreEqual(expectedCode, actualCode);
56+
}
57+
3258
[Test]
3359
[Category("QuickFixes")]
3460
public void ProcedureShouldBeFunction_QuickFixWorks_NoAsTypeClauseInParam()

0 commit comments

Comments
 (0)