Skip to content

Commit a187120

Browse files
MDoernerMDoerner
committed
Make ParameterCanBeByValInspection respect ByRef RaiseEvent calls
1 parent 1f0bd93 commit a187120

File tree

3 files changed

+204
-12
lines changed

3 files changed

+204
-12
lines changed

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,12 @@ private IEnumerable<ParameterDeclaration> EventMembersThatCanBeChangedToBePassed
170170
}
171171

172172
private bool IsPotentiallyUsedAsByRefParameter(ParameterDeclaration parameter)
173+
{
174+
return IsPotentiallyUsedAsByRefMethodParameter(parameter)
175+
|| IsPotentiallyUsedAsByRefEventParameter(parameter);
176+
}
177+
178+
private bool IsPotentiallyUsedAsByRefMethodParameter(ParameterDeclaration parameter)
173179
{
174180
//The condition on the text of the argument context excludes the cases where the argument is either passed explicitly by value
175181
//or used inside a non-trivial expression, e.g. an arithmetic expression.
@@ -196,5 +202,33 @@ private bool IsPotentiallyUsedAsByRefParameter(ParameterDeclaration parameter)
196202

197203
return false;
198204
}
205+
206+
private bool IsPotentiallyUsedAsByRefEventParameter(ParameterDeclaration parameter)
207+
{
208+
//The condition on the text of the eventArgument context excludes the cases where the argument is either passed explicitly by value
209+
//or used inside a non-trivial expression, e.g. an arithmetic expression.
210+
var argumentsBeingTheParameter = parameter.References
211+
.Select(reference => reference.Context.GetAncestor<VBAParser.EventArgumentContext>())
212+
.Where(context => context != null && context.GetText().Equals(parameter.IdentifierName, StringComparison.OrdinalIgnoreCase));
213+
214+
foreach (var argument in argumentsBeingTheParameter)
215+
{
216+
var parameterCorrespondingToArgument = State.DeclarationFinder
217+
.FindParameterFromSimpleEventArgumentNotPassedByValExplicitly(argument, parameter.QualifiedModuleName);
218+
219+
if (parameterCorrespondingToArgument == null)
220+
{
221+
//We have no idea what parameter it is passed to ar argument. So, we have to err on the safe side and assume it is passed by reference.
222+
return true;
223+
}
224+
225+
if (parameterCorrespondingToArgument.IsByRef)
226+
{
227+
return true;
228+
}
229+
}
230+
231+
return false;
232+
}
199233
}
200234
}

‎Rubberduck.Parsing/VBA/DeclarationCaching/DeclarationFinder.cs

Lines changed: 53 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -476,16 +476,16 @@ public IEnumerable<ModuleBodyElementDeclaration> FindInterfaceImplementationMemb
476476
: Enumerable.Empty<ModuleBodyElementDeclaration>();
477477
}
478478

479-
public ParameterDeclaration FindParameter(Declaration procedure, string parameterName)
479+
public ParameterDeclaration FindParameter(Declaration parameterizedMember, string parameterName)
480480
{
481-
return _parametersByParent.TryGetValue(procedure, out List<ParameterDeclaration> parameters)
481+
return _parametersByParent.TryGetValue(parameterizedMember, out List<ParameterDeclaration> parameters)
482482
? parameters.SingleOrDefault(parameter => parameter.IdentifierName == parameterName)
483483
: null;
484484
}
485485

486-
public IEnumerable<ParameterDeclaration> Parameters(Declaration procedure)
486+
public IEnumerable<ParameterDeclaration> Parameters(Declaration parameterizedMember)
487487
{
488-
return _parametersByParent.TryGetValue(procedure, out List<ParameterDeclaration> result)
488+
return _parametersByParent.TryGetValue(parameterizedMember, out List<ParameterDeclaration> result)
489489
? result
490490
: Enumerable.Empty<ParameterDeclaration>();
491491
}
@@ -584,8 +584,8 @@ public ParameterDeclaration FindParameterOfNonDefaultMemberFromSimpleArgumentNot
584584
var arguments = argumentList.GetDescendents<VBAParser.PositionalArgumentContext>().ToArray();
585585

586586
var parameterIndex = arguments
587-
.Select((param, index) => param.GetDescendent<VBAParser.ArgumentExpressionContext>() == argumentExpression ? (param, index) : (null, -1))
588-
.SingleOrDefault(item => item.param != null).index;
587+
.Select((arg, index) => arg.GetDescendent<VBAParser.ArgumentExpressionContext>() == argumentExpression ? (arg, index) : (null, -1))
588+
.SingleOrDefault(item => item.arg != null).index;
589589

