Fix S2952 FP: Internal Disposable fields must allow Dispose() call on re-creation.

[Aycon3296]

Description

The rule requires that all fields implementing the IDisposable interface be disposed in the parent's Dispose() method.
The rule should not be triggered if the field is disposed AND in both the method AND the Dispose() function of the containing class.
This is because a class can create multiple IDisposable instances during its lifecycle, store them in the same field, and free them multiple times when replacing them. Calling the Dispose() method early promotes more efficient memory cleanup than passively losing the reference.

Reproducer

internal sealed class Foo: IDisposable
{
    // Any IDisposable field
    private CancellationTokenSource _disposableMember;
    
    private CancellationToken _processToken;
    private readonly SemaphoreSlim _processSync;

    private bool _isDisposed;
    
    private int MagicNumber { get; set; }

    public bool IsWorkComplete { get; set; }

    // I cannot annotate this constructor which instantiate Process through property in .NET Framework 4.7.2
    public Foo()
    {
        _processSync = new SemaphoreSlim(1, 1);
        _disposableMember = new CancellationTokenSource();
        _processToken = _disposableMember.Token;
        IsWorkComplete = false;
        MagicNumber = -1;
        _isDisposed = false;
    }

    private Task<int> Process
    {
        get;
        set
        {
            // Dispose old instance at once
            // HERE WARNING
            _disposableMember.Dispose();

            // Recreate instance
            _disposableMember = new CancellationTokenSource();
            _processToken = _disposableMember.Token;
            field = value;
        }
    }

    public Task<int> GetHardWorkProcessAsync(CancellationToken token = default) // Intentionally synchronous here
    {
        // Attach token
        token.Register(_disposableMember.Cancel);
        return StartHardWorkAsync(token); 
    }

    public int Bar(int defaultMagicNumber = -1)
    {
        if (IsWorkComplete)
            return MagicNumber;

        return defaultMagicNumber;
    }
    
    public async Task<int> BarAsync(int defaultMagicNumber = -1, CancellationToken token = default)
    {
        await _processSync .WaitAsync(token);
        try
        {
            if (IsWorkComplete)
                return MagicNumber;

            token.Register(_disposableMember.Cancel);
            MagicNumber = await Process.ConfigureAwait(false);
            IsWorkComplete = true;

            return MagicNumber;
        }
        catch (Exception)
        {
            // Intentional exception suppression here
            // You can catch the process exception by awaiting the task from GetHardWorkProcessAsync()
            Process = StartHardWorkAsync(_processToken);
            IsWorkComplete = false;
            return defaultMagicNumber;
        }
        finally
        {
            _processSync.Release();
        }
    }
    
    private static async Task<int> StartHardWorkAsync(CancellationToken token = default)
    {
        await Task.Delay(1000, token);
        return 42;
    }
    
    public void Dispose()
    {
        if (_isDisposed)
            return;
        
        _disposableMember.Dispose(); // The member is Disposed!
        _processSync.Dispose();
        _isDisposed = true;
    }
}

Product and Version

SonarQube for IDE (Jetbrains Rider) v11.9.0.83922

[jilles-sg]

The general concept seems OK (although such a pattern is more complicated and more error-prone), but for me it does not follow from the example code. Perhaps _disposableMember can just be a local variable, and created using CancellationTokenSource.CreateLinkedTokenSource(). In any case, if token.Register(_disposableMember.Cancel); is used, the resulting CancellationTokenRegistration should be disposed at some point, lest the reference to that particular CancellationTokenSource remain for the lifetime of the incoming token.

[Aycon3296]

This isn't the most common use of disposable objects, of course, but it's certainly possible. For example, it could be used to implement a pool of wrappers over unmanaged objects or factories with cached rotation.
I found the proposal somewhere that led to this rule being introduced. The original idea was to prevent developers from disposing from incorrect methods, such as "Clear()," "Close()," and others. For this purpose, it's sufficient to ensure that the field was disposed in the Dispose() method. Users who adhere to the semantics of the Dispose pattern will avoid making a mistake by calling the correct method; the rest is the developer's responsibility.

[Aycon3296]

In this particular example, making CancellationToken a local variable is difficult, since it must actually live longer than the lifetime of a single external registration token (actually, its lifetime is only valid for as long as the process is running). In fact, you can bind an internal token to any number of external tokens, and each of them can abort the operation and reinitialize the Process.