Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix S3655 FN: VB implicit conversions #6991

Merged
merged 4 commits into from
Mar 28, 2023
Merged

Fix S3655 FN: VB implicit conversions #6991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a3b56b5
Expand ShouldExecute to cover more cases
Tim-Pohlmann Mar 27, 2023
7982bca
Removed wrong tests
Tim-Pohlmann Mar 27, 2023
20b2bb8
Add FP test cases
Tim-Pohlmann Mar 27, 2023
e4b3cdb
Review
Tim-Pohlmann Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions analyzers/src/SonarAnalyzer.VisualBasic/SymbolicExecution/Roslyn/EmptyNullableValueAccess.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,19 +65,16 @@ public override void VisitPredefinedCastExpression(PredefinedCastExpressionSynta
public override void VisitCTypeExpression(CTypeExpressionSyntax node) =>
HasPotentialNullableValueAccess = true;

public override void VisitInvocationExpression(InvocationExpressionSyntax node)
{
if (node.NameIs("CTypeDynamic"))
{
HasPotentialNullableValueAccess = true;
}
else
{
base.VisitInvocationExpression(node);
}
}
public override void VisitInvocationExpression(InvocationExpressionSyntax node) =>
HasPotentialNullableValueAccess = true;

public override void VisitAssignmentStatement(AssignmentStatementSyntax node) =>
HasPotentialNullableValueAccess = true;

public override void VisitEqualsValue(EqualsValueSyntax node) =>
HasPotentialNullableValueAccess = true;

public override void VisitReturnStatement(ReturnStatementSyntax node) =>
HasPotentialNullableValueAccess = true;
}
}
56 changes: 52 additions & 4 deletions ...sts/SonarAnalyzer.UnitTest/TestCases/SymbolicExecution/Roslyn/EmptyNullableValueAccess.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,9 +509,10 @@ Class Assignments
nullable = Nothing
Dim y As Integer = nullable ' Noncompliant
End Sub
Sub Assignment2()
Dim nullable As Integer? = Nothing
Dim x As Integer = nullable ' FN
Sub Assignment2(nullable As Integer?)
If nullable Is Nothing Then
Dim x As Integer = nullable ' Noncompliant
End If
End Sub
End Class

Expand Down Expand Up @@ -574,6 +575,33 @@ Class Casts
Sub CTypeDynamicWithNonNullLiteral(i As Integer?)
Dim x = (CTypeDynamic(Of Integer?)(42)).Value
End Sub

Function ReturnStatement(nullable As Integer?) As Integer
If nullable Is Nothing Then
Return nullable ' Noncompliant
End If
End Function

Function ReturnAssignment(nullable As Integer?) As Integer
If nullable Is Nothing Then
ReturnAssignment = nullable ' Noncompliant
End If
End Function

Sub Method(nullable As Integer?)
If nullable Is Nothing Then
Method2(nullable) ' Noncompliant
End If
End Sub

Sub Method2(i As Integer)
End Sub

Sub IfExpression(nullable As Integer?)
If nullable Is Nothing Then
Dim x As Integer = If(True, nullable, nullable) ' Noncompliant
End If
End Sub
End Class

Class WithAliases
Expand All @@ -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
Copy link
Contributor

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.

Copy link
Contributor

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.

Dim x As Integer = nullable ' Noncompliant FP, nullable was not actually checked for null
End If
End Sub

Sub UnequalsNothing(nullable As Integer?)
If nullable <> Nothing Then ' Always evaluates to Nothing (=> false) due to null propagation
Dim x As Integer = nullable ' Compliant
End If
End Sub

Sub UqualsOrUnequals(nullable As Integer?)
If (nullable <> Nothing) OrElse (nullable = Nothing) Then
Dim x As Integer = nullable ' Noncompliant FP
End If
End Sub
End Class

Namespace TypeWithValueProperty
Class TypeWithValueProperty
Private Sub Basics1()
Expand All @@ -597,7 +645,7 @@ Namespace TypeWithValueProperty
End Sub

Private Sub ImplicitCast()
Dim i As StructWithValuePropertyAndCastOperators = CTypeDynamic(Of Integer?)(Nothing)
Dim i As StructWithValuePropertyAndCastOperators = Nothing
Dim x = i.Value
End Sub

Expand Down