Roslyn-analyzers: Port FxCop rule CA2213: DisposableFieldsShouldBeDisposed

[lgolding]

Title: Disposable fields should be disposed

Description:

A type that implements System.IDisposable declares fields that are of types that also implement IDisposable. The Dispose method of the field is not called by the Dispose method of the declaring type.

Dependency: Dataflow

Notes:

[srivatsn]

We decided to cut this rule because it is very hard to get it right without deeper analysis, tracking variables and possibly having annotations. The current implementation is very noisy and has a lot of false positives and the value of the rule is not worth all the noise it generates.

[StilgarISCA]

:-1: on this decision. Despite the fact that this causes a lot of false positives, I think it's worth keeping the rule for the correctly identified issues. End users always have the option of disabling rules for which they find no value.

[lgolding]

@StilgarISCA Thanks for the feedback, Glenn. We have a document Proposed FxCop rule changes in Roslyn where we are tracking community feedback on the decisions we've made on which rules to port and how to implement them. I've added your feedback to this document.

At some time in the near future (I don't know an exact date, but it's not like "years from now" -- this is something we definitely want to do) we'll review the feedback and decide whether to revisit some of our initial decisions. Please let us know any other rules you think we should reconsider.

Thanks,
Larry

@srivatsn @michaelcfanning @nguerrera FYI

[mattwhitfield]

I second @StilgarISCA - This rule does have value, and personally I have never found the false positive rate to be that bad. But as a team lead trying to coerce people into focusing on quality, I don't want to have to say 'oh yes you can ignore that rule' because it gets people into bad habits.

[arthurward]

I agree with @StilgarISCA. With VS2015's ability to ignore existing issues, false positives are not such a problem any more. There is still a benefit to flagging new instances of undisposed disposables.

[jeroen-mostert]

Another vote for keeping it. Forgetting to dispose disposables is a common error in our code bases, and even with false positives, this rule does a good job at weeding them out. I'll gladly take the false positives and disable them if it means detecting the common case of someone adding a disposable field but forgetting to dispose it (very easy to do if you don't realize it actually is a disposable).

I came here from getting a false positive on field?.Dispose() (issue #291). While full flow analysis is overkill, the pattern this generates (var expr = field; if (expr != null) expr.Dispose()) is easy enough to recognize as a valid dispose pattern.

You could introduce a lesser rule that has little to no chance for false positives, at the expense of not guaranteeing that disposable fields are indeed disposed: verify that all disposable fields of an object are referenced at least once in the Dispose or Dispose(bool) method (whichever exists), without checking if Dispose is actually called on them. This leaves a lot of room for mistakes, but it catches the most common mistake of forgetting the field entirely.

[JimF42]

I was hoping that this would have been fixed in VS 2017, but alas. Maybe this could still be reconsidered???

[lflender]

Still waiting for this rule to be fixed! So many false positives...

[kamenlitchev]

I'd really also like to see this rule not being dropped, but at least having the most common (for me) use fixed when safe-navigation operator is used in Dispose() override:

object?.Dispose()

[cornillemichiel]

Is there any update on this being picked up again?

[mavasani]

We will evaluate a prototype analyzer to implement this rule without DFA support in 15.6 (2.7 release of the analyzer package).

[JimF42]

@mavasani-Sounds great, thanks. But for us lesser informed... What does "without DFA support" mean?

[mavasani]

DFA - Data flow analysis. Idea is to first implement a simpler analyzer as explained in detail in https://github.com/dotnet/roslyn-analyzers/issues/695#issuecomment-232643339. This analyzer will not analyze control flow and might generate lot of noise, but should flag all real bugs. Users have more work in separating noise from real issues and suppressing the former. In future, if we are able to perform flow analysis in analyzers, we will improve the analyzer to reduce noise.

[JimF42]

Thanks for the quick reply. Ah, I think I understand. You see a disposable field and see if somewhere in the method/scope a Dispose() variant is called without analyzing if the _code flow_ guarantees it gets there.

I'm not sure if I understand how that will generate more noise, if what you mean by that are false positive messages. Seems like it would be the opposite, no message when the code flow doesn't always reach it.

Hey, better than nothing. Thanks.

[mavasani]

@JimF42 Yes, sorry I was thinking in terms of https://github.com/dotnet/roslyn-analyzers/issues/628 (which we also recently re-opened to have a non-DFA based analyzer). For that scenario, we will generate noise.

[JimF42]

@mavasani-OK, last stupid question for a while. When reading the 695 thread, I noticed the statement

...we have decided not to port CA2100 from FxCop to Roslyn analyzers.

I, perhaps naïvely, didn't realize there was more than one analyzer. Is this issue specifically about the Roslyn suggestions that are in the left margin and not changes to FxCop?

I came to this thread was FxCop was having issues with object?.Dispose(); and never assumed it was for something else...

[mavasani]

@JimF42 This repo and all issues are regarding Roslyn based source analyzers, a majority of them being ports of corresponding FXCop rules. Please see https://github.com/dotnet/roslyn-analyzers/blob/master/docs/FxCopPort/Porting%20FxCop%20Rules%20to%20Roslyn.md for further details.

[mavasani]

An initial DFA based implementation for this rule is out for review: https://github.com/dotnet/roslyn-analyzers/pull/1595.