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

IDISP003 false positives for some patterns of disposing before re-assigning #481

Open
JeremyMorton opened this issue Apr 14, 2023 · 5 comments

Comments

@JeremyMorton
Copy link

public sealed class TestReassigning : IDisposable
{
  private bool _disposed;

  private Disposable? _disposable;

  /// <inheritdoc />
  public void Dispose()
  {
    if (_disposed)
    {
      return;
    }

    _disposed = true;
    _disposable?.Dispose();
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNewWithUsing()
  {
    using var _ = _disposable;
    _disposable = new(); // false positive IDISP003
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNullWithUsing()
  {
    using var _ = _disposable;
    _disposable = null; // false positive IDISP003
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignVariableWithUsing()
  {
    var       disposable = new Disposable();
    using var _          = _disposable;
    _disposable = disposable; // false positive IDISP003
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNewWithUsingDeclaration()
  {
    using (var _ = _disposable)
    {
      _disposable = new();
    }
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNullWithUsingDeclaration()
  {
    using (var _ = _disposable)
    {
      _disposable = null;
    }
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignVariableWithUsingDeclaration()
  {
    var disposable = new Disposable();
    using (var _ = _disposable)
    {
      _disposable = disposable;
    }
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNewWithTemp()
  {
    var oldDisposable = _disposable;
    _disposable = new();
    oldDisposable?.Dispose();
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNullWithTemp()
  {
    var oldDisposable = _disposable;
    _disposable = null;
    oldDisposable?.Dispose();
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignVariableWithTemp()
  {
    var disposable    = new Disposable();
    var oldDisposable = _disposable;
    _disposable = disposable;
    oldDisposable?.Dispose();
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNewWithUsingWithConditional()
  {
    if (Random.Shared.Next() < 10)
    {
      using var _ = _disposable;
      _disposable = new(); // false positive IDISP003
    }
    else
    {
      using var _ = _disposable;
      _disposable = new(); // false positive IDISP003
    }
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNullWithUsingWithConditional()
  {
    if (Random.Shared.Next() < 10)
    {
      using var _ = _disposable;
      _disposable = null; // false positive IDISP003
    }
    else
    {
      using var _ = _disposable;
      _disposable = null; // false positive IDISP003
    }
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignVariableWithUsingWithConditional()
  {
    if (Random.Shared.Next() < 10)
    {
      var       disposable = new Disposable();
      using var _          = _disposable;
      _disposable = disposable; // false positive IDISP003
    }
    else
    {
      var       disposable = new Disposable();
      using var _          = _disposable;
      _disposable = disposable; // false positive IDISP003
    }
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNewWithUsingDeclarationWithConditional()
  {
    if (Random.Shared.Next() < 10)
    {
      using (var _ = _disposable)
      {
        _disposable = new(); // false positive IDISP003
      }
    }
    else
    {
      using (var _ = _disposable)
      {
        _disposable = new(); // false positive IDISP003
      }
    }
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNullWithUsingDeclarationWithConditional()
  {
    if (Random.Shared.Next() < 10)
    {
      using (var _ = _disposable)
      {
        _disposable = null; // false positive IDISP003
      }
    }
    else
    {
      using (var _ = _disposable)
      {
        _disposable = null; // false positive IDISP003
      }
    }
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignVariableWithUsingDeclarationWithConditional()
  {
    if (Random.Shared.Next() < 10)
    {
      var disposable = new Disposable();
      using (var _ = _disposable)
      {
        _disposable = disposable; // false positive IDISP003
      }
    }
    else
    {
      var disposable = new Disposable();
      using (var _ = _disposable)
      {
        _disposable = disposable; // false positive IDISP003
      }
    }
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNewWithTempWithConditional()
  {
    if (Random.Shared.Next() < 10)
    {
      var oldDisposable = _disposable;
      _disposable = new(); // false positive IDISP003
      oldDisposable?.Dispose();
    }
    else
    {
      _disposable = new(); // false positive IDISP003
    }
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignNullWithTempWithConditional()
  {
    if (Random.Shared.Next() < 10)
    {
      var oldDisposable = _disposable;
      _disposable = null; // false positive IDISP003
      oldDisposable?.Dispose();
    }
    else
    {
      var oldDisposable = _disposable;
      _disposable = null; // false positive IDISP003
      oldDisposable?.Dispose();
    }
  }

  /// <summary>Test reassigning to a disposable field.</summary>
  public void ReassignVariableWithTempWithConditional()
  {
    if (Random.Shared.Next() < 10)
    {
      var disposable    = new Disposable();
      var oldDisposable = _disposable;
      _disposable = disposable; // false positive IDISP003
      oldDisposable?.Dispose();
    }
    else
    {
      var disposable    = new Disposable();
      var oldDisposable = _disposable;
      _disposable = disposable; // false positive IDISP003
      oldDisposable?.Dispose();
    }
  }

  private sealed class Disposable : IDisposable
  {
    /// <inheritdoc />
    public void Dispose()
    {
    }
  }
}
@JohanLarsson
Copy link
Collaborator

Are all regressions?

@JeremyMorton
Copy link
Author

As far as I know, these have always been this way (I only started using this analyzer a couple of weeks ago). They are just things that I would expect to not have warnings.

@JohanLarsson
Copy link
Collaborator

Ok, was just curious if I had dumbed things badly. Does not change things, needs tests and fixes either way.

@TheR00st3r
Copy link

I found one more case that gives me a false positive for IDisp003

  /// <summary>Test reassigning to a disposable field with nullcheck.</summary>
  public void ReassignWithNullCheck()
  {
    if (_disposable != null)
    {
      return null;
    }
    else
    {
      _disposable = new Disposable(); // false positive IDISP003
    }
  }

@JohanLarsson
Copy link
Collaborator

JohanLarsson commented Apr 18, 2023

@TheR00st3r my preference is many small issues.
I work on this library when I have minutes between things.
Many small issues is faster as it minimizes time spent reading github.
Your sample code looks nice, that is a time saver, thank you for reporting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants