[java] MissingBreakInSwitch - last default case does not contain a break

[zolyfarkas]

Rule: MissingBreakInSwitch

Description:

A switch statement does not contain a break, even if it is the default case.

Code Sample demonstrating the issue:

https://www.codacy.com/app/zolyfarkas/spf4j/issues?&filters=W3siaWQiOiJDYXRlZ29yeSIsInZhbHVlcyI6WyJFcnJvciBQcm9uZSJdfV0=

        switch (precission) { 
            case 32: 
                mc = MathContext.DECIMAL32; 
                break; 
            case 64: 
                mc = MathContext.DECIMAL64; 
                break; 
            case 128: 
                mc = MathContext.DECIMAL128; 
                break; 
            default: 
                mc = new MathContext(precission); 
        } 

Running PMD through: Codacy

[jsotuyod]

I think a couple of things should be considered here:

1. Is the point of having a break on all cases to be able to reorder the cases at will without breaking? If so, then having it at the last one is needed.
2. Shall any case skip the break, or only a default tag? Bare in mind, the convention is to have default last, but Java actually supports it in any position.

Maybe the best scenario would be to have a boolean property to allow simply skipping the last case, and in any case, and keep a separate rule to enforce the default being last (java-bestpractices/DefaultLabelNotLastInSwitchStmt ). This is equivalent to what the equivalent Checkstyle rule does, defaulting to skipping the last case.

[marcelmore]

The combination of best practce rule for default being last and allowing to skip the last, whichever it is, sounds like the optimal solution to me.

[nomaed]

Are there any plans to support last default without a break?

While it's a good idea to raise a warning on non-last default cases without breaks, of course, the standard code style in pretty much everywhere I've seen is to use default as the last case - without a break, but the current rule creates false positives.

[jsotuyod]

The approved approach is to extend MissingBreakInSwitch to include a skipLastCase boolean property defaulting to false.

There is currently no-one taking on this, but being an XPath rule should be easily extensible by anyone willing to contribute to PMD.

[linusjf]

package networking;

import java.io.IOException;
import java.io.InputStream;
import java.net.Socket;
import java.util.Calendar;
import java.util.Date;
import java.util.TimeZone;

@SuppressWarnings("PMD.ShortClassName")
public final class Time {
  private static final String HOSTNAME = "time.nist.gov";

  private Time() {
    throw new IllegalStateException("Private constructor");
  }

  @SuppressWarnings("fallthrough")
  public static void main(String[] args) {
    try {
      Date d;
      switch (args.length) {
        case 0:
          d = Time.getDateFromNetwork();
          System.out.println("It is " + d);
          System.exit(0);

        case 1:
          d = Time.getDateFromNetwork(args[0], 37);
          System.out.println("It is " + d);
          System.exit(0);

        case 2:

        default:
          d = Time.getDateFromNetwork(args[0], Integer.parseInt(args[1]));
          System.out.println("It is " + d);
          System.exit(0);
      }
    } catch (IOException e) {
      System.err.println(e.getMessage());
    }
  }

  public static Date getDateFromNetwork() throws IOException {
    return getDateFromNetwork(HOSTNAME, 37);
  }

  public static Date getDateFromNetwork(String host, int port)
      throws IOException {
    // The time protocol sets the epoch at 1900,
    // the Java Date class at 1970. This number
    // converts between them.
    // long differenceBetweenEpochs = 2208988800L;
    TimeZone gmt = TimeZone.getTimeZone("GMT");
    Calendar epoch1900 = Calendar.getInstance(gmt);
    epoch1900.set(1900, 01, 01, 00, 00, 00);
    long epoch1900ms = epoch1900.getTime().getTime();
    Calendar epoch1970 = Calendar.getInstance(gmt);
    epoch1970.set(1970, 01, 01, 00, 00, 00);
    long epoch1970ms = epoch1970.getTime().getTime();
    long differenceInMS = epoch1970ms - epoch1900ms;
    long differenceBetweenEpochs = differenceInMS / 1000;
    Socket socket = new Socket(host, port);
    socket.setSoTimeout(15_000);
    InputStream raw = socket.getInputStream();

    long secondsSince1900 = 0;
    for (int i = 0; i < 4; i++) {
      secondsSince1900 = (secondsSince1900 << 8) | raw.read();
    }
    long secondsSince1970 = secondsSince1900 - differenceBetweenEpochs;
    long msSince1970 = secondsSince1970 * 1000;
    return new Date(msSince1970);
  }
}

The empty case statement breaks the above rule giving a false positive. Obviously, there's not a fix in for this as yet. Do you have a workaround or do we suppress the rule until a fix is in?

Why can't PMD recognise the SuppressWarnings for fall through or comments beside the case statements stating the same?

[adangel]

@Fernal73 I guess, the rule triggers, because you have not a single break in your switch statement. Since you already have the variable d introduced, I'd suggest to move out the printing end exiting after the switch and use breaks inside the switch cases. I'm not sure, whether you really need to distinguish between "2" and "default" - you could just remove the case 2, then you don't have a fall-through.

Why can't PMD recognise the SuppressWarnings for fall through or comments beside the case
statements stating the same?

Would you mind creating a new feature request for this? We have this a little bit e.g. for rules, that check unused code - @SuppressWarnings("unused") suppresses e.g. the rules "UnusedPrivateField", "UnusedLocalVariable", "UnusedPrivateMethod", and "UnusedFormalParameter".
Adding more common known tags would make sense. e.g. the eclipse compiler lists the tags here: https://help.eclipse.org/2019-06/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Ftasks%2Ftask-suppress_warnings.htm

[linusjf]

I didn't need a break statement since System.exit exits not just the function but also the program. The error flagged was about a failure to catch the fall through. Adding a break statement would simply make it unreachable anyway. I didn't connect it to not having a single break statement. Thanks for helping refactor the code. That does take out the PMD error. Is the SuppressWarning("fallthrough") suppressing any or all of the PMD related rules? My intention is not to test every PMD rule and ways to break it. That's incidental. I guess this PMD rule wasn't 'fool-proof'. It wouldn't catch bad programming practice. A question arises though: Should System.exit be put on par with break and return tokens for this rule? How does this rule handle throw statements? But then again, why in heaven would anyone want to do that? Any scenarios come to mind? The last time I wrote up an issue, you closed it as a duplicate. What guarantee is there that you won't do the same once more? https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.html

[linusjf]

I didn't need a break statement since System.exit exits not just the function but also the program. The error flagged was about a failure to catch the fall through. Adding a break statement would simply make it unreachable anyway. I didn't connect it to not having a single break statement. Thanks for helping refactor the code. That does take out the PMD error. Is the SuppressWarning("fallthrough") suppressing any or all of the PMD related rules? My intention is not to test every PMD rule and ways to break it. That's incidental. I guess this PMD rule wasn't 'fool-proof'. It wouldn't catch bad programming practice. A question arises though: Should System.exit be put on par with break and return tokens for this rule? How does this rule handle throw statements? But then again, why in heaven would anyone want to do that? Any scenarios come to mind? The last time I wrote up an issue, you closed it as a duplicate. What guarantee is there that you won't do the same once more? https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/FallThroughCheck.html

Revisiting this post, I couldn't help wondering if PMD needs an UnreachableCode rule in the case of statements like above where the program is exited and evidently any statements after that are null and void. Any statements immediately following System.exit, Runtime.halt and Runtime.exit in the same code block must be considered unreachable.

[adangel]

This has been fixed with PMD 7.0.0-rc1.