The Mystery of Cascading FxCop Errors

As I mentioned in my last post, I've been working through FxCop concerns on my current project. To provide some context, this is a large project with more than 500K lines of code, and FxCop was added late to the development process. Naturally, out of the gate there were thousands of errors, so we had to ease it into the development process by slowly turning on rules until we had the warnings under control.

To facilitate this, we had a dedicated team go through the entire code base and address as many of the errors as they could: implementing IDispose correctly, validating arguments, etc. In the end, we had a hand picked set of rules that applied to our scenarios and a code base that adhered to them. Once the existing violations were under control, we flipped the switch to treat new analysis warnings as errors to ensure that the violations were not introduced into source control.

There were some bumps along the way, but in the end it was manageable (royal pain in the butt, but manageable).

Recently however, my team started to see serious problems with code analysis throughout parts of the application. They found that if they made a minor change, a series of FxCop violations would appear -- even in areas that they didn't touch or had previously been reviewed and considered "warning free". The team believed that FxCop was somehow incrementally analyzing files, thereby finding new problems which had previously been skipped. While this theory fit into the symptom of new violations for existing code, it just didn't seem right: FxCop analyzes compiled assemblies, not individual files.

The problem with code analysis errors is that they're not always obvious what is causing the problem. It's easy to fall into a trap to believe that Code Analysis is only looking at the internal structure or logic of a class for correctness. There are many FxCop rules that look for correctness, such as Methods that can be private or static, arguments that can be null, etc. The really obscure errors are the ones that look at how a class is used by other classes. What may seem like a trivial change can introduce a different execution path that suddenly exposes the class and its dependencies for consideration of new FxCop violations.

Let's take a look at a contrived example. Let's say I'm using a Service Locator to wire up a ViewModel and like any good Inversion of Control junkie, I'm using an interface for that ViewModel. According to FxCop, the following example is good and life is fine.

public class CustomApp : Application 
{ 
    public override void OnStartup() 
    { 
        var view = new MainView(); 
        view.DataContext = new MainViewModel(); 
        view.Show(); 
    } 
}

internal class MainViewModel 
{ 
    public ISearchViewModel SearchViewModel { get; set;}

    public MainViewModel() 
    { 
        SearchViewModel = ServiceLocator.Get<ISearchViewModel>(); 
    } 
}

As smart developers, we decide that we don't really need the ServiceLocator to get our SearchViewModel. The concrete SearchViewModel doesn't have any complex construction logic and we probably will never swap it out with a different instance at runtime -- this is a little hit against dependency inversion but from a testing perspective, I can always introduce a Mock through property injection. Upon introducing this change we discover several new FxCop violations that weren't present before -- nearly a dozen warnings in unrelated classes about objects not being properly disposed. Out of all the warnings, only one warning is specific to the change we've made in this class but it doesn't seem obvious: CA2000: Dispose objects before losing scope.

public class MainViewModel 
{ 
    public ISearchViewModel SearchViewModel { get; set;}

    public MainViewModel() 
    { 
        // FxCop: CA2000 - Dispose object before losing scope
        SearchViewModel = new PrimarySearchViewModel();
    } 
}

This violation takes a few minutes to sort out because we're assigning the object to a property, and it isn't going out of scope. Upon further inpsection, it seems that the concrete class implements IDisposable, but the interface does not. FxCop is warning us that once the constructor goes out of scope, our concrete class will become polymorphically assigned to an interface that doesn't support IDispose and as such, we won't have an opportunity to call the Dispose method on the concrete class. Clever girl.

The easy fix is to add IDispose onto the interface and then implement the dispose pattern for the MainViewModel. All of this makes sense, and we've just fixed a memory leak that was there all along. But why did we have violations in all these other classes?

It turns out that PrimarySearchViewModel implemented IDispose improperly and it also constructed several other IDisposable types with the same problem in its constructor. These violations, like the PrimarySearchViewModel one, are legitimate. Now, if you're following along, you'll realize I haven't answered the question. If all these classes implemented IDispose improperly, why weren't these found earlier?

The answer is because we changed the usage. No one was calling the constructor previously -- the ServiceLocator used reflection to hunt down and construct our object for us, but by switching to use the constructor directly the analysis engine did it's part to highlight problems. It's hard to say whether the analysis engine should have caught this potential problem or if it's a feature that only inspects currently active code. At any rate, I hope this help you or your team -- before dismissing FxCop violations as fluke, look at how the usage of your objects has changed.