Three Gendarme rules for non-C# code

[GoogleCodeExporter]

This task is to create three (3) rule for Gendarme [1] to check conditions
that aren't possible in C# - but otherwise possible with other .NET
compilers (or in IL assemnby). Note: Rules and tests *must* be written in C#

1. Gendarme.Rules.Security.StaticConstructorsShouldBePrivateRule

This rule should warn if it finds any static constructor that isn't
private. This is a security rule because non-private static ctor could be
called, once or multiple time, from user code.

2. Gendarme.Rules.Design.FinalizersShouldBeProtected

This rule should warn if it finds any finalizer that isn't protected. This
is normally enforced by compiler but can be a mistake done when using IL.

3. Gendarme.Rules.Design.FinalizersShouldCallBaseClassFinalizer

The rule should scan the IL of existing finalizer to ensure it calls it's
base type finalizer. This is normally enforced by compiler but can be a
mistake done when using IL. There is a existing rule, EmptyDestructorRule,
that does something very similar.

So three easy rules for Gendarme. The main difficulity of this task will be
to create the unit tests (in C#). The trick is to use Cecil to create the
assemblies (AssemblyDefinition) to be tested. Note that this trick is
already used in some unit tests available in SVN.

To complete the task it must include:
- the rules source code;
- unit tests to ensure the rules works correctly;
- documentation for the web site (see [2] for an example)

This tasks should take between 2-5 days (depending if you already know
Gendarme/Cecil or not).

[1] http://www.mono-project.com/Gendarme
[2] http://groups.google.com/group/gendarme

Original issue reported on code.google.com by sebastie...@gmail.com on 10 Jan 2008 at 10:26

[GoogleCodeExporter]

Sometimes I'm thinking that test fixtures use *too* much cecil-related common 
code.
Maybe we should refactor 'em a little (e.g. before next release).

P.S. I want to claim this when my last one gets closed (i.e. when I submit the
missing piece of code and you review it).

Original comment by dan.abra...@gmail.com on 12 Jan 2008 at 11:22

[GoogleCodeExporter]

Hi, I'm currently posting some stuff I've already done (so that you won't have 
to 
wait until I get it finished).
As of FinalizersShouldBeProtected, I'm not really sure if it's needed. Look, 
AFAIK 
finalizer is somewhat called 'Finalize ()' overriding base class' *protected* 
Finalize () method so I can see no way for it to be other than protected, 
unless the 
compiler is really crazy (and ilasm won't eat it anyways) :-) . Maybe I don't 
get 
the point, do I?

Cheers,
Dan.

P.S. Hope to hear from you soon 'cause I'm going to school on 14th and 'll be a 
bit 
busy with schoolwork :-)

Original comment by dan.abra...@gmail.com on 12 Jan 2008 at 6:50

Attachments:

• FinalizersShouldCallBaseClassFinalizerRule.cs
• FinalizersShouldCallBaseClassFinalizerTest.cs
• StaticConstructorsShouldBePrivateRule.cs
• StaticConstructorsShouldBePrivateTest.cs

[GoogleCodeExporter]

Hey Dan, you need to complete task #65 (missing rule documentation) before I can
assign this task to you.

Original comment by sebastie...@gmail.com on 12 Jan 2008 at 7:04

[GoogleCodeExporter]

It's yours

> and ilasm won't eat it anyways

well I beg to differ ;-)

.assembly extern mscorlib
{
    .ver 1:0:5000:0
    .publickeytoken = (B7 7A 5C 56 19 34 E0 89 )
}
.class public auto ansi beforefieldinit BadFinalizer extends 
[mscorlib]System.Object
{
     .method public hidebysig instance void Finalize() cil managed
     {
     }
}

compile with both mono and ms ilasm.

Original comment by sebastie...@gmail.com on 12 Jan 2008 at 7:19

• Changed state: Claimed
• Added labels: ClaimedBy-dan.abramov, DueDate-2008-01-19

[GoogleCodeExporter]

I thought that I've sent them before I realized that I didn't (and had to write 
them
again).

As of these three rules, here are docs for the (?) completed ones:

FinalizersShouldCallBaseClassFinalizerRule

This rule is used to warn the developer if a finalizer does not call base class
finalizer. In C#, this is enforced by compiler but some .NET languages may 
allow such
kind of behavior, which should not be allowed.

Good example:

public class GoodFinalizer {
    ~GoodFinalizer () 
    {
        // C# compiler will insert base.Finalize () call here
        // so any compiler-generated code will be valid
    }
}

Bad example (IL):

.assembly extern mscorlib
{
    .ver 1:0:5000:0
    .publickeytoken = (B7 7A 5C 56 19 34 E0 89 )
}
.class public auto ansi beforefieldinit BadFinalizer extends 
[mscorlib]System.Object
{
     .method protected hidebysig instance void Finalize() cil managed
     {
             // no base call so rule will fire here
     }
}

