I claim this task.

it's all yours!

Small clarification on c) (The name of the rule differs from the description)
Should this rule fire if:
 1. Types implement IDisposable and have no finalizer (name)
 2. Types have native/disposable fields and no finalizer (description)

I think the rule should only check for native fields and not for disposable 
ones.
Types don't need a finalizer if they just contain Disposable fields (they 
should have
their own finalizers so no need to call Dispose on them from the checked types
Finalizer ).

TypesWithNativeFieldsShoudHaveFinalizerRule ?

a bit of the two ;-)

1) if the type implements IDisposable
and
2) if the type has native fields (like IntPtr, UIntPtr and HandleRef - as you're
right that other disposable fields don't need to trigger the rule.

So the best name would be
DisposableTypesWithNativeFieldsShouldHaveFinalizerRule
but that's a bit long (even for my taste), so I think the original name
DisposableTypesShouldHaveFinalizerRule
is ok, with the correct documentation of course.

I was unable to run the test cases under linux. nunit-console2 reports 
System.ArgumentException: Encoding name 'Windows-1252' not supported
no idea why..

The test cases run under windows though and invoking gendarme on the Test 
assembly
produces all the required warnings.

Great! I'll review this tonight (in about 4-5 hours).

Not that some files, well at least 
DisposableFieldsShouldBeDisposed[Rule|Test].cs,
doesn't follow Mono's source conventions (spaces instead of tabs, location of { 
for
methods).

Updated formatting.

some minor issues

In general
* you should upgrade your copyrights notice to 2008 ;-) also check that the 
header
rule name match the source
* have a look to reduce your using namespaces (VS, if you use it, even has an 
option
to removed unused ones). E.g. Only 3 out of 9 are needed for DisposableHelper.cs

DisposableHelper
* ToTypeDefinition - great :) but there's already code (not in SVN yet since 
there's
a bug in it) to do similar stuff (and a bit more). Once this get fixed we 
should be
able to modify most rocks to accept reference (and call the resolver if needed) 
which
should keep the rules clean (hopefully ;-). Can you adapt your rules/tests to 
work
without it ?
* ImplementsIDisposable doesn't help rule readability (the main point for the 
rocks).
Simply use type.Implements("System.IDisposable") in the rules
* GetDisposedMethod - this won't work. I know it looks identical to 
GetFinalizer but
that was special case (only one can exists) while you can have both Dispose and
IDisposable.Dispose in the same class (which needs to be unit tested BTW ;-)
* IsNative (I'll move it to then framework before the commits)

TypesWithDisposableFieldsShouldBeDisposableRule
* You're only reporting a single field. This would force people to run the rules
several times to see all problems. Use the MessageCollection to add each defect 
and
change the message to include the field.Name (instead of the type.Name). 

TypesWithNativeFieldsShouldBeDisposableRule
(same comment as previous)

I'll check the two other rules later...

DisposableTypesShouldHaveFinalizerRule
* here having a single message is perfect :) since the culprit is the type 
itself

DisposableFieldsShouldBeDisposedRule
* it doesn't check array... but what would happen if I had
    IntPtr[] array_of_unmanaged_stuff;
  ??? (which makes a nice test case ;-) I guess you'll need to check the array type
* don't throw exception in rules, if there's a problem either
    - return a Message for this; or
    - let the rule crash, the original exception should be investigated
* each non-disposed field should be reported in it's own Message (you probably
guessed with your TODO) that, that way the developers know each field to fix 
(and
this will, eventually, make it easier to click on the error and get the to 
field in
the source code). I'll add a Location ctor that accept a FieldDescription :)
* IMO TestExtendsDispose should warn the developer. It override Dispose without
calling base.Dispose so it takes full responsibility of the base fields. Very 
good
test case btw!

Once those are fixed (it can be a rule at the time) I'll do false-positive 
testing
with them - but I don't expect much since the rules are kind of strict :)

Thanks!

oh, it just hit me... for performance reason the first two rules should check 
the
type isn't a enum (which will have a bunch of non-disposable and non-native 
fields).

New Location ctor for FieldDefinition is in SVN (r92666).

New Version:

In general
* you should upgrade your copyrights notice to 2008 ;-) also check that the 
header
rule name match the source
* have a look to reduce your using namespaces (VS, if you use it, even has an 
option
to removed unused ones). E.g. Only 3 out of 9 are needed for DisposableHelper.cs
-> fixed

