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.

Sign up for GitHub

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

Enhancement Add DisposeWith #2178

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Enhancement Add DisposeWith #2178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9f3090
Feature Add DisposeWith
ChrisPulman Nov 14, 2024
cccfded
Update API test
ChrisPulman Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions Rx.NET/Source/src/System.Reactive/Disposables/DisposableMixins.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT License.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;

namespace System.Reactive.Disposables;

/// <summary>
/// Extension methods associated with the IDisposable interface.
/// </summary>
public static class DisposableMixins
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking through Rx at all the other types that define extension methods, they tend to end in Extensions. E.g., TaskObservableExtensions, ObservableExtensions, VirtualTimeSchedulerExtensions.

So far we haven't used the terminology mixin at all.

Do you think this is a fundamentally different sort of thing from the existing things that use the Extensions suffix? If this has a different-looking name from other similar things, and if that signifies an important conceptual difference, then that's reasonable.

But if there is such a difference, I'm not sure what it is. Is there one? Or is this more of a naming preference thing? If so, how would you feel about aligning with existing conventions in this library here?

{
/// <summary>
/// Ensures the provided disposable is disposed with the specified ICollection of IDisposable./&gt;.
/// </summary>
/// <typeparam name="T">The type of the disposable.</typeparam>
/// <param name="item">The disposable we are going to want to be disposed by the disposable collection.</param>
/// <param name="disposableCollection">The composite disposable.</param>
/// <returns>
/// The disposable.
/// </returns>
/// <exception cref="System.ArgumentNullException">compositeDisposable</exception>
public static T DisposeWith<T>(this T item, ICollection<IDisposable> disposableCollection)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly baffled now that I look at this. It appears to be ICollection.Add in disguise. And it does not appear to ensure that the IDisposable gets disposed.

For example, this seems to let me write the following:

List<IDisposable> disposables = new();
someDisposable.DisposeWith(disposables);

This is, as far as I can tell, exactly equivalent to:

List<IDisposable> disposables = new();
disposables.Add(someDisposable);

This DisposeWith method name is making promises it can't keep.

If there were something constraining the disposableCollection argument to be something that would actually dispose everything it contains, then this would make some sense. E.g. if its type were CompositeDisposable, then it would work, although even then, how much better is this:

someDisposable.DisposeWith(compositeDisposable);

than this:

compositeDisposable.Add(someDisposable);

Moreover, at that point it no longer works for anything other than CompositeDisposable in which case it might as well be a member of CompositeDisposable—there doesn't seem to be any value in it being an extension method. I presume that's why you chose the more general argument type of ICollection<IDisposable> (even though the XML doc comment says it's a "composite disposable".)

It looks like a basic presumption behind this method is that it's inconceivable that anything implementing ICollection<IDisposable> wouldn't dispose its contents. But that's untrue for List<IDisposable> (a type we use in several places internally in Rx).

I suppose it might be possible to constrain the second argument too, e.g. something like:

public static TResource DisposeWith<TResource, TCollection>(this T item, ICollection<IDisposable> disposableCollection)
    where TResource : IDisposable
    where TCollection : IDisposable

That still makes a fairly big assumption: that the collection type will dispose all of its contents. But I don't think that's necessarily true. If the collection type specifically implements ICollection<IDisposable> and IDisposable, then I'd go as far as to say that it seems quite likely that it would dispose each of its members. But if a collection type is generic, and just implements ICollection<T> (and IDisposable) and if it has not constrained T to be IDisposable, then I would expect it not to.

So here's how you'd actually want to constrain the collection type to be reasonably confident that DisposeWith would do what it promises:

  • implements IDisposable
  • implements ICollection<IDisposable>
  • is not a generic type which, if instantiated with different type arguments, might have implemented ICollection<NotDisposable> instead

I don't believe it's possible to express that particular set of constraints in C#. So either you make this method so narrow that it is really just another name for CompositeDisposable.Add, or you leave it open in ways that means it won't necessarily function as advertise.

So, before we draw any conclusions from this analysis, do you think I've missed anything in the above?

where T : IDisposable
{
if (disposableCollection == null)
{
throw new ArgumentNullException(nameof(disposableCollection));
}

disposableCollection.Add(item);
return item;
}
}
5 changes: 5 additions & 0 deletions Rx.NET/Source/tests/Tests.System.Reactive.ApiApprovals/Api/ApiApprovalTests.Core.verified.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,11 @@ public static class Disposable
public static System.IDisposable Create(System.Action dispose) { }
public static System.IDisposable Create<TState>(TState state, System.Action<TState> dispose) { }
}
public static class DisposableMixins
{
public static T DisposeWith<T>(this T item, System.Collections.Generic.ICollection<System.IDisposable> disposableCollection)
where T : System.IDisposable { }
}
public interface ICancelable : System.IDisposable
{
bool IsDisposed { get; }
Expand Down
13 changes: 13 additions & 0 deletions Rx.NET/Source/tests/Tests.System.Reactive/Tests/Disposables/DisposableTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,19 @@ public void CompositeDisposable_Empty_GetEnumerator()
Assert.False(composite.GetEnumerator().MoveNext());
}

[TestMethod]
public void CompositeDisposable_DisposeWith()
{
var c = new CompositeDisposable();
var d = new BooleanDisposable();
d.DisposeWith(c);
Assert.True(c.Contains(d));

c.Dispose();
Assert.True(d.IsDisposed);
Assert.True(c.IsDisposed);
}

[TestMethod]
public void CompositeDisposable_NonCollection_Enumerable_Init()
{
Expand Down
Loading