590590
parameter = parameters
591591
.OrderBy(p => p.Selection)
@@ -664,6 +664,53 @@ private ModuleBodyElementDeclaration CallingNonDefaultMember(VBAParser.ArgumentE
664664
return referencedMember;
665665
}
666666

667+
public ParameterDeclaration FindParameterFromSimpleEventArgumentNotPassedByValExplicitly(VBAParser.EventArgumentContext eventArgument, QualifiedModuleName module)
668+
{
669+
if (eventArgument == null
670+
|| eventArgument.GetDescendent<VBAParser.ParenthesizedExprContext>() != null
671+
|| eventArgument.BYVAL() != null)
672+
{
673+
// not a simple argument, or argument is parenthesized and thus passed ByVal
674+
return null;
675+
}
676+
677+
var raisedEvent = RaisedEvent(eventArgument, module);
678+
if (raisedEvent == null)
679+
{
680+
return null;
681+
}
682+
683+
var parameters = Parameters(raisedEvent);
684+
685+
// event arguments are always positional: work out the index
686+
var argumentList = eventArgument.GetAncestor<VBAParser.EventArgumentListContext>();
687+
var arguments = argumentList.eventArgument();
688+
689+
var parameterIndex = arguments
690+
.Select((arg, index) => arg == eventArgument ? (arg, index) : (null, -1))
691+
.SingleOrDefault(tpl => tpl.arg != null).index;
692+
693+
var parameter = parameters
694+
.OrderBy(p => p.Selection)
695+
.Select((param, index) => (param, index))
696+
.SingleOrDefault(tpl => tpl.index == parameterIndex).param;
697+
698+
return parameter;
699+
}
700+
701+
private EventDeclaration RaisedEvent(VBAParser.EventArgumentContext argument, QualifiedModuleName module)
702+
{
703+
var raiseEventContext = argument.GetAncestor<VBAParser.RaiseEventStmtContext>();
704+
var eventIdentifier = raiseEventContext.identifier();
705+
706+
var referencedMember = IdentifierReferences(eventIdentifier, module)
707+
.Select(reference => reference.Declaration)
708+
.OfType<EventDeclaration>()
709+
.FirstOrDefault();
710+
711+
return referencedMember;
712+
}
713+
667714
private string ToNormalizedName(string name)
668715
{
669716
var lower = name.ToLowerInvariant();

‎RubberduckTests/Inspections/ParameterCanBeByValInspectionTests.cs

Lines changed: 117 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -263,14 +263,14 @@ public void GivenArrayParameter_ReturnsNoResult()
263263

264264
[Test]
265265
[Category("Inspections")]
266-
public void ParameterCanBeByVal_ReturnsResult_PassedToByRefProc_NoAssignment()
266+
public void ParameterCanBeByVal_DoesNotReturnResult_PassedToByRefProc_NoAssignment()
267267
{
268268
const string inputCode =
269269
@"Sub DoSomething(foo As Integer)
270270
DoSomethingElse foo
271271
End Sub
272272
273-
Sub DoSomethingElse(ByVal bar As Integer)
273+
Sub DoSomethingElse(ByRef bar As Integer)
274274
End Sub";
275275
var vbe = MockVbeBuilder.BuildFromSingleStandardModule(inputCode, out _);
276276
using (var state = MockParser.CreateAndParse(vbe.Object))
@@ -279,7 +279,7 @@ Sub DoSomethingElse(ByVal bar As Integer)
279279
var inspection = new ParameterCanBeByValInspection(state);
280280
var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
281281

282-
Assert.AreEqual(1, inspectionResults.Count());
282+
Assert.IsFalse(inspectionResults.Any(result => result.Target.IdentifierName.Equals("foo")));
283283
}
284284
}
285285

@@ -308,14 +308,37 @@ Sub DoSomethingElse(ByRef bar As Integer)
308308

309309
[Test]
310310
[Category("Inspections")]
311-
public void ParameterCanBeByVal_ReturnsResult_PassedToByValProc_WithAssignment()
311+
public void ParameterCanBeByVal_ReturnsResult_PassedToByRefProc_ExplicitlyByVal()
312312
{
313313
const string inputCode =
314314
@"Sub DoSomething(foo As Integer)
315-
DoSomethingElse foo
315+
DoSomethingElse (foo)
316+
End Sub
317+
318+
Sub DoSomethingElse(ByRef bar As Integer)
319+
bar = 42
320+
End Sub";
321+
var vbe = MockVbeBuilder.BuildFromSingleStandardModule(inputCode, out _);
322+
using (var state = MockParser.CreateAndParse(vbe.Object))
323+
{
324+
325+
var inspection = new ParameterCanBeByValInspection(state);
326+
var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
327+
328+
Assert.AreEqual(1, inspectionResults.Count());
329+
}
330+
}
331+
332+
[Test]
333+
[Category("Inspections")]
334+
public void ParameterCanBeByVal_ReturnsResult_PassedToByRefProc_PartOfExpression()
335+
{
336+
const string inputCode =
337+
@"Sub DoSomething(foo As Integer)
338+
DoSomethingElse foo + 2
316339
End Sub
317340
318-
Sub DoSomethingElse(ByVal bar As Integer)
341+
Sub DoSomethingElse(ByRef bar As Integer)
319342
bar = 42
320343
End Sub";
321344
var vbe = MockVbeBuilder.BuildFromSingleStandardModule(inputCode, out _);
@@ -329,6 +352,94 @@ Sub DoSomethingElse(ByVal bar As Integer)
329352
}
330353
}
331354

355+
[Test]
356+
[Category("Inspections")]
357+
public void ParameterCanBeByVal_ReturnsResult_PassedToByValEvent()
358+
{
359+
const string inputCode =
360+
@" Public Event Bar(ByVal baz As Integer)
361+
362+
Sub DoSomething(foo As Integer)
363+
RaiseEvent Bar(foo)
364+
End Sub";
365+
var vbe = MockVbeBuilder.BuildFromSingleStandardModule(inputCode, out _);
366+
using (var state = MockParser.CreateAndParse(vbe.Object))
367+
{
368+
369+
var inspection = new ParameterCanBeByValInspection(state);
370+
var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
371+
372+
Assert.AreEqual(1, inspectionResults.Count());
373+
}
374+
}
375+
376+
[Test]
377+
[Category("Inspections")]
378+
public void ParameterCanBeByVal_DoesNotReturnResult_PassedToByRefEvent()
379+
{
380+
const string inputCode =
381+
@" Public Event Bar(ByRef baz As Integer)
382+
383+
Sub DoSomething(foo As Integer)
384+
RaiseEvent Bar(foo)
385+
End Sub";
386+
var vbe = MockVbeBuilder.BuildFromSingleStandardModule(inputCode, out _);
387+
using (var state = MockParser.CreateAndParse(vbe.Object))
388+
{
389+
390+
var inspection = new ParameterCanBeByValInspection(state);
391+
var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
392+
393+
Assert.IsFalse(inspectionResults.Any(result => result.Target.IdentifierName.Equals("foo")));
394+
}
395+
}
396+
397+
398+
[Test]
399+
[Category("Inspections")]
400+
public void ParameterCanBeByVal_ReturnsResult_PassedToByRefEvent_ExplicilyByVal()
401+
{
402+
const string inputCode =
403+
@" Public Event Bar(ByRef baz As Integer)
404+
405+
Sub DoSomething(foo As Integer)
406+
RaiseEvent Bar(BYVAL foo)
407+
End Sub";
408+
var vbe = MockVbeBuilder.BuildFromSingleStandardModule(inputCode, out _);
409+
using (var state = MockParser.CreateAndParse(vbe.Object))
410+
{
411+
412+
var inspection = new ParameterCanBeByValInspection(state);
413+
var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
414+
var fooInspectionResults = inspectionResults.Where(result => result.Target.IdentifierName.Equals("foo"));
415+
416+
Assert.AreEqual(1, fooInspectionResults.Count());
417+
}
418+
}
419+
420+
421+
[Test]
422+
[Category("Inspections")]
423+
public void ParameterCanBeByVal_ReturnsResult_PassedToByRefEvent_PartOfExpression()
424+
{
425+
const string inputCode =
426+
@" Public Event Bar(ByRef baz As Integer)
427+
428+
Sub DoSomething(foo As Integer)
429+
RaiseEvent Bar(foo + 2)
430+
End Sub";
431+
var vbe = MockVbeBuilder.BuildFromSingleStandardModule(inputCode, out _);
432+
using (var state = MockParser.CreateAndParse(vbe.Object))
433+
{
434+
435+
var inspection = new ParameterCanBeByValInspection(state);
436+
var inspectionResults = inspection.GetInspectionResults(CancellationToken.None);
437+
var fooInspectionResults = inspectionResults.Where(result => result.Target.IdentifierName.Equals("foo"));
438+
439+
Assert.AreEqual(1, fooInspectionResults.Count());
440+
}
441+
}
442+
332443
[Test]
333444
[Category("Inspections")]
334445
public void ParameterCanBeByVal_Ignored_DoesNotReturnResult()

0 commit comments

Comments
 (0)