Intent for Roslyn-based implementations of FxCop rules?

Topics: General
Apr 8, 2014 at 12:57 PM
Is there a plan to fully replace FxCop/VS Code Analysis by Roslyn-based diagnostics? If not, what is the purpose of the rule porting item on the roadmap page (i.e.: sample only, enabling fixers, or something else)? It's rather difficult to provide feedback "to prioritize which of these should be implemented first" if we don't even know why exactly they are being implemented in the first place...

Also, if there is a plan to replace the existing tooling, would this be done in stages (e.g.: vNext ships with some subset of rules, and the older tools still need to be used to run the remaining rules), or would it only ship as a complete(-ish) replacement?
Apr 8, 2014 at 1:41 PM
I don't see Roslyn doing anything for FxCop as it is.

As it is, FxCop is an IL tool not a source tool.

But I can see Roslyn as the core of StyleCop.
Apr 8, 2014 at 2:23 PM
Paulo,

I would agree that a Roslyn-based StyleCop replacement would seem more relevant, but the roadmap page specifically mentions re-implementation of FxCop rules only. (Based on the appearance of "flow analysis APIs" on the roadmap, I rather suspect that a full FxCop/VS Code Analysis replacement is intended, including the data flow rules.)

While I would tend to agree with you that replacing IL-based FxCop rules with Roslyn diagnostics doesn't seem nearly as compelling, I can see some potential advantages (e.g.: enabling fixer creation and reducing the number of analysis engines to be maintained). Also, there are some FxCop rules that could be improved quite using additional information only available from the source code, so there's some potential concrete benefit in such a migration even for "old school" users.

This could definitely go in any of a number of directions, and it would be useful to know what is planned, particularly if the team wants community involvement in rule prioritization.

Nicole
Developer
Apr 8, 2014 at 6:02 PM
There are several reasons for porting the FXCop rules over to Roslyn. The first and most important is to see if it can be done. Is out analysis API complete enough to handle the scenarios that people care about? The second is to have some set of diagnostics that exist for when we change the API. If, when making and API change, the diagnostic code becomes more difficult to understand or write, then we rollback the change and rethink the design.
Developer
Apr 8, 2014 at 7:18 PM
A lot of what FxCop does today is simple code analysis which is achieved far more efficiently through source code analysis (think of async rules for example). There are also, of course, many FxCop rules which are best implemented through IL (or some other lowered form) analysis (think of rules that looks for boxing operations in generated code for example) and many rules that perform data flow analysis as well.

The Roslyn diagnostic analyzer APIs today are meant to make source analysis easier. The APIs we have today can do basic structural analysis and very little flow analysis. Our intent is to build upon these APIs to support richer forms of flow analysis as well. However, we are still focused on analyzing sources and not binaries - so in that sense this is not a FxCop replacement.

Since we are building an extensible framework here, the experiment is to see how much of FxCop we can write as source analysis and improve the VS code analysis experience. I wouldn't expect a wholesale replacement but instead an augmentation of the code analysis experience through better (or domain-specific) rules that the community (or domain experts) can write.
Apr 9, 2014 at 12:20 AM
@srivatsn,

I agree that every scenario where the compiler generates "source code" is better/easily analyzed in the source code.

However, some rules are easily analyzed on the IL and that analysis is independent of the source language, which requires only one analyzer and not one per language.

I know some people use FxCop (or VS Static Analysis) and don't know that it works (for now) only on the IL. But there are many that know it works on the IL.

If FxCop is going to be enhanced with source code analysis, the messaging needs to be clear and loud. And it would be nice to have a consistent API for rule developers. Otherwise; it will be another tool and give it another name.
Apr 9, 2014 at 3:45 PM
@jmarolf and @srivatsn,

Is the intent to actually ship these rules as part of the core Roslyn packages included in Visual Studio? If so, and you're not planning on replacing FxCop, duplication of rules across the tools is likely to cause considerable end-user confusion, particularly for cases where false positives or negatives do not align exactly.

If the main intent here is mainly to validate the tooling, might it not make more sense to reproduce StyleCop rules rather than FxCop rules? The number of FxCop rules that actually need or could benefit from source code information is rather small, so these rules aren't necessarily a great breadth-wise test of the Roslyn APIs. Unless you're actually looking at replacing FxCop, it would seem much more interesting to verify whether source code-relevant rules can be authored effectively on the toolset.

FWIW, the single most compelling FxCop rule to reproduce (or actually replace) in Roslyn would probably be CA1502 (AvoidExcessiveComplexity), which really ought to target source code instead of potentially optimized IL. Otherwise, porting Code Clone Detection would probably provide a much more interesting test of the Roslyn APIs than most of the other FxCop rules, as well as being very useful since the current incarnation has an unfortunate dependency on the VS object model.