*********************************************************************

StaticConstructorsShouldBePrivateRule

To avoid calls from user code, all static constructors must be private. C# 
allows
only private static constructors but some .NET languages (including VB .NET) do 
not
permit defining non-private static constructors (Shared in VB .NET), which is 
not a
good practice.

Good example (C#):

public class PrivateCctor {
    ~PrivateCctor () { } // it is private
}

Good example (VB .NET):

Public Class PrivateCctor
    Private Shared Sub New ()
    End Sub
End Class

Bad example (VB .NET):

Public Class PublicCctor
    Public Shared Sub New ()
    End Sub
End Class

Original comment by renessan...@gmail.com on 13 Jan 2008 at 10:30

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

That was me, of course :-)

Original comment by dan.abra...@gmail.com on 13 Jan 2008 at 10:33

[GoogleCodeExporter]

About StaticConstructorsShouldBePrivateRule, are you sure about that ?

Good example (C#):

public class PrivateCctor {
    ~PrivateCctor () { } // it is private
}

otherwise this rule is fine ;-)

Original comment by sebastie...@gmail.com on 13 Jan 2008 at 4:25

[GoogleCodeExporter]

StaticConstructorsShouldBePrivateRule is now in SVN :)

FinalizersShouldCallBaseClassFinalizerRule is needlessly complex. There's no 
need to
create a stack then pop all instruction from it (it only takes more time and 
memory).
Instead you should just iterate the instructions and see if there's a call to
base.Finalize (not just any Finalize), all other instructions don't matter.

Original comment by sebastie...@gmail.com on 13 Jan 2008 at 4:54

[GoogleCodeExporter]

Updates.

Docs for FinalizersShouldBeProtectedRule:

FinalizersShouldBeProtectedRule

All finalizers must be protected because otherwise they can be called from the 
user
code. In C# this is enforced by compiler, but some languages may not have such a
restriction, thus making developer able to declare finalizer non-protected, 
which is
a mistake.

Good example (C#):
public class GoodProtectedFinalizer ()
{
    ~GoodProtectedFinalizer () { } // compiler makes it protected
}

Good example (IL):
.class family auto ansi beforefieldinit GoodProtectedFinalizer extends
[mscorlib]System.Object
{
     .method family hidebysig instance void Finalize() cil managed
     {
             // ...
     }
}

Bad example (IL):
.class family auto ansi beforefieldinit BadPublicFinalizer extends
[mscorlib]System.Object
{
     .method public hidebysig instance void Finalize() cil managed
     {
             // ...
     }
}

P.S. Change "protected" to "family" in method signature in docs to
FinalizersShouldCallBaseClassFinalizerRule. Also, I think we can exclude 
.assembly
section there.

Original comment by fotoinf...@gmail.com on 14 Jan 2008 at 12:00

Attachments:

• FinalizersShouldBeProtectedRule.cs
• FinalizersShouldCallBaseClassFinalizerRule.cs
• FinalizersShouldBeProtectedTest.cs
• FinalizersShouldCallBaseClassFinalizerTest.cs

[GoogleCodeExporter]

dude, how many google accounts do you have ? ;-)

FinalizersShouldCallBaseClassFinalizerRule
* can you think of _one_ case where this won't be true but still be ok ? ;-)

Otherwise all is fine :)

Original comment by sebastie...@gmail.com on 14 Jan 2008 at 12:53

[GoogleCodeExporter]

Google Toolbar certainly needs a quick account switcher :) .

> can you think of _one_ case where this won't be true but still be ok

Dunno, really. What d'you mean?

Original comment by dan.abra...@gmail.com on 14 Jan 2008 at 1:06

[GoogleCodeExporter]

I'll tell you but you'll wish I had not...

System.Object cannot call it's base class - and because every hierarchy has a
starting point it's still valid.

Told ya ;-)

Original comment by sebastie...@gmail.com on 14 Jan 2008 at 1:10

[GoogleCodeExporter]

Updated.
The only problem is since we have no AssemblyResolver yet, I'm not sure if I 
can load
System.Object to test it (as I can't know where exactly is runtime installed).

Original comment by dan.abra...@gmail.com on 14 Jan 2008 at 6:39

Attachments:

• FinalizersShouldCallBaseClassFinalizerRule.cs

[GoogleCodeExporter]

AssemblyResolver should not be a problem (in this case) since this will occur 
only
while scanning mscorlib.dll. Checking the type name (or the base type like you 
did)
is ok.

Closing, I'll commit the rules later.
Thanks!
Sebastien

Original comment by sebastie...@gmail.com on 14 Jan 2008 at 1:37

• Changed state: Closed