DisposableHelper
* ToTypeDefinition - great :) but there's already code (not in SVN yet since 
there's
a bug in it) to do similar stuff (and a bit more). Once this get fixed we 
should be
able to modify most rocks to accept reference (and call the resolver if needed) 
which
should keep the rules clean (hopefully ;-). Can you adapt your rules/tests to 
work
without it ?
-> 2 to TypeDefinition () is used just to call Implements. This can be removed 
as
soon as your new rocks code is in place. In 
DisposableFieldsShouldBeDisposedRule I
need a TypeDefinition to check for a base implementation of Dispose().
* ImplementsIDisposable doesn't help rule readability (the main point for the 
rocks).
Simply use type.Implements("System.IDisposable") in the rules
-> At first I used another way to check for IDisposable. Since I use the 
Implements
rock now this methods indeed makes no sense anymore. Removed it.
* GetDisposedMethod - this won't work. I know it looks identical to 
GetFinalizer but
that was special case (only one can exists) while you can have both Dispose and
IDisposable.Dispose in the same class (which needs to be unit tested BTW ;-)
-> I changed this to just support Dispose() for the moment. The behavior for 
multiple
Dispose() methods is somewhat strange..I'll need do do some research before 
fixing this.

TypesWithDisposableFieldsShouldBeDisposableRule
* You're only reporting a single field. This would force people to run the rules
several times to see all problems. Use the MessageCollection to add each defect 
and
change the message to include the field.Name (instead of the type.Name). 
-> changed this + Used the new Location FieldDefinition-ctor (also in other 
rules).

DisposableFieldsShouldBeDisposedRule
* unmanaged arrays
-> The *ShouldBeDisposableRules now check for arrays. But I can't imagine a 
useful
way of checking if Dispose() is called for all items in an array without 
getting a
lot of false positives.
* IMO TestExtendsDispose should warn the developer. It override Dispose without
calling base.Dispose so it takes full responsibility of the base fields. Very 
good
test case btw!
-> Added a warning

- no more exceptions (except for ToTypeDefinition)
- isEnum / isInterface checks added.

Small Overview:
DisposableFieldsShouldBeDisposedRule:
 - no warning if Dispose is not implemented
 - warns if base.Dispose is not called (where applicable)
 - warns if Dispose() is not called for a field (no support for arrays)

DisposableTypesShouldHaveFinalizerRule
 - warns if a type has a native field / an array of native fields and no finalizer
defined

TypesWithDisposableFieldsShouldBeDisposableRule
 - warns if a type has disposable fields / arrays and has no Dispose method defined
(also warns if Dispose is abstract)

TypesWithNativeFieldsShouldBeDisposableRule
 - warns if a type has native fields / arrays and has no Dispose method defined (also
warns if Dispose is abstract)

Hey,

A few quick points...

>> I changed this to just support Dispose() for the moment. The behavior for 
multiple
>> Dispose() methods is somewhat strange..I'll need do do some research before 
fixing
>> this.

See
http://msdn2.microsoft.com/en-us/library/system.idisposable.dispose(VS.80).aspx
for the Dispose and Dispose(bool) pattern. IDisposable.Dispose also needs to be
checked because in some cases, like FileStream, the real "Dispose" is called
something else, like Close, so IDisposable is needed.

The interface check is a good idea - but it's not how I think it should be 
handled. 

E.g. TypesWithNativeFieldsShouldBeDisposableRule 

- if this is an interface, with native fields, then it should implement 
IDisposable
(or *warn*) since it would force all types, implementing the interface, to 
support
IDisposable (good design)
- if it's a type, with native fields, and Dispose is abstract when *warn* 
because it
delegates the disposal job to others - augmenting the risk of defects (note: no
matter if the type is abstract or not)
- the current reports should be changed to MessageType.Error (since there's no
possible confusion in this case)
- please keep the type.Implement ("System.IDisposable") inside the rule. This 
makes
it easier to understand (than wondering if GetDisposedMethod does it or not) and
anyway it needs to be changed to deal with, potentially, multiple Dispose 
methods.

 - Interfaces cannot contain fields, so no need to check them.
 - If Dispose is abstract a warning is already issued (see the
TestAbstractNativeFields for example)
 - changed all Error levels except for the base.Dispose() call.
 - readded the Implement checks.

 - TypesWith...ShouldBeDisposableRule now also accept IDisposable.Dispose
 - DisposableFieldsShouldBeDisposedRule will check IDisposable.Dispose (if no
Dispose() is found), follow a call to Dispose(bool), and checks for 
base.Dispose()

