-
Notifications
You must be signed in to change notification settings - Fork 331
/
SpotBugs.shtml
390 lines (337 loc) · 18.7 KB
/
SpotBugs.shtml
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/strict.dtd">
<html lang="en">
<head>
<meta name="generator" content=
"HTML Tidy for Mac OS X (vers 31 October 2006 - Apple Inc. build 15.17), see www.w3.org">
<title>JMRI: Static Analysis with SpotBugs</title>
<meta name="author" content="Bob Jacobsen">
<meta name="keywords" content="JMRI technical code bugs">
<!-- The combination of "Define" and {Header,Style, Logo and Footer} comments -->
<!-- are an arbitrary design pattern used by the update.pl script to -->
<!-- easily replace the common header/footer code for all the web pages -->
<!-- delete the following 2 Defines if you want to use the default JMRI logo -->
<!-- or change them to reflect your alternative logo -->
<!-- Style -->
<meta http-equiv="Content-Type" content=
"text/html; charset=us-ascii">
<link rel="stylesheet" type="text/css" href="/css/default.css"
media="screen">
<link rel="stylesheet" type="text/css" href="/css/print.css"
media="print">
<link rel="icon" href="/images/jmri.ico" type="image/png">
<link rel="home" title="Home" href="/">
<!-- /Style -->
</head>
<body>
<!--#include virtual="/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.
<a href="http://jmri.tagadab.com/jenkins/job/Development/job/SpotBugs/">
<img src="http://jmri.tagadab.com/jenkins/job/Development/job/SpotBugs/findbugs/trendGraph/png?url=PRIORITY" align="right"/></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="http://jmri.tagadab.com/jenkins/job/Development/job/SpotBugs/lastBuild/findbugsResult/">detailed results of the most recent (daily) run</a>
along with
<a href="http://jmri.tagadab.com/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("FE_FLOATING_POINT_EQUALITY", "Even tiny differences should trigger update")
</pre>
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.
<p>For clarity, this annotation also supports a form that lets you be more verbose:</p>
<pre style="font-family: monospace;">
@edu.umd.cs.findbugs.annotations.SuppressFBWarnings(value = "FE_FLOATING_POINT_EQUALITY",
justification = "OK to compare floats, as even tiny differences should trigger update")
</pre>
This can make it easier to see what is what when quickly
scanning through the code.
<p>If you need to put more than one message type in an
annotation, use array syntax:</p>
<pre style="font-family: monospace;">
@edu.umd.cs.findbugs.annotations.SuppressFBWarnings("{type1},{type2}","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=
"http://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>
<a name="annotations" id="annotations" />
<p>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=
"http://static.javadoc.io/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="http://static.javadoc.io/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 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
<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=
"http://static.javadoc.io/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=
"http://static.javadoc.io/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) be invoke super in the overriding method.
Examples of such methods include finalize() and clone().
<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=
"http://static.javadoc.io/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's no point in calling it without checking its
return value.</li>
<li><a href=
"http://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'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.</li>
<li><a href=
"http://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=
"http://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=
"http://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.
</li>
</ul>For more information about these annotations, please see
links above and
<ul>
<li>The JSR-305 <a href=
"http://www.javadoc.io/doc/com.google.code.findbugs/jsr305/3.0.1">
code annotations page</a>,</li>
<li>the <a href=
"http://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 name="nullable" 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="http://jmri.tagadab.com/jenkins/job/Development/job/SpotBugs/lastBuild/findbugsResult/">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.
<p>
To run it locally:
<ul>
<li>Install a local copy of SpotBugs. There
are multiple ways to do this, see the
<a href="https://spotbugs.github.io">SpotBugs web site</a>.
<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-3.1.7
</pre>
<li>Then run SpotBugs in ant via:
<pre style="font-family: monospace;">
ant spotbugs
</pre>
which will create a <code>spotbugs.html</code>
file that you can open in a browser.
</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="http://jmri.tagadab.com/jenkins/job/Development/job/SpotBugs/lastBuild/findbugsResult/">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)
<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=
"http://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=
"http://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=
"http://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 will come back to it some later time.</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 used serialization, and is
unlikely to do so in the future, so we suppress these to
raise the average quality of the issued warnings.</dd>
</dl>
<h4>Status and Counts</h4>
<p>We've been making slow but continuous progress on removing these:</p>
<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></tr>
<tr><td>Bad practice Warnings</td><td>164</td><td>13</td><td></td></tr>
<tr><td>Correctness Warnings</td><td>77</td><td></td><td></td></tr>
<tr><td>Experimental Warnings</td><td>7</td><td></td><td></td></tr>
<tr><td>Malicious code vulnerability Warnings</td><td>221</td><td>(disabled)</td><td></td></tr>
<tr><td>Multithreaded correctness Warnings</td><td>90</td><td>175</td><td>165</td></tr>
<tr><td>Performance Warnings</td><td>459</td><td>15</td><td>1</td></tr>
<tr><td>Style / Dodgy Code Warnings</td><td>304</td><td>406</td><td>127</td></tr>
<tr><th>Total</th><th>1322</th><th>636</th><th>293</th></tr>
<tr><th>Medium Priority</th><th></th><th>199</th><th>79</th></tr>
<tr><th>Low Priority</th><th></th><th>437</th><th>214</th></tr>
<tr><th>@SuppressFBWarnings<br>Lines</th><th></th><th></th><th>868</th></tr>
</table>
<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="/Footer.shtml" -->
</div><!-- closes #mainContent-->
</div><!-- closes #mBody-->
</body>
</html>