Crusha K. Rool Crusha K. Rool - 1 year ago 164 Question

SonarLint - questions about some of the rules for VB.NET

The large majority of SonarLint rules that I've come across in Java seemed plausible and justified. However, ever since I've started using SonarLint for VB.NET, I've come across several rules that left me questioning their usefulness or even whether or not they are working correctly.

I'd like to know if this is simply a problem of me using some VB.NET constructs in a suboptimal way or whether the rule really is flawed.
(Apologies if this question is a little longer. I didn't know if I should create a separate question for each individual rule.)

The following rules I found to leave some cases unconsidered that would actually turn up as false-positives:

  • S1871: Two branches in the same conditional structure should not have exactly the same implementation

    I found this one to bring up a lot of false-positives for me, because sometimes the order in which the conditions are checked actually does matter. Take the following pseudo code as example:

    If conditionA() Then
    ElseIf conditionB() AndAlso conditionC() Then
    ElseIf conditionD() OrElse conditionE() Then
    '... feel free to have even more cases in between here
    Else Then
    doSomething() 'Non-compliant
    End If

    If I wanted to follow this Sonar rule and still make the code behave the same way, I'd have to add the negated version of each ElseIf-condition to the first If-condition.

    Another example would be the following switch:

    Select Case i
    Case 0 To 40
    value = 0
    Case 41 To 60
    value = 1
    Case 61 To 80
    value = 3
    Case 81 To 100
    value = 5
    Case Else
    value = 0 'Non-compliant

    There shouldn't be anything wrong with having that last case in a switch. True, I could have initialized
    beforehand to 0 and ignored that last case, but then I'd have one more assignment operation than necessary. And the Java ruleset has conditioned me to always put a
    case in every switch.

  • S1764: Identical expressions should not be used on both sides of a binary operator

    This rule does not seem to take into account that some functions may return different values every time you call them, for instance collections where accessing an element removes it from the collection:

    stack.Push(stack.Pop() / stack.Pop()) 'Non-compliant

    I understand if this is too much of an edge case to make special exceptions for it, though.

The following rules I am not actually sure about:

  • S3385: "Exit" statements should not be used

    While I agree that
    is more readable than
    Exit Sub
    , is it really bad to use a single
    Exit For
    to break out of a
    or a
    For Each
    loop? The SonarLint rule for Java permits the use of a single
    in a loop before flagging it as an issue. Is there a reason why the default in VB.NET is more strict in that regard? Or is the rule built on the assumption that you can solve nearly all your loop problems with LINQ extension methods and lambdas?

  • S2374: Signed types should be preferred to unsigned ones

    This rule basically states that unsigned types should not be used at all because they "have different arithmetic operators than signed ones - operators that few developers understand". In my code I am only using UInteger for ID values (because I don't need negative values and a Long would be a waste of memory in my case). They are stored in List(Of UInteger) and only ever compared to other UIntegers. Is this rule even relevant to my case (are comparisons part of these "arithmetic operators" mentioned by the rule) and what exactly would be the pitfall? And if not, wouldn't it be better to make that rule apply to arithmetic operations involving unsigned types, rather than their declaration?

  • S2355: Array literals should be used instead of array creation expressions

    Maybe I don't know VB.NET well enough, but how exactly would I satisfy this rule in the following case where I want to create a fixed-size array where the initialization length is only known at runtime? Is this a false-positive?

    Dim myObjects As Object() = New Object(someOtherList.Count - 3) {} 'Non-compliant

    Sure, I could probably just use a List(Of Object). But I am curious anyway.

Answer Source

Thanks for raising these points. Note that not all rules apply every time. There are cases when we need to balance between false positives/false negatives/real cases. For example with identical expressions on both sides of an operator rule. Is it a bug to have the same operands? No it's not. If it was, then the compiler would report it. Is it a bad smell, is it usually a mistake? Yes in many cases. See this for example in Roslyn. Should we tune this rule to exclude some cases? Yes we should, there's nothing wrong with 2 << 2. So there's a lot of balancing that needs to happen, and we try to settle for an implementation that brings the most value for the users.

For the points you raised:

  • Two branches in the same conditional structure should not have exactly the same implementation

This rule generally states that having two blocks of code match exactly is a bad sign. Copy-pasted code should be avoided for many reasons, for example if you need to fix the code in one place, you'll need to fix it in the other too. You're right that adding negated conditions would be a mess, but if you extract each condition into its own method (and call the negated methods inside them) with proper names, then it would probably improves the readability of your code.

For the Select Case, again, copy pasted code is always a bad sign. In this case you could do this:

Select Case i
  Case 0 To 40
  Case Else
    value = 0 ' Compliant
End Select

Or simply remove the 0-40 case.

  • Identical expressions should not be used on both sides of a binary operator

I think this is a corner case. See the first paragraph of the answer.

  • "Exit" statements should not be used

It's almost always true that by choosing another type of loop, or changing the stop condition, you can get away without using any "Exit" statements. It's good practice to have a single exit point from loops.

  • Signed types should be preferred to unsigned ones

This is a legacy rule from SonarQube VB.NET, and I agree with you that it shouldn't be enabled by default in SonarLint. I created the following ticket in our JIRA:

  • Array literals should be used instead of array creation expressions

Yes, it seems to be a false positive, we shouldn't report on array creations when the size is explicitly specified.

Recommended from our users: Dynamic Network Monitoring from WhatsUp Gold from IPSwitch. Free Download