Static Code Analysis With Eclipse

Static Code Analysis with Eclipse

Taking Warnings Seriously

Elliotte Rusty Harold

Wednesday, August 31, 2005

elharo@metalab.unc.edu

http://www.cafeaulait.org/

There's a bug in this class

package com.elharo.security;

import java.io.*;
import java.security.*;

public class EasyDigestOutputStream extends FilterOutputStream {

  private boolean on = true;
  private boolean closed = false;
  protected byte[] result = null;
  protected MessageDigest digest;

  public EasyDigestOutputStream(OutputStream out, String algorithm)
   throws NoSuchAlgorithmException {
    super(out);
    digest = MessageDigest.getInstance(algorithm);
  }

  public EasyDigestOutputStream(OutputStream out, String algorithm,                                 
   String provider) throws NoSuchAlgorithmException, NoSuchProviderException {
    super(out);
    digest = MessageDigest.getInstance(algorithm, provider);
  }

  public void write(int b) throws IOException {
    if (on) digest.update((byte)b);
    out.write(b);
  }

  public void write(byte[] data, int offset, int length) throws IOException {
    if (on) digest.update(data, offset, length);
    out.write(data, offset, length);
  }

  public void on(boolean on) {
    this.on = on;
  }
  
  public void close() throws IOException {
    out.close();
    result = digest.digest();
    closed = true;
  }
  
  public byte[] getDigest() {
    return result;
  }
}

Here's the bug

The closed field is never read
Consequently it's possible to update the digest on a close stream we can't write to
This points us at another bug: the digest should be calculated after writing; not before to avoid digesting code that isn't written due to an IOException.
In over six years the book was print nobody noticed this bug.
Eclipse found it immediately.

Corrected program

package com.elharo.security;

import java.io.*;
import java.security.*;

public class EasyDigestOutputStream extends FilterOutputStream {

  private boolean on = true;
  protected byte[] result = null;
  protected MessageDigest digest;

  public EasyDigestOutputStream(OutputStream out, String algorithm)
   throws NoSuchAlgorithmException {
    super(out);
    digest = MessageDigest.getInstance(algorithm);
  }

  public EasyDigestOutputStream(OutputStream out, String algorithm,                                 
   String provider) throws NoSuchAlgorithmException, NoSuchProviderException {
    super(out);
    digest = MessageDigest.getInstance(algorithm, provider);
  }

  public void write(int b) throws IOException {
    out.write(b);
    if (on) digest.update((byte)b);
  }

  public void write(byte[] data, int offset, int length) throws IOException {
    out.write(data, offset, length);
    if (on) digest.update(data, offset, length);
  }

  public void on(boolean on) {
    this.on = on;
  }
  
  public void close() throws IOException {
    out.close();
    result = digest.digest();
  }
  
  public byte[] getDigest() {
    return result;
  }
}

Severity vs. Effort

Not all bugs Eclipse finds are this bad. Some are less serious. Some are more.
Only in code that has already been analyzed by similar tools (or developed in Eclipse) will Eclipse fail to find any real problems.
Not always a big problem, but it's so cheap to fix that the cost-benefit ratio is very high.
Occasionally a project has so many errors and warnings it becomes difficult to work with. Then it's time to customize the rules.

What is Static Code Analysis?

Automatic detection of:
- Common bug patterns
- Confusing code that's likely to cause bugs
- Bad practices
- Violation of coding conventions
Syntax errors on steroids

Turning on Static Code Analysis in Eclipse

Error Levels

Three Levels of errors:
- Error: blocks build
- Warning: noted by Eclipse but not fatal
- Ignore: not indicated
These are configurable
Sometimes different projects require different settings.

Problems Eclipse can find

Methods with a constructor name

Example:

public class A {

  private double x;

  void A(double x) {
    this.x = x;
  }
  
}

Problem because:
- Confusing
- Almost always a bug; normally a constructor was intended
Severity: low; normally discovered as soon as you try to use it
May not be noticed if there are multiple constructors
Should be an error

Methods overridden but not package visible

Example:

package com.elharo.foo;
	
public class A {
  void test() {
    System.out.println("Class A");
  }
}

package com.elharo.bar;
	
public class B extends A {
  void test() {
    System.out.println("Class A");
  }
}

Problem because:
- Confusing if it was not intended to be overridden
- A bug if it was intended to be overridden: wrong method is invoked
Severity: High. Often not detected at either compile or run time. Can be very hard to recognize.

Non-static access to a static member

Example:

public class A {
    
    private static double x = 3.4;
    
    void test() {
      this.x = this.x + 2.1;
    }
}

Problem because:
- Probably not a bug; hard to read though
Severity: minor

Possible accidental boolean assignment

Example:

    boolean a = isRed();
    boolean b = isLoud();
    if (a = b) {
      System.out.println("Loudness correlates with redness");
    }

Problem because:
- Almost certainly a bug
Severity: medium; severity mitigated only because boolean variables are rarely compared for equality

Empty statement

Example:


    boolean a = isRed();
    boolean b = isLoud();;
    if (a == b) {
      System.out.println("Loudness correlates with redness");
    }

More serious example:


    boolean a = isRed();
    boolean b = isLoud();
    if (a == b) ;
      System.out.println("Loudness correlates with redness");

Problem because:
- Ugly
- Can alter control-flow (though the real problem is am unbracjeted multiline if)
Severity: aesthetic to serious; usually not a bug, but can be

Undocumented Empty Block

Example:
```
while (a.getNext()) {}
```
Problem because:
- Weird. Why is this done?
Severity: minor; usually not a bug but could be.
Fix: add a comment inside the loop

finally does not complete normally

Example:

public class Test {

    public static void main(String[] args) throws Exception {
        
        int n = getFoo();
        System.out.println(n);
        
    }

    private static int getFoo() {
      try {
        return 10;
      }
      finally {
        return 20;
      }
    }
}

Worse Example:

      try {
        break;
      } finally {
        continue;
      }
      
      try {
        throw ex;
      } finally {
        return;
      }

Problem because:
- Overrides return/break/continue/throw statements elsewhere in the method
Severity: medium; sometimes a bug
Fix: redesign the logic

Interface method conflicts with protected 'Object' method

Example:

public interface A {
    long hashCode();
}

Problem because:
- Absolutely a bug; this interface cannot be implemented
Severity: low; discovered as soon as you try to implement the interface

Access to a non-accessible member of an enclosing type

Example:

public class A {
  private int count = 0;	
	
  private class Inner {
    void test() {
      System.out.println(count);
    }
  }
}

Problem because:
- Not a bug; possible performance issue
Severity: minor; possible design flaw; members should be private; just maybe slower

Advanced

Assignment has no effect

Buggy example:

public class A {
    private int count = 0;	
	
    A(int count) {
      count = count;
    }
    
}

More often this is just an invocation of a method that has side effects whose return value is assigned to a variable for no particular reason:

// Test reachability of host
try {
  Socket s = new Socket("www.yahoo.com", 80);
  System.out.println("Yahoo is up");
}
catch (IOException ex) {
  System.out.println("Yahoo is down");
}

Problem because:
- Did you mean to do something else?
Severity: medium; possible bug; hard to spot

Local variable declaration hides another field or variable

Example:

public class A {
    private int count = 0;	
	
    void test() {
      int count = 7;
    }
    
}

Eclipse is smart enough to ignore the common cases of constructors and setter methods; i.e. this pattern is not flagged:
```
public class A {
    private int count = 0;	
	
    public A(int count) {
      this.count = count;
    }
    
    public void setCount(int count) {
      this.count = count;
    }
    
}
```
However you can request this pattern to be flagged too if you want.
Problem because:
- Did you really mean to use the field?
Severity: high; possible bug; hard to spot

Field declaration hides another field or variable

Example:

public class Test {

  protected int count = 0;
  
  public int get() {
     return count; 
  }
  
}

class Foo extends Test {
 
  private int count = 2;
  
  public int get() {
     return count; 
  }
  
}

More serious: inner class hides outer members:

public class A {
    private int count = 0;	
	
    private class B {
    
      private int count = 7;
    }
    
}

Problem because: possibly confusing to developers
Severity: minimal; ignored by default. You did make your fields private, right? Can be more important if fields are non-private.

Using a char array in string concatenation

Example:

public class A {
	
    void test() {
      char[] data = {'d','a','t','a'};
      String s = "test " + data;
      System.out.println(s);
    }
    
}

