Re: [groovy-dev] Bugs found in groovy-1.0-jsr-01 by FindBugs

Hi Bill

On 9 Apr 2005, at 14:34, Bill Pugh wrote:
> Hi everyone. We've been developing a static analysis tool called  
> FindBugs
> to look for various kinds of errors or bug patterns in Java programs.
>
> I ran it over groovy-1.0-jsr-01, and found a number of errors. I've  
> included
> listings of a couple of the bugs we found below, and I'd like to  
> encourage
> you guys to fix these and download FindBugs and try it yourself to get  
> more bug warnings. FindBugs is available from
> 	http://findbugs.sourceforge.net/
>
> For the bugs below, I've given a one-line version of the msg generated  
> by
> FindBugs, my comments on the bug, and a patch when I could figure out
> how to fix the code, and a listing when I couldn't.
>
> Prof. William Pugh
> Univ. of Maryland
> member of the FindBugs project

Great stuff!

BTW FindBugs rocks. I keep meaning to get around to enabling it by  
default on all the open source projects I work on - thanks so much for  
a great tool!

I've been through all these bugs and fixed them in CVS HEAD - details  
snipped a little but kept for reference...

> P.S. Don't be embarrassed by these bugs; Groovy actually came out  
> pretty clean. We've seen lots of errors like this in code such as the  
> JDK, Eclipse, app servers, etc.
>
> -----
>
>
> H C IL: There is an apparent infinite recursive loop in  
> groovy.lang.Sequence.equals(Object).  At Sequence.java:[line 95]
>
> Due it incorrectly cut and paste code, two Sequences will never  
> compare as
> equal, and if a Sequence is compared to a Tuple, the result will be an
> infinite recursive loop resulting in a StackOverflowException.

Fixed - nicely spotted

> H C IL: There is an apparent infinite recursive loop in  
> groovy.lang.SpreadList.equals(Object).  At SpreadList.java:[line 79]
> H C EC: Call to equals() comparing different types in  
> groovy.lang.SpreadList.equals(Object)  At SpreadList.java:[line 79]

fixed

> H C UwF: Unwritten field: groovy.ui.ShellCompleter.completions
>
> This field is never initialized or even written to, but is deferenced.
> Thus, methods such as findCompletions will always throw a  
> NullPointerException

fixed

> H C RV:  
> org.codehaus.groovy.classgen.AsmClassGenerator.visitVariableExpression( 
> org.codehaus.groovy.ast.expr.VariableExpression) ignores return value  
> of groovy.lang.GroovyRuntimeException.<init>(String)  At  
> AsmClassGenerator.java:[line 2967]
>
> The code creates a GroovyRuntimeException, but forgets to throw the  
> resulting
> Exception object.

fixed
>

> H C HE: org.codehaus.groovy.runtime.ClosureListener defines equals and  
> uses Object.hashCode()
>
> ClosureListener defines a equals method such that is identical to the  
> equals method
> for class Object. The equals method should be deleted.

fixed

> M C NP: Possible null pointer dereference in  
> org.codehaus.groovy.classgen.AsmClassGenerator.checkValidType(String,or 
> g.codehaus.groovy.ast.ASTNode,String)  At AsmClassGenerator.java:[line  
> 4805]
>
> Not clear what the patch is here. Somebody obviously thinks type could  
> be null. If it is null, this method will throw a NullPointerException.  
> Is that what is intended?
>
>    protected String checkValidType(String type, ASTNode node, String  
> message) {
>         if (type!= null && type.length() == 0)
>             return "java.lang.Object";
>         if (type.endsWith("[]")) {

fixed

James
-------
http://radio.weblogs.com/0112098/