A rule a day keeps the doctor away

When I presented on managed Static Code Analysis lately, I was asked (and not for the first time) how good the Static Code Analysis is at finding bugs. Before answering the question it is always necessary to clarify: "What do you mean by bugs?"

The problem with the "bugs" question is that "bug" is very generic term. Bug may originate from incorrect usage of your custom framework classes – while it is a bug, it still may be perfectly valid managed code (valid in a sense that appropriate language constructs and .Net framework classes are used).

Static Code Analysis tools currently are mostly concerned with validity (as defined above) of the code; so they will assist you in finding (potential) problems related to code quality, but they will not help you with the issues specific to your design (and frankly, how could you blame them for that?).

That said – how valuable is the generic "code quality" vs. design specific bugs? Should one bother with those tools at all? I am convinced that there is value in the automated static code analysis tools; and to drive that point home I decided to start showcasing some of Microsoft Static Code Analysis rules. I am not promising to maintain rule-a-day pace, but I will try my best.

So for starters let us have a look at "Initialize reference type static fields inline" InitializeReferenceTypeStaticFieldsInline rule (CA1810). The rule is triggered by the reference type declaring static constructor as shown below:

public class RuleViolation

{

    private static Hashtable _values;

    static RuleViolation()

    {

        _values = new Hashtable();

    }

}

The suggested approach is to initialize static fields inline

public class NoRuleViolation

{

    private static Hashtable _values = new Hashtable();

}

The rule is labeled as Performance rule and the rationale behind it is as follows: the explicit static constructor is expensive, since when one is declared the MSIL code will have to implement a check whether this constructor is called as a pre-requisite for every static member is accessed. On the other hand, when fields are initialized inline, static constructor gets generated by the compiler and gets called only prior to the fields initialization, so that the static fields' initialization is guaranteed to occur before they are accessed (and no check is performed for static methods).

Interesting part of it is that the inline initialization is achieved by compiler in MSIL by setting beforefieldinit flag on the class definition; and it turns out that it may have some side effects (viz., if the static method indirectly depends on static field initialization, it is not guaranteed that the field initialization routine is called when static method is executed). For those that are interested in understanding that in depth, there are couple of very interesting articles on the topic (one by Satya Komatineni and another by John Skeet).

So what's going on? The rule tries to improve performance and instead you may end up with additional problems? Well, that really depends on how one fixes the rule violation. My take on this rule is this – static constructor to me is less about performance and more about correct implementation. Let me elaborate.

When static methods are implemented in managed code, in all probability the code either provides some utility methods or implements a singleton. Since utility methods should not have any state, the rule is not applicable to this situation. What about singleton? It will have internal state and there the rule may be applicable.

If the code violating the rule is singleton implementation, it is naïve and flawed one. To give an example of the implementation that triggers rule violation

public class ConfigurationManager

{

    private static string _configurationValue1;

    private static int _configurationValue2;

    private static DateTime _configurationValue3;

    static ConfigurationManager()

    {

        // in real code - reading from storage

        _configurationValue1 = "value1";

        _configurationValue2 = 1;

        _configurationValue3 = DateTime.Now;

    }

}

If the above is flawed singleton implementation, can we fix rule violation and also improve on design? If instead of just initializing fields inline we rework the implementation, both objectives may be attained:

public class ConfigurationManager

{

    private string _configurationValue1;

    private int _configurationValue2;

    private DateTime _configurationValue3;

    private ConfigurationManager()

    {

        // in real code - reading from storage

        _configurationValue1 = "value1";

        _configurationValue2 = 1;

        _configurationValue3 = DateTime.Now;

    }

    public static readonly ConfigurationManager Instance = new ConfigurationManager();

}

The "good" singleton implementation has only one static field, so field initialization problem for static methods mentioned above is irrelevant. And it does not have potential performance penalty of static constructor as well (since there is no explicit constructor). And finally, the implementation becomes cleaner and more universal than the previous one (since all it does is to uses additional plumbing on top of the ordinary class).

Thus to conclude, violation of InitializeReferenceTypeStaticFieldsInline rule probably gives a signal about design flaw in the implementation, viz. of singleton implemented as class with static fields/properties/methods. The resolution of that violation would be to redesign the class (in such manner that it will implement singleton pattern properly and will have single static field/property), rather than to initialize every static field inline.

Overall, I think this rule amply demonstrates the usefulness of static code analysis rules. This one rule

• Identifies potential performance problem in code
  
• Provides extra educational value (I bet you have learned something new about static initialization in .Net :)
  
• Has added value in automated code review for design flaw detection
  

As usual you are welcome to blast my opinion or voice your agreement in comments to this post.