SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
755 stars 223 forks source link

Fix S3881 FP: Take care of reference with current instance keyword "this." #9452

Closed NETSphereSoft closed 1 week ago

NETSphereSoft commented 1 week ago

Description

Correct implementation of Dispose pattern with reference this. will not detected.

Repro steps

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

Related information

CristianAmbrosini commented 1 week ago

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 1 week ago

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
    }
}