FxCop warnings in R#

I just thought I would run FxCop against R# to see if things had improved since the last time I did it. I'm afraid not.
FxCop reports a lot of warnings (thousands before giving up)!
I filtered it down to find really problematic stuff.

Quite a lot of pinvoke problems. This is a problem for me because I'm running on 64 bit.
And a huge number of 'Implement IDisposable correctly' warning.
Lots of 'com visible base types should be comvisible' and 'avoid overloads in comvisible interfaces'.
Countless other stuff.

6 comments
Comment actions Permalink

I wrote that post in a hurry last night.
To reiterate, I used FxCop 1.35 against R# build 805 binaries.
FxCop doesn't like the fact that some assemblies are linked agains .NET 1.0.
FxCop has a large number of warnings that I would personally want fixed such as:

Use managed equivalent of win32 api
Pointers should not be visible
Pinvokes should not be visible
Pinvoke entry point should exist
Pinvoke declarations should be portable
Mark boolean pinvoke arguments with MarshalAs
Disposable fields should be disposed
Avoid overloads in ComVisible interfaces
Implement ISerializable correctly
Do not raise reserved exception types
Do not dispose objects multiple times
Dispose methods should call base class dispose
Disposable types should declare finalizer
Types that own disposable fields should be disposable
Disposable types should be disposed
Implement IDisposable correctly
do not concatenate strings inside loops
disposable types should declare finalizer
Plus HUGE numbers of other warnings

I'm not trying to be rude, but if I was managing this project I would be seriously worried about code quality.
How can you trace difficult to find bugs when there are so many problems?
The problem with R# using excessive amounts of memory seems highly likely to be connected with the many IDispose warnings (hundreds if not thousands).
Incorrect Pinvoke is just going to cause random and difficult to find bugs especially on 64 bit systems.

There are similar problems with 3.1.

0
Comment actions Permalink

Hello Philip,

Thanks for pointing this out. Last time I used FxCop against ReSharper it
issued so many false positives, that I gave up and never run it again. Probably,
I should try again.

Sincerely,
Ilya Ryzhenkov

JetBrains, Inc
http://www.jetbrains.com
"Develop with pleasure!"


IR> I wrote that post in a hurry last night.
IR> To reiterate, I used FxCop 1.35 against R# build 805 binaries.
IR> FxCop doesn't like the fact that some assemblies are linked agains
IR> .NET 1.0.
IR> FxCop has a large number of warnings that I would personally want
IR> fixed such as:
IR> Use managed equivalent of win32 api
IR> Pointers should not be visible
IR> Pinvokes should not be visible
IR> Pinvoke entry point should exist
IR> Pinvoke declarations should be portable
IR> Mark boolean pinvoke arguments with MarshalAs
IR> Disposable fields should be disposed
IR> Avoid overloads in ComVisible interfaces
IR> Implement ISerializable correctly
IR> Do not raise reserved exception types
IR> Do not dispose objects multiple times
IR> Dispose methods should call base class dispose
IR> Disposable types should declare finalizer
IR> Types that own disposable fields should be disposable
IR> Disposable types should be disposed
IR> Implement IDisposable correctly
IR> do not concatenate strings inside loops
IR> disposable types should declare finalizer
IR> Plus HUGE numbers of other warnings
IR> I'm not trying to be rude, but if I was managing this project I
IR> would be seriously worried about code quality.
IR>
IR> How can you trace difficult to find bugs when there are so many
IR> problems?
IR>
IR> The problem with R# using excessive amounts of memory seems highly
IR> likely to be connected with the many IDispose warnings (hundreds
IR> if not thousands).
IR>
IR> Incorrect Pinvoke is just going to cause random and difficult to
IR> find bugs especially on 64 bit systems.
IR>
IR> There are similar problems with 3.1.
IR>


0
Comment actions Permalink

Ilya,

I know that FxCop gives a lot of warning that may have no relevance to you, such as naming and globablization, but you can selectively turn these off.
If you use FxCop all the time and deal with warnings as they occur you then see warnings that indicate real problems. This is what I do since I have team edition. It's just always on.
By the way I had to analyze your assemblies in batches of 10 otherwise FxCop ran out of memory.
I hope you deal with the Pinvoke issues asap.
I would like you to deal with the IDispose issues, but there are so many...

0
Comment actions Permalink

Hello,

I just thought I would run FxCop against R#


Inspired by the heated thread, I bothered to run fxcop against our code base.

There were lots of interesting messages, like:

Methods where the type parameter cannot be inferred from the parameters
and therefore has to be defined in the method call are too difficult to understand.

Or (this one being less difficult to understand):

A namespace should generally have more than five types.

And the editor's choice: imagine the world...

Consider a design where 'System.Func`3' has no more than 2 type
parameters.

Surely enough, there were useful issues too ... about a dozen out of ~50,000.
Could have missed something, of course.

to see if things had
improved since the last time I did it. I'm afraid not.


As such, it's hard to consider a useful metric for measuring the improvement.
Just because of the noise level.

Quite a lot of pinvoke problems.


Those I considered with most attention. Well, there's one potential issue,
that about copy-memory. Haven't ever called it yet, and it looks more like
a macro than a function, so it probably has to be removed. But for that method,
I could see no errors. Have I overlooked something? Maybe you were running
against an extended set of rules, considering the memory exhaustion and all?
I were able run thru the DLLs in just two steps, Platform and the rest.

