Sonarcloud considers the following VB.NET code a critical bug (actually two ones) under the
“S4275 - Getters and setters should access the expected fields” rule:
Class A
Private Property x As Integer
Public Property X As Integer
Get
Return x
End Get
Set(ByVal value As Integer)
x = value
End Set
End Property
End Class
(notice the additional “Property” in the second line)
I assume considering this a critical bug is a false positive, but what exactly is the impact of this error?
Welcome to our community! The provided reproducer doesn’t compile, because you can’t have two properties named x and X at the same time. There’s probably some copy/paste glitch.
Can you please edit the post to demonstrate the original issue?
yes, of course that does not compile, sorry. I was not posting a real example but it should have been a version reduced to my point. Just imagine the upper ‘X’ to be a lower one. (I can’t seem to find the edit function anymore?)
The intended point was the ‘Property’ in line 2. It will trigger the rule.
Edit:
Here the code as I meant it, for easy copy and paste:
Class A
Private Property x As Integer
Public Property x As Integer
Get
Return x
End Get
Set(ByVal value As Integer)
x = value
End Set
End Property
End Class
Thanks, now I see the problem! We have these wrong examples in the rule description itself. We’ll fix that.
What the rule should detect are cases like this - you have a field ready for the property, but you by accident or copy/paste mistake use some other one.
Class A
Private _x As Integer
Private _y As Integer
Public Property X As Integer
Get
Return _y ' Noncompliant, _x field should be used in X property
End Get
Set(ByVal value As Integer)
_y = value ' Noncompliant, _x field should be used in X property
End Set
End Property
End Class
Yes, I got what the rule meant to check. I was adding that the rule also triggers on a different bug, namely an additional “Property”, and am curious to know if this is just a minor thing the compiler seems to ignore anyway, or if this has more impact.
Having compilable example like this where property Y returns value of another property even when there exists a private field _y looks like the same copy/paste issue.
So I think it makes sense for the rule to trigger on this
Public Class Sample
Private _y As Integer
Public Property X As Integer
Public Property Y As Integer
Get
Return X
End Get
Set(value As Integer)
X = value
End Set
End Property
End Class
Please try to compile the following (should compile successfully).
The only issue here is the “Property” in the “Private Property x As Integer” line (line 2).
It triggers the rule too, which is a false positive, at least for the definition of the rule.
That redundant “Property” is very easy to miss, and I would like to know if that is an issue, or just something the compiler ignores anyway.
BC30269 'Private Property x As Integer' has multiple definitions with identical signatures.
BC30521 Overload resolution failed because no accessible 'x' is most specific for these arguments:
'Private Property x As Integer': Not most specific.
'Public Property x As Integer': Not most specific.
Looks like I can’t even write a very basic piece of code, but give me one last chance to show what I mean, this time real code I found:
Class A
Private Property _realName As String
Public Property RealName() As String
Get
Return _realName
End Get
Set(value As String)
_realName = value
End Set
End Property
End Class
This triggers two times rule vbnet:S4275, "Refactor this getter/setter so that it actually refers to the field ‘__realName’ ".
This is wrong, while the actual issue is the second line.
Now I see the issue raised Thanks for following up on the topic.
Why was it raised?
You now have an auto-property _realName. And compiler-generated backing field called __realName. That one is not visible in the code and intelli-sense, but it exists and you can use it (notice the double underscore):
Public Property RealName() As String
Get
Return __realName
End Get
Set(value As String)
__realName = value
End Set
End Property
The rule message complains about this __realName field because property RealName doesn’t use an existing field with the same name as the property (ignoring leading underscores)
Understanding your code
Why is there private auto property in the example? Does it have some purpose or was it intended to be a field instead and the S4275 was just a difficult way how to discover it?
If you intended to have a private field instead, would you like to have a new code smell rule like Use field instead when private auto-property is used?
Why is there private auto property in the example? Does it have some purpose or was it intended to be a field instead and the S4275 was just a difficult way how to discover it?
It was just an error a dev made while wanting a field. I stumbled over S4275 there and didn’t see what really caused it initially. A new rule would be better than flagging this a bug I guess, but I am not sure about the implications, maybe there is some legit use for this…?
I can think of two examples that we can easily take into account:
Public Interface INoisy
Property Count As Integer
End Interface
Public Class Collection
Implements INoisy
Private Property Count As Integer Implements INoisy.Count
End Class
and property decorated with attributes (maybe for serialization).
Otherwise, I don’t see a good reason to waste MSIL instructions/performance on the backing field access.
Thanks for following up on the topic.