Adding FxCop Analysis

[Travis Illig]

We previously only had two Extras assemblies running FxCop - EnterpriseLibraryConfigurator and Multitenant. I've added it to the CommonServiceLocator assembly.

I realize the Portable libraries are going to have a different ruleset than the Full framework libraries so I've got a "Full.ruleset" in the root of the project - in anticipation of a "Portable.ruleset" to go along with it. I've also added a CodeAnalysisDictionary.xml to the root of the project so all the assemblies can use the same set of custom words and we can ensure consistency in across projects.

Right now "Full.ruleset" is a copy of "AllRules.ruleset" minus the rule requiring an assembly version be present - in debug, the build assigns 0.0.0.0 as the assembly version and that fails FxCop.

I'm not glued to that ruleset, but since I haven't applied it to anything else yet I don't know what else should be turned off. I figured I'd start off with the bar set really high and see how it needs to be lowered. I'll attach it to a couple more assemblies and see how much red shows up in the build. If it's small stuff to fix, I'll just fix it; if not, we can probably just exclude it or turn the rule off. I'm not a fan of following rules just to follow rules, but I do value the consistency that automated analysis like this can provide. If anyone feels strongly about a particular rule being on or off, chime in.

[Alex Meyer-Gleaves]

Sounds like a good place to start. I'm sure we will end up refining the list. To help with code formatting and style I have added a Solution Team-Shared settings file for ReSharper.

--
You received this message because you are subscribed to the Google Groups "Autofac" group.
To view this discussion on the web visit https://groups.google.com/d/msg/autofac/-/ROoueyFWYX8J.
To post to this group, send email to aut...@googlegroups.com.
To unsubscribe from this group, send email to autofac+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/autofac?hl=en.

[Travis Illig]

Oh, you with your fancy ReSharper. :) I'm a CodeRush guy.

Any interest in StyleCop? Refactoring tool agnostic, easy to use/integrate, causes actual build warnings if something's off...? I've used it before and if you turn some of the more pedantic stuff off it's pretty good (sort of like FxCop).

[Travis Illig]

So far the FxCop addition has been very smooth. I have all of the Extras covered as well as Autofac.Configuration and Autofac.Integration.Mef. Issues I've encountered have been minor and legitimate - things like missing argument null checks.

I've turned a couple more rules off, things involving nesting generics in method signatures, since that really interferes with creating a nice, fluent interface. I've also added a couple of words ("Mef," "Moq," etc.) to the dictionary. But really, we're already pretty well compliant from what I can tell.

I've pushed the changes so far. I'll see what happens when I start getting into some of the larger libraries. I'm probably going to save the core Autofac for last because it's the biggest and, honestly, I'm not sure what to do about analysis on portable libraries yet.

[Travis Illig]

I'm going to disable rule "CA1026: Default parameters should not be used." Basically it says that optional parameters don't necessarily translate across languages. They recommend using method overloads rather than default parameter values. We have quite a few of these in some of the Integration libraries.

Does anyone feel strongly about keeping this in place? I think for our target languages (C#, VB.NET) it properly interoperates, but I don't know how default parameters in a C#-based library translate to, say, F# or other languages. From a safety perspective, the "most compatible option" is to keep the rule in place and fix the issues. Phil Haack also mentions some potential interface versioning issues with default parameters. On the other hand, all the overloads makes for a more cluttered API.

Thoughts?

[Travis Illig]

Also, since FxCop's spelling dictionary defaults to US English and the .NET BCL is all US English (spelling-wise), I'm going to update "behaviour" to "behavior" in a couple of spots. It will end up being a breaking change, but it will make us consistent across libraries (where other locations are "behavior"). Not saying "behaviour" is wrong, just inconsistent with the dictionary and the BCL. </cultural-sensitivity>

[Travis Illig]

OK, everything but core Autofac has FxCop on it and all issues are resolved. Really minor stuff:

• Missing checks for null arguments. Particularly in static extension methods, where it's actually possible for an extension to be called on a null object. (I know this FxCop rule is one of those, "Really?!" pedantic-style rules, but I'd rather have a ton of seemingly pointless and redundant argument null checks than have to try and debug a NullReferenceException based on a potentially cryptic stack trace.)
• IDisposable implementation fixes. With IDisposable, you can only really dispose of managed stuff if it's not happening during garbage collection. I added the requisite Dispose(bool) overloads and safety to ensure multiple calls to dispose won't cause issues. This was only in like four spots.
• Sealed attributes. I was torn on this one. FxCop wants you to seal attributes. I don't feel strongly one way or the other so I left it. We can always unseal 'em and turn that rule off if people like it a different way.
• [ComVisible(false)] on full framework assemblies. It doesn't hurt anything and helps a little in environments where folks are using COM, so... eh, why not.
• "Use properties where appropriate." FxCop complains if you have no-parameter methods that start with Get like "GetLifetimeScope()". I excluded these warnings but I left the rule running. The sentiment is right - if it should be a property, make it a property - but in all the places it flagged, the operation being performed wasn't something that belonged in a property format so exclusion was the right way to go. Keeping the rule around will force folks to think about the design and make a conscious decision. It may be that we want to use different verbs like "Create" instead of "Get" on future method names to indicate more than simple retrieval is happening.

That was pretty much it. I was really impressed with how easy it was to get all this in place. It shows the high quality and standards that were already there.

I left CA1026 ("don't use default parameters") turned off. I'd like to hear from folks on the potential issues before turning it back on. If we turn it on post-release, the fixes will be breaking changes so it'd be good to make the decision early. The more I think about it, with the interoperability we're going for in Portable Class Libraries and all that, it might be better to turn it on and make the fixes. Just to avoid some of the "works here but not over there" gotchas we could hit.

I'll see about getting FxCop on core Autofac today or tomorrow. I'm on holiday next week so will be going dark for a bit. I'd like to get that all done and mark it set before taking off.

[Travis Illig]

I've got a Portable ruleset for FxCop which turns off a few more things. I'm not really sure if it's "Portable" so much as "stuff the Autofac core assembly abides by but the other assemblies can be a little more strict."

There are a few more rules turned off for the Portable ruleset, Stuff like "avoid excessive parameters on generic types," "avoid namespaces with few types," and "implement standard constructors for exceptions." Some of those are because of the fluent interface and structure of the assembly, others are because of the PCL aspect of things.

The rest of the stuff I fixed was pretty much the same as before. Missing null checks, some typos in strings, adding "CultureInfo.CurrentCulture" to string.format calls. I think there were only three things I would consider potentially "breaking":

• Renamed the "PropertyWiringFlags" enumeration to "PropertyWiringOptions" because they want people to avoid "flags" as a suffix.
• Renamed the "Default" option in PropertyWiringOptions to "None" - the 0 option is generally named "None" even if it is default.
• Removed the "Default" member of ContainerBuildOptions enumeration and just used "None" in there. The idea is that you're only supposed to have one "zero value" in an enum, which makes sense.

I'm not sure if that means we should introduce some statics somewhere with default settings in case they need to be changed at some point.

It's a pretty big commit, but just tiny changes in each touched file. I don't think it should cause any major conflicts.

That means we have FxCop on the entire product suite, which makes me smile. I feel like FxCop is sort of like another suite of tests, building confidence.