Skip to content

Commit 66c1360

Browse files
retailcoderretailcoder
authored
Merge pull request #6044 from BZngr/ReadOnlyAssignmentInspectionFP
Fix ReadOnlyAssignmentInspection false positives
2 parents b9d962c + f26c1d8 commit 66c1360

File tree

2 files changed

+134
-79
lines changed

2 files changed

+134
-79
lines changed

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

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -100,28 +100,29 @@ public ReadOnlyPropertyAssignmentInspection(IDeclarationFinderProvider declarati
100100

101101
protected override bool IsResultReference(IdentifierReference reference, DeclarationFinder finder)
102102
{
103-
if (!reference.Declaration.DeclarationType.HasFlag(DeclarationType.Property)
104-
//Ignore expressions found within Property declaration contexts
105-
|| (reference.Declaration.Context?.Contains(reference.Context) ?? false))
103+
if (!reference.Declaration.DeclarationType.HasFlag(DeclarationType.Property))
106104
{
107105
return false;
108106
}
109107

110-
var propertyDeclarations = finder.MatchName(reference.Declaration.IdentifierName)
111-
.Where(d => d.DeclarationType.HasFlag(DeclarationType.Property)
112-
&& d.QualifiedModuleName == reference.QualifiedModuleName);
113-
114-
if (reference.IsSetAssignment)
108+
//Ignore assignment expressions found within Property Get declaration contexts
109+
if (!IsReadOnlyPropertyReference(reference, finder)
110+
|| reference.Declaration.Context.Contains(reference.Context))
115111
{
116-
return !propertyDeclarations.Any(pd => pd.DeclarationType.HasFlag(DeclarationType.PropertySet));
112+
return false;
117113
}
118114

119-
if (reference.IsAssignment && !reference.IsSetAssignment)
120-
{
121-
return !propertyDeclarations.Any(pd => pd.DeclarationType.HasFlag(DeclarationType.PropertyLet));
122-
}
115+
return reference.IsAssignment;
116+
}
117+
118+
private bool IsReadOnlyPropertyReference(IdentifierReference reference, DeclarationFinder finder)
119+
{
120+
var propertyDeclarations = finder.MatchName(reference.Declaration.IdentifierName)
121+
.Where(d => d.DeclarationType.HasFlag(DeclarationType.Property)
122+
&& d.QualifiedModuleName == reference.QualifiedModuleName);
123123

124-
return false;
124+
return propertyDeclarations.Count() == 1
125+
&& propertyDeclarations.First().DeclarationType.HasFlag(DeclarationType.PropertyGet);
125126
}
126127

127128
protected override string ResultDescription(IdentifierReference reference)
@@ -131,4 +132,4 @@ protected override string ResultDescription(IdentifierReference reference)
131132
InspectionResults.ReadOnlyPropertyAssignmentInspection, identifierName);
132133
}
133134
}
134-
}
135+
}

‎RubberduckTests/Inspections/ReadOnlyPropertyAssignmentTests.cs

