JMRI Code: Static Analysis with SpotBugs

SpotBugs is a tool that can examine code to find possible problems. It does a "static analysis", looking through the code to find certain "known bad ideas": Things that are likely to cause occasional/intermittent problems, poor performance, etc. Because those kinds of problems are hard to find with tests, finding them by inspection is often the only realistic approach. Having a tool that can do those inspections automatically, so that they can be done every time something is changed, keeps the code from slowly getting worse and worse without anybody noticing until it's too late.

For more information on SpotBugs, see the SpotBugs home page.

We routinely run SpotBugs as part of our continuous integration process. You can see the detailed results of the most recent (daily) run along with recent progress trend online.

If SpotBugs is finding a false positive, a bug that's not really a bug, you can turn it off with an annotation such as:

import edu.umd.cs.findbugs.annotations;
@SuppressFBWarnings("FE_FLOATING_POINT_EQUALITY", "Even tiny differences should trigger update")

Although Java itself considers it optional, we require the second "justification" argument. Explaining why you've added this annotation to suppress a message will help whoever comes after you and is trying to understand the code. It will also help make sure you properly understand the cause of the underlying bug report: Sometimes what seems a false positive really isn't. Annotations without a justification clause will periodically be removed. Note that the @SuppressFBWarnings contents in this form should be all on one line so that automated scanners can more reliably see it.

For clarity, this annotation also supports a form that lets you be more verbose:

@edu.umd.cs.findbugs.annotations.SuppressFBWarnings(value = "FE_FLOATING_POINT_EQUALITY", 
        justification = "OK to compare floats, as even tiny differences should trigger update")

This can make it easier to see what is what when quickly scanning through the code.

If you need to put more than one message type in an annotation, use array syntax:

@edu.umd.cs.findbugs.annotations.SuppressFBWarnings("{type1},{type2}","why both are needed")

There are also Java and SpotBugs annotations that can help it better understand your code. Sometimes they'll give it enough understanding of e.g. when a variable can be null, that it'll no longer make false-positive mistakes. For more on this, see the Java annotations and SpotBugs annotation pages.

The basics of annotations are covered in a Java annotations tutorial.

It can be useful to mark code with one of the following annotations so that SpotBugs does a good job of reasoning about it:

javax.annotation.Nonnull - The annotated element must not be null. Annotated fields must not be null after construction has completed. Annotated methods must have non-null return values. Use javax.annotation.ParametersAreNonnullByDefault on the class declaration (the start of the class) to set @Nonnull for an entire class. Note that SpotBugs won't let you compare an @Nonnull value to null; that's an error that SpotBugs wants to find via static analysis. If your annotating a part of the external API, and you want to double-check at runtime, use java.util.Objects.requireNonNull(..), for example:
```
        public Car(@Nonnull Engine engine, @Nonnull Transmission transmission) {
            this.engine = java.util.Objects.requireNonNull(engine, "Engine cannot be null");
            this.transmission = java.util.Objects.requireNonNull(transmission, "Transmission cannot be null");
        }
```
javax.annotation.CheckForNull - the annotated variable, parameter or return value may have a null value, so all uses should appropriately handle that. Please put this on method definitions to say that the return value should be checked for null.
javax.annotation.OverridingMethodsMustInvokeSuper - Used to annotate a method that, if overridden, must (or should) be invoke super in the overriding method. Examples of such methods include finalize() and clone().
Note this replaces the deprecated edu.umd.cs.findbugs.annotations.OverrideMustInvoke.
javax.annotation.CheckReturnValue - annotates a method to say the method has no side-effects, so there's no point in calling it without checking its return value.
net.jcip.annotations.Immutable - An object of this class can't be changed after it's created. This allows both better checking of logic, and also simplifies use by your colleagues, so it's good to create classes that have this property and then annotate them.
net.jcip.annotations.NotThreadSafe - a class that isn't thread-safe, so shouldn't be checked for concurrency issues. Often used for Swing-based classes, but note that some Swing components (e.g. monitor windows, classes with listeners) do have to accept input from other threads.
net.jcip.annotations.ThreadSafe - classes that do have to be usable for multiple threads. SpotBugs generally assumes this, but it's good to put it on a class that's intended to be thread-safe as a reminder to future developers.
net.jcip.annotations.GuardedBy - "The field or method to which this annotation is applied can only be accessed when holding a particular lock, which may be a built-in (synchronization) lock, or may be an explicit java.util.concurrent.Lock."
Your code should ensure that the synchronization is done around references to the annotated field or method. SpotBugs will (sometimes) check that.

