Skip to content

Commit 164eb39

Browse files
committed
Fix WithMemberAccesses in MemberMayReturnNothingInspection
This caused an NRE. In addition, a warning (doc-)comment is added to indicate that this base class is not suitable for members that can be accessed unqualified; there simply is no branch handling this case and it would either not work or even find the wrong usage context. This restriction rules out all user code and all members on global instances as well as on document and form classes.
1 parent 537a781 commit 164eb39

File tree

2 files changed

+57
-7
lines changed

2 files changed

+57
-7
lines changed

Rubberduck.CodeAnalysis/Inspections/Abstract/MemberAccessMayReturnNothingInspectionBase.cs

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,20 @@ protected MemberAccessMayReturnNothingInspectionBase(IDeclarationFinderProvider
1919
: base(declarationFinderProvider)
2020
{}
2121

22+
/// <summary>
23+
/// Members that might return Nothing
24+
/// </summary>
25+
/// <remarks>
26+
/// It must not be legal to call the members unqualified. In particular, user-defined members will not be considered.
27+
/// Moreover, this disqualifies all members on global objects.
28+
/// </remarks>
2229
public abstract IEnumerable<Declaration> MembersUnderTest(DeclarationFinder finder);
2330
public abstract string ResultTemplate { get; }
2431

2532
protected override IEnumerable<Declaration> ObjectionableDeclarations(DeclarationFinder finder)
2633
{
27-
return MembersUnderTest(finder);
34+
//This restriction is in place because the inspection currently cannot handle unqualified accesses.
35+
return MembersUnderTest(finder).Where(member => !member.IsUserDefined);
2836
}
2937

3038
protected override bool IsResultReference(IdentifierReference reference, DeclarationFinder finder)
@@ -46,7 +54,7 @@ protected override bool IsResultReference(IdentifierReference reference, Declara
4654
{
4755
return usageContext is VBAParser.MemberAccessExprContext
4856
|| !(usageContext is VBAParser.CallStmtContext)
49-
&& !ContextIsNothingTest(usageContext);
57+
&& !ContextIsNothing(usageContext);
5058
}
5159

5260
var assignedTo = AssignmentTarget(reference, finder, setter);
@@ -65,14 +73,21 @@ private static IdentifierReference AssignmentTarget(IdentifierReference referenc
6573

6674
private static RuleContext UsageContext(IdentifierReference reference)
6775
{
68-
var access = reference.Context.GetAncestor<VBAParser.MemberAccessExprContext>();
69-
var usageContext = access.Parent is VBAParser.IndexExprContext indexExpr
76+
//We prefer the with member access over the member access, because the accesses are resolved right to left.
77+
var access = reference.Context.GetAncestor<VBAParser.WithMemberAccessExprContext>() as VBAParser.LExpressionContext
78+
?? reference.Context.GetAncestor<VBAParser.MemberAccessExprContext>();
79+
80+
if (access == null)
81+
{
82+
return null;
83+
}
84+
85+
return access.Parent is VBAParser.IndexExprContext indexExpr
7086
? indexExpr.Parent
7187
: access.Parent;
72-
return usageContext;
7388
}
7489

75-
private static bool ContextIsNothingTest(IParseTree context)
90+
private static bool ContextIsNothing(IParseTree context)
7691
{
7792
return context is VBAParser.LExprContext
7893
&& context.Parent is VBAParser.RelationalOpContext comparison
@@ -86,7 +101,7 @@ private static bool IsUsedBeforeCheckingForNothing(IdentifierReference assignedT
86101
var firstUse = GetReferenceNodes(tree).FirstOrDefault();
87102

88103
return !(firstUse is null)
89-
&& !ContextIsNothingTest(firstUse.Reference.Context.Parent);
104+
&& !ContextIsNothing(firstUse.Reference.Context.Parent);
90105
}
91106

92107
private static IEnumerable<INode> GetReferenceNodes(INode node)

RubberduckTests/Inspections/ExcelMemberMayReturnNothingInspectionTests.cs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,22 @@ End Sub
2626
Assert.AreEqual(1, InspectionResults(inputCode).Count());
2727
}
2828

29+
[Test]
30+
[Category("Inspections")]
31+
public void ExcelMemberMayReturnNothing_ReturnsResult_WithMemberAccessOnFind()
32+
{
33+
const string inputCode =
34+
@"Sub UnderTest()
35+
Dim ws As Worksheet
36+
Set ws = Sheet1
37+
With ws.UsedRange
38+
foo = .Find(""foo"").Row
39+
End With
40+
End Sub
41+
";
42+
Assert.AreEqual(1, InspectionResults(inputCode).Count());
43+
}
44+
2945
[Test]
3046
[Category("Inspections")]
3147
public void ExcelMemberMayReturnNothing_Ignored_DoesNotReturnResult()
@@ -111,6 +127,25 @@ End Sub
111127
Assert.AreEqual(1, InspectionResults(inputCode).Count());
112128
}
113129

130+
[Test]
131+
[Category("Inspections")]
132+
public void ExcelMemberMayReturnNothing_ReturnsResult_AssignedAndNotTested_FromWithMemberAccess()
133+
{
134+
const string inputCode =
135+
@"Sub UnderTest()
136+
Dim ws As Worksheet
137+
Set ws = Sheet1
138+
Dim result As Range
139+
With ws.UsedRange
140+
Set result = .Find(""foo"")
141+
End With
142+
result.Value = ""bar""
143+
End Sub
144+
";
145+
146+
Assert.AreEqual(1, InspectionResults(inputCode).Count());
147+
}
148+
114149
[Test]
115150
[Category("Inspections")]
116151
public void ExcelMemberMayReturnNothing_ReturnsResult_ResultIsSomethingElse()

0 commit comments

Comments
 (0)