search

shana / google-highly-open-participation-mono

Automatically exported from code.google.com/p/google-highly-open-participation-mono
0 stars 0 forks source link

Three Gendarme rules about attributes #65

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
This task is to create three (3) new rules for Gendarme [1].

a) AvoidUnsealedAttributesRule

For performance reasons attribute types should be sealed. This rule should
warn the developer of any unsealed attributes inside an assembly.

b) MissingAttributeUsageOnCustomAttributeRule

Every custom attribute should specify how it is intended to be used. This
is done by adding an [AttributeUsage] attribute on the type. The rule
should ensure that every attribute type is decorated with an
[AttributeUsage] attribute.

c) AttributeArgumentsShouldHaveAccessorsRule

All the arguments to the attribute constructors should map to properties on
the attribute type. The rule should check that all (parameters) of them has
a corresponding public property defined.

So 2 easy rules (to learn about Gendarme and Cecil) and one a bit harder
(the real challenge ;-).

To complete the task it must include:
- the rules source code;
- unit tests to ensure the rule 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).

Note: There are other similar cases (where root is needed) and this rule
will be extended in the future to track similar patterns (but this is
outside this task).

[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 29 Dec 2007 at 2:52

GoogleCodeExporter commented 9 years ago 
I want to take this (soon as you mark my last one 'closed').

Original comment by dan.abra...@gmail.com on 29 Dec 2007 at 10:28

GoogleCodeExporter commented 9 years ago 
go!

Original comment by sebastie...@gmail.com on 29 Dec 2007 at 4:19

GoogleCodeExporter commented 9 years ago 
lol, yes you can ignore the following note since you discovered by
uber-copy-paste-hack ;-)

Note: There are other similar cases (where root is needed) and this rule
will be extended in the future to track similar patterns (but this is
outside this task).

Original comment by sebastie...@gmail.com on 29 Dec 2007 at 4:22

GoogleCodeExporter commented 9 years ago 
Done (quick, huh) :-) !

Original comment by dan.abra...@gmail.com on 29 Dec 2007 at 4:31

Attachments:

GoogleCodeExporter commented 9 years ago 
If I can throw in my 2 cents, I think AvoidUnsealedAttributeRule should be
AvoidUnsealedConcreteAttributeRule. I'm working on a project right now that has 
a
complex system of custom attributes, many of which are abstract. It would be 
annoying
to get a warning for each of abstract Attribute type. I think the rule ought to 
be,
warn if the type inherits from Attribute and is not abstract and is not sealed. 
My 2
cents.

Original comment by lunchtim...@gmail.com on 29 Dec 2007 at 8:43

GoogleCodeExporter commented 9 years ago 
Yes, I think you're right.

Original comment by dan.abra...@gmail.com on 29 Dec 2007 at 11:17

Attachments:

GoogleCodeExporter commented 9 years ago 
Two other things I noticed while going over the code: You have the same
InheritsFromAttribute method in all three classes. It would be better to 
abstract
this and reduce code duplication (this might be something to consider doing 
with the
other Attribute-related rules we already have, but that's outside the scope of 
this
issue). Also, in MissingAttributeUsageOnCustomAttributeRule.cs, you should make 
the
ctorName string on line 59 into a private const string.

Original comment by lunchtim...@gmail.com on 30 Dec 2007 at 3:53

GoogleCodeExporter commented 9 years ago 
Both valid points, thanks :)

In the case of InheritsFromAttribute I think it would be nice to "promote" it 
as a
framework rock, just like we already have a Implements extension method (simply
renaming it and adding the full type name as a parameter would be enough).

see 
http://groups.google.com/group/gendarme/browse_thread/thread/6a13eec0b7bb7f77

This "promotion" will be more important when the AssemblyResolver (not yet 
ready to
be committed) is added to Gendarme (which will let us go from 
[Method|Type]Reference
to [Method|Type]Definition.

Original comment by sebastie...@gmail.com on 30 Dec 2007 at 4:02

GoogleCodeExporter commented 9 years ago 
Oh, I forgot about it!
Since extension methods don't compile with my version of Mono, I decided to 
test code
first without them, and then let Sebastien move common code into Rocks.

Original comment by dan.abra...@gmail.com on 30 Dec 2007 at 10:35

GoogleCodeExporter commented 9 years ago 
Done some things (e.g. added IsAttribute () to rocks) but I'm unsure that this 
works
because I can't compile it (and therefore test it).
Also I moved tests for it to rocks test units.
Again, I'm not sure it all compiles (and works) correctly, so it might need some
additional work and testing.
P.S. Seems like count of Gendarme completed tasks will overgrow generic mono 
tasks'
count. Keep going!
P.P.S. I think I really loved Mono, Cecil and Gendarme while doing these tasks. 
Thank
you!

Original comment by dan.abra...@gmail.com on 30 Dec 2007 at 12:07

GoogleCodeExporter commented 9 years ago 
Oh - and the files themselves!

Original comment by dan.abra...@gmail.com on 30 Dec 2007 at 12:12

Attachments:

GoogleCodeExporter commented 9 years ago 
> Also I moved tests for it to rocks test units.
I meant unit tests, of course.

Original comment by dan.abra...@gmail.com on 30 Dec 2007 at 4:19

GoogleCodeExporter commented 9 years ago 
Sorry Dan but I don't have enough time to test this properly before going away. 
I'll
be back on the Jan 2nd but, iirc, you'll already be away on vacations. I'll 
ping you
(this task) when I back, but from past work, I don't expect much problem :)

Have nice vacations with your family!
Sebastien

Original comment by sebastie...@gmail.com on 31 Dec 2007 at 3:29

GoogleCodeExporter commented 9 years ago 
Thanks, you too :) !
See you after vacations (I'll be back on 12-13th).

Good bye!

Original comment by dan.abra...@gmail.com on 31 Dec 2007 at 12:35

GoogleCodeExporter commented 9 years ago 
I modified the TypeRock parts to implement IsAttribute on top of a new Inherits
extension method. I also simplified MissingAttributeUsageOnCustomAttributeRule 
a bit
by using the HasAttribute rock.

One last thing you attached AttributeArgumentsShouldHaveAccessorsTest.cs twice, 
so I
don't have the source of the rule to review it :(

Changing due date to January 15th (after Dan's vacations).

Original comment by sebastie...@gmail.com on 2 Jan 2008 at 9:16

GoogleCodeExporter commented 9 years ago 
AvoidUnsealedConcreteAttributes[Rule|Test].cs is in SVN

Original comment by sebastie...@gmail.com on 4 Jan 2008 at 2:01

GoogleCodeExporter commented 9 years ago 
MissingAttributeUsageOnCustomAttribute[Rule|Test].cs is in SVN

Original comment by sebastie...@gmail.com on 5 Jan 2008 at 3:27

GoogleCodeExporter commented 9 years ago 
And here I am again :-) !

I had great time in Finland, no f.{5}g computers there but lots of snow, girls 
and
fun instead :) .