Problem because:
- Did you mean to concatenate the contents instead?
Severity: minor; possible bug; not too hard to spot

Usage of deprecated API

Example:

Thread t = new Thread();
t.start();
t.stop();

Can separately configure whether or not to flag deprecated methods called by deprecated code, and overriding and/or implementing of deprecated methods.
Problem because:
- Sometimes deprecated methods don't work quite right (e.g. Thread.stop()).
- Deprecation messages confuse novices and itch experts.
- Helps to educate programmers about API changes
Severity: minimal:
- Most deprecated methods and fields are just fine.
- The compiler will catch this anyway.
- Sun will never remove deprecated API.

Unused Code

Vestigial code makes classes larger
Vestigial code makes classes harder to read
Bugs: Some unused code really should be used, but isn't.

Unused Code Problems

Local variable is never read
Private field is never read
Private method is never called
Import is unused
Unnecessary cast
Unnecessary instanceof
Unnecessary else (ignored by default)
Unused methods and fields in anonymous inner classes

Unused Code Example

import java.util.ArrayList;
import java.util.List;
import java.util.Date;

public class UnusedCode {

    private int value = 7;
    
    public static void main(String[] args) {

        Object o = (Object) "Hello World!";
        
        ArrayList a = new ArrayList();
        if (a instanceof List) {
            return;
        }
        else {
            System.out.println("Bye Bye!");
        }

    }
    
    private double getFoo() {
        return 5.6;
    }

}

Thornier Problems

Parameter (method argument) is never read:

class AVControlsAction extends AbstractAction {

    private JFrame frame;
    
    AVControlsAction(JFrame frame) {
        this.frame = frame;
        putValue(Action.NAME, "Show A/V Controls");
    } 
    
    public void actionPerformed(ActionEvent event) {
        AVControlsPalette palette = new AVControlsPalette(frame);
        palette.setVisible(true);
    }

}

Unnecessary declaration of checked exception (i.e. unnecessary throws clause)
Sometimes needed to implement the contract of a superclass or interface
Sometimes can be used by overriding methods in subclasses
Check overriding and implementing methods
Bug 101849

Hidden catch blocks

Example:

  try {
    InputStream f = new FileInputStream("data.txt");
    // ...
  }
  catch (Exception ex) {
    System.err.println(ex.getMessage());
  }
  catch (IOException ex) {
    throw ex;
  }

Problem because:
- Probably a bug; the second catch block is never executed
Severity: medium; hard to spot, but normally not a huge problem

JavaDoc

If you use JavaDoc at all, turn on Process JavaDoc comments. By default this detects:

@throws tags for checked exceptions that aren't thrown
@param tags that don't match any actual arguments
@see tags that don't seem to point anywhere
Autocorrect can fill in most missing tags

Checking for undocumented code

By default, Eclipse does not check for tags and comments that are missing (as opposed to wrong).
You can turn on checks for:
- Methods and fields that aren't commented
- Arguments, return values, and exceptions that aren't documented
You can specify whether these checks should only be for public, public and protected; public, protected and default; or all members.
Context Menu/Source/Add Comment will create comment template in first place

Spell checking

Preferences > General > Editors > Text Editors > Spelling
A user dictionary may be necessary; e.g. /usr/share/dict/words
Non-ASCII characters in the dictionary may break spell checking (Bug 105293 and 104022)

Java 5 Checks

Unchecked Generic type operation

Example:

        ArrayList list = new ArrayList<String>();
        list.add("test");

Problem because:
- Eliminates generic type safety
Severity: minor; we lived without this for 10 years. Do we really need it now?

Generic type parameter declared with a final type bound

Example:

    public static int sumLengths(List<? extends String> list) {
        int total = 0;
        for(String s : list) total += s.length();
        return total;
    }

Problem because:

Since a final class has no subclasses this could equally well be written as:

    public static int sumLengths(List<String> list) {
        int total = 0;
        for(String s : list) total += s.length();
        return total;
    }

Severity: Minor. Code is less clear than it could be.

Inexact type match for varargs arguments

"an invocation was made, but result is hard to predict unless you understand deeply the rules of varargs" -- Philippe P. Mulet

  void test(){
    invoke(new Class[0]); // is Class[] passed directly as an Object[] (1),
                          // or wrappered into a new Object[] (2) ?
      }
      Object invoke(Object... args) {
            return null;
      }
      invoke((Object[])new Class[0]);

