Fix S3881 FP: Take care of reference with current instance keyword "this."
Closed this issue · 2 comments
NETSphereSoft commented
Description
Correct implementation of Dispose
pattern with reference this.
will not detected.
Repro steps
- Write a class with
IDisposable
implementation and usethis.
together with the method calls.
public class TestDisposable :
IDisposable
{
~TestDisposable()
{
this.Dispose(false)
}
public Dispose()
{
this.Dispose(true);
GC.SuppressFinalize(this);
}
private Dispose(Boolean disposing)
{
// Dispose activities
}
}
Expected behavior
Scanner should take care about referencing with this.
.
Actual behavior
Dispose pattern reported false positive.
Known workarounds
- Use of
pragma
todisable
the reporting - Change the code without reference of
this.
Related information
- C#/VB.NET Plugins version ?
- Visual Studio version 2019
- MSBuild / dotnet version 17.9.8.16306
- SonarScanner for .NET version 6.2.0.85879
- Operating System Windows SDK Docker Image 10.0.17763
CristianAmbrosini commented
Hi @NETSphereSoft!
Just a clarification regarding the reproducer you provided, both versions are raising the issue on my side:
public class TestDisposablee : IDisposable // Noncompliant S3881
{
~TestDisposablee()
{
Dispose(false); // Also with this.Dispose(false);
}
public void Dispose()
{
Dispose(true); // Also with this.Dispose(true);
GC.SuppressFinalize(this);
}
private void Dispose(Boolean disposing)
{
// Dispose activities
}
}
It doesn't seem related to the this
keyword. Can you please provide a compliant example that becomes noncompliant when adding this
keyword to the invocation?
Tim-Pohlmann commented
As pointed out by Cristian, the problem is not the this
. The Disposable pattern is not implemented correctly. The Dispose(bool disposing)
method needs to be protected virtual
:
public class TestDisposable :
IDisposable
{
~TestDisposable()
{
this.Dispose(false);
}
public void Dispose()
{
this.Dispose(true);
GC.SuppressFinalize(this);
}
protected virtual void Dispose(Boolean disposing)
{
// Dispose activities
}
}