-
Notifications
You must be signed in to change notification settings - Fork 231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix S3655 FN: VB implicit conversions #6991
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a problem on this extension of the ShouldExecute
: we are now finding false positives every time a = Nothing
is made.
When I paste the following example
Sub Assignment2(nullable As Integer?)
If nullable = Nothing Then
Dim x As Integer = nullable ' Noncompliant
End If
into a VB.NET Project or LINQPad I get the following warning from Roslyn: Warning BC42037 This expression will always evaluate to Nothing (due to null propagation from the equals operator). To check if the value is null consider using 'Is Nothing'.
nullable = Nothing
always evaluates to Nothing
, which is implicitly converted to the Boolean
value False
. As a result of that, Dim x As Integer = nullable
is never executed, and the non-compliance is in this case a false positive.
The same applies to all cases below. I think the right way of extending the rule was to capture Is Nothing
instead, which is the VB.NET equivalent of == null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I would just expand tests to include <> Nothing
.
@@ -584,6 +612,14 @@ Class WithAliases | |||
End Sub | |||
End Class | |||
|
|||
Class EqualsOperator | |||
Sub EqualsNothing(nullable As Integer?) | |||
If nullable = Nothing Then ' Always evaluates to Nothing (=> false) due to null propagation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a test with <> Nothing
(which has a behavior very similar to = Nothing
:
Function ReturnStatement(nullable As Integer?) As Integer
If nullable <> Nothing Then
Return nullable ' Noncompliant FP
End If
End Function
And also the following case, which shows how =
and <>
break tautology w.r.t. OrElse
:
Function ReturnStatement(nullable As Integer?) As Integer
If (nullable <> Nothing) OrElse (nullable = Nothing) Then
Return nullable ' Noncompliant
End If
End Function
Of course you could "exploit" the fact that =
and <>
lift Nothing
to get all sort of ternary logic behaviors. I would let you decide how deep you think it's worth exploring this path.
7ab51a7
to
ab5f3d3
Compare
119d33f
to
ad802d1
Compare
ab5f3d3
to
20b2bb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -584,6 +612,26 @@ Class WithAliases | |||
End Sub | |||
End Class | |||
|
|||
Class EqualsOperator | |||
Sub EqualsNothing(nullable As Integer?) | |||
If nullable = Nothing Then ' Always evaluates to Nothing (=> false) due to null propagation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would repeat this unusual comment in the testcases below as well.
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
Follow up to #6795