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 (
since it seems to be likely that most of the rules could be defined at the package level
), the solution should provide mechanism for removal of annotations from the build.

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)

The special module has these pros and cons (sometimes it depends on the point of view):
  • 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)

Not logged in. Log in, Register

By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo