squid:S128 and allowing developer specified FALL-THROUGH in the switch block

[Paul Wagland]

Hi all,

First up, thanks again for a wonderful product!

As I understand it, one of the design goals for SonarQube is not require modifications to the source in order to be able to fully utilise it.

One of the the common themes that I see in our code is that we have $FALL-THROUGH$ comments in many, but not all!!, of our switch statements. For example:

      switch (topTag.state)

      {

      case Tag.STATE_WROTE_STAG:

      case Tag.STATE_WROTE_ATTR:

        xw.write(">");

        // we fall through here since we have just finished writing out a tag,

        // and we want to pretty indent the new tag on a new line.

        //$FALL-THROUGH$

      case Tag.STATE_WROTE_CHILD:

        xw.newline();

        xw.indent(topTag.level + 1);

        break;

      default:

        throw new XmlWriteException("writeSTag: " + name + " state=" + topTag.state);

      }

SonarQube still complains about this, even though it has been marked as being OK by the developer. It would be nice if SonarQube could look at that $FALL-THROUGH$ and not raise a squid:S128 for this case.

Cheers,

Paul

[Nicolas Peru]

Hi Paul, 

This comment is Eclipse specific. While we could think about supporting such cases to not raise an issue in this case, we think it is not the best approach to have some incitation towards cluttering the code with tool specific comment to shutdown warnings and rather enforce the use of marking issues as won't fix in SonarQube UI. This is the same idea behind not enforcing usages of // NOSONAR

The tool should not be intrusive to the code. 

HTH, 

Cheers, 

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/91467c56-e040-43bf-a3bc-dae0fce741e9%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

[Paul Wagland]

Hi Nicolas,

For something like this, I believe that the code should reflect the intention. It is very frustrating to your workflow to have to go to another tool in order to find out whether or not the fall through in this case statement is allowed or not.

For example:

      switch (topTag.state)

      {

      case Tag.STATE_WROTE_STAG:

      case Tag.STATE_WROTE_ATTR:

        xw.write(">");

      case Tag.STATE_WROTE_CHILD:

        xw.newline();

        xw.indent(topTag.level + 1);

        break;

      default:

        throw new XmlWriteException("writeSTag: " + name + " state=" + topTag.state);

      }

Is the above code wrong or not? Well, if you go to our internal SonarQube instance you can see that this is intentional. I think that we would both agree having the comment here adds value. I would propose doing something similar to what CheckStyle does whereby "By default the comments "fallthru", "fall through", "falls through" and "fallthrough" are recognized.". Putting comments like this into the code is very common, for example Google: https://google.github.io/styleguide/javaguide.html#s4.8.4-switch  and Oracle: http://www.oracle.com/technetwork/java/codeconventions-150003.pdf (Section 7.8) both recommend this.

This should also, ideally, also work for the Eclipse annotation as well.

And while I am happy to say that the tool should not be intrusive to the code, where coding guidelines recommend that there should be comments in place, I think that SonarQube could use those comments as extra information. No-one is saying that you have to use them, but if you do, why enforce that the warning must be removed from multiple tools?

Cheers,

Paul

[sraul....@gmail.com]

Hello everyone.

Nicolas, I agree with Paul. I came across this problem also recently.

The code should reflect the intention. And I don't think we can compare this with //NOSONAR example, because I don't see anyone using the //$FALL-THROUGH$ comment just to ignore the issue.

Developer may mark the issue as "won't fix", but this is simply consuming time, unnecessarily.

cheers,

Silvestre

[Michael Piefel]

Am Dienstag, 14. Februar 2017 14:24:37 UTC+1 schrieb Nicolas Peru:

This comment is Eclipse specific. While we could think about supporting such cases to not raise an issue in this case, we think it is not the best approach to have some incitation towards cluttering the code with tool specific comment to shutdown warnings and rather enforce the use of marking issues as won't fix in SonarQube UI. This is the same idea behind not enforcing usages of // NOSONAR

The tool should not be intrusive to the code. 

Hello Nicolas,

just to add once more that I disagree very much on that matter. Marking something in the UI is not good enough. It will pop up in other branches when Bamboo auto-creates Sonarqube projects for them. It will pop up in other tools. It will pop up during code reviews. It will pop up later during maintenance. Suppressing warnings because they are false positives has to be done on the code itself, and checked into version control.

Of course a tool-specific comment is not ideal. But it’s the best we have when there is no standard Java @SuppressWarnings for the issue. Each fall-through in a switch statement has to be documented anyway, so a tool-specific comment will not clutter the code at all.

Bye,

   Mike

[Paul Wagland]

Hello Sonar,

With the supporting arguments below, is there any chance that we could get this added to the SonarQube rule? Again today the only reason that I needed to go to SonarQube is to mark this as WONTFIX, even though the code is already appropriately commented. This isn't "getting out of my way", it's just adding busy work to the process.

Cheers,

Paul

[Nicolas Peru]

Hi, 

sorry for the late reply. 