"The warning indicates that (1) got picked, but should be clarified by inserting a cast in the code: invoke((Object[])new Class[0]);"
Problem because:
- Hard to understand, possibly a bug
Severity: Medium

Boxing and unboxing conversions

Example:
```
Integer i = 7;
int j = i + 3;
```

More serious example:

Integer I = 0; 
Integer J = 0;
System.out.println(I == J); // prints true
Integer I = 300;
Inetger J = 300;
System.out.println(I == J); // prints false

Integer.valueOf(int) caches values comprised between -128 and 127.

Problem because:
- May hide performance issues; breaks assumptions on object identity
Severity: minor; ignored by default.

Missing @Override annotation

Example:

// @Override should go here
public String toString() {
  return "foo";
}

Problem because:
- Java 5 uncool
- Hinders tools that depend on annotations to understand code
- Possible bug: Perhaps the programmer didn't realize they were overriding a method?
Severity: minor; ignored by default

Annotation is used as a superinterface

Example:

public interface TestI extends Deprecated {

}

Problem because:
- Bad form
- Cannot be used as an annotation
Severity: Minor

Not all enum constants covered on switch

Example:

public class EnumSwitchTest {

    private static enum PrimaryColor {RED, GREEN, BLUE};
    
    public static void main(String[] args) {
        
        PrimaryColor n = PrimaryColor.RED;
        
        switch (n) {
        case RED:
            System.out.println("red");
            break;
        case GREEN:
            System.out.println("green");
            break;
        }
        

    }

}

Problem because:
- What happens if one of the uncovered enums appears?
Severity: medium; possible bug
Fix: At a minimum throw a RuntimeException in a default block

Unhandled warning tokens in @SuppressWarnings

Example:
```
@SuppressWarnings("foo")
```
Problem because:
- You were trying to turn off something that can't be turned off
- You were trying to turn off something that can be turned off, but failed to due to a typo
Severity: Minor. At worst you'll get an extra warning.

Lint Comments

Via @SuppressWarnings annotation:

	 @SuppressWarnings("serial")
  public static class TFrame extends Frame {
        
  }

Current tokens include (according to the build notes, officially undocumented):
- all : any warning
- boxing: autoboxing conversion
- dep-ann: missing @Deprecated annotation
- deprecation: deprecation outside deprecated code
- incomplete-switch: incomplete enum switch
- hiding: field hiding another variable, local variable hiding, type paramneter hiding another type. or hidden catch block
- finally: finally block not completing normally
- static-access: indirect reference to static member or non-static reference to static member
- maskedCatchBlock: hidden catch block
- nls: string literal lacking non-nls tag //$NON-NLS-<n>$
- serial: missing serialVersionUID
- unqualified-field-access: unqualified reference to field
- unchecked: unchecked type operation
- unused: unread method parameter, local variable, private member, or declared thrown exception
- syntheticAccess: synthetic access for innerclass
"Note that these are subject to changing in the near future."

Plug-Ins

PMD
Checkstyle
FindBugs
Java PathFinder
EclipsePro Audit ($499)

PMD

Has Eclipse Plugin
Source code based
Can be extended with XPath

Checkstyle

Has Eclipse Plugin
Source code based
More focused on coding conventions; less on actual bug patterns

FindBugs

Has Eclipse Plugin
Byte code based

Java PathFinder

Developed by NASA
A Java Virtual Machine (JVM) that is used as an explicit state software model checker, systematically exploring all potential execution paths of a program to find violations of properties like deadlocks or unhandled exceptions.
Reports the entire execution path that leads to a defect.
Designed for analyzing multi-threaded applications; can find deadlocks.

Thanks To

Phillipe P. Mulet at IBM France for explaining some of the trickier ones to me

To Learn More

This presentation: http://www.cafeaulait.org/slides/eclipseworld2005/staticcode/
PMD: http://pmd.sourceforge.net/
FindBugs: http://findbugs.sourceforge.net/
Checkstyle: http://checkstyle.sourceforge.net/
Java PathFinder: http://ase.arc.nasa.gov/havelund/jpf.html

Index | Cafe con Leche