[Rule Proposal] Instantiate argument exception correctly
Néstor Salceda
Description: This rule will ensure you are passing the arguments to some
exceptions in the correct order.
References: http://msdn.microsoft.com/en-us/library/ms182347(VS.80).aspx
You will see the usefulness in this example:
Bad example:
{
...
throw new ArgumentException ("param", "The param is not valid");
}
Good example:
{
...
throw new ArgumentException ("The param is not valid", "param");
}
As the two parameters are strings, you could become messed by other
signatures as ArgumentOutOfRangeException(string paramName, string
message).
In the reference you can see a complete list of the exceptions I will
catch and treat.
Best,
Néstor.
Sebastien Pouliot
This is a nice one to have IMO since wrong reports from exceptions are
often more time wasting than no information (e.g. using the default,
empty, ctor on exception) and we already have a rule for that.
Sebastien
Néstor Salceda
A bit delayed, but I think the rule is ready for being commited to the
repository. Take a look and review, please :)
I think that this rule is going to be really useful, because there are
a lot of violations and I have tweaked it in order to achieve really few
false positives.
Néstor.
Sebastien Pouliot
Sorry for the delay in replying to this email (it seems that other
emails just kept popping up on top of it ;-)
Could you give me (or add to the unit tests, ignored) some examples
where the rule reports false positives ?
...
>
>
>
>
>
> C# source attachment (InstantiateArgumentExceptionCorrectlyRule.cs)
>
...
> namespace Gendarme.Rules.Exceptions {
> [Problem ("You are not passing the arguments in the correct order to ArgumentException and derived. You are hiding useful information to developers.")]
it's not just about order (like the patches you sent me proves :-) it
could also be some bad value, so what about something like:
"This method throws ArgumentException (or derived) exceptions
without specifying an existing parameter name. This can hide
useful information to developers."
> [Solution ("Fix the order of the parameters following the recomendations.")]
typo, "recommendation" - but since it's not just about ordering I
suggest:
"Fix the exception parameters to use the correct parameter name
(or make sure the parameters are in the right order)."
> public class InstantiateArgumentExceptionCorrectlyRule : Rule, IMethodRule {
> static string[] checkedExceptions = {
> "System.ArgumentException",
> "System.ArgumentNullException",
> "System.ArgumentOutOfRangeException",
> "System.DuplicateWaitObjectException"
> };
>
> public static TypeReference GetArgumentExceptionThrown (Instruction throwInstruction)
> {
> if (throwInstruction.Previous.OpCode == OpCodes.Newobj) {
> Instruction instantiation = throwInstruction.Previous;
> MethodReference method = (MethodReference) instantiation.Operand;
> if (Array.IndexOf (checkedExceptions, method.DeclaringType.FullName) != -1)
I think you could reuse the IsArgumentException method here
> return method.DeclaringType;
> }
> return null;
> }
>
> public static bool ParameterNameIsLastOperand (MethodDefinition method, Instruction throwInstruction, int exceptionParameters)
> {
> Instruction current = throwInstruction;
> while (current != null && exceptionParameters != 0) {
> if (current.OpCode == OpCodes.Ldstr) {
> string operand = (string) current.Operand;
> //The second operand, a parameter name
> if (exceptionParameters == 2) {
> if (!MatchesAnyParameter (method, operand))
> return false;
> }
> //The first operand, would be handy to
> //have a description
> else {
> //Where are you calling in order
> //to get the message
> if (current.Next != null && current.Next.OpCode.FlowControl == FlowControl.Call) {
> exceptionParameters--;
> continue;
> }
> if (!operand.Contains (" "))
> return false;
> }
> exceptionParameters--;
> }
> current = current.Previous;
> }
> return true;
> }
>
> static bool MatchesAnyParameter (MethodDefinition method, string operand)
> {
> foreach (ParameterDefinition parameter in method.Parameters) {
> if (String.Compare (parameter.Name, operand) == 0)
> return true;
> }
> return false;
> }
>
> public static bool ParameterIsDescription (MethodDefinition method, Instruction throwInstruction)
> {
> Instruction current = throwInstruction;
>
> while (current != null) {
> if (current.OpCode == OpCodes.Ldstr)
> return !MatchesAnyParameter (method, (string) current.Operand);
> current = current.Previous;
> }
> return true;
> }
>
> private static bool IsArgumentException (TypeReference exceptionType)
> {
> return Array.IndexOf (checkedExceptions, exceptionType.FullName) == 0;
> }
>
> private static bool ContainsOnlyStringsAsParameters (MethodReference method)
> {
> foreach (ParameterDefinition parameter in method.Parameters) {
> if (parameter.ParameterType.FullName != "System.String")
> return false;
> }
> return true;
> }
>
> public RuleResult CheckMethod (MethodDefinition method)
> {
> if (!method.HasBody)
> return RuleResult.DoesNotApply;
>
> foreach (Instruction current in method.Body.Instructions) {
> if (current.OpCode != OpCodes.Throw)
Don't compare OpCode (struct), compare OpCode.Code (enum) instead.
> continue;
Would not the logic be easier if the rules checks for NewObj ? and
simply look at the type being created ?
That would also cover the case where a method returns a constructed
exception (or null) for the caller to throw (if non-null) - i.e. the
newobj and throw instruction can be in separate methods.
>
> TypeReference exceptionType = GetArgumentExceptionThrown (current);
> if (exceptionType == null)
> continue;
>
> MethodReference constructor = (MethodReference) current.Previous.Operand;
> if (!ContainsOnlyStringsAsParameters (constructor))
> continue;
>
> int parameters = constructor.Parameters.Count;
>
> if (IsArgumentException (exceptionType)) {
> if (parameters == 1 && !ParameterIsDescription (method, current)) {
> Runner.Report (method, current, Severity.High, Confidence.Normal, "The parameter for this signature should be a description, not a parameter name.");
> continue;
> }
>
> if (parameters == 2 && !ParameterNameIsLastOperand (method, current, parameters))
> Runner.Report (method, current, Severity.High, Confidence.Normal, "The parameter order should be first the description and second the parameter name.");
> }
> else {
> if (parameters == 1 && ParameterIsDescription (method, current)) {
> Runner.Report (method, current, Severity.High, Confidence.Normal, "The parameter for this signature should be the parameter name.");
> continue;
> }
> if (parameters == 2 && ParameterNameIsLastOperand (method, current, parameters))
> Runner.Report (method, current, Severity.High, Confidence.Normal, "The parameter order should be first the parameter name and second the description.");
> }
> }
>
> return Runner.CurrentRuleResult;
> }
> }
> }
Feel free to commit after the fixes!
>
>
>
>
>
>
> C# source attachment (InstantiateArgumentExceptionCorrectlyTest.cs)
...
Tests looks great (I'll look at coverage once they are committed) but
please add the one where an exception is returned, without being thrown,
by a method.
Thanks!
Sebastien
Néstor Salceda
> Hola Nestor,
>
> Sorry for the delay in replying to this email (it seems that other
> emails just kept popping up on top of it ;-)
>
> On Tue, 2008-07-29 at 20:14 +0200, Néstor Salceda wrote:
> > Hey,
> >
> > A bit delayed, but I think the rule is ready for being commited to the
> > repository. Take a look and review, please :)
> >
> > I think that this rule is going to be really useful, because there are
> > a lot of violations and I have tweaked it in order to achieve really few
> > false positives.
>
> Could you give me (or add to the unit tests, ignored) some examples
> where the rule reports false positives ?
Yes, I have detected a false positive when you are loading the string
message from a local, by example:
string msg = String.Format ("The value {0} isn't the expected", value);
throw new ArgumentNullException ("parameter", msg);
It only happens with ArgumentNullException, ArgumentOutOfRangeException
and DuplicateWaitObjectException.
Anyways, I could do as the case when you are calling a method in order
to get the message.
> ...
>
> >
> >
> >
> >
> >
> > C# source attachment (InstantiateArgumentExceptionCorrectlyRule.cs)
> >
>
> ...
>
> > namespace Gendarme.Rules.Exceptions {
> > [Problem ("You are not passing the arguments in the correct order to ArgumentException and derived. You are hiding useful information to developers.")]
>
> it's not just about order (like the patches you sent me proves :-) it
> could also be some bad value, so what about something like:
>
> "This method throws ArgumentException (or derived) exceptions
> without specifying an existing parameter name. This can hide
> useful information to developers."
>
> > [Solution ("Fix the order of the parameters following the recomendations.")]
>
> typo, "recommendation" - but since it's not just about ordering I
> suggest:
>
> "Fix the exception parameters to use the correct parameter name
> (or make sure the parameters are in the right order)."
Cool, your descriptions are better than mine ones.
> > public class InstantiateArgumentExceptionCorrectlyRule : Rule, IMethodRule {
> > static string[] checkedExceptions = {
> > "System.ArgumentException",
> > "System.ArgumentNullException",
> > "System.ArgumentOutOfRangeException",
> > "System.DuplicateWaitObjectException"
> > };
> >
> > public static TypeReference GetArgumentExceptionThrown (Instruction throwInstruction)
> > {
> > if (throwInstruction.Previous.OpCode == OpCodes.Newobj) {
> > Instruction instantiation = throwInstruction.Previous;
> > MethodReference method = (MethodReference) instantiation.Operand;
> > if (Array.IndexOf (checkedExceptions, method.DeclaringType.FullName) != -1)
>
> I think you could reuse the IsArgumentException method here
Not really, because in this condition I'm checking if the exception
type is a type which should be checked. The IsArgumentException only
returns true with ArgumentException (it checks if the index is the first
position).
Okey, this is patched now.
> > continue;
>
> Would not the logic be easier if the rules checks for NewObj ? and
> simply look at the type being created ?
>
> That would also cover the case where a method returns a constructed
> exception (or null) for the caller to throw (if non-null) - i.e. the
> newobj and throw instruction can be in separate methods.
I will study this corner case, because it's interesting. Anyways, the
more frequent case is create a new ArgumentException and throw it.
> >
> > TypeReference exceptionType = GetArgumentExceptionThrown (current);
> > if (exceptionType == null)
> > continue;
> >
> > MethodReference constructor = (MethodReference) current.Previous.Operand;
> > if (!ContainsOnlyStringsAsParameters (constructor))
> > continue;
> >
> > int parameters = constructor.Parameters.Count;
> >
> > if (IsArgumentException (exceptionType)) {
> > if (parameters == 1 && !ParameterIsDescription (method, current)) {
> > Runner.Report (method, current, Severity.High, Confidence.Normal, "The parameter for this signature should be a description, not a parameter name.");
> > continue;
> > }
> >
> > if (parameters == 2 && !ParameterNameIsLastOperand (method, current, parameters))
> > Runner.Report (method, current, Severity.High, Confidence.Normal, "The parameter order should be first the description and second the parameter name.");
> > }
> > else {
> > if (parameters == 1 && ParameterIsDescription (method, current)) {
> > Runner.Report (method, current, Severity.High, Confidence.Normal, "The parameter for this signature should be the parameter name.");
> > continue;
> > }
> > if (parameters == 2 && ParameterNameIsLastOperand (method, current, parameters))
> > Runner.Report (method, current, Severity.High, Confidence.Normal, "The parameter order should be first the parameter name and second the description.");
> > }
> > }
> >
> > return Runner.CurrentRuleResult;
> > }
> > }
> > }
>
> Feel free to commit after the fixes!
Cool :)
> >
> >
> >
> >
> >
> > C# source attachment (InstantiateArgumentExceptionCorrectlyTest.cs)
>
> ...
>
> Tests looks great (I'll look at coverage once they are committed) but
> please add the one where an exception is returned, without being thrown,
> by a method.
Okey Sebastien, I have added the test; and as I expected that method
isn't caught by the rule (it returns success).
Thanks you!
Néstor.