0

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
    doSomething()
ElseIf conditionB() AndAlso conditionC() Then
    doSomethingElse()
ElseIf conditionD() OrElse conditionE() Then
    doYetAnotherThing()
'... 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 value 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 default 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 Return is more readable than Exit Sub, is it really bad to use a single Exit For to break out of a For or a For Each loop? The SonarLint rule for Java permits the use of a single break; 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.

Crusha K. Rool
  • 1,502
  • 15
  • 24
  • More than one question per post is frowned on; as is you are asking for a coding tutorial more or less. For the first snippet all you need to do is get rid of `If conditionA() Then` to make the robot happy. While the rule might generally be a good idea, leaving `conditionA` in place can be of value: a) you can see it is accounted for at a glance b) it might be a placeholder for ever evolving requirements. Same with the case statement. Also there is a difference between Exit Sub/Return and Exit For. If the robot doesnt know that, get rid of it. – Ňɏssa Pøngjǣrdenlarp Oct 15 '16 at 18:18
  • Taking `If conditionA()` out of the equation would change the outcome. Just assume that conditionA(), conditionB() and conditionC() are all evaluating to be `true`. As I said, the order matters. You'd actually have to add the negated version of conditionA() to all ElseIf constructs to get the same outcome if you take the first case out. Sorry about the multiple questions. The post was actually more directed at the SonarLint devs themselves, since their website was pointing to Stackoverlow for issues. Hence why I just tried to crum multiple potential issues into one post. – Crusha K. Rool Oct 15 '16 at 18:26

1 Answers1

1

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: https://jira.sonarsource.com/browse/SLVS-1074

  • 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. https://jira.sonarsource.com/browse/SLVS-1075

Tamas
  • 6,260
  • 19
  • 30