Given this stackoverflow question : https://stackoverflow.com/questions/13145679/what-does-fall-through-do-in-eclipse and especially the link to the introduction and reasons of this eclipse specific comment http://archive.eclipse.org/eclipse/downloads/drops/R-3.5-200906111540/eclipse-news-part2.html 

If my understanding is correct this was mainly introduced for code prior to java 5 which would not be able to use @SuppressWarnings("fallthrough"). Given that we are on the edge of releasing java 9 I tend to think the correct way to solve this issue and having a nice way to not have SonarJava wrongly raise an issue would be to support the  @SuppressWarnings("fallthrough") annotation for squid:S128 (so not relying on a comment). 

What do you think ? 

Cheers, 

--

You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/16646dde-cefe-480c-aefd-8c373d1226ab%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Nicolas Peru | SonarSource

Senior Developer

http://twitter.com/benzonico

http://sonarsource.com

[sraul....@gmail.com]

Hi,

I still think the comment inline is a better way to explicitly tell to whoever reads that code, that the fall-through case statement was intentional, because he/she will see that near to the code that it refers to.

IMO this rule could support both: if an inline comment or @SuppressWarnings("fallthrough") annotation is present, then do not raise an issue.

But if I have to add a fall-through case statement, I will avoid to use the @SuppressWarnings("fallthrough") because it automatically suppresses all cases, even a new one is added that is actually not intended to be fall-through.

cheers,

Silvestre

--

[Paul Wagland]

Hi all,

I normally hate "me too" type replies, but as the originator of this thread I would still like to chime in.

I agree with Silvestre completely.

Personally, I have suppressing all warnings within a method, and would much rather introduce additional variables in order to localise the suppression, than putting the suppression on the method. For pretty much the same reasons that Silvestre outlined, I don't like "area of effect" exclusions.

Cheers,

Paul

[Nicolas Peru]

Hi, 

I am now closing this topic. To be clear : we are not going to implement a tool specific comment which was introduced to deal with old version of Java. Given your feedback and the lack of demand for it I am neither creating a ticket for supporting @SuppressWarning("fallthrough") as there seems to be no interest for it. 

This is most probably not the answer you expected but we struggle to see the ROI for this feature. 

Cheers, 

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/c7b85b47-a636-4944-ab1f-d69598591723%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[sraul....@gmail.com]

Well, that's sad :(

But I understand your point. However the fact that "there seems to be no interest for it", I'm a little ceptic, because a lot of people complain about these Sonar issues, but they simply don't report here.

Anyway... so the only way to ignore this issue is to either marking it as WON'T FIX or to add a //NOSONAR comment.

Is there any template rule we could use similar to //NOSONAR but where we could configure the comment?
That would allow for anyone to customize what lines to ignore, not only for this particular issue but for any project/tool specific usage, without polluting the code with Sonar specific comments.

cheers,

Silvestre

[Michael Piefel]

Am Dienstag, 18. Juli 2017 09:24:32 UTC+2 schrieb Nicolas Peru:

I am now closing this topic. To be clear : we are not going to implement a tool specific comment which was introduced to deal with old version of Java. Given your feedback and the lack of demand for it I am neither creating a ticket for supporting @SuppressWarning("fallthrough") as there seems to be no interest for it. 

Hi Nicolas,

close it if you think that’s right. But the reasoning remains faulty: “introduced to deal with old version of Java” was wrong even in that Eclipse changelog and the StackOverflow article you cite, because the annotation simply cannot annotate a single switch-case-branch. The comment serves a more specific purpose, the annotation is a very blunt weapon.

Bye,
    Michael

[Paul Wagland]

Hi Nicolas,

This is not the answer I wanted, you are correct. But it is your call.

As to it being a tool specific comment, that is not entirely true, see the below list of tools that would work with this comment or a variant of it:

1. IntelliJ would also accept this comment: https://youtrack.jetbrains.com/issue/WEB-8350
   
2. Netbeans would accept something similar: https://netbeans.org/bugzilla/show_bug.cgi?id=216547
3. Oracle coding conventions: (section 7.8) http://www.oracle.com/technetwork/java/codeconventions-150003.pdf
4. Google coding guideline (section 4.8.4.2) https://google.github.io/styleguide/javaguide.html#s4.8.4-switch
5. Google error prone: https://github.com/google/error-prone/issues/68

Cheers,

Paul

[Michael Gumowski]

Hey guys,

We discussed internally how we could handle it, and created the following ticket: SONARJAVA-2418

Comments "fall-trough", or "falls-trough" (with or without the dash, ignoring case) will be supported when missing break in case statement.

Cheers,

Michael

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/25745b78-b718-4075-b11c-14a352b56f5b%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Michael Gumowski | SonarSource

Software Developer, Language Team
http://sonarsource.com

[Paul Wagland]

Hi Michael,

This is great news! Thanks for the update, and I look forward to the updated Java scanner that has this fix :-)

Cheers,

Paul