FindBugsAnnotationsProposal
FindBugs annotations proposal (draft)
In this document we propose the usage of FindBugs annotations in the NetBeans source.
Why to use FindBugs
FindBugs is a tool which perform static analysis of the code in order to find bug patterns. The tool searches for obvious bugs, performance issues, fragile code and anti-patterns. Its usage provably improves the code quality and in many cases the developer can find and fix bugs which before users run to them. This significantly lowers the price of sustaining and bug fixing, as the sooner a bug is found in an early stage of development the lower the expenses are.
Right now FindBugs reports more than 10000 warnings in the full IDE. Even in the case only the 1% of these warnings are real bugs (not performance issues, inefficient usage of API or I18N issues), there still are 100 bugs that can be easily eliminated.
According to the FindBugs authors NetBeans has more bugs than one of its competitors (see "Sample output" at http://findbugs.sourceforge.net/).
FindBugs is extensible and custom checks can be written.
Real world (NetBeans IDE) examples of bugs that FindBugs already found in our sources.
Inconsistent synchronization (line 091 and 092) and inefficient usage of Integer constructor - both reported by FB. FishEye view of the original source.
072 String[] tags;
073
074 /** @return tags */
075 @Override
076 public synchronized String[] getTags() {
077 if (tags == null) {
078 tags = new String[] {
079 getString("LEFT"),
080 getString("CENTER"),
081 getString("RIGHT")
082 };
083 }
084 return tags;
085 }
086
087 @Override
088 public void setAsText(String s) {
089 Integer i;
090 getTags();
091 if (s.equals(tags[0])) i = new Integer(java.awt.Label.LEFT);
092 else if (s.equals(tags[1])) i = new Integer(java.awt.Label.CENTER);
093 else i = new Integer(java.awt.Label.RIGHT);
094 setValue(i);
095 }
Static instance of SimpleDateFormat (line 093) - reported by FB. FishEye view of the original source.
...
093 public static final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat ("yyyy/MM/dd"); // NOI18N
...
No loop around wait (line 610) and no condition precheck (line 610) - both reported by FB. More accurate description for the second bug would be inconsistent synchronization, but FB reports it as a no condition precheck (finished variable never occurs inside the synchronized block). FishEye view of the original source.
586 private static class ListResult implements Result {
...
588 private boolean finished = false;
589 private Object LOCK = new Object ();
...
595 public void finish () {
596 finished = true;
597 synchronized (LOCK) {
598 LOCK.notify ();
599 }
600 }
601
602 public boolean isFinished () {
603 return finished;
604 }
605
606 void waitFinished () {
607 if (finished) return;
608 synchronized (LOCK) {
609 try {
610 LOCK.wait ();
611 } catch (InterruptedException ex) {
612 }
613 }
614 }
...
618 }
And many, many others.
Some numbers (full IDE sources, 2007/12/08)
Following table does not contain the number of real bugs (just warnings) however these checks are usually realiable and bug statistic would be very similar. Table does not list all warnings.
| Warning Type | FindBugs Warning Type | Count |
|---|---|---|
| Infinite loop | IL_INFINITE_LOOP | 5 |
| Infinite recursive loop | IL_INFINITE_RECURSIVE_LOOP | 17 |
| Self assignment of field | SA_FIELD_SELF_ASSIGNMENT | 24 |
| Double checked locking | DC_DOUBLECHECK | 31 |
| Fails to close stream | OS_OPEN_STREAM | 85 |
| Unsynchronized lazy init of static | LI_LAZY_INIT_UPDATE_STATIC | 142 |
| String concatenation in loop | SBSC_USE_STRINGBUFFER_CONCATENATION | 240 |
| Inconsistent synchronization | IS2_INCONSISTENT_SYNC | 457 |
Why to use FindBugs annotations
FindBugs supports several special annotations which can improve the analysis and clearly state the developer intention. All supported annotations are described in FindBugs Manual (http://findbugs.sourceforge.net/manual/annotations.html). Annotations can significantly improve the accuracy of the checks. Aside that any developer can check what the original intention was. FindBugs also supports thread safety related annotations - due to lack of any documentation on locking and synchronization in non API classes usage of such annotation could help a lot.
Latest version of FindBugs (1.3.0) also supports JSR-305 annotations (JSR 305: Annotations for Software Defect Detection). These annotations could establish standard and are supported by FindBugs too. Unfortunately there is no official document produced by this JSR (just code). JSR-305 does not contain (so far) any thread safety related annotations.
How can annotations help
Lets have the package org.netbeans.findbugs.samples with following files.
File package-info.java.
001 @DefaultAnnotationForMethods(NonNull.class) 002 @DefaultAnnotationForParameters(NonNull.class) 003 package org.netbeans.findbugs.samples; 004 005 import edu.umd.cs.findbugs.annotations.DefaultAnnotationForMethods; 006 import edu.umd.cs.findbugs.annotations.DefaultAnnotationForParameters; 007 import edu.umd.cs.findbugs.annotations.NonNull;
File TestSample.java.
001 package org.netbeans.findbugs.samples;
002
003 import edu.umd.cs.findbugs.annotations.CheckForNull;
004
005 public class TestSample {
006
007 private String[] names;
008
009 public TestSample(String[] names) {
010 this.names = names.clone();
011 }
012
013 @CheckForNull
014 public String getName(int index) {
015 return names[Index];
016 }
017
018 public String getDescription(int index) {
019 return null;
020 }
021 }
File TestMain.java.
001 package org.netbeans.findbugs.samples;
002
003 public class TestMain {
004
005 public static void main(String[] args) {
006 TestSample sample = new TestSample(null);
007 String name = sample.getName(0);
008 System.out.println(name.length());
009 }
010 }
Not considering annotations (when commented out) the Findbugs would report only one bug.
| Type | Warning | File | Line |
|---|---|---|---|
| Correctness | Non-virtual method call passes null for unconditionally dereferenced parameter | TestMain.java | 006 |
If annotations are used for analysis FindBugs reports three warnings.
| Type | Warning | File | Line |
|---|---|---|---|
| Correctness | Method call passes null to a nonnull parameter | TestMain.java | 006 |
| Correctness | Method may return null, but is declared @NonNull | TestSample.java | 019 |
| Dodgy | Possible null pointer dereference due to return value of called method | TestMain.java | 008 |
How this should be achieved
Generally speaking, there are two options. Either separate module wrapping annotations or integration to the NetBeans build system. FindBugs annotations are defined with CLASS retention policy. JSR-305 annotations are defined with RUNTIME retention policy and the final release will have at least CLASS retention policy as well. Although it is questionable that the performance would be worse (The build system integration has these pros and cons (sometimes it depends on the point of view):
- Simplified usage
- No explicit module dependency just for these code quality annotations
- Migration to JSR-305 if any (sometime in future, with jdk7) could be easier
- If the annotations would stay in class files runtime issues could occur (elaborate bit more)
We've decided to go with build system (2008/01/23)
- Any module developer can use it in already known way
- Developer has to depend on module that has no meaning in runtime
- If JSR-305 will be adapted, almost the whole such module will be made deprecated
- If JSR-305 will be adapted (now or in future) this module can contain custom developed annotations
The other decision that has to be made is whether to use FindBugs annotations or those from JSR-305. Unfortunately JSR-305 does not define any roadmap and the only declared target is Java 7. Because of this we should consider usage of FindBugs annotations - possibly clearly defined subset for easier migration to JSR-305.
Proposed annotations and corresponding JSR-305 equivalent.
| FindBugs annotation | JSR-305 annotation equivalent (RI 2007/12) |
|---|---|
| edu.umd.cs.findbugs.annotations.CheckForNull | javax.annotation.CheckForNull |
| edu.umd.cs.findbugs.annotations.NonNull | javax.annotation.Nonnull |
| edu.umd.cs.findbugs.annotations.Nullable | javax.annotation.Nullable |
| edu.umd.cs.findbugs.annotations.CheckReturnValue | javax.annotation.CheckReturnValue |
| edu.umd.cs.findbugs.annotations.SupressWarnings | java.lang.SupressWarnings |
Proposed patch
TBD
References
FindBugs (http://findbugs.sourceforge.net/)
FindBugs Tutorials (http://code.google.com/p/findbugs-tutorials/)
FindBugs Talk by Bill Pugh @Sun 08/07 (http://wiki.glassfish.java.net/attach/FindBugs/Glassfish_FindBugs.pdf)
JSR-305 (http://jcp.org/en/jsr/detail?id=305)
JSR-305 Discussion (http://groups.google.com/group/jsr-305/topics)
JSR-305 Proposed Annotations (http://groups.google.com/group/jsr-305/web/proposed-annotations)
NetBeans FindBugs Test Wiki ( FindBugsTest)
