[Rule Proposal] Attribute string literals should parse correctly
Néstor Salceda
Description: As attributes are used at compile time, only constants can
be passed to constructors. You can use an uri but you have to pass the
uri as an string, and this can lead you at some errors in run time.
References: http://msdn.microsoft.com/en-us/library/bb264490(VS.80).aspx
Bad example:
[assembly: AssemblyFileVersion ("fooo")]
The "fooo" string isn't a valid version.
Good example:
[assembly: AssemblyFileVersion ("0.0.1.*")]
The "0.0.1.*" string is a valid version.
I will check version, uri and guid, I will try to parse the operand
passed to the constructor.
Néstor.
Néstor Salceda
I think the rule is ready for its first review :) Yesterday, I was
reviewing the results against mono class libraries, and I have to say
that I haven't found defects; I didn't expect this result, but I'm happy
because it means that, in this aspect, mono rocks :)
There are more test files, in order to test the rule as assembly rule
too.
Néstor.
Sebastien Pouliot
On Sun, 2008-08-10 at 15:39 +0200, Néstor Salceda wrote:
> Hey,
>
> I think the rule is ready for its first review :)
Cool (note: I have not forgotten about the previous one, I'll find it
back and review it asap).
> Yesterday, I was
> reviewing the results against mono class libraries, and I have to say
> that I haven't found defects; I didn't expect this result, but I'm happy
> because it means that, in this aspect, mono rocks :)
Great news (you should blog this ;-)
> There are more test files, in order to test the rule as assembly rule
> too.
Would it be possible to hack around using multiple assemblies by using
Cecil to change the values (inside a try/finally) ? Otherwise this will
complexify the build :(
>
>
>
>
>
> C# source attachment (AttributeStringLiteralsShouldParseCorrectlyRule.cs)
...
> using System;
> using System.Collections;
^^^ needed ?
> using Gendarme.Framework;
> using Gendarme.Framework.Rocks;
> using Mono.Cecil;
>
> namespace Gendarme.Rules.Correctness {
> [Problem ("As you are representing urls, versions or guids as strings, those parameters could be bad formatted and could cause some troubles at run time.")]
> [Solution ("You should format correctly the reported parameters.")]
> public class AttributeStringLiteralsShouldParseCorrectlyRule : Rule, IMethodRule, ITypeRule, IAssemblyRule {
>
> private static bool Contains (string original, string value)
> {
> return original.IndexOf (value, 0, StringComparison.OrdinalIgnoreCase) != -1;
> }
>
> private void CheckParametersAndValues (IMetadataTokenProvider provider, MethodDefinition constructor, IList values)
> {
> for (int index = 0; index < values.Count; index++) {
> ParameterDefinition parameter = constructor.Parameters[index];
> if (String.Compare (parameter.ParameterType.FullName, "System.String") == 0) {
> string value = (string) values[index];
> try {
> if (Contains (parameter.Name, "version")) {
> new Version (value);
> continue;
> }
> if (Contains (parameter.Name, "url") ||
> Contains (parameter.Name, "uri") ||
> Contains (parameter.Name, "urn")) {
> new Uri (value);
> continue;
> }
> if (Contains (parameter.Name, "guid")) {
> new Guid (value);
> continue;
> }
> }
> catch {
can we catch something more specific ? (since we got a rule against "catch all" ;-)
also do some of them provide TryParse methods ? throwing/catching exception is kind of costly and it would be nice to avoid it (not enough to include our own parsing code, but we should use the provided TryParse if available).
> Runner.Report (provider, Severity.High, Confidence.High, String.Format ("The parameter {0} in the attribute {1} is not a valid string for representing the type you are referring.", value, constructor.DeclaringType.FullName));
> }
> }
> }
> }
>
> private void CheckAttributesIn (IMetadataTokenProvider provider)
> {
> ICustomAttributeProvider attributeProvider = provider as ICustomAttributeProvider;
> if (attributeProvider == null)
> return;
AFAIK all the types you're using implements ICustomAttributeProvider. So it would be simpler to use this type (no cast) as the method parameter.
>
> foreach (CustomAttribute attribute in attributeProvider.CustomAttributes)
> CheckParametersAndValues (provider, attribute.Constructor.Resolve (), attribute.ConstructorParameters);
maybe this method could return a RuleResult ? this would remove a bit of duplication on the Check* methods.
> }
>
> public RuleResult CheckMethod (MethodDefinition method)
> {
>
> if (method.CustomAttributes.Count == 0)
> return RuleResult.DoesNotApply;
>
> CheckAttributesIn (method);
>
> return Runner.CurrentRuleResult;
> }
>
> public RuleResult CheckType (TypeDefinition type)
> {
> if (type.CustomAttributes.Count == 0 && type.Fields.Count == 0)
> return RuleResult.DoesNotApply;
>
> CheckAttributesIn (type);
>
> foreach (FieldDefinition field in type.Fields)
> CheckAttributesIn (field);
>
> return Runner.CurrentRuleResult;
> }
>
> public RuleResult CheckAssembly (AssemblyDefinition assembly)
> {
> if (assembly.CustomAttributes.Count == 0)
> return RuleResult.DoesNotApply;
>
> CheckAttributesIn (assembly);
>
> return Runner.CurrentRuleResult;
> }
> }
> }
>
>
>
>
>
>
>
> C# source attachment (AttributeStringLiteralsShouldParseCorrectlyTest.cs)
>
> using System;
> using Gendarme.Rules.Correctness;
> using Test.Rules.Fixtures;
> using Test.Rules.Definitions;
> using NUnit.Framework;
>
> namespace Test.Rules.Correctness {
> [AttributeUsage (AttributeTargets.All)]
> public class ValidSince : Attribute {
> public ValidSince (string assemblyVersion)
> {
> }
> }
>
> [AttributeUsage (AttributeTargets.All)]
> public class Reference : Attribute {
> public Reference (string url)
> {
> }
> }
>
> [AttributeUsage (AttributeTargets.All)]
> public class Uses : Attribute {
> public Uses (string guid)
> {
> }
> }
>
> [TestFixture]
> public class AttributeStringLiteralsShouldParseCorrectlyMethodTest : MethodRuleTestFixture<AttributeStringLiteralsShouldParseCorrectlyRule> {
> [Test]
> public void SkipOnAttributelessMethodsTest ()
> {
> AssertRuleDoesNotApply (SimpleMethods.EmptyMethod);
> }
>
> [ValidSince ("1.0.0.0")]
> [Reference ("http://www.mono-project.com/Gendarme")]
> [Uses ("00000101-0000-0000-c000-000000000046")]
> public void WellAttributedMethod ()
> {
> }
>
> [Test]
> public void SuccessOnWellAttributedMethodTest ()
> {
> AssertRuleSuccess<AttributeStringLiteralsShouldParseCorrectlyMethodTest> ("WellAttributedMethod");
> }
>
> [ValidSince ("foo")]
> [Reference ("bar")]
> [Uses ("0")]
> public void BadAttributedMethod ()
> {
> }
>
> [Test]
> public void FailOnBadAttributedMethodTest ()
> {
> AssertRuleFailure<AttributeStringLiteralsShouldParseCorrectlyMethodTest> ("BadAttributedMethod", 3);
> }
> }
>
> [TestFixture]
> public class AttributeStringLiteralsShouldParseCorrectlyTypeTest : TypeRuleTestFixture<AttributeStringLiteralsShouldParseCorrectlyRule> {
>
> [Test]
> public void SkipOnAttributelessTypesTest ()
> {
> AssertRuleDoesNotApply (SimpleTypes.Class);
> }
>
> [ValidSince ("1.0.0.0")]
> [Reference ("http://www.mono-project.com/Gendarme")]
> [Uses ("00000101-0000-0000-c000-000000000046")]
> class WellAttributedClass {
> }
>
> [Test]
> public void SuccessOnWellAttributedClassTest ()
> {
> AssertRuleSuccess<WellAttributedClass> ();
> }
>
>
> [ValidSince ("foo")]
> [Reference ("bar")]
> [Uses ("0")]
> class BadAttributedClass {
> }
>
> [Test]
> public void FailOnBadAttributedClassTest ()
> {
> AssertRuleFailure<BadAttributedClass> (3);
> }
>
> class WellAttributedClassWithFields {
> [ValidSince ("1.0.0.0")]
> [Reference ("http://www.mono-project.com/Gendarme")]
> [Uses ("00000101-0000-0000-c000-000000000046")]
> object obj;
>
> }
>
> [Test]
> public void SuccessOnWellAttributedClassWithFieldsTest () {
> AssertRuleSuccess<WellAttributedClassWithFields> ();
> }
>
> class BadAttributedClassWithFields {
> [ValidSince ("foo")]
> [Reference ("bar")]
> [Uses ("0")]
> int foo;
> }
>
> [Test]
> public void FailOnBadAttributedClassWithFieldsTest ()
> {
> AssertRuleFailure<BadAttributedClassWithFields> (3);
> }
> }
> }
>
>
>
>
>
>
>
> C# source attachment (BadAssemblyAttributeStringLiteralsShouldParseCorrectlyTest.cs)
>
> [assembly: ValidSince ("foo")]
> [assembly: Reference ("bar")]
> [assembly: Uses ("0")]
>
> namespace Test.Rules.Correctness {
>
> [TestFixture]
> public class BadAssemblyAttributeStringLiteralsShouldParseCorrectlyTest {
> private AssemblyDefinition assembly;
> private TestRunner runner;
>
> [TestFixtureSetUp]
> public void FixtureSetUp ()
> {
> string unit = Assembly.GetExecutingAssembly ().Location;
> assembly = AssemblyFactory.GetAssembly (unit);
> runner = new TestRunner (new AttributeStringLiteralsShouldParseCorrectlyRule ());
> }
We should use the new syntax (Dan's work) for all new unit tests.
> [Test]
> public void FailOnThisAssembly ()
> {
> Assert.AreEqual (RuleResult.Failure, runner.CheckAssembly (assembly));
> Assert.AreEqual (3, runner.Defects.Count);
> }
> }
> }
>
>
>
>
>
>
>
> C# source attachment (GoodAssemblyAttributeStringLiteralsShouldParseCorrectlyTest.cs)
...
>
> [assembly: ValidSince ("1.0.0.0")]
> [assembly: Reference ("http://www.mono-project.com/Gendarme")]
> [assembly: Uses ("00000101-0000-0000-c000-000000000046")]
>
> namespace Test.Rules.Correctness {
> [TestFixture]
> public class GoodAssemblyAttributeStringLiteralsShouldParseCorrectlyTest {
> private IAssemblyRule rule;
> private AssemblyDefinition assembly;
> private TestRunner runner;
>
> [TestFixtureSetUp]
> public void FixtureSetUp ()
> {
> string unit = Assembly.GetExecutingAssembly ().Location;
> assembly = AssemblyFactory.GetAssembly (unit);
> runner = new TestRunner (new AttributeStringLiteralsShouldParseCorrectlyRule ());
> }
We should use the new syntax (Dan's work) for all new unit tests.
>
> [Test]
> public void SuccessOnThisAssembly ()
> {
> Assert.AreEqual (RuleResult.Success, runner.CheckAssembly (assembly));
> Assert.AreEqual (0, runner.Defects.Count);
> }
>
>
> }
> }
Thanks!
Sebastien
Néstor Salceda
> Hola Nestor,
>
> On Sun, 2008-08-10 at 15:39 +0200, Néstor Salceda wrote:
> > Hey,
> >
> > I think the rule is ready for its first review :)
>
> Cool (note: I have not forgotten about the previous one, I'll find it
> back and review it asap).
towards avoiding to annoy in your vacations :)
> > Yesterday, I was
> > reviewing the results against mono class libraries, and I have to say
> > that I haven't found defects; I didn't expect this result, but I'm happy
> > because it means that, in this aspect, mono rocks :)
>
> Great news (you should blog this ;-)
> > There are more test files, in order to test the rule as assembly rule
> > too.
>
> Would it be possible to hack around using multiple assemblies by using
> Cecil to change the values (inside a try/finally) ? Otherwise this will
> complexify the build :(
because I haven't though about that option.
> >
> >
> >
> >
> >
> > C# source attachment (AttributeStringLiteralsShouldParseCorrectlyRule.cs)
>
> ...
>
> > using System;
> > using System.Collections;
>
> ^^^ needed ?
> also do some of them provide TryParse methods ? throwing/catching exception is kind of costly and it would be nice to avoid it (not enough to include our own parsing code, but we should use the provided TryParse if available).
the Uri class. For coherence I wrote that code, but I'm thinking I can
pick the already written parsing method for each class.
> > Runner.Report (provider, Severity.High, Confidence.High, String.Format ("The parameter {0} in the attribute {1} is not a valid string for representing the type you are referring.", value, constructor.DeclaringType.FullName));
> > }
> > }
> > }
> > }
> >
> > private void CheckAttributesIn (IMetadataTokenProvider provider)
> > {
> > ICustomAttributeProvider attributeProvider = provider as ICustomAttributeProvider;
> > if (attributeProvider == null)
> > return;
>
> AFAIK all the types you're using implements ICustomAttributeProvider. So it would be simpler to use this type (no cast) as the method parameter.
possible violation in the CheckParametersAndValues method.
> >
> > foreach (CustomAttribute attribute in attributeProvider.CustomAttributes)
> > CheckParametersAndValues (provider, attribute.Constructor.Resolve (), attribute.ConstructorParameters);
>
> maybe this method could return a RuleResult ? this would remove a bit of duplication on the Check* methods.
to the existing dan's work.
Néstor Salceda
El mar, 12-08-2008 a las 10:03 -0400, Sebastien Pouliot escribió:
> Hola Nestor,
>
> Would it be possible to hack around using multiple assemblies by using
> Cecil to change the values (inside a try/finally) ? Otherwise this will
> complexify the build :(
It's done now, I have removed those old tests and added the only new
ones. I liked it a lot, and I will blog about how to tests assembly
rules :)
> >
> > private void CheckParametersAndValues (IMetadataTokenProvider provider, MethodDefinition constructor, IList values)
> > {
> > for (int index = 0; index < values.Count; index++) {
> > ParameterDefinition parameter = constructor.Parameters[index];
> > if (String.Compare (parameter.ParameterType.FullName, "System.String") == 0) {
> > string value = (string) values[index];
> > try {
> > if (Contains (parameter.Name, "version")) {
> > new Version (value);
> > continue;
> > }
> > if (Contains (parameter.Name, "url") ||
> > Contains (parameter.Name, "uri") ||
> > Contains (parameter.Name, "urn")) {
> > new Uri (value);
> > continue;
> > }
> > if (Contains (parameter.Name, "guid")) {
> > new Guid (value);
> > continue;
> > }
> > }
> > catch {
>
> can we catch something more specific ? (since we got a rule against "catch all" ;-)
>
> also do some of them provide TryParse methods ? throwing/catching exception is kind of costly and it would be nice to avoid it (not enough to include our own parsing code, but we should use the provided TryParse if available).
Yes, I have tried with the Uri.TryCreate method, but the tests failed,
and then I decided take a look to the TryCreate method implementation
and I have created our "own" ones. And I have achieved better error
reporting too.
> > Runner.Report (provider, Severity.High, Confidence.High, String.Format ("The parameter {0} in the attribute {1} is not a valid string for representing the type you are referring.", value, constructor.DeclaringType.FullName));
> > }
> > }
> > }
> > }
> >
> > private void CheckAttributesIn (IMetadataTokenProvider provider)
> > {
> > ICustomAttributeProvider attributeProvider = provider as ICustomAttributeProvider;
> > if (attributeProvider == null)
> > return;
>
> AFAIK all the types you're using implements ICustomAttributeProvider. So it would be simpler to use this type (no cast) as the method parameter.
Well, the main problem with this, is that the Runner.Report method,
needs a IMetadataTokenProvider and there aren't relationship between the
two interfaces.
> >
> > foreach (CustomAttribute attribute in attributeProvider.CustomAttributes)
> > CheckParametersAndValues (provider, attribute.Constructor.Resolve (), attribute.ConstructorParameters);
>
> maybe this method could return a RuleResult ? this would remove a bit of duplication on the Check* methods.
This is a nice catch, and I have fixed :)
> > C# source attachment (BadAssemblyAttributeStringLiteralsShouldParseCorrectlyTest.cs)
> >
>
> > [assembly: ValidSince ("foo")]
> > [assembly: Reference ("bar")]
> > [assembly: Uses ("0")]
> >
> > namespace Test.Rules.Correctness {
> >
> > [TestFixture]
> > public class BadAssemblyAttributeStringLiteralsShouldParseCorrectlyTest {
> > private AssemblyDefinition assembly;
> > private TestRunner runner;
> >
> > [TestFixtureSetUp]
> > public void FixtureSetUp ()
> > {
> > string unit = Assembly.GetExecutingAssembly ().Location;
> > assembly = AssemblyFactory.GetAssembly (unit);
> > runner = new TestRunner (new AttributeStringLiteralsShouldParseCorrectlyRule ());
> > }
>
> We should use the new syntax (Dan's work) for all new unit tests.
Yes, I have sent a patch to the group for add the
AssemblyRuleTestFixture and I'm using the dan's syntax now :)
Thanks!
Néstor.