For more information about these annotations, please see links above and

The JSR-305 code annotations page,
the Concurrency API annotations page, and
for some older information, the SpotBugs annotation page.

We do not use these annotations:

javax.annotation.Nullable - This doesn't really mean what people think it does, as SpotBugs doesn't really check anything when this is used. From the SpotBugs documentation:
SpotBugs will treat the annotated items as though they had no annotation. In practice this annotation is useful only for overriding an overarching NonNull annotation. Use javax.annotation.ParametersAreNullableByDefault to set it for an entire class. Prefer the use of CheckForNull.

Running SpotBugs

SpotBugs is automatically run by Travis CI as part of the Pull Request process, and by Jenkins as part of the daily builds. If new errors appear during CI, the job will (usually) fail. You can look at the Jenkins output to see where there are existing errors to work on.

To run it locally:

Install a local copy of SpotBugs. There are multiple ways to do this, see the SpotBugs web site.
You have to tell the JMRI build systems where that installation is. You can do that with various environment variables, etc, but the most straight-forward is to add a line like the following to your local.properties file:
```
    spotbugs.home = /Users/jake/.spotbugs/spotbugs-3.1.7
    
```
Then run SpotBugs in ant via:
```
    ant spotbugs
    
```
which will create a spotbugs.html file that you can open in a browser.

The ant spotbugs-ci target is similar, except it creates the output as a spotbugs.xml XML file that Jenkins uses to track which items have been fixed, etc.

The .spotbugs.xml file specifies which items in SpotBugs reports are suppressed for the JMRI project's usual reporting via Jenkins. The .spotbugs-check.xml file is a super-set which identifies which are significant enough to fail CI. (This is specified two places in the pom.xml, which is used by the scripts/travis.sh to control Maven running, which in turn is used by the .travis.yml file as the Travis run-time script)

Background

Simon White added FindBugs support to our Ant-based build chain during the development of JMRI 2.5.5.

Suppressed Warnings

We have turned off the routine SpotBugs checking of certain kinds of conditions:

RI_REDUNDANT_INTERFACES: This flags cases where a class implements an interface, and also inherits from a superclass that already implements that interface. This is redundant and untidy, but it can't cause the code to malfunction. We have enough of them that we've turned off the warning, and will come back to it some later time.
SIC_INNER_SHOULD_BE_STATIC_ANON, SIC_INNER_SHOULD_BE_STATIC_NEEDS_THIS: Static, as opposed to non-static, inner classes (classes defined within another class) take less space because they don't maintain references to the containing object. This warning suggests moving an anonymous (defined in-line to the code) inner class to a regular (defined not in-line) class so it can be made static. Although probably a small improvement, it's a bit of work for a small improvement. We have enough of them that we've turned off the warning, and will come back to it some later time.
DM_CONVERT_CASE, DM_DEFAULT_ENCODING,: These cover aspects of handling characters in non-Western languages. Most of our String handling doesn't have anything to do with this, so it would be a huge number of annotations for only a small amount of I18N gain. Perhaps we'll revisit these later.
Malicious Code: This is a class of warnings centered around the idea that data and code methods shouldn't be too visible, especially when static. This is true, but JMRI isn't a hardened library that's being released into a world of people trying to break it, as these changes are a lower priority.
Se,SvVI: This is a large class of warnings associated with Java serialization. JMRI doesn't used serialization, and is unlikely to do so in the future, so we suppress these to raise the average quality of the issued warnings.

Status and Counts

Running this on JMRI 2.5.4 produced the following:

Category	JMRI 2.5.4	JMRI 4.13.3
Bad practice Warnings	164	13
Correctness Warnings	77
Experimental Warnings	7
Malicious code vulnerability Warnings	221	(disabled)
Multithreaded correctness Warnings	90	175
Performance Warnings	459	15
Style / Dodgy Code Warnings	304	406
Total	1322	636
Medium Priority		199
Low Priority		437

A lot of work has gone into JMRI cycle to bring the bug count down. The Continuous Integration server runs SpotBugs twice a day, which helps developers see the results of their coding without having to set up to run SpotBugs themselves.

If you are checking code in to the JMRI repository, please check the CI server and make sure that your changes do not introduce new errors.