Skip to content

Commit cc1327e

Browse files
MDoernerMDoerner
committed
Fix bug in VariableNotAssignedInspection and add tests for module variables
The inspection considered variables to be assigned by ref even if the variable only appeared as a sum-expression of the actual argument. This also improves `GetAncestot<TContext>` to use pattern matching, which avoids an invalid cast exception.
1 parent 6386d27 commit cc1327e

File tree

3 files changed

+370
-46
lines changed

3 files changed

+370
-46
lines changed

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

Lines changed: 43 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,11 @@
1-
using System.Collections.Generic;
21
using System.Linq;
32
using Rubberduck.Inspections.Abstract;
4-
using Rubberduck.Inspections.Results;
53
using Rubberduck.Parsing;
64
using Rubberduck.Parsing.Grammar;
7-
using Rubberduck.Parsing.Inspections.Abstract;
85
using Rubberduck.Resources.Inspections;
96
using Rubberduck.Parsing.Symbols;
107
using Rubberduck.Parsing.VBA;
11-
using Rubberduck.Inspections.Inspections.Extensions;
8+
using Rubberduck.Parsing.VBA.DeclarationCaching;
129

1310
namespace Rubberduck.Inspections.Concrete
1411
{
@@ -37,37 +34,61 @@ namespace Rubberduck.Inspections.Concrete
3734
/// End Sub
3835
/// ]]>
3936
/// </example>
40-
public sealed class VariableNotAssignedInspection : InspectionBase
37+
public sealed class VariableNotAssignedInspection : DeclarationInspectionBase
4138
{
4239
public VariableNotAssignedInspection(RubberduckParserState state)
43-
: base(state) { }
40+
: base(state, DeclarationType.Variable) { }
4441

45-
protected override IEnumerable<IInspectionResult> DoGetInspectionResults()
42+
protected override bool IsResultDeclaration(Declaration declaration, DeclarationFinder finder)
4643
{
47-
// ignore arrays. todo: ArrayIndicesNotAccessedInspection
48-
var arrays = State.DeclarationFinder.UserDeclarations(DeclarationType.Variable).Where(declaration => declaration.IsArray);
49-
50-
var declarations = State.DeclarationFinder.UserDeclarations(DeclarationType.Variable)
51-
.Except(arrays)
52-
.Where(declaration =>
53-
!declaration.IsWithEvents
54-
&& State.DeclarationFinder.MatchName(declaration.AsTypeName).All(item => item.DeclarationType != DeclarationType.UserDefinedType) // UDT variables don't need to be assigned
55-
&& !declaration.IsSelfAssigned
56-
&& !declaration.References.Any(reference => reference.IsAssignment || IsAssignedByRefArgument(reference.ParentScoping, reference)));
44+
return declaration != null
45+
&& !declaration.IsArray // ignore arrays. todo: ArrayIndicesNotAccessedInspection
46+
&& !declaration.IsWithEvents
47+
&& !declaration.IsSelfAssigned
48+
&& !HasUdtType(declaration, finder) // UDT variables don't need to be assigned
49+
&& !declaration.References.Any(reference => reference.IsAssignment || IsAssignedByRefArgument(reference.ParentScoping, reference, finder));
50+
}
5751

58-
return declarations.Select(issue =>
59-
new DeclarationInspectionResult(this, string.Format(InspectionResults.VariableNotAssignedInspection, issue.IdentifierName), issue));
52+
private static bool HasUdtType(Declaration declaration, DeclarationFinder finder)
53+
{
54+
return finder.MatchName(declaration.AsTypeName)
55+
.Any(item => item.DeclarationType == DeclarationType.UserDefinedType);
6056
}
6157

62-
private bool IsAssignedByRefArgument(Declaration enclosingProcedure, IdentifierReference reference)
58+
private static bool IsAssignedByRefArgument(Declaration enclosingProcedure, IdentifierReference reference, DeclarationFinder finder)
6359
{
64-
var argExpression = reference.Context.GetAncestor<VBAParser.ArgumentExpressionContext>();
65-
var parameter = State.DeclarationFinder.FindParameterOfNonDefaultMemberFromSimpleArgumentNotPassedByValExplicitly(argExpression, enclosingProcedure);
60+
var argExpression = ImmediateArgumentExpressionContext(reference);
61+
62+
if (argExpression is null)
63+
{
64+
return false;
65+
}
66+
67+
var parameter = finder.FindParameterOfNonDefaultMemberFromSimpleArgumentNotPassedByValExplicitly(argExpression, enclosingProcedure);
6668

6769
// note: not recursive, by design.
6870
return parameter != null
6971
&& (parameter.IsImplicitByRef || parameter.IsByRef)
7072
&& parameter.References.Any(r => r.IsAssignment);
7173
}
74+
75+
private static VBAParser.ArgumentExpressionContext ImmediateArgumentExpressionContext(IdentifierReference reference)
76+
{
77+
var context = reference.Context;
78+
//The context is either already a simpleNameExprContext or an IdentifierValueContext used in a sub-rule of some other lExpression alternative.
79+
var lExpressionNameContext = context is VBAParser.SimpleNameExprContext simpleName
80+
? simpleName
81+
: context.GetAncestor<VBAParser.LExpressionContext>();
82+
83+
//To be an immediate argument and, thus, assignable by ref, the structure must be argumentExpression -> expression -> lExpression.
84+
return lExpressionNameContext?
85+
.Parent?
86+
.Parent as VBAParser.ArgumentExpressionContext;
87+
}
88+
89+
protected override string ResultDescription(Declaration declaration)
90+
{
91+
return string.Format(InspectionResults.VariableNotAssignedInspection, declaration.IdentifierName);
92+
}
7293
}
7394
}

