Closed GoogleCodeExporter closed 9 years ago
I claim this task.
Original comment by andreas....@gmail.com
on 8 Jan 2008 at 2:18
it's all yours!
Original comment by sebastie...@gmail.com
on 8 Jan 2008 at 2:45
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 ?
Original comment by andreas....@gmail.com
on 10 Jan 2008 at 12:23
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.
Original comment by sebastie...@gmail.com
on 10 Jan 2008 at 12:44
Original comment by andreas....@gmail.com
on 10 Jan 2008 at 8:02
Attachments:
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.
Original comment by andreas....@gmail.com
on 10 Jan 2008 at 8:06
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).
Original comment by sebastie...@gmail.com
on 10 Jan 2008 at 8:14
Updated formatting.
Original comment by andreas....@gmail.com
on 10 Jan 2008 at 8:33
Attachments:
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...
Original comment by sebastie...@gmail.com
on 10 Jan 2008 at 10:03
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!
Original comment by sebastie...@gmail.com
on 11 Jan 2008 at 2:13
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).
Original comment by sebastie...@gmail.com
on 11 Jan 2008 at 3:19
New Location ctor for FieldDefinition is in SVN (r92666).
Original comment by sebastie...@gmail.com
on 11 Jan 2008 at 1:05
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)
Original comment by andreas....@gmail.com
on 11 Jan 2008 at 9:04
Attachments:
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.
Original comment by sebastie...@gmail.com
on 11 Jan 2008 at 10:19
- 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()
Original comment by andreas....@gmail.com
on 12 Jan 2008 at 1:22
Attachments:
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 :)
Original comment by sebastie...@gmail.com
on 12 Jan 2008 at 4:44
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)
Original comment by andreas....@gmail.com
on 12 Jan 2008 at 10:57
Attachments:
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
Original comment by sebastie...@gmail.com
on 13 Jan 2008 at 1:55
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;
}
}
Original comment by andreas....@gmail.com
on 13 Jan 2008 at 10:54
Attachments:
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).
Original comment by sebastie...@gmail.com
on 13 Jan 2008 at 2:43
Original issue reported on code.google.com by
sebastie...@gmail.com
on 29 Dec 2007 at 7:28