Nicole
Developer
Apr 9, 2014 at 7:18 PM
The intent is to ship them in some form. We aren't far enough along in the effort yet to know whether that's in the box in Visual Studio.

@Nicole, Your point about avoiding duplication is well taken. If these Roslyn-based rules are in the box then the corresponding implementations based on binary analysis will have to be removed so as to avoid confusion.

@Paulo, There is currently no public supported FxCop SDK - there a few blogs that show how to do it but it's capabilities are very limited and unsupported. I think going forward, we would want Roslyn to be the API for code analysis rule authors. Initially that would mean that you are only able to do simple source analysis. We would then want to enable flow analysis using this API and then possibly in the future see if we can enable binary analysis using Roslyn as well (or atleast something that fits into this model).

I realize that the answer might still sound vague and so let me describe how we are approaching code analysis in visual studio:
  • First step is to develop a set of extension APIs that let you use well known Roslyn data structures to analyze code in an efficient way that plugs all the way in to the compiler and through MSBuild.
    In parallel, over a period of time, we would want to add more data structures that Roslyn can expose to perform, for eg., flow analysis.
  • Next is to understand how this extension mechanism fits into visual studio and how APIs can come with their own analyzers.
  • Once we have an extensible system, then rationalize the set of rules that ship in Visual Studio today. It is when we get to this step that I think we'll have an answer to some of the questions above. We would like to rewrite as many of the FxCop rules as makes sense so that we can reduce our servicing cost for FxCop - but that's only possible if we can replicate all of the VS experiences that are available today with this new system.
@Nicole, to your original question of what feedback do we want exactly - when we get to step to rationalize the existing VS code analysis rules, we'll need to know which ones to ship, which ones don't make sense anymore (there's of bunch of these as most of the fxcop rules were written in the .NET 1.0 era and some are even incorrect now or just not applicable) and which ones actually add value. Hope that clarifies this a little bit.
Apr 9, 2014 at 7:49 PM
@srivatsn,

Thanks, that's much clearer. How would you like us to provide feedback on things like rule relevance and priority: through the issues list or discussion topics?

Thanks,
Nicole
Developer
Apr 10, 2014 at 8:52 PM
I think we should setup a wiki of some sort but until we get that in place, this thread is as good a place as any. I've noted down CA1502 in our backlog of things to try next.
Apr 11, 2014 at 7:54 PM
Thanks! I'll try to take a look through the full list sometime soon to see what looks "interesting". WRT the rules that are no longer relevant, which versions of the .NET Framework are you planning on supporting?
Apr 14, 2014 at 6:55 PM
@srivatsn,

I just clued into something regarding the following:
Your point about avoiding duplication is well taken. If these Roslyn-based rules are in the box then the corresponding implementations based on binary analysis will have to be removed so as to avoid confusion.
Given that Roslyn a subset of .NET languages (albeit a very important subset by usage <g>), removing FxCop rules once they've been implemented in Roslyn really wouldn't seem to make all that much sense. Sure, folks using other languages or compilers could switch to (or supplement with) something like Gendarme to restore a more complete rule set against IL, but is that really your intent?

Might it not make more sense, at least in the short-to-middle term, to build in a mechanism (preferably overridable <g>) to disable a rule in a VS-triggered FxCop run if it has already been executed in Roslyn? This could be done fairly easily by adding a bit of MSBuild magic to construct /ruleid:-CA____ command line arguments for fxcopcmd.exe.

Thanks,
Nicole
Developer
Apr 16, 2014 at 5:40 PM
No, we wouldn't want to delete the rule implementations. We should just make it so that they are not available to C#\VB projects. One way to do that will be to teach the ruleset editor to only show rules that are in scope for a given project. We would need to do this anyway for showing these 3rd party analyzers which would be different for different projects.
Apr 16, 2014 at 7:00 PM
How would that fit with using the same rule IDs for Roslyn-based and FxCop rules? If they're both being configured via the same ruleset file, wouldn't the following apply equally to both?
<Rule Id="CA1705" Action="Error" />
Or were you thinking of using a separate Rules node with a new AnalyzerId for the Roslyn rules? That would make it impossible to configure static analysis for a mixed language (VB/C# + something else) solution with a single ruleset file without running the duplicate rules for the VB and C# projects.

Wouldn't it be easier to add a bit of MSBuild logic to trigger disabling of the Roslyn-equivalent rules when running FxCop so that no special voodoo is required for separating the rules in the rulesets, either by you guys or end-users?