AttributeArgumentsShouldHaveAccessorsRule'll be submitted this evening (as I 
have to
set up the internet connection at my grandma's place where I'm currently 
living).

Nice to hear from you,
Dan.

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

GoogleCodeExporter commented 9 years ago 
Here it is.

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

Attachments:

GoogleCodeExporter commented 9 years ago 
Hey, Happy 2008! I'm glad you enjoyed your vacations (and the snow, there's 
been too
much here to enjoy - at least for my taste ;-).

This code is nice, but here's a simpler way to do the first part (no need to 
change
your code). It's a special case (i.e. we can't use it often) because you're 
checking
properties from a TypeDefinition, so you can iterate them differently. So...

            foreach (PropertyDefinition property in typeDefinition.Properties) {
                if (property.GetMethod != null) {
                    allProperties.Add (property.Name);
                }
            }
... can replace ...
            foreach (MethodDefinition method in typeDefinition.Methods) {
                if ((method.SemanticsAttributes & MethodSemanticsAttributes.Getter) != 0) {
                    // yep, it's getter
                    string propertyName = method.Name.Substring (4);
                    allProperties.Add (propertyName);
                }
            }

Now I think the only missing thing for closing this task is documentation! I'll
commit the (updated) code/tests to SVN now.

Thanks!
Sebastien

Original comment by sebastie...@gmail.com on 12 Jan 2008 at 3:34

GoogleCodeExporter commented 9 years ago 
Last rule was committed in SVN.
Only documentation is missing to close this task ;-)

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

GoogleCodeExporter commented 9 years ago 
Forgot about docs, sorry.

Docs themselves huh:

AvoidUnsealedAttributesRule

This rule is used to warn the developer if both unsealed and concrete (not 
abstract) 
attribute classes are defined in the assembly. If you want other attributes to 
be 
able to derive from your attribute class, make it abstract. Otherwise, make it 
sealed to improve the performance.

Bad example:

[AttributeUsage (AttributeTargets.All)]
public class BadAttribute : Attribute {
}

Good example #1:

[AttributeUsage (AttributeTargets.All)]
public sealed class SealedAttribute : Attribute {
}

Good example #2:

[AttributeUsage (AttributeTargets.All)]
public abstract class AbstractAttribute : Attribute {
}

[AttributeUsage (AttributeTargets.All)]
public sealed class ConcreteAttribute : AbstractAttribute {
}

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

MissingAttributeUsageOnCustomAttributeRule

Every custom attribute must be decorated with [AttributeUsage] attribute to 
specify 
to which kind of code members it can be applied.

Bad example:

public sealed class SomeAttribute : Attribute {
}

Good examples:

[AttributeUsage (AttributeTargets.All)]
public sealed class AttributeApplyingToAnything : Attribute {
}

[AttributeUsage (AttributeTargets.Field)]
public sealed class AttributeApplyingToFields : Attribute {
}

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

AttributeArgumentsShouldHaveAccessorsRule

All parameters passed to the attribute constructor must be visible through 
properties with properly cased names (e.g. myValue parameter would map to 
MyValue 
property).

Bad example:

[AttributeUsage (AttributeTargets.All)]
public sealed class AttributeWithRequiredProperties : Attribute {
    private int storedFoo;
    private string storedBar;

    public AttributeWithRequiredProperties (int foo, string bar)
    {
        storedFoo = foo;
        storedBar = bar; // we have no corresponding property with the name 'Bar' so 
the rule will fail
    }

    public int Foo {
        get { return storedFoo; }
    }
}

Good example:

[AttributeUsage (AttributeTargets.All)]
public sealed class AttributeWithRequiredProperties : Attribute {
    private int storedFoo;
    private string storedBar;

    public AttributeWithRequiredProperties (int foo, string bar)
    {
        storedFoo = foo;
        storedBar = bar;
    }

    public int Foo {
        get { return storedFoo; }
    }

    public string Bar {
        get { return storedBar; }
    }
}

Original comment by dan.abra...@gmail.com on 12 Jan 2008 at 7:14

GoogleCodeExporter commented 9 years ago 
Closing, Thanks!

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