TypesWithNativeFieldsShouldBeDisposableRule

The conditions are there but the messages don't always match. If the Dispose is
abstract the message still says to implement IDisposable (which the type has). 
In
this case the message (warning, no an error) should say that Dispose is 
abstract and
inheritors may not be aware of the extra job they need to do (for the native 
fields).

Also I don't like GetDisposeMethod(true) because you can't be sure which 
IDisposable
will be returned, e.g. if only one is abstract ? Since there is potential for
confusion, I think it's best to loop all methods for Dispose candidates and 
report
for each of them.

I think other rules only need ToTypeDefinition because their unit tests use
IDisposable types from another assembly. This should be fixed (in the test) 
because,
while correct, I cannot add ToTypeDefinition in Gendarme - at least not like 
this.
First we must be ensure AssemblyDefinition are cached (otherwise we could end up
loading corlib thousands of times) and it must also fix other problems I have 
;-).
Anyway it was a good catch, but far outside your task :)

Changed the message and changed the code to check both Dispose methods (if both 
are
defined).

The second version has ToTypeDefinition removed and ignores TypeReferences 
(UnitTests
updated to use an internal disposable type)

Everything seems fine except that DisposableFieldsShouldBeDisposedRule tries to 
kill
my machine when I process System.Data (2.0). Eating too much memory or 
recursion, not
sure yet... I'll get a look into it tomorrow.

Otherwise only the documentation is missing!
Thanks

It entered an infinite loop.

Documentation:

TypesWithDisposableFieldsShouldBeDisposableRule

If a type has fields that implement IDisposable, the type should implement
IDisposable and dispose those fields.


Bad Example:

class DoesNotImplementIDisposable {
    IDisposable field;
}

class AbstractDispose : IDisposable {
    IDisposable field;
    public abstract void Dispose ();
}


GoodExample:

class Dispose : IDisposable {
    IDisposable field;
    public void Dispose ()
    {
        field.Dispose ();
    }
}



TypesWithNativeFieldsShouldBeDisposableRule

The rule inspects all fields in a type for it's use of specific types, like 
IntPtr,
UIntPtr and HandleRef. If so the rule warns if the type itself doesn't implement
IDisposable.


Bad Example:

class DoesNotImplementIDisposable {
    IntPtr field;
}

class AbstractDispose : IDisposable {
    IntPtr field;
    public abstract void Dispose ();
}


GoodExample:

class Dispose : IDisposable {
    IDisposable field;
    public void Dispose ()
    {
        UnmanagedFree (field);
    }
}



DisposableTypesShouldHaveFinalizerRule

The rule inspects all fields for native types and reports an error if the type
doesn't have a finalizer (destructor).


Bad Example:

class NoFinalizer {
    IntPtr field;
}


Good Example:

class Finalizer: IDisposable {
    IDisposable field;
    public void Dispose ()
    {
        UnmanagedFree (field);
    }
}



DisposableFieldsShouldBeDisposedRule

The rule inspects all fields for disposable types and, if IDisposable is 
implemented,
check that the Dispose method does indeed call Dispose on all the fields (that
implements IDisposable).


Bad Example:

class DoesNotDisposeMember : IDisposable {
    byte[] buffer;
    IDisposable field;
    public void Dispose ()
    {
        buffer = null;
    }
}


Good Example:

class CallsDispose : IDisposable {
    byte[] buffer;
    IDisposable field;
    public void Dispose ()
    {
        buffer = null;
        field.Dispose ();
    }
}

class DisposePattern : IDisposable {
    byte[] buffer;
    IDisposable field;
    bool disposed;

    public void Dispose ()
    {
        Dispose (true);
    }
    private void Dispose (bool disposing)
    {
        if (disposed)
            return;
        if (disposing) {
            field.Dispose ();
            buffer = null;
        }
        disposed = true;
    }
}

Everything is complete. The last rule was far more complex than I anticipated 
(sorry
;-) in a challenging task (on purpose :) but you did excellent job on it! I'll 
commit
all of them in SVN shortly.

Closing task. Thanks.

p.s. I made two small changes in the last rule to make it process Mono.C5.dll
(checking for method.HasBody and ensuring a FieldDefinition, not Reference, in
ProcessMethod).