The blog of dlaa.me

Previous posts introduced the Delay.FxCop custom code analysis assembly and demonstrated the benefits of automated code analysis for easily identifying problem areas in an assembly. The Delay.FxCop project included two rules, DF1000: Check spelling of all string literals and DF1001: Resources should be referenced - today I'm introducing another! The new rule follows in the footsteps of DF1001 by identifying unused parts of an assembly that can be removed to save space and reduce complexity. But while DF1001 operated on resources, today's DF1002: Uncalled methods should be removed analyzes the methods and properties of an assembly to help find those stale bits of code that aren't being used any more.

Note: If this functionality seems familiar, it's because CA1811: Avoid uncalled private code is one of the standard FxCop rules. I've always been a big fan of CA1811, but frequently wished it could look beyond just private code to consider all code. Of course, limiting the scope of the "in-box" rule makes perfect sense from an FxCop point of view: you don't want the default rules to be noisy or else they'll get turned off and ignored. But the Delay.FxCop assembly isn't subject to the same restrictions, so I thought it would be neat to experiment with an implementation that analyzed all of an assembly's code.

Further note: One of the downsides of this increased scope is that DF1002 can't distinguish between methods that are part of a library's public API and those that are accidentally unused. As far as DF1002 is concerned, they're both examples of code that's not called from within the assembly. Therefore, running this rule on a library involves some extra overhead to suppress the warnings for public APIs. If it's just a little extra work, maybe it's still worthwhile - but if it's overwhelming, you can always disable DF1002 for library assemblies and restrict it to applications where it's more relevant.

 

Implementation-wise, DF1002: Uncalled methods should be removed isn't all that different from its predecessors - in fact, it extends and reuses the same assembly node enumeration helper introduced with DF1001. During analysis, every method of the assembly is visited and if it isn't "used" (more on this in a moment), a code analysis warning is output:

DF1002 : Performance : The method 'SilverlightApplication.MainPage.UnusedPublicMethod' does not appear to be used in code.

Of course, these warnings can be suppressed in the usual manner:

[assembly: SuppressMessage("Usage", "DF1001:ResourcesShouldBeReferenced",
           MessageId = "app.xaml", Scope = "resource", Target = "SilverlightApplication.g.resources",
           Justification = "Loaded by Silverlight for App.xaml.")]

 

It's interesting to consider what it means for a method or a property to be "used"... (Internally, properties are implemented as a pair of get/set methods.) Clearly, a direct call to a method means it's used - but that logic alone results in a lot of false positives! For example, a class implementing an interface must define all the relevant interface methods in order to compile successfully. Therefore, explicit and implicit interface method implementations (even if uncalled) do not result in a DF1002 warning. Similarly, a method override may not be directly called within an assembly, but can still be executed and should not trigger a warning. Other kinds of "unused" methods that do not result in a warning include: static constructors, assembly entry-points, and methods passed as parameters (ex: to a delegate for use by an event).

With all those special cases, you might think nothing would ever be misdiagnosed. :) But there's a particular scenario that leads to many DF1002 warnings in a perfectly correct application: reflection-based access to properties and methods. Granted, reflection is rare at the application level - but at the framework level, it forms the very foundation of data binding as implemented by WPF and Silverlight! Therefore, running DF1002 against a XAML application with data binding can result in warnings for the property getters on all model classes...

To avoid that problem, I've considered whether it would make sense to suppress DF1002 for classes that implement INotifyPropertyChanged (which most model classes do), but it seems like that would also mask a bunch of legitimate errors. The same reasoning applies to subclasses of DependencyObject or implementations of DependencyProperty (though the latter might turn out to be a decent heuristic with a bit more work). Another approach might be for the rule to also parse the XAML in an assembly and identify the various forms of data binding within. That seems promising, but goes way beyond the initial scope of DF1002! :)

Of course, there may be other common patterns which generate false positives - please let me know if you find one and I'll look at whether I can improve things for the next release.

 

[Click here to download the Delay.FxCop rule assembly, associated .ruleset files, samples, and the complete source code.]

For directions about running Delay.FxCop on a standalone assembly or integrating it into a project, please refer to the steps in my original post.

 

Unused code is an unnecessary tax on the development process. It's a distraction when reading, incurs additional costs during coding (ex: when refactoring), and it can mislead others about how an application really works. That's why there's DF1002: Uncalled methods should be removed - to help you easily identify unused methods. Try running it on your favorite .NET application; you might be surprised by what you find! :)