Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Useless enforcement of CG.SuppressFinalize #21734

Closed
ericbl opened this issue Nov 26, 2020 · 2 comments
Closed

Useless enforcement of CG.SuppressFinalize #21734

ericbl opened this issue Nov 26, 2020 · 2 comments

Comments

@ericbl
Copy link

ericbl commented Nov 26, 2020

The rules CA1816 and CA1063 are annoying me for a small detail for over 10 years: why is the CA1816 and CA1063 still forcing a call to the CG.SuppressFinalize method when the class do NOT have any finalizer?
As stated in the IDispose snippet in VS, by Stephen Cleary in his blog here:
https://blog.stephencleary.com/2009/08/second-rule-of-implementing-idisposable.html
a class implementing IDisposable with only managed objects should NOT have any finalizer, and do NOT need a call to CG.SuppressFinalize

Adding CG.SuppressFinalize will not harm since
https://docs.microsoft.com/en-us/dotnet/api/system.gc.suppressfinalize?redirectedfrom=MSDN&view=net-5.0#System_GC_SuppressFinalize_System_Object_

If obj does not have a finalizer or the GC has already signaled the finalizer thread to run the finalizer, the call to the SuppressFinalize method has no effect.

but this is just useless: every .NET developer dealing mostly with managed code is adding a line of code for nothing in his code.

IMHO, it would be much better to detect if the class has a finalizer, and if no finalizer, do not ask for SuppressFinalize !
Even better would be to detect for unmanaged ressources and only then ask for a finalizer and possibly SuppressFinalize as in the 3rd rule of implementing IDisposable:
https://blog.stephencleary.com/2009/08/third-rule-of-implementing-idisposable.html


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@Evangelink
Copy link
Member

I am wondering if there are cases where it brings something.

@mavasani if this ticket moves forward the analyzer should be updated.

@gewarren
Copy link
Contributor

gewarren commented Dec 2, 2020

This issue was moved to dotnet/roslyn-analyzers#4482

@gewarren gewarren closed this as completed Dec 2, 2020
@dotnet-bot dotnet-bot removed the ⌚ Not Triaged Not triaged label Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants