Instagram
Static Analysis

MSOCAF rules

Jesse Houwing

• 3 min read

As you might have read previously, I've been trying to incorporate MSOCAF directly in both Visual Studio 2010 and Team Build. The simplest solution would be to code up an msbuild task and put those in a target and reference that target in all project files. I know this, but it has a few shortcomings... (mostly that it doesn't play nice with the Supress option in Visual Studio).

So I've been decompiling re-targetting, recompiling the rules a number of times now, and I've become quite good not only in fixing the issues that come from converting FxCop < 10.0 rules to the latest version, but it's becoming easier to read the rules and actually understand what's going on.

Last week a developer came to me and gave me the bad news that MSOCAF had upgraded and that new rules had been added which aren't yet part of our converted ruleset. So I upgraded my local version of MSOCAF and did my usual 7 step program.

Upon running the new rules on our Sharepoint solution, FxCop spiked, took ages and then threw a StackOverFlowException somewhere in System.Core. Oops :)! The most likely candidate for this error was of course the newly added rule and disabling that rule fixed the build.

Going over the sources, we found the problem pretty quickly. But before I tell you, let's go over what this rule actually does... The rule in question is SharePointFeatureReceiverCheck. It checks whether you, as a developer are adding the proper calls to SPDiagnosticsServiceBase.WriteTrace().

And because having to put all the code of getting a category, a message etc would be too cumbersome, it allows you to call a method which in turn should call SPDiagnosticsServiceBase.WriteTrace().

This check is done recursively. And is not bounded by any other checks than the fact that a call is made to some API that has been signed by the Microsoft. Now, there are two errors in this implementation.

  1. Recursion is implemented on the stack.
  2. The recursive function doesn't check for endless loops.

Recursion on the stack

You might be wondering what recursion on the stack means. Phil Haack explains it pretty well in this post. basically the MSOCAF team was using the first pattern to implement recursion. Due to the unfiltered endless loop, this resulted in the crashing of FxCop.

I could have rewritten the code in this check to use the second method, but that wouldn't have solved my problem. It would have meant that FxCop would run forever trying to find a call to SPDiagnosticsServiceBase.WriteTrace().

So I decided to add two checks to the existing code.

Recusive function that doesn't check for endless loops

If you write a recursive function it is important to check for endless loops. The simplest way is to pick some arbitrary number and keep a counter. When the depth of the recursion exceeds that arbitrary number, stop the recursion; and in this case return false.

A better solution is to keep track of call sites you've already visited and prevent going into a loop at all.

I chose to implement both. I set my arbitrary number to 32, if, after 32 stack frames, you still haven't called  SPDiagnosticsServiceBase.WriteTrace(), then chances of that happening are very slim. And you should refactor your code, a simple log statement should not take 32 stack frames, that's way to expensive!

I also added a list of methods which had previously been visited. If the method I'm trying to resolve is already in that list, I know I can return false.

So, after recompiling the rules, deploying them to TFS I now have the new rules running happily and with a lot less CPU usage than before.

Things I might do with this rule

These rules are very expensive, as in, they result in long build times. They don't work well with Dependency Injection (but what part of SharePoint does) and they're not extensible. If we have a method or class which is used for logging (and which fills the requirements for this rule), then please allow me to say so. It saves us a lot of time during the build waiting for Code Analysis to finish its pass of our SharePoint binaries. We tried building our own SPDiagnosticsServicebase implementation which can be pre-configured with a category, but as the WriteTrace method isn't overrideable, this turned out not to be possible... So I've started refactoring this rule to take the following stuff in into account:

  1. I'm loading a list of known methods that call  SPDiagnosticsServiceBase.WriteTrace(). And return true directly after I've found a match
  2. I'm changing the implementation to one that doesn't do recursion on the stack
  3. I'm adding support for static methods (as the current rule seems to be happily ignoring custom static methods that call  SPDiagnosticsServiceBase.WriteTrace(). directly.
  4. And while I'm at it, I'm changing the way the problems are reported, so that the rule plays nice with Visual Studio and will allow me to suppress the rule directly in the source file, instead of adding them to the globalsuppressions.cs

I´ve also been looking to get into contact with the MSOCAF team to share my findings, but as with the download and the documentation, they´re hard to find. Anyone?!