What's wrong with what FxCop does for globalization, Part 1

by Michael S. Kaplan, published on 2007/10/01 03:31 -04:00, original URI: http://blogs.msdn.com/b/michkap/archive/2007/10/01/5218976.aspx


It was way back in December of last year that I was talking about problems with FxCop and globalization (in posts like this one and this other one).

Then I sort of forgot about it.

Sorry, I have been busy. :-)

But then in the course of a month I was once again reminded of the problems in the form of three large projects that were (for lack of a better word) assaulted by these problems.

If you are a fan of the programs from the BuffyVerse (Buffy the Vampire Slayer and Angel) then you know that one of the rules with vampires is that they can't come in to your home unless you invite them in.

Well, FxCop is the same way -- you have to run it and then do something to follow its advice; if you don't, then nothing bad can befall you. :-)

Here is how the FxCop experience works:

1) You find the FxCop and run it.

2) You don't understand the errors at all as they seem wrong, then you discover that you are running the wrong version of FxCop for your project.

3) Repeat steps 1 and 2 another time or two while you get the right version.

4) Look at the errors and warnings, there are a bunch of level 1 and level 2 errors so you decide for this version to leave the level 3 and level 4 errors alone1.

5) You ship your product.

6) In the space between ending development work on the version you just shipped and starting the development on the next version, someone decides to tackle all of the globalization errors.

Here is where the trouble happens, where some well-meaning developer has decided to let the vampires in, and indeed to help to extract the blood for them from your project.

