Serge NOEL
487 lines
20 KiB
Plaintext
487 lines
20 KiB
Plaintext
<!DOCTYPE html>
|
|
<html lang="en">
|
|
<head>
|
|
<meta name="generator" content="HTML Tidy for HTML5 for Apple macOS version 5.8.0">
|
|
<title>JMRI: Static Analysis with SpotBugs</title>
|
|
<meta name="author" content="Bob Jacobsen">
|
|
<meta name="keywords" content="JMRI technical code bugs"><!--#include virtual="/help/en/parts/Style.shtml" -->
|
|
</head>
|
|
<body>
|
|
<!--#include virtual="/help/en/parts/Header.shtml" -->
|
|
|
|
<div id="mBody">
|
|
<!--#include virtual="Sidebar.shtml" -->
|
|
|
|
<div id="mainContent">
|
|
<h1>JMRI Code: Static Analysis with SpotBugs</h1>
|
|
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. See the <a href=
|
|
"https://builds.jmri.org/jenkins/job/development/job/spotbugs/">spotbugs history page.</a>
|
|
<p>For more information on SpotBugs, see <a href="https://spotbugs.github.io">the SpotBugs
|
|
home page</a>.</p>
|
|
|
|
<p>We routinely run SpotBugs as part of our <a href="ContinuousIntegration.shtml">continuous
|
|
integration process</a>. You can see the <a href=
|
|
"https://builds.jmri.org/jenkins/job/development/job/spotbugs/lastBuild/spotbugs/">detailed
|
|
results of the most recent (daily) run</a> along with <a href=
|
|
"https://builds.jmri.org/jenkins/job/development/job/spotbugs/">recent progress trend</a>
|
|
online.</p>
|
|
|
|
<p>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:</p>
|
|
|
|
<pre style="font-family: monospace;">
|
|
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
|
|
|
|
@SuppressFBWarnings(value = "FE_FLOATING_POINT_EQUALITY", justification = "Even tiny differences should trigger update")
|
|
</pre>
|
|
|
|
<p>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.</p>
|
|
|
|
<p>Annotations without a justification clause will periodically be removed.</p>
|
|
|
|
<p>Note that the <code>@SuppressFBWarnings</code> contents in this form should be all on one line
|
|
so that automated scanners can more reliably see it.</p>
|
|
|
|
<p>If you need to put more than one message type in an annotation, use array syntax:</p>
|
|
|
|
<pre style="font-family: monospace;">
|
|
@SuppressFBWarnings(value = {"type1", "type2"}, justification = "Why both are needed")
|
|
</pre>
|
|
<p>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 <a href=
|
|
"https://docs.oracle.com/javase/tutorial/java/annotations/">Java annotations</a> and <a href=
|
|
"https://findbugs.sourceforge.net/manual/annotations.html">SpotBugs annotation pages</a>.</p>
|
|
|
|
<p>The basics of annotations are covered in a <a href=
|
|
"https://docs.oracle.com/javase/tutorial/java/annotations/">Java annotations
|
|
tutorial</a>.</p>
|
|
|
|
<p id="annotations">It can be useful to mark code with one of the following annotations so
|
|
that SpotBugs does a good job of reasoning about it:</p>
|
|
|
|
<ul>
|
|
<li>
|
|
<a href=
|
|
"https://www.javadoc.io/doc/com.google.code.findbugs/jsr305/3.0.1/javax/annotation/Nonnull.html">
|
|
<code>javax.annotation.Nonnull</code></a> - 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 <a href=
|
|
"https://www.javadoc.io/doc/com.google.code.findbugs/jsr305/3.0.1/javax/annotation/ParametersAreNonnullByDefault.html">
|
|
javax.annotation.ParametersAreNonnullByDefault</a> on the class declaration (the start of
|
|
the class) to set @Nonnull for an entire class. Note that SpotBugs won't let you compare
|
|
a @Nonnull value to null; that's an error that SpotBugs wants to find via static
|
|
analysis. If you are annotating a part of the external API, and you want to double-check at
|
|
runtime, use <a href=
|
|
"https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#requireNonNull-T-">java.util.Objects.requireNonNull(..)</a>,
|
|
for example:
|
|
|
|
<pre style="font-family: monospace;">
|
|
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");
|
|
}
|
|
</pre>
|
|
</li>
|
|
|
|
<li>
|
|
<a href=
|
|
"https://www.javadoc.io/doc/com.google.code.findbugs/jsr305/3.0.1/javax/annotation/CheckForNull.html">
|
|
<code>javax.annotation.CheckForNull</code></a> - 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.
|
|
</li>
|
|
|
|
<li>
|
|
<a href=
|
|
"https://www.javadoc.io/doc/com.google.code.findbugs/jsr305/3.0.1/javax/annotation/OverridingMethodsMustInvokeSuper.html">
|
|
<code>javax.annotation.OverridingMethodsMustInvokeSuper</code></a> - Used to annotate a
|
|
method that, if overridden, must (or should) invoke super in the overriding method.
|
|
Examples of such methods include <code>finalize()</code> and <code>clone()</code>.
|
|
<p>Note this replaces the deprecated <a href=
|
|
"http://findbugs.sourceforge.net/manual/annotations.html"><code>edu.umd.cs.findbugs.annotations.OverrideMustInvoke</code></a>.</p>
|
|
</li>
|
|
|
|
<li>
|
|
<a href=
|
|
"https://www.javadoc.io/doc/com.google.code.findbugs/jsr305/3.0.1/javax/annotation/CheckReturnValue.html">
|
|
<code>javax.annotation.CheckReturnValue</code></a> - annotates a method to say the method
|
|
has no side effects, so there is no point in calling it without checking its return value.
|
|
</li>
|
|
|
|
<li>
|
|
<a href=
|
|
"https://jcip.net/annotations/doc/net/jcip/annotations/Immutable.html"><code>net.jcip.annotations.Immutable</code></a>
|
|
- An object of this class can't be changed after it was 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.
|
|
</li>
|
|
|
|
<li>
|
|
<a href=
|
|
"https://jcip.net/annotations/doc/net/jcip/annotations/NotThreadSafe.html"><code>net.jcip.annotations.NotThreadSafe</code></a>
|
|
- 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.
|
|
</li>
|
|
|
|
<li>
|
|
<a href=
|
|
"https://jcip.net/annotations/doc/net/jcip/annotations/ThreadSafe.html"><code>net.jcip.annotations.ThreadSafe</code></a>
|
|
- 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.
|
|
</li>
|
|
|
|
<li>
|
|
<a href=
|
|
"https://jcip.net/annotations/doc/net/jcip/annotations/GuardedBy.html"><code>net.jcip.annotations.GuardedBy</code></a>
|
|
- "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."
|
|
<p>Your code should ensure that the synchronization is done around references to the
|
|
annotated field or method. SpotBugs will (sometimes) check that.</p>
|
|
</li>
|
|
</ul>
|
|
For more information about these annotations, please see links above and
|
|
<ul>
|
|
<li>The JSR-305 <a href="https://www.javadoc.io/doc/com.google.code.findbugs/jsr305/3.0.1/index.html">
|
|
code annotations page</a>,
|
|
</li>
|
|
|
|
<li>the <a href="https://jcip.net/annotations/doc/index.html">Concurrency API annotations
|
|
page</a>, and
|
|
</li>
|
|
|
|
<li>for some older information, the <a href=
|
|
"http://findbugs.sourceforge.net/manual/annotations.html">SpotBugs annotation page</a>.
|
|
</li>
|
|
</ul>
|
|
|
|
<p>We do not use these annotations:</p>
|
|
|
|
<ul>
|
|
<li>
|
|
<a id="nullable"></a> <a href=
|
|
"http://findbugs.sourceforge.net/manual/annotations.html"><code>javax.annotation.Nullable</code></a>
|
|
- 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:
|
|
<blockquote>
|
|
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 <code>CheckForNull</code>.
|
|
</blockquote>
|
|
</li>
|
|
</ul>
|
|
|
|
<h3>Running SpotBugs</h3>
|
|
SpotBugs is automatically run by <a href="ContinuousIntegration.shtml#travis">Travis CI</a>
|
|
as part of the <a href="gitdeveloper.shtml">Pull Request</a> process, and by <a href=
|
|
"ContinuousIntegration.shtml#jenkins">Jenkins</a> as part of the <a href=
|
|
"https://builds.jmri.org/jenkins/job/development/job/spotbugs/lastBuild/">daily
|
|
builds</a>. 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.
|
|
There's a very nice browser
|
|
that lets you select items by type, by file, by package, etc,
|
|
available by clicking on the "SpotBugs Warnings" button on the left of the
|
|
<a href="https://builds.jmri.org/jenkins/job/development/job/spotbugs/">Jenkins SpotBugs job</a>.
|
|
|
|
<p>To run it locally:</p>
|
|
|
|
<ul>
|
|
<li>Install a local copy of SpotBugs. There are multiple ways to do this, see the
|
|
<a href="https://spotbugs.github.io">SpotBugs website</a>.
|
|
</li>
|
|
|
|
<li>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 <a href="StartUpScripts.shtml">local.properties</a> file:
|
|
|
|
<pre style="font-family: monospace;">
|
|
spotbugs.home = /Users/jake/.spotbugs/spotbugs-4.2.3
|
|
</pre>
|
|
</li>
|
|
|
|
<li>In order to check for spotbugs issues in logging statements, we use the <a href=
|
|
"https://www.kengo-toda.jp/findbugs-slf4j/">Findbugs-slf4j</a> plugin by KengoTODA. To use
|
|
this plugin in ant builds, download the plugin and install it in the plugin directory of
|
|
your Spotbugs installation.
|
|
</li>
|
|
|
|
<li>Then run SpotBugs in ant via:
|
|
|
|
<pre style="font-family: monospace;">
|
|
ant spotbugs
|
|
</pre>
|
|
|
|
or for the test code
|
|
<pre style="font-family: monospace;">
|
|
ant tests-spotbugs
|
|
</pre>
|
|
|
|
which will create a <code>spotbugs.html</code> or <code>tests-spotbugs.html</code> file in the JMRI base directory that you can open in a browser.
|
|
</li>
|
|
</ul>
|
|
The <code>ant spotbugs-ci</code> target is similar, except it creates the output as a
|
|
<code>spotbugs.xml</code> XML file that Jenkins uses to track which items have been fixed,
|
|
etc.
|
|
<p>The <a href="https://github.com/JMRI/JMRI/blob/master/.spotbugs.xml">.spotbugs.xml</a>
|
|
file specifies which items in SpotBugs reports are suppressed for the JMRI project's <a href=
|
|
"https://builds.jmri.org/jenkins/job/development/job/spotbugs/lastBuild/">usual
|
|
reporting via Jenkins</a>. The <a href=
|
|
"https://github.com/JMRI/JMRI/blob/master/.spotbugs-check.xml">.spotbugs-check.xml</a> file
|
|
is a super-set which identifies which are significant enough to fail CI. (This is specified
|
|
two places in the <code>pom.xml</code>, which is used by the <code>scripts/travis.sh</code>
|
|
to control Maven running, which in turn is used by the <code>.travis.yml</code> file as the
|
|
Travis run-time script)</p>
|
|
|
|
<h3>Background</h3>
|
|
|
|
<p>Simon White added FindBugs support to our Ant-based build chain during the development of
|
|
JMRI 2.5.5.</p>
|
|
|
|
<h4>Suppressed Warnings</h4>
|
|
We have turned off the routine SpotBugs checking of certain kinds of conditions:
|
|
<dl>
|
|
<dt>
|
|
<a href=
|
|
"https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html#ri-class-implements-same-interface-as-superclass-ri-redundant-interfaces">
|
|
RI_REDUNDANT_INTERFACES</a>
|
|
</dt>
|
|
|
|
<dd>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.</dd>
|
|
|
|
<dt>
|
|
<a href=
|
|
"https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html#sic-could-be-refactored-into-a-named-static-inner-class-sic-inner-should-be-static-anon">
|
|
SIC_INNER_SHOULD_BE_STATIC_ANON</a>, <a href=
|
|
"https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html#sic-could-be-refactored-into-a-static-inner-class-sic-inner-should-be-static-needs-this">
|
|
SIC_INNER_SHOULD_BE_STATIC_NEEDS_THIS</a>
|
|
</dt>
|
|
|
|
<dd>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 we'll come back to it later.</dd>
|
|
|
|
<dt>
|
|
<a href=
|
|
"https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html#dm-consider-using-locale-parameterized-version-of-invoked-method-dm-convert-case">
|
|
DM_CONVERT_CASE</a>, <a href=
|
|
"https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html#dm-reliance-on-default-encoding-dm-default-encoding">
|
|
DM_DEFAULT_ENCODING</a>,
|
|
</dt>
|
|
|
|
<dd>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.</dd>
|
|
|
|
<dt>Malicious Code</dt>
|
|
|
|
<dd>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.</dd>
|
|
|
|
<dt>Se,SvVI</dt>
|
|
|
|
<dd>This is a large class of warnings associated with Java serialization. JMRI doesn't use
|
|
serialization, and is unlikely to do so in the future, so we suppress these to raise the
|
|
average quality of the issued warnings.</dd>
|
|
|
|
<dt>SLF4J_LOGGER_SHOULD_BE_NON_STATIC</dt>
|
|
|
|
<dd>This warning indicates that loggers should be non-static variables, but the JMRI
|
|
standard is for the logger to be a static final variable, so every logger would be flagged
|
|
if this were enabled.</dd>
|
|
</dl>
|
|
|
|
<h4>Status and Counts</h4>
|
|
|
|
<p>We've been making slow but continuous progress on removing these, although new Bug patterns are constantly being added to Spotbugs:</p>
|
|
|
|
<h4>Main JMRI Codebase</h4>
|
|
|
|
<table border="1">
|
|
<tr>
|
|
<th>Category</th>
|
|
<th>JMRI 2.5.4</th>
|
|
<th>JMRI 4.13.3</th>
|
|
<th>JMRI 4.17.3</th>
|
|
<th>JMRI 5.5.3</th>
|
|
</tr>
|
|
|
|
<tr>
|
|
<td>Bad practice Warnings</td>
|
|
<td>164</td>
|
|
<td>13</td>
|
|
<td>0</td>
|
|
<td>0</td>
|
|
</tr>
|
|
|
|
<tr>
|
|
<td>Correctness Warnings</td>
|
|
<td>77</td>
|
|
<td></td>
|
|
<td>25</td>
|
|
<td>25</td>
|
|
</tr>
|
|
|
|
<tr>
|
|
<td>Experimental Warnings</td>
|
|
<td>7</td>
|
|
<td></td>
|
|
<td></td>
|
|
<td></td>
|
|
</tr>
|
|
|
|
<tr>
|
|
<td>Malicious code vulnerability Warnings</td>
|
|
<td>221</td>
|
|
<td>(disabled)</td>
|
|
<td></td>
|
|
<td></td>
|
|
</tr>
|
|
|
|
<tr>
|
|
<td>Multithreaded correctness Warnings</td>
|
|
<td>90</td>
|
|
<td>175</td>
|
|
<td>165</td>
|
|
<td>198</td>
|
|
</tr>
|
|
|
|
<tr>
|
|
<td>Performance Warnings</td>
|
|
<td>459</td>
|
|
<td>15</td>
|
|
<td>1</td>
|
|
<td>0</td>
|
|
</tr>
|
|
|
|
<tr>
|
|
<td>Style / Dodgy Code Warnings</td>
|
|
<td>304</td>
|
|
<td>406</td>
|
|
<td>127</td>
|
|
<td>295</td>
|
|
</tr>
|
|
|
|
<tr>
|
|
<th>Total</th>
|
|
<th>1322</th>
|
|
<th>636</th>
|
|
<th>293</th>
|
|
<th>518</th>
|
|
</tr>
|
|
|
|
<tr>
|
|
<th>Medium Priority</th>
|
|
<th></th>
|
|
<th>199</th>
|
|
<th>79</th>
|
|
<th>427</th>
|
|
</tr>
|
|
|
|
<tr>
|
|
<th>Low Priority</th>
|
|
<th></th>
|
|
<th>437</th>
|
|
<th>214</th>
|
|
<th>128</th>
|
|
</tr>
|
|
|
|
<tr>
|
|
<th>@SuppressFBWarnings<br> Lines</th>
|
|
<th></th>
|
|
<th></th>
|
|
<th>868</th>
|
|
<th>873</th>
|
|
</tr>
|
|
</table>
|
|
|
|
<h4>JMRI Test Code</h4>
|
|
|
|
<table border="1">
|
|
<tr><th></th>
|
|
<th>JMRI 5.1.1<br>SB 4.7.0</th>
|
|
<th>JMRI 5.5.3<br>SB 4.7.3</th>
|
|
</tr>
|
|
|
|
<tr><td>High Priority Total</td>
|
|
<td>251</td>
|
|
<td>4</td>
|
|
</tr>
|
|
|
|
<tr><td>Medium Priority Total</td>
|
|
<td>4692</td>
|
|
<td>328</td>
|
|
</tr>
|
|
|
|
<tr><td>Low Priority Total</td>
|
|
<td>988</td>
|
|
<td>371</td>
|
|
</tr>
|
|
|
|
<tr><td><strong>Total Warnings</strong></td>
|
|
<td><strong>5931</strong></td>
|
|
<td><strong>703</strong></td>
|
|
</tr>
|
|
|
|
<tr><td></td><td></td><td></td></tr>
|
|
|
|
<tr><td>High Priority Density</td>
|
|
<td>0.95</td>
|
|
<td>0.01</td>
|
|
</tr>
|
|
|
|
<tr><td>Medium Priority Density</td>
|
|
<td>17.77</td>
|
|
<td>1.19</td>
|
|
</tr>
|
|
|
|
<tr><td>Low Priority Density</td>
|
|
<td>3.74</td>
|
|
<td>1.35</td>
|
|
</tr>
|
|
|
|
<tr><td><strong>Total Density</strong></td>
|
|
<td><strong>22.46</strong></td>
|
|
<td><strong>2.55</strong></td>
|
|
</tr>
|
|
</table>
|
|
|
|
<p>Density - Defects per Thousand lines of non-commenting source statements.</p>
|
|
|
|
<p>A lot of work has gone into JMRI cycle to bring the bug count down. The <a href=
|
|
"ContinuousIntegration.shtml">Continuous Integration server</a> runs SpotBugs twice a day,
|
|
which helps developers see the results of their coding without having to set up to run
|
|
SpotBugs themselves.</p>
|
|
|
|
<p>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.</p>
|
|
<!--#include virtual="/help/en/parts/Footer.shtml" -->
|
|
</div>
|
|
<!-- closes #mainContent-->
|
|
</div>
|
|
<!-- closes #mBody-->
|
|
<script src="/js/help.js"></script>
|
|
</body>
|
|
</html>
|