This is a problem for me because I'm running on 64 bit.


Wishful thinking, that. You're running 32bit, just like any other Visual
Studio installation.

Portability issues or not, but Microsoft failed to ship a 64-bit version.
Our present installer would have refused to treat such a version, in the
first place.

And a huge number of 'Implement IDisposable correctly' warning.


"Correctly" means adding a boolean method and a finalizer, which is rarely
needed.

Generally, cheking through these messages means opening each disposable class
and reviewing its design, in which fxcop is of little help, since R# can
locate IDisposable implementors on its own. We're doing it from time to time
anyway, with the help of a memory profiler and special watchdog exceptions.
No fxcop can help in catching ASPX support memory leaks (like the one recently
patched), as it goes far beyond just methods and fields. If only it were
that easy...

Lots of 'com visible base types should be comvisible' and 'avoid
overloads in comvisible interfaces'.


Irrelevant. Just because public classes are not marked as com-visible-false
does not make them a problem.

Countless other stuff.


If only they were all meaningful...

We're bringing fxcop-like checks into the R# onboard analysis, more and more
with each new version. As for fxcop itself, we're not running it much. Even
if we were, all of the suppressed-in-project warnings would show up in external
fxcop runs.

As you probably have deep fxcop experience, we will be eager to hear about
particular issues that look worth working on. Just general "there's something
wrong in pinvoke" resolves into nothing really sensitive. Same for other
items in the list.

Which means that code quality and fxcop fitness are not the same thing, after
all.


Serge Baltic
JetBrains, Inc — http://www.jetbrains.com
“Develop with pleasure!”


0
Comment actions Permalink

Serge,

sorry I didn't mean it to be 'heated'.

The pinvoke warnings I get that I find 'worrying' are:

MarkBooleanPInvokeArgumentsWithMarshalAs
PInvokeDeclarationsShouldBePortable

And as you say 'implement IDisposable correctly' can be easily done - so why isn't it? If these classes don't require an overridable implementation then mark them as sealed - job done. If they do need an overridable implementation then best to do it. I'm from a C++ background so I worry about releasing memory correctly.

However warnings like:

do not dispose objects multiple times
types that own disposable objects should be disposable
disposable fields should be disposed
dispose method should call base class dispose

must surely be of some concern, especially with the problems many people have with R# memory usage.

I generally use all the help I can get when coding be it FxCop or R#. I use both all the time. I know that getting rid of all FxCop warnings doesn't mean your code's any good, and the same is true for R# warnings. But it doesn't hurt and when you've got rid of all the fluff there's usually something worth knowing about.

Maybe your code is so complex that it's completely defeating FxCop, and anyway R# would spot any problems wouldn' it?

Here's one that FxCop found:

'JetBrains.ReSharper.Daemon.SolutionAnalysis.UI.Indicator.ProgressStatusControl'
contains field 'myGottenDirty' that is of IDisposable
type: JetBrains.DataFlow.SimpleSignal. Change JetBrains.ReSharper.Daemon.SolutionAnalysis.UI.Indicator.ProgressStatusControl's
Dispose method to call Dispose or Close on this field."

Is that valid or not?

As I'm typing this devenv.exe is up to 720Mb. I know that it will just get worse until I have to restart it.

0
Comment actions Permalink

On the one hand, I agree with almost everything both of you say. Nevertheless, the number of FxCop failures definitely leaves an impression. It will take years to clean up any existing product, but nevertheless I recommend the effort be spent. Always start with the Security category (and leave Naming for last, and Globalization for second-last if your product does not target multiple cultures). A mcrosoft blog has indicated the settings they usually use, which is a good short-term goal ( http://blogs.msdn.com/fxcop/archive/2007/08/09/what-rules-do-microsoft-have-turned-on-internally.aspx ). A reasonaly schedule is to pick up one new rule every week for existing projects, but start with all rules enabled for new projects. But here again,TFS (if you are using it for your source code manaement) will give you headaches as it supports rules at the solution level rather than the project level. If you insert the SuppressMessage attributes in the source code itself, you get a much better idea of how things stand (some SuppressMessage attributes always have to go in the separate GlobalSuppressions file, but most can be inserted directly in the source code file). Also any generated code tends to have orders of magnitide more rule violations (I recommend keeping generated code in a separate project, and in this case only put SuppressMessage in the GlobalWatnings file -- I like to leave generated code "as-is" as much as possible).

Unfortunateley IDisposable is one of the worst designed mechanisms ever, and seems abandoned. For example, the failure to include "IsDisposed" makes it nearly useless "as-is", and any method that allows a reference to a class that implements IDisposable argument should, by default, assert that said argument has not already been disposed (with an attribute available to allow already disposed references in those rare circumstances where appropriate). Same with all methods/properties in an IDisposable class -- their should be an automatic self-check that "this" has not been disposed. This is right up there with lack of "NotNull" support in C# itself as serious flaws that have huge (and negative) impacts.

Long-term, I think RS should provide the ability to "fix" FxCop violations, and for me this is a much higher priority. As these capabilities are developed, we have approached "ReSharper -- heal thyself". It would also be nice to see RS provide a useful Disposable approach, just as RS has added CanBeNull/NotNull features to address that C# shortcoming.

0

Please sign in to leave a comment.