Enable and audit dispose rule suppressions in Roslyn.sln (CA2000 and CA2213)

[mavasani]

#25179 attempts to move us to newer analyzer packages that includes the experimental implementation of CA2000 and CA2213. There are quite a few differing opinions on what should be suppressed vs fixed, and we also have differing opinions on how dispose ownership transfer should be designed for the rule (dotnet/roslyn-analyzers#1617). Hence, we have decided to add suppressions for majority of dispose violations flagged in #25179. This issue tracks auditing these suppressions and fixing OR resolving by design.

[sharwell]

Here are my expectations for these rules:

1. A set of rules (guidance) exist for the use of IDisposable, which the analyzers aim to enforce
2. The rules of (1) represent a sufficient condition for safe use of unmanaged resources through IDisposable
3. The analyzers implement a documented set of rules which form an approximation of the rules of (1)
   • The approximation should be a conservative subset of (1), but this may not be possible
4. Outside of extenuating circumstances, code using IDisposable can be written according to the rules of (3) without suppressing analyzer warnings
5. Code adhering to (4) does not report warnings from the analyzers

I consider steps 1-4 hard requirements (prerequisites) for enabling the analyzers in Roslyn.sln, and item 5 may only be relaxed for cases of bugs affecting a minority of code adhering to (4) where the analyzers failed to correctly implement the documented rules of (3). This approach is quite flexible in terms of the final approach, and represents a reasonable minimum acceptance bar that customers will have.

📝 I've requested this item go to the design meeting. This comment aims to document my expectations prior to the meeting so everyone is aware of them.

[mavasani]

I consider steps 1-4 hard requirements (prerequisites) for enabling the analyzers in Roslyn.sln

Note that these are not new rules, which are being designed from scratch. There are already existing implementations of these rules in the binary FxCop world and there are extremely large number of code bases that rely on the current design/heuristics, especially since they have done the work to audit/suppress the violations. The analyzer implementation matches prior FxCop rule design/heuristics. This was already discussed at prior design meeting(s) and we decided:

1. Match binary FxCop implementation as much as possible - this is a strict compat requirement.
2. If we can adopt any further heuristics to reduce the false positives, we should do so. However, we can do so only if we know for sure that we will not introduce any more false positives due to these new hueristic(s) and/or mask true violations that were flagged by prior implementation. None of the alternate heuristics suggested in Design for "Dispose ownership transfer" in dispose rules roslyn-analyzers#1617 by @sharwell satisfy this requirement, and hence have not been implemented.

Hence, we have already satisfied the requirements set by design time for this rule implementation.

[sharwell]

It is possible, and acceptable, for the design of CA2000 and CA2213 to deviate from the design of a set of usable IDisposable rules (due the compatibility requirement limitation of the former) to the point that the rules never meet the requirements for adoption in Roslyn.sln. In such a situation, I would expect the original rules to be locked to the legacy rules, and then a new rule (or set of rules) implemented for use in code that didn't originally use FXCop. Once the new rule(s) existed (perhaps in beta), it could be enabled for Roslyn.sln. In the interim, Roslyn.sln would not use analyzer rules for IDisposable.

[mavasani]

It is possible, and acceptable, for the design of CA2000 and CA2213 to deviate from the design of a set of usable IDisposable rules (due the compatibility requirement limitation of the former) to the point that the rules never meet the requirements for adoption in Roslyn.sln.

I would be strongly opposed to having multiple heuristic based implementations of a specific rule, just to satisfy preferred heursitics for different code bases. This is not how we write analyzers, and is not a valuable use of anyone's time and resources. Instead we should attempt to improve the rule implementation to reduce dependence on heuristics, as suggested by possible approaches in dotnet/roslyn-analyzers#1617 (comment).

[CyrusNajmabadi]

Closing out due to lack of feedback.