The specific warnings are described here, there are really just three of them in particular, though they can easily leads to hundreds or even thousands of occurrences in a single project. They apply to every single method that either has an overload that takes a

  • Set locale for data types
  • Specify CultureInfo
  • Specify IFormatProvider
  • The problems with the warnings are twofold:

    1) FxCop assumes that if you are running the method that does not let you specify and IFormatProvider or a CultureInfo or you use the method with an override that dies not let you do so that your usage is wrong.

    2) FxCop shuts up if you specify such a thing, even if it will return results that can be entirely wrong (like passing CurrentUICulture for formatting) or will be ignored )like passing a DateTimeFormatInfo style IFormatProvider when one is formatting numbers).

    #1 can make the code less readable even if it was right before, and #2 the problems are I think self-explanatory though when one considers how easy it is to add CurrentUICulture to get rid of 90% of the warnings even though the answer is almost always wrong, you see where I start ascribing field-like attributes to our FxCop vampire we invited in.

    In one such project, between the time that Vista was released and the beta 3 date of Server 2008, someone had updated the code in 250 different places, almost all of them wrong, and FxCop was happy with each mistake since it assumes that if you take the time to specify an IFormatProvider or a CultureInfo that you must have chosen one correctly, even if the choice is dead wrong.

    Needless to say, the developer who was left holding the bag on this one (she was not the well-meaning developer who introduced the regressions) Kathy, once we walked through the very basic rules by which one could intelligently decide what the right fix should have been, was able to bring the product back to health in short order.

    Unfortunately, not everyone is as skilled as she is, and not everyone has a team like the International Fundamentals team to ask for help.

    But Kathy was kind enough to let me talk about the project as long as I didn't identify it and as long as I gave the rules that she was able to use to quickly bring the code back to a state of readiness. Here they are:

    But getting back to the problem with FxCop, it does not do enough real analysis of the usage. And as long as FxCop wants to warn that the code below is bad (switch to "English (United Kingdom)" in Regional Options to see the differences):

    Console.WriteLine(DateTime.Now.ToString());

    Even when it thinks the code below which returns identical results and is just completely wrong and weird is somehow fine to it:

    Console.WriteLine(DateTime.Now.ToString(NumberFormatInfo.InvariantInfo));

    When the intended code which will return different results would be:

    Console.WriteLine(DateTime.Now.ToString(CultureInfo.InvariantCulture));

    Then FxCop for globalization errors has a net effect of leaving the code in a possibly still incorrect and in all likelihood less readable form....

    To spread the blame around a bit, the .NET Framework decision to have the IFormatProvider parameter be one that is implemented as "if the param applies then use it, otherwise treat is as null" is also slightly boneheaded, since that is the sort of thing that can easily lead to the wrong code being put in there in the first place.

    But it is too late to fix all of them, so with our last defense as FxCop the tool's strength at defending against these scenarios is quite limited....

     

    1 - The bulk of the globalization errors are there in those lower levels, I assume not because globalization isn't important but because there are a lot of false positives so if you average them together then they are less important per warning.

     

    This post brought to you by (U+1001, a.k.a. MYANMAR LETTER KHA)


    # Mike Dimmick on 1 Oct 2007 8:19 AM:

    Ahem. en-GB, not en-UK. But you don't specify that in Control Panel anyway, you specify "English (United Kingdom)".

    # Michael S. Kaplan on 1 Oct 2007 9:39 AM:

    Heh heh, good point. Fixed now.

    Or maybe I meant English (Ukraine)? :-)

    I didn't, of course, though I once had such a custom culture installed....

    # Arun Philip on 2 Oct 2007 2:50 AM:

    Hmm, for strings, I usually cop out ("cop out" -> FxCop, get it? huh? huh??) and use CultureInfo.InvariantCulture.

    Thank you for providing these rules, especially the usage of OrdinalIgnoreCase for filenames.

    # Dean Harding on 2 Oct 2007 5:21 AM:

    FxCop is OK if you've used it right from the beginning of your project and managed to stay on top of *all* the warnings (well, assuming you know the basic rules for string comparison/equality -- which I suppose many people don't). The problem is when you've got a relatively large code base and suddenly decide to run FxCop over it.

    I tried it once on our source code at work, but it spat out something like 15,000 warnings so I just decided not to bother :p

    # David M. Kean on 2 Oct 2007 11:30 PM:

    (Disclosure for others reading this - I work on the FxCop team)

    Now, Michael, first of all, I need to say that I have tremendous amount of respect for you, but I'm going to have to disagree with you. ;)

    > 1) FxCop assumes that if you are running the method that does not let you specify and IFormatProvider or a CultureInfo or you use the method with an override that dies not let you do so that your usage is wrong.

    Remember FxCop orginally was designed to run over the BCL. In the BCL and other low level Frameworks that sit below an application, using the default overload for most methods that take an IFormatProvider (which typically use CultureInfo.CurrentCulture as the default) is almost always the wrong thing to do.  Unfortunately, in things like Windows Applications, this is a tough choice, we have no idea what you're going to do with the result, you could be persisting it to a database, or you could be displaying it to the end user. In this situation, I think its better to be explict about this similar to string comparison.

    Now, we did make a change to warning text for SpecifyIFormatProvider and SpecifyCultureInfo rules for Orcas RTM. They now explictly provide a little more information as to when to use CurrentCulture and InvarientCulture.

    > 2) FxCop shuts up if you specify such a thing, even if it will return results that can be entirely wrong (like passing CurrentUICulture for formatting) or will be ignored )like passing a DateTimeFormatInfo style IFormatProvider when one is formatting numbers).

    We wanted to fire on passing CurrentUICulture when an IFormatProvider (and we have a bug open about it) is expected, however, we ran out-of-time and couldn't get it into Orcas. With regards to passing a DateTimeFormatInfo when formatting numbers; FxCop is not a replacement for common-sense nor code reviews. Yes we could change the rule to catch this situation, however, this would take away precious time from catching other not so obvious problems.

    My take is that if you are making fixes without first understanding the reasoning around the rule and what's it trying to catch - then we've got other problems. FxCop is just a Code Analysis tool whose intentions are good...

    # Michael S. Kaplan on 3 Oct 2007 12:59 AM:

    Fair enough, though given the security questions related to casing and collation, it is hard to say that the current shipping behavior is right (and Orcas has not RTM'ed yet, has it?)....


    referenced by

    2007/10/08 The roadꂸ to the solution starts with identifying the actual problem.NET

    go to newer or older post, or back to index or month or day