‎Rubberduck.Parsing/ParserRuleContextExtensions.cs

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,12 @@ public static TContext GetChild<TContext>(this ParserRuleContext context) where
6666
return default;
6767
}
6868

69-
var results = context.children.Where(child => child is TContext);
70-
return results.Any() ? (TContext)results.First() : default;
69+
var results = context.children
70+
.Where(child => child is TContext)
71+
.ToList();
72+
return results.Any()
73+
? (TContext)results.First()
74+
: default;
7175
}
7276

7377
/// <summary>
@@ -109,27 +113,24 @@ private static bool IsDescendentOf_Recursive(IParseTree context, IParseTree targ
109113
{
110114
return false;
111115
}
112-
if (context == targetParent)
113-
{
114-
return true;
115-
}
116-
return IsDescendentOf_Recursive(context.Parent, targetParent);
116+
return context == targetParent
117+
|| IsDescendentOf_Recursive(context.Parent, targetParent);
117118
}
118119

119120
/// <summary>
120121
/// Returns the context's first ancestor of the generic Type.
121122
/// </summary>
122123
public static TContext GetAncestor<TContext>(this ParserRuleContext context)
123124
{
124-
if (context == null)
125+
switch (context)
125126
{
126-
return default;
127+
case null:
128+
return default;
129+
case TContext _:
130+
return GetAncestor_Recursive<TContext>((ParserRuleContext)context.Parent);
131+
default:
132+
return GetAncestor_Recursive<TContext>(context);
127133
}
128-
if (context is TContext)
129-
{
130-
return GetAncestor_Recursive<TContext>((ParserRuleContext)context.Parent);
131-
}
132-
return GetAncestor_Recursive<TContext>(context);
133134
}
134135

135136
/// <summary>
@@ -143,15 +144,15 @@ public static bool TryGetAncestor<TContext>(this ParserRuleContext context, out
143144

144145
private static TContext GetAncestor_Recursive<TContext>(ParserRuleContext context)
145146
{
146-
if (context == null)
147+
switch (context)
147148
{
148-
return default;
149+
case null:
150+
return default;
151+
case TContext tContext:
152+
return tContext;
153+
default:
154+
return GetAncestor_Recursive<TContext>((ParserRuleContext)context.Parent);
149155
}
150-
if (context is TContext)
151-
{
152-
return (TContext)System.Convert.ChangeType(context, typeof(TContext));
153-
}
154-
return GetAncestor_Recursive<TContext>((ParserRuleContext)context.Parent);
155156
}
156157

157158
/// <summary>
@@ -169,9 +170,7 @@ public static ParserRuleContext GetAncestorContainingTokenIndex(this ParserRuleC
169170
return context;
170171
}
171172

172-
var parent = context.Parent as ParserRuleContext;
173-
174-
if (parent == null)
173+
if (!(context.Parent is ParserRuleContext parent))
175174
{
176175
return default;
177176
}
@@ -288,6 +287,7 @@ public static bool IsOptionCompareBinary(this ParserRuleContext context)
288287
return (optionContext is null) || !(optionContext.BINARY() is null);
289288
}
290289

290+
/// <summary>
291291
/// Returns the context's widest descendent of the generic type containing the token with the specified token index.
292292
/// </summary>
293293
public static TContext GetWidestDescendentContainingTokenIndex<TContext>(this ParserRuleContext context, int tokenIndex) where TContext : ParserRuleContext
@@ -296,6 +296,7 @@ public static TContext GetWidestDescendentContainingTokenIndex<TContext>(this Pa
296296
return descendents.FirstOrDefault();
297297
}
298298

299+
/// <summary>
299300
/// Returns the context's smallest descendent of the generic type containing the token with the specified token index.
300301
/// </summary>
301302
public static TContext GetSmallestDescendentContainingTokenIndex<TContext>(this ParserRuleContext context, int tokenIndex) where TContext : ParserRuleContext
@@ -333,6 +334,7 @@ public static IEnumerable<TContext> GetDescendentsContainingTokenIndex<TContext>
333334
return matches;
334335
}
335336

337+
/// <summary>
336338
/// Returns the context's widest descendent of the generic type containing the specified selection.
337339
/// </summary>
338340
public static TContext GetWidestDescendentContainingSelection<TContext>(this ParserRuleContext context, Selection selection) where TContext : ParserRuleContext
@@ -341,6 +343,7 @@ public static TContext GetWidestDescendentContainingSelection<TContext>(this Par
341343
return descendents.FirstOrDefault();
342344
}
343345

346+
/// <summary>
344347
/// Returns the context's smallest descendent of the generic type containing the specified selection.
345348
/// </summary>
346349
public static TContext GetSmallestDescendentContainingSelection<TContext>(this ParserRuleContext context, Selection selection) where TContext : ParserRuleContext

0 commit comments

Comments
 (0)