Lines changed: 118 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,14 @@ namespace RubberduckTests.Inspections
1515
[TestFixture]
1616
public class ReadOnlyPropertyAssignmentTests : InspectionTestsBase
1717
{
18-
[Test]
18+
[TestCase("MyData", 0)]
19+
[TestCase("MyData2", 1)] //Results in a readonly MyData property
1920
[Category("Inspections")]
2021
[Category("ReadOnlyPropertyAssignment")]
21-
public void SetUserDefinedClassMCVE_Flags()
22+
public void SetUserDefinedClass_CalledFromSameModule(string setPropertyName, long expectedCount)
2223
{
2324
var sutInputCode =
24-
@"Private mData As AClass
25+
$@"Private mData As AClass
2526
2627
Public Sub Test()
2728
Set MyData = New AClass
@@ -30,48 +31,59 @@ End Sub
3031
Public Property Get MyData() As AClass
3132
Set MyData = mData
3233
End Property
34+
35+
Public Property Set {setPropertyName}(RHS As AClass)
36+
Set mData = MyData
37+
End Property
3338
";
34-
Assert.AreEqual(1, InspectionResultsForModules(
39+
Assert.AreEqual(expectedCount, InspectionResultsForModules(
3540
(MockVbeBuilder.TestModuleName, sutInputCode, ComponentType.StandardModule),
3641
("AClass", $"Option Explicit{Environment.NewLine}", ComponentType.ClassModule)).Count());
3742
}
3843

39-
[Test]
44+
[TestCase("TheVariant", 0)]
45+
[TestCase("TheVariant2", 1)] //Results in a readonly MyData property
4046
[Category("Inspections")]
4147
[Category("ReadOnlyPropertyAssignment")]
42-
public void SetUserDefinedClassSetExists_NotFlagged()
48+
public void VariantLet_CalledFromSameModule(string letPropertyName, long expectedCount)
4349
{
4450
var sutInputCode =
45-
@"Private mData As AClass
51+
$@"Option Explicit
52+
53+
Private myVariant As Variant
4654
4755
Public Sub Test()
48-
Set MyData = New AClass
56+
TheVariant = 7
4957
End Sub
5058
51-
Public Property Get MyData() As AClass
52-
Set MyData = mData
59+
Public Property Get TheVariant() As Variant
60+
If IsObject(myVariant) Then
61+
Set TheVariant = myVariant
62+
Else
63+
TheVariant = myVariant
64+
End If
5365
End Property
54-
Public Property Set MyData(RHS As AClass)
55-
Set mData = MyData
66+
Public Property Let {letPropertyName}(RHS As Variant)
67+
myVariant = RHS
5668
End Property
5769
";
58-
Assert.AreEqual(0, InspectionResultsForModules(
59-
(MockVbeBuilder.TestModuleName, sutInputCode, ComponentType.StandardModule),
60-
("AClass", $"Option Explicit{Environment.NewLine}", ComponentType.ClassModule)).Count());
70+
Assert.AreEqual(expectedCount, InspectionResultsForModules(
71+
(MockVbeBuilder.TestModuleName, sutInputCode, ComponentType.StandardModule)).Count());
6172
}
6273

63-
[Test]
74+
[TestCase("TheVariant", 0)]
75+
[TestCase("TheVariant2", 1)] //Results in a readonly MyData property
6476
[Category("Inspections")]
6577
[Category("ReadOnlyPropertyAssignment")]
66-
public void LetVariantMCVE_Flags()
78+
public void VariantSet_CalledFromSameModule(string setPropertyName, long expectedCount)
6779
{
6880
var sutInputCode =
69-
@"Option Explicit
81+
$@"Option Explicit
7082
7183
Private myVariant As Variant
7284
7385
Public Sub Test()
74-
TheVariant = 7
86+
Set TheVariant = new AClass
7587
End Sub
7688
7789
Public Property Get TheVariant() As Variant
@@ -81,98 +93,140 @@ If IsObject(myVariant) Then
8193
TheVariant = myVariant
8294
End If
8395
End Property
96+
Public Property Set {setPropertyName}(RHS As Variant)
97+
Set myVariant = RHS
98+
End Property
8499
";
85100

86-
Assert.AreEqual(1, InspectionResultsForModules(
87-
(MockVbeBuilder.TestModuleName, sutInputCode, ComponentType.StandardModule)).Count());
101+
Assert.AreEqual(expectedCount, InspectionResultsForModules(
102+
(MockVbeBuilder.TestModuleName, sutInputCode, ComponentType.StandardModule),
103+
("AClass", $"Option Explicit{Environment.NewLine}", ComponentType.ClassModule)).Count());
88104
}
89105

90-
[Test]
106+
[TestCase("MyData", 0)]
107+
[TestCase("MyData2", 1)] //Results in a readonly MyData property
91108
[Category("Inspections")]
92109
[Category("ReadOnlyPropertyAssignment")]
93-
public void LetVariantLetExists_NotFlagged()
110+
public void ObjectDataType_CalledFromOtherModule(string setPropertyName, long expectedCount)
94111
{
95112
var sutInputCode =
96-
@"Option Explicit
113+
$@"
114+
Option Explicit
97115
98-
Private myVariant As Variant
116+
Private mData As Collection
99117
100118
Public Sub Test()
101-
TheVariant = 7
119+
Set MyData = New Collection
102120
End Sub
103121
104-
Public Property Get TheVariant() As Variant
105-
If IsObject(myVariant) Then
106-
Set TheVariant = myVariant
107-
Else
108-
TheVariant = myVariant
109-
End If
122+
Public Property Get MyData() As Collection
123+
Set MyData = mData
110124
End Property
111-
Public Property Let TheVariant(RHS As Variant)
112-
myVariant = RHS
125+
126+
Public Property Set {setPropertyName}(ByVal RHS As Collection)
127+
Set mData = RHS
113128
End Property
114129
";
115-
Assert.AreEqual(0, InspectionResultsForModules(
116-
(MockVbeBuilder.TestModuleName, sutInputCode, ComponentType.StandardModule)).Count());
130+
131+
var callingModule =
132+
$@"
133+
Option Explicit
134+
135+
Public Sub Test()
136+
Set {MockVbeBuilder.TestModuleName}.MyData = New Collection
137+
End Sub
138+
";
139+
140+
Assert.AreEqual(expectedCount, InspectionResultsForModules(
141+
(MockVbeBuilder.TestModuleName, sutInputCode, ComponentType.StandardModule),
142+
("CallingModule", callingModule, ComponentType.StandardModule)).Count());
117143
}
118144

119-
[Test]
145+
[TestCase("MyData", 0)]
146+
[TestCase("MyData2", 1)] //Results in a readonly MyData property
120147
[Category("Inspections")]
121148
[Category("ReadOnlyPropertyAssignment")]
122-
public void SetVariantMCVE_Flags()
149+
public void ScalarDataType_CalledFromOtherModule(string letPropertyName, long expectedCount)
123150
{
124151
var sutInputCode =
125-
@"Option Explicit
152+
$@"
153+
Option Explicit
126154
127-
Private myVariant As Variant
155+
Private mData As Long
128156
129157
Public Sub Test()
130-
Set TheVariant = new AClass
158+
Set MyData = 8
131159
End Sub
132160
133-
Public Property Get TheVariant() As Variant
134-
If IsObject(myVariant) Then
135-
Set TheVariant = myVariant
136-
Else
137-
TheVariant = myVariant
138-
End If
161+
Public Property Get MyData() As Long
162+
MyData = mData
139163
End Property
164+
165+
Public Property Let {letPropertyName}(ByVal RHS As Long)
166+
mData = RHS
167+
End Property
168+
";
169+
170+
var callingModule =
171+
$@"
172+
Option Explicit
173+
174+
Public Sub Test()
175+
{MockVbeBuilder.TestModuleName}.MyData = 80
176+
End Sub
140177
";
141178

142-
Assert.AreEqual(1, InspectionResultsForModules(
179+
Assert.AreEqual(expectedCount, InspectionResultsForModules(
143180
(MockVbeBuilder.TestModuleName, sutInputCode, ComponentType.StandardModule),
144-
("AClass", $"Option Explicit{Environment.NewLine}", ComponentType.ClassModule)).Count());
181+
("CallingModule", callingModule, ComponentType.StandardModule)).Count());
145182
}
146183

184+
//TODO: Remove 'Ignore' once this false positive scenario is resolved
147185
[Test]
186+
[Ignore("False Positive")]
148187
[Category("Inspections")]
149188
[Category("ReadOnlyPropertyAssignment")]
150-
public void SetVariantSetExists_NotFlagged()
189+
public void FalsePositiveMCVE_ShouldNotFlag()
151190
{
152-
var sutInputCode =
153-
@"Option Explicit
191+
var fooClassCode =
192+
@"
193+
Option Explicit
154194
155-
Private myVariant As Variant
195+
Private mFooValue As String
156196
157-
Public Sub Test()
158-
Set TheVariant = new AClass
197+
'If you remove this Sub, the false positive does not occur
198+
Private Sub Class_Initialize()
199+
mFooValue = ""Test""
159200
End Sub
160201
161-
Public Property Get TheVariant() As Variant
162-
If IsObject(myVariant) Then
163-
Set TheVariant = myVariant
164-
Else
165-
TheVariant = myVariant
166-
End If
202+
Public Property Get FooValue() As String
203+
FooValue = mFooValue
167204
End Property
168-
Public Property Set TheVariant(RHS As Variant)
169-
Set myVariant = RHS
205+
206+
Public Property Let FooValue(ByVal RHS As String)
207+
mFooValue = RHS
208+
End Property
209+
";
210+
var sutInputCode =
211+
@"
212+
Option Explicit
213+
214+
'If Sub Test is placed after Property Get FooValue the
215+
'False Positive does not occur
216+
Public Sub Test()
217+
Dim fc As FooClass
218+
Set fc = New FooClass
219+
fc.FooValue = FooValue
220+
End Sub
221+
222+
Public Property Get FooValue() As String
223+
FooValue = ""Test""
170224
End Property
171225
";
172226

173227
Assert.AreEqual(0, InspectionResultsForModules(
174228
(MockVbeBuilder.TestModuleName, sutInputCode, ComponentType.StandardModule),
175-
("AClass", $"Option Explicit{Environment.NewLine}", ComponentType.ClassModule)).Count());
229+
("FooClass", fooClassCode, ComponentType.ClassModule)).Count());
176230
}
177231

178232
protected override IInspection InspectionUnderTest(RubberduckParserState state)

0 commit comments

Comments
 (0)