Skip to content

Commit 1f0bd93

Browse files
MDoernerMDoerner
committed
Refactor the ParameterCanBeByValInspection
1 parent 8cb1a2d commit 1f0bd93

File tree

1 file changed

+137
-91
lines changed

1 file changed

+137
-91
lines changed

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

Lines changed: 137 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,17 @@
1+
using System;
12
using System.Collections.Generic;
23
using System.Diagnostics;
34
using System.Linq;
45
using Rubberduck.Common;
56
using Rubberduck.Inspections.Abstract;
67
using Rubberduck.Inspections.Results;
8+
using Rubberduck.Parsing;
79
using Rubberduck.Parsing.Grammar;
810
using Rubberduck.Parsing.Inspections.Abstract;
911
using Rubberduck.Resources.Inspections;
1012
using Rubberduck.Parsing.Symbols;
1113
using Rubberduck.Parsing.VBA;
14+
using Rubberduck.Parsing.VBA.Extensions;
1215

1316
namespace Rubberduck.Inspections.Concrete
1417
{
@@ -19,134 +22,177 @@ public ParameterCanBeByValInspection(RubberduckParserState state)
1922

2023
protected override IEnumerable<IInspectionResult> DoGetInspectionResults()
2124
{
22-
var declarations = UserDeclarations.ToArray();
23-
var issues = new List<IInspectionResult>();
24-
25-
var interfaceDeclarationMembers = State.DeclarationFinder.FindAllInterfaceMembers().ToArray();
26-
var interfaceScopes = State.DeclarationFinder.FindAllInterfaceImplementingMembers().Concat(interfaceDeclarationMembers).Select(s => s.Scope).ToArray();
27-
28-
issues.AddRange(GetResults(declarations, interfaceDeclarationMembers));
29-
30-
var eventMembers = declarations.Where(item => item.DeclarationType == DeclarationType.Event).ToArray();
31-
var formEventHandlerScopes = State.FindFormEventHandlers().Select(handler => handler.Scope).ToArray();
32-
var eventHandlerScopes = State.DeclarationFinder.FindEventHandlers().Concat(declarations.FindUserEventHandlers()).Select(e => e.Scope).ToArray();
33-
var eventScopes = eventMembers.Select(s => s.Scope)
34-
.Concat(formEventHandlerScopes)
35-
.Concat(eventHandlerScopes)
36-
.ToArray();
37-
38-
issues.AddRange(GetResults(declarations, eventMembers));
39-
40-
var declareScopes = declarations.Where(item =>
41-
item.DeclarationType == DeclarationType.LibraryFunction
42-
|| item.DeclarationType == DeclarationType.LibraryProcedure)
43-
.Select(e => e.Scope)
44-
.ToArray();
45-
46-
issues.AddRange(declarations.OfType<ParameterDeclaration>()
47-
.Where(declaration => IsIssue(declaration, declarations, declareScopes, eventScopes, interfaceScopes))
48-
.Select(issue => new DeclarationInspectionResult(this, string.Format(InspectionResults.ParameterCanBeByValInspection, issue.IdentifierName), issue)));
49-
50-
return issues;
25+
var parameters = State.DeclarationFinder
26+
.UserDeclarations(DeclarationType.Parameter)
27+
.OfType<ParameterDeclaration>().ToList();
28+
var parametersThatCanBeChangedToBePassedByVal = new List<ParameterDeclaration>();
29+
30+
var interfaceDeclarationMembers = State.DeclarationFinder.FindAllInterfaceMembers().ToList();
31+
var interfaceScopeDeclarations = State.DeclarationFinder
32+
.FindAllInterfaceImplementingMembers()
33+
.Concat(interfaceDeclarationMembers)
34+
.ToHashSet();
35+
36+
parametersThatCanBeChangedToBePassedByVal.AddRange(InterFaceMembersThatCanBeChangedToBePassedByVal(interfaceDeclarationMembers));
37+
38+
var eventMembers = State.DeclarationFinder.UserDeclarations(DeclarationType.Event).ToList();
39+
var formEventHandlerScopeDeclarations = State.FindFormEventHandlers();
40+
var eventHandlerScopeDeclarations = State.DeclarationFinder.FindEventHandlers().Concat(parameters.FindUserEventHandlers());
41+
var eventScopeDeclarations = eventMembers
42+
.Concat(formEventHandlerScopeDeclarations)
43+
.Concat(eventHandlerScopeDeclarations)
44+
.ToHashSet();
45+
46+
parametersThatCanBeChangedToBePassedByVal.AddRange(EventMembersThatCanBeChangedToBePassedByVal(eventMembers));
47+
48+
parametersThatCanBeChangedToBePassedByVal
49+
.AddRange(parameters.Where(parameter => CanBeChangedToBePassedByVal(parameter, eventScopeDeclarations, interfaceScopeDeclarations)));
50+
51+
return parametersThatCanBeChangedToBePassedByVal
52+
.Where(parameter => !IsIgnoringInspectionResultFor(parameter, AnnotationName))
53+
.Select(parameter => new DeclarationInspectionResult(this, string.Format(InspectionResults.ParameterCanBeByValInspection, parameter.IdentifierName), parameter));
5154
}
5255

53-
private bool IsIssue(ParameterDeclaration declaration, Declaration[] userDeclarations, string[] declareScopes, string[] eventScopes, string[] interfaceScopes)
56+
private bool CanBeChangedToBePassedByVal(ParameterDeclaration parameter, HashSet<Declaration> eventScopeDeclarations, HashSet<Declaration> interfaceScopeDeclarations)
5457
{
55-
var isIssue =
56-
!declaration.IsArray
57-
&& !declaration.IsParamArray
58-
&& (declaration.IsByRef || declaration.IsImplicitByRef)
59-
&& (declaration.AsTypeDeclaration == null || declaration.AsTypeDeclaration.DeclarationType != DeclarationType.ClassModule && declaration.AsTypeDeclaration.DeclarationType != DeclarationType.UserDefinedType && declaration.AsTypeDeclaration.DeclarationType != DeclarationType.Enumeration)
60-
&& !declareScopes.Contains(declaration.ParentScope)
61-
&& !eventScopes.Contains(declaration.ParentScope)
62-
&& !interfaceScopes.Contains(declaration.ParentScope)
63-
&& !IsUsedAsByRefParam(userDeclarations, declaration)
64-
&& (!declaration.References.Any() || !declaration.References.Any(reference => reference.IsAssignment));
58+
var enclosingMember = parameter.ParentScopeDeclaration;
59+
var isIssue = !interfaceScopeDeclarations.Contains(enclosingMember)
60+
&& !eventScopeDeclarations.Contains(enclosingMember)
61+
&& CanBeChangedToBePassedByValIndividually(parameter);
6562
return isIssue;
6663
}
6764

68-
private IEnumerable<IInspectionResult> GetResults(Declaration[] declarations, Declaration[] declarationMembers)
65+
private bool CanBeChangedToBePassedByValIndividually(ParameterDeclaration parameter)
6966
{
70-
foreach (var declaration in declarationMembers)
71-
{
72-
var declarationParameters = declarations.OfType<ParameterDeclaration>()
73-
.Where(d => Equals(d.ParentDeclaration, declaration))
74-
.OrderBy(o => o.Selection.StartLine)
75-
.ThenBy(t => t.Selection.StartColumn)
76-
.ToList();
67+
var canPossiblyBeChangedToBePassedByVal =
68+
!parameter.IsArray
69+
&& !parameter.IsParamArray
70+
&& (parameter.IsByRef || parameter.IsImplicitByRef)
71+
&& !IsParameterOfDeclaredLibraryFunction(parameter)
72+
&& (parameter.AsTypeDeclaration == null
73+
|| (parameter.AsTypeDeclaration.DeclarationType != DeclarationType.ClassModule
74+
&& parameter.AsTypeDeclaration.DeclarationType != DeclarationType.UserDefinedType
75+
&& parameter.AsTypeDeclaration.DeclarationType != DeclarationType.Enumeration))
76+
&& !parameter.References.Any(reference => reference.IsAssignment)
77+
&& !IsPotentiallyUsedAsByRefParameter(parameter);
78+
return canPossiblyBeChangedToBePassedByVal;
79+
}
80+
81+
private static bool IsParameterOfDeclaredLibraryFunction(ParameterDeclaration parameter)
82+
{
83+
var parentMember = parameter.ParentScopeDeclaration;
84+
return parentMember.DeclarationType == DeclarationType.LibraryFunction
85+
|| parentMember.DeclarationType == DeclarationType.LibraryProcedure;
86+
}
7787

78-
if (!declarationParameters.Any()) { continue; }
79-
var parametersAreByRef = declarationParameters.Select(s => true).ToList();
88+
private IEnumerable<ParameterDeclaration> InterFaceMembersThatCanBeChangedToBePassedByVal(List<Declaration> interfaceMembers)
89+
{
90+
foreach (var memberDeclaration in interfaceMembers.OfType<ModuleBodyElementDeclaration>())
91+
{
92+
var interfaceParameters = memberDeclaration.Parameters.ToList();
93+
if (!interfaceParameters.Any())
94+
{
95+
continue;
96+
}
8097

81-
var members = declarationMembers.Any(a => a.DeclarationType == DeclarationType.Event)
82-
? declarations.FindHandlersForEvent(declaration).Select(s => s.Item2).ToList()
83-
: State.DeclarationFinder.FindInterfaceImplementationMembers(declaration).Cast<Declaration>().ToList();
98+
var parameterCanBeChangedToBeByVal = interfaceParameters.Select(parameter => parameter.IsByRef).ToList();
8499

85-
foreach (var member in members)
100+
var implementingMembers = State.DeclarationFinder.FindInterfaceImplementationMembers(memberDeclaration);
101+
foreach (var implementingMember in implementingMembers)
86102
{
87-
var parameters = declarations.OfType<ParameterDeclaration>()
88-
.Where(d => Equals(d.ParentDeclaration, member))
89-
.OrderBy(o => o.Selection.StartLine)
90-
.ThenBy(t => t.Selection.StartColumn)
91-
.ToList();
103+
var implementationParameters = implementingMember.Parameters.ToList();
92104

93105
//If you hit this assert, reopen https://github.com/rubberduck-vba/Rubberduck/issues/3906
94-
Debug.Assert(parametersAreByRef.Count == parameters.Count);
106+
Debug.Assert(parameterCanBeChangedToBeByVal.Count == implementationParameters.Count);
95107

96-
for (var i = 0; i < parameters.Count; i++)
108+
for (var i = 0; i < implementationParameters.Count; i++)
97109
{
98-
parametersAreByRef[i] = parametersAreByRef[i] &&
99-
!IsUsedAsByRefParam(declarations, parameters[i]) &&
100-
((VBAParser.ArgContext) parameters[i].Context).BYVAL() == null &&
101-
!parameters[i].References.Any(reference => reference.IsAssignment);
110+
parameterCanBeChangedToBeByVal[i] = parameterCanBeChangedToBeByVal[i]
111+
&& !IsPotentiallyUsedAsByRefParameter(implementationParameters[i])
112+
&& ((VBAParser.ArgContext)implementationParameters[i].Context).BYVAL() == null
113+
&& !implementationParameters[i].References.Any(reference => reference.IsAssignment);
102114
}
103115
}
104116

105-
for (var i = 0; i < declarationParameters.Count; i++)
117+
for (var i = 0; i < parameterCanBeChangedToBeByVal.Count; i++)
106118
{
107-
if (parametersAreByRef[i])
119+
if (parameterCanBeChangedToBeByVal[i])
108120
{
109-
yield return new DeclarationInspectionResult(this,
110-
string.Format(InspectionResults.ParameterCanBeByValInspection, declarationParameters[i].IdentifierName),
111-
declarationParameters[i]);
121+
yield return interfaceParameters[i];
112122
}
113123
}
114124
}
115125
}
116126

117-
private static bool IsUsedAsByRefParam(IEnumerable<Declaration> declarations, Declaration parameter)
127+
private IEnumerable<ParameterDeclaration> EventMembersThatCanBeChangedToBePassedByVal(IEnumerable<Declaration> eventMembers)
118128
{
119-
// find the procedure calls in the procedure of the parameter.
120-
// note: works harder than it needs to when procedure has more than a single procedure call...
121-
// ...but caching [declarations] would be a memory leak
122-
var items = declarations as List<Declaration> ?? declarations.ToList();
129+
foreach (var memberDeclaration in eventMembers)
130+
{
131+
var eventParameters = (memberDeclaration as IParameterizedDeclaration)?.Parameters.ToList();
132+
if (!eventParameters?.Any() ?? false)
133+
{
134+
continue;
135+
}
123136

124-
var procedureCalls = items.Where(item => item.DeclarationType.HasFlag(DeclarationType.Member))
125-
.SelectMany(member => member.References.Where(reference => reference.ParentScoping.Equals(parameter.ParentScopeDeclaration)))
126-
.GroupBy(call => call.Declaration)
127-
.ToList(); // only check a procedure once. its declaration doesn't change if it's called 20 times anyway.
137+
var parameterCanBeChangedToBeByVal = eventParameters.Select(parameter => parameter.IsByRef).ToList();
128138

129-
foreach (var item in procedureCalls)
130-
{
131-
var calledProcedureArgs = items
132-
.Where(arg => arg.DeclarationType == DeclarationType.Parameter && arg.ParentScope == item.Key.Scope)
133-
.OrderBy(arg => arg.Selection.StartLine)
134-
.ThenBy(arg => arg.Selection.StartColumn)
135-
.ToArray();
139+
//todo: Find a better way to find the handlers.
140+
var eventHandlers = State.DeclarationFinder
141+
.AllUserDeclarations
142+
.FindHandlersForEvent(memberDeclaration)
143+
.Select(s => s.Item2)
144+
.ToList();
136145

137-
foreach (var declaration in calledProcedureArgs)
146+
foreach (var eventHandler in eventHandlers.OfType<IParameterizedDeclaration>())
138147
{
139-
if (((VBAParser.ArgContext) declaration.Context).BYVAL() != null)
148+
var handlerParameters = eventHandler.Parameters.ToList();
149+
150+
//If you hit this assert, reopen https://github.com/rubberduck-vba/Rubberduck/issues/3906
151+
Debug.Assert(parameterCanBeChangedToBeByVal.Count == handlerParameters.Count);
152+
153+
for (var i = 0; i < handlerParameters.Count; i++)
140154
{
141-
continue;
155+
parameterCanBeChangedToBeByVal[i] = parameterCanBeChangedToBeByVal[i]
156+
&& !IsPotentiallyUsedAsByRefParameter(handlerParameters[i])
157+
&& ((VBAParser.ArgContext)handlerParameters[i].Context).BYVAL() == null
158+
&& !handlerParameters[i].References.Any(reference => reference.IsAssignment);
142159
}
160+
}
143161

144-
if (declaration.References.Any(reference => reference.IsAssignment))
162+
for (var i = 0; i < parameterCanBeChangedToBeByVal.Count; i++)
163+
{
164+
if (parameterCanBeChangedToBeByVal[i])
145165
{
146-
return true;
166+
yield return eventParameters[i];
147167
}
148168
}
149169
}
170+
}
171+
172+
private bool IsPotentiallyUsedAsByRefParameter(ParameterDeclaration parameter)
173+
{
174+
//The condition on the text of the argument context excludes the cases where the argument is either passed explicitly by value
175+
//or used inside a non-trivial expression, e.g. an arithmetic expression.
176+
var argumentsBeingTheParameter = parameter.References
177+
.Select(reference => reference.Context.GetAncestor<VBAParser.ArgumentExpressionContext>())
178+
.Where(context => context != null && context.GetText().Equals(parameter.IdentifierName, StringComparison.OrdinalIgnoreCase));
179+
180+
foreach (var argument in argumentsBeingTheParameter)
181+
{
182+
var parameterCorrespondingToArgument = State.DeclarationFinder
183+
.FindParameterOfNonDefaultMemberFromSimpleArgumentNotPassedByValExplicitly(argument, parameter.QualifiedModuleName);
184+
185+
if (parameterCorrespondingToArgument == null)
186+
{
187+
//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.
188+
return true;
189+
}
190+
191+
if (parameterCorrespondingToArgument.IsByRef)
192+
{
193+
return true;
194+
}
195+
}
150196

151197
return false;
152198
}

0 commit comments

Comments
 (0)