S125 False positive when comment ends with a ;

Rules and Languages Report False-positive / False-negative...
1
  • What language is this for? C#
  • Which rule? S125
  • Why do you believe it’s a false-positive/false-negative?

I acknowledge there’s an older topic about this false positive and I’ve also seen one GitHub issue about it, but I wanted to ask if we could reconsider or if the team could at least try to improve this rule. In the short few months I’ve been using Sonar, I’ve already encountered a few irritating instances of it (and, ironically although not surprisingly, not even a single true positive).

One big offender for me is putting ‘ends with a semicolon’ in the list of rules of your heuristic – that just seems too broad for me. There is at least one common way where phrases can terminate with a semicolon, and that’s lists:

  • This one;
  • Then this one;
  • Then that one; and
  • Finally, this one here.

The first two lines above are flagged by Sonar as commented code.

But then, considering source files need line breaks in between long expressions or long sentences in comments, then you also need to be careful where you break your lines so that you don’t break in the wrong place;
Otherwise Sonar will force you to rewrite your sentences!

And then there’s of course the issue of not being able to legitimately include code in comments (eg. giving examples or explaining how/why something was done).

And many many other false positives generated by this rule that you can find people complaining about on the internet.

In both of the issues I saw about this particular false positive, a Sonar team member responded saying that this is by design.

So is my understanding correct that the following truly is the Sonar’s team recommendation for developers who frequently hit this?
1) globally disable the rule in your projects; or
2) litter your code with #pragmas; or
3) change your writing style.

When a rule frequently forces you to write code – or, in this case, natural language text!! – differently only so that you don’t hit the rule (as opposed to writing better code), what is the point?

Does you data really suggest that this rule triggers more true positives than false ones? I truly do find that hard to believe.

Would it at least be possible to allow customization of the heuristic?

  • Are you using
    • SonarQube Cloud? No
    • SonarQube Server / Community Build - which version? No
    • SonarQube for IDE - which IDE/version? SonarQube for Visual Studio version 8.30.0.15605
      • in connected mode with SonarQube Server / Community Build or SonarQube Cloud? No
  • How can we reproduce the problem? Give us a self-contained snippet of code (formatted text, no screenshots) A few examples above;
Sonar Community Roundup; Feb 7 - 13
3

If it is just a regular comment:

// * This one;
// * Then this one;
// * Then that one; and
// * Finally, this one here.

Can be written as:

// * This one
// * Then this one
// * Then that one
// * Finally, this one here

And if the punctuation matters:

/// <remarks>
/// * This one;
/// * Then this one;
/// * Then that one; and
/// * Finally, this one here.
/// </remarks>

The latter does not report as far as I know, (and otherwise should not).

4

Hi @andre-ss6,

I understand that the heuristic is not precise. It’s heuristic. So yes, all your 1), 2) and 3) are the possible solutions here, depending on your project and situation.

What is your proposal to improve it, while preserving most of it’s value and performance? C# code is known for ending with a semicolon, so it’s the most natural heuristic rule to choose.

6

Hi Pavel,

while preserving most of it’s value and performance

So, one of my questions is exactly how much value does detecting this specific pattern (the semicolon) add to the rule? Do you have data on that (eg how many times S125 is triggered because of a semicolon, how many are ignored, etc)? Not meaning to be inquisitorial, just curious. I know there are many other reports about this same issue, but maybe we’re a minority.

Of course I have a very limited and biased sample - especially given I’ve been using the analyzer for only a few months - but for what it’s worth, as I said in the OP my experience has been that the warnings triggered by this pattern have not been worth it. The heuristic was able to correctly detect (where it was an actual issue) commented code in a couple instances - and was able to detect them even if I removed the semicolons - while all the instances where it detected commented code only because of a semicolon were a false positive.

What is your proposal to improve it

I hinted at two ideas/suggestions in the OP. Expanding a little bit more on them:

  1. Reconsider this design decision, ie whether having lines ending with a semicolon in the heuristic is actually bringing value enough to outweigh the hassle.

  2. allow customization of the rules/patterns included in the heuristic, eg. via editorconfig, so that we could, for instance, disable the “line terminates with semicolon” pattern. Not sure how realistic this is.

So yes, all your 1), 2) and 3) are the possible solutions here, depending on your project and situation.

Those being the only solutions seems to me like evidence that the rule should change.

7

Hi,

You’re missing a third option: disable the rule. We work hard to cover 90% of cases for 90% of people. It seems that for this rule at least, you’re in the 10%. You’ve said you don’t get value from the rule. So perhaps its time to pull the plug on it.

 
HTH,
Ann

8

Most of the C# code lines ends with a semicolon. That by itself covers most of the true-positive findings. While we don’t have this precise data (we don’t see why rule decided to raise), I think it’s a safe assumption to make based on our test analysis.

The parameter would be an option. Yet we’re trying to avoid parameter hell of too many parameters on our rules as a general principle. That’s why we have only very little one, like a line length on the “Line is too long” rule, where it’s unavoidable.

There’s also 4th option:
To maximize the value of our product and your experience, you could analyze your project on SonarQube Cloud, or SonarQube Server, and setup the Connected Mode in your SonarQube for IDE extension. Then you can mark raised issues as “Accepted” or “False Positive”. Such issues than do not show up again.

1 Like
10

Hello internet person,

If you had decided to actually read the original post and respect other people’s time and opinions before writing your reply, you would see that your “missing third option” was the first of three options already presented there, and which have also been mentioned in the two replies above yours.

A bit of empathy combined with even a little bit more respect might get you 100% there.

Thanks.

11

Most of the C# code lines ends with a semicolon. That by itself covers most of the true-positive findings.

I understand that. What I was questioning is whether the rest of the patterns aren’t already sufficient, especially considering the tradeoffs of each.

Well, I’ve presented my arguments. I still don’t think a rule that asks you to change your natural language writing style – which does nothing to improve code readability, safety, maintainability or any other code quality metric – makes a lot of sense, and the only solution to that being to disable the rule makes me further question its validity. I’m not the first to raise this issue; A Google search on the topic shows tens of different issues/requests/questions about this. I don’t know where the 10% number the person above quoted comes from, but whether your data actually show that or the team is simply unwilling to reconsider, there isn’t much else I can argue.

Thanks.

12

After giving it some thought, I’ve created internal ticket NET-3164 (not accessible from outside) to improve the heuristic by adding “if it looks like sentence, don’t raise”.

It won’t solve all the problems, but might improve the experience.

2 Likes