[Rule Proposal] Provide Correct Arguments To Formatting Methods
Néstor Salceda
Description: When you are using some formatter methods (as the well
known String.Format by example) the argument passed does not contain a
correct format item and can lead us to a FormatException.
References:
http://msdn.microsoft.com/en-us/library/ms182361(VS.80).aspx
Good example:
public void MyMethod (object value)
{
string message = String.Format ("The value {0} is not valid.", value);
//More code ...
}
Bad example:
public void MyMethod (object value)
{
string message = String.Format ("The value {0} is not valid");
//this will throw a format exception!
//More code ...
}
As always, feedback is welcome.
Néstor.
Exception
I would like to take this one if no one minds.
Best,
Dan
Sebastien Pouliot
> Hello,
> I would like to take this one if no one minds.
Seriously the email was not a solicitation. Nestor is c.c. the gendarme
mailing list for the work he is doing part of the Google Summer of Code
2008 (itself managed by another mailing-list) in order to get feedback
before starting to work on a new rule.
However there are other challenging rules to be hacked upon :)
Sebastien
Exception
Sure.
On 17 июл, 01:34, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
Sebastien Pouliot
On Wed, 2008-07-16 at 18:21 +0200, Néstor Salceda wrote:
> Title: Provide Correct Arguments To Formatting Methods
>
> Description: When you are using some formatter methods (as the well
> known String.Format by example)
You will need to build a list of such calls.
> the argument passed does not contain a
> correct format item and can lead us to a FormatException.
I'm sure that dealing with a variable number of parameters will be
fun ;-)
> References:
> http://msdn.microsoft.com/en-us/library/ms182361(VS.80).aspx
>
> Good example:
>
> public void MyMethod (object value)
> {
> string message = String.Format ("The value {0} is not valid.", value);
> //More code ...
> }
>
> Bad example:
>
> public void MyMethod (object value)
> {
> string message = String.Format ("The value {0} is not valid");
> //this will throw a format exception!
>
> //More code ...
> }
Please also consider the case where
String.Format ("I forgot to include a ... inside my string");
with a less severe Severity level (since it won't throw an exception).
> As always, feedback is welcome.
Sebastien
Néstor Salceda
Yes, my intention with this e-mails is getting some feedback before
starting the rule. I can evaluate the rule utility and get some useful
comments that can guide me through the implementation.
Anyways, if you wanted take this rule, I'm sure you have some ideas
about the implementation and perhaps some corner cases that could escape
from my mind. Then, I will appreciate your comments too, Dan.
Best,
Néstor.
Néstor Salceda
> Hola Nestor,
>
> On Wed, 2008-07-16 at 18:21 +0200, Néstor Salceda wrote:
> > Title: Provide Correct Arguments To Formatting Methods
> >
> > Description: When you are using some formatter methods (as the well
> > known String.Format by example)
>
> You will need to build a list of such calls.
Yes, by the moment I check only calls to String.Format method, but it
wouldn't be hard extend this checking to the other calls (perhaps
Console.WriteLine, Console.Err.WriteLine ...).
> > the argument passed does not contain a
> > correct format item and can lead us to a FormatException.
>
> I'm sure that dealing with a variable number of parameters will be
> fun ;-)
Hehe, I'm doing this through a regular expression that I'm doing by
myself (I will check the String.Format code later, because it's probable
this regexp or some recognizer has been done yet).
The main problem is that strings like this:
"{0} is the value of {1}. I repeat {0}" this string matches 3 times
with the regexp, but the {0} element should be counted once, not twice.
> > References:
> > http://msdn.microsoft.com/en-us/library/ms182361(VS.80).aspx
> >
> > Good example:
> >
> > public void MyMethod (object value)
> > {
> > string message = String.Format ("The value {0} is not valid.", value);
> > //More code ...
> > }
> >
> > Bad example:
> >
> > public void MyMethod (object value)
> > {
> > string message = String.Format ("The value {0} is not valid");
> > //this will throw a format exception!
> >
> > //More code ...
> > }
>
> Please also consider the case where
> String.Format ("I forgot to include a ... inside my string");
> with a less severe Severity level (since it won't throw an exception).
Okey.
As data, I can say the first time I ran the rule against classlib, I
get about 1200 violations. A lot of false positives, and now I'm
getting about 170 (still with some false positives pending).
Thanks for your comments, Sebastien.
Néstor.
Néstor Salceda
I think the rule has achieved enough false positives for its first
revision.
As always, the rule is also in the svn, and I should say that only 37
defects has been found, and I have tried to minimize the false positives
(the first time I ran the rule against mono, I got about 1290 errors!).
Thanks!
Néstor.
Sebastien Pouliot
> Hey,
>
> I think the rule has achieved enough false positives for its first
> revision.
I guess/hope you mean enough "reduction in" false positives ? ;-)
> As always, the rule is also in the svn, and I should say that only 37
> defects has been found, and I have tried to minimize the false positives
> (the first time I ran the rule against mono, I got about 1290 errors!).
I'm probably too "performance minded" right now ;-) but all I can see
are the memory allocations (and there's a lot of them) inside the rule.
Commit it (as-is) for now (HEAD only) and let's take some time next week
(I'll be in vacations for the next two weeks) to look into optimizing
it. I have been thinking of blogging an HOWTO for this* and it looks
like a good opportunity :-)
Have a nice weekend!
Sebastien
* and a developers FAQ too...
Néstor Salceda
El vie, 18-07-2008 a las 16:01 -0400, Sebastien Pouliot escribió:
> On Thu, 2008-07-17 at 20:13 +0200, Néstor Salceda wrote:
> > Hey,
> >
> > I think the rule has achieved enough false positives for its first
> > revision.
>
> I guess/hope you mean enough "reduction in" false positives ? ;-)
Heh, you are right :)
> > As always, the rule is also in the svn, and I should say that only 37
> > defects has been found, and I have tried to minimize the false positives
> > (the first time I ran the rule against mono, I got about 1290 errors!).
>
> I'm probably too "performance minded" right now ;-) but all I can see
> are the memory allocations (and there's a lot of them) inside the rule.
Ooops.
> Commit it (as-is) for now (HEAD only) and let's take some time next week
> (I'll be in vacations for the next two weeks) to look into optimizing
> it. I have been thinking of blogging an HOWTO for this* and it looks
> like a good opportunity :-)
Okey, I agree.
The rule has been commited in 108271 and 108272.
> Have a nice weekend!
> Sebastien
>
> * and a developers FAQ too...
Thanks!
Néstor.
Exception
Why do you use regular expressions instead of just checking each
string's character in the loop? That'd be, seems to me, a little but
faster. Also, I would replace List<char> with HashSet<char> which
seems to be more natural for this since it can handle multiple Add ()
calls without really adding the same thing twice.
private readonly HashSet<char> expectedParameters = new HashSet<char>
();
// GetExpectedParameters
expectedParameters.Clear ();
for (int i = 0; i < loaded.Length; i++) {
if (loaded [i] == '{' && Char.IsDigit (loaded [i+1]))
expectedParameters.Add (loaded [i+1]);
}
return expectedParameters.Count;
I haven't checked this code but it must work.
Here are string.Format-like some methods you'll need to handle:
StringBuilder.AppendFormat (...)
Console.WriteLine (...) (obviously)
TextWriter.WriteLine (...) (and you'll need to handle classes deriving
from it)
Good luck!
Dan
Néstor Salceda
El vie, 18-07-2008 a las 22:31 -0700, Exception escribió:
> Hola Néstor,
> Why do you use regular expressions instead of just checking each
> string's character in the loop? That'd be, seems to me, a little but
> faster.
need a more complex approach for counting the amount of parameters, but
it's proven I don't need it. It's a good catch :)
> Also, I would replace List<char> with HashSet<char> which
> seems to be more natural for this since it can handle multiple Add ()
> calls without really adding the same thing twice.
This is a good catch too.
> private readonly HashSet<char> expectedParameters = new HashSet<char>
> ();
>
> // GetExpectedParameters
> expectedParameters.Clear ();
> for (int i = 0; i < loaded.Length; i++) {
> if (loaded [i] == '{' && Char.IsDigit (loaded [i+1]))
> expectedParameters.Add (loaded [i+1]);
> }
>
> return expectedParameters.Count;
>
> I haven't checked this code but it must work.
Sure, I think so.
> Here are string.Format-like some methods you'll need to handle:
> StringBuilder.AppendFormat (...)
> Console.WriteLine (...) (obviously)
> TextWriter.WriteLine (...) (and you'll need to handle classes deriving
> from it)
Oh! thanks for this list too, Dan.
> Good luck!
Thanks you very much for your comments!
Néstor.