Language-ext: Will/should Option<T> implement IDisposable?

[StefanBertels]

Why does Option<T> does not implement IDisposable? Can this be changed?

If I have a IDisposabletype T and wrap it with Option<T> e.g. resources will not get freed automatically (in time). Think about Option<FileStream>.

[louthy]

@StefanBertels That would be a bad idea. Option doesn't own the T, it is given to the constructor. And therefore it can't be responsible for its lifetime. Also because Option is a struct, it gets instantiated and finalised all the time, so it's difficult to know whos (Ignore that second sentence, I have was having a brain freeze! The first sentence still stands though)

[StefanBertels]

I thought about StreamReader. Compare
http://stackoverflow.com/questions/1065168/does-disposing-streamreader-close-the-stream
BTW: StreamReader has an optional constructor parameter to disable it (bool leaveopen).

Why not see Option as owner of T here (in the same way StreamReader does with its Stream)?
Is this just another point of view or is this (ownership) itself a bad idea for Option (and similar types)?

I mean: Dipose (or using) on an Option itself does not have any use case because Option itself does not have any resources. So one obviously would like to Dipose the contained value (if Some).

Anyway: I'm fine with your decision, to not change this. Maybe there are other use cases against it.

[louthy]

@StefanBertels Hi Stefan, sure let me think about it. I suspect that this probably makes sense:
c# using(var x = Some(disposableThing)) { ... }
I'm midway through some other changes, so I'll let it settle for the moment and revisit this when it's done.

[StefanBertels]

Just some note:

IDisposable on a struct has its issues...

And this article by Eric Lippert shows that it works some way.

I'm not sure but as far as I can see making Option IDisposable should be ok because you have to call Dispose() by intent (directly or by using). So nothing bad happens for normal use cases. And a simple using construct should work as intended.

But if we see Option as a container (a list of at most one element) making Option IDisposable seems questionable. (But maybe disposing a container should just mean dispose all elements.)

Anyway: IIRC the reason for this feature request / question was that your echo lib diposes state objects if they are IDisposable and if you had Option<T> as state object with T being IDisposable you probably need some solution (or not use Option there).

[TysonMN]

I don't think Option<T> should implement IDisposable.

My reasoning for why mostly comes from Mark Seemann, an expert in dependency injection and author of Dependency Injection in .NET. In particular, I think that Mark would also be against Option<T> implementing IDisposable. I will attempt to justify this as I think Mark would. To read some of Mark's own words on this topics, see this blog post.

I begin with a quote from that blog post.

the object that composes the object graph is the only object with enough knowledge to dispose of any disposable objects within the object graph that it created

In our case, we are considering the object graph that conceptually is just two objects: the Option<T> and its T within. The Option<T> class did not create the T; it was given the T. So it should not be responsible for its disposal. The object that instantiated the disposable T should be the one to dispose it. Ideally, this would happen in the composition root, possibly with the help of a DI container.

The difference with StreamReader and its child Stream object is that it might be the StreamReader object that instantiated the Stream object. In that case, it is the only object that knows about the need to dispose the Stream object, so it must implement IDisposable "in that case". This case would happen when calling the constructor StreamReader(string path) that

Initializes a new instance of the StreamReader class for the specified file name.

Where it gets weird is that StreamReader has other constructors, such as StreamReader(Stream stream), that accept a Stream object. Some other object instantiated this Stream object, so it _should_ be the one that is responsible for disposable as well. My guess is that, given this API for StreamReader, the decision was made to both simplify the logic and "help out the user" by disposing the Steam object even when it was provided in a constructor.

So I don't think the example of StreamReader and its Stream object is a good example to be considering for this issue. It could very well be that the designers of StreamReader now regret the two vastly different ways of instantiating the class and would do things differently if given another chance.

[louthy]

My reasoning for why mostly comes from Mark Seemann, an expert in dependency injection and author of Dependency Injection in .NET.

Be aware, the whole dependency injection framework _thing_ is something that starts me twitching like Chief Inspector Dreyfus from the Pink Panther. I wouldn't take the advice of a proponent of it based on that primary credential. Dependency injection frameworks need to be thrown on a fire in my opinion. DI can be done properly using higher-order functions, actors, and free-monads. The frameworks are just another example of how the OO world just keeps digging bigger holes for itself.

</rant>

I prefer to approach these kinds of questions from a pragmatic point-of-view because this is exactly one of those subjects that the OO world has probably gotten wrong. The concept of ownership is tentative at best. If there was truly such an ownership scheme then the disposal would be entirely handled by the IDisposable type - or via some formal ownership concept. But that isn't the case, something else has to call obj.Dispose() - and there is no formal relationship between the disposable type and any other concept in the language (bar the using statement). Ultimately, it's the developer that decides when to dispose, either through calling Dispose or wrapping the instantiation with a using. In my opinion, the 'owner' is the _programmer_, not some mystical object graph.

So, if we depart with the OO dogma for a second, and start to look at what's actually valuable then I think we'd get to a better solution for all.

Let's stick with the Option type for now and whether it should be disposable or not...

Option is not like StreamReader and Stream, Option is very much a _value_. Or, to be more precise it's either _a value_ - Some(x) - or _not a value_ - None. The only other precedent for that in the BCL is Nullable, which isn't IDisposable. So that's one strike against Option being IDisposable.

If we consider Option to be _invisible_ and just consider the absence or presence of the _value_, then it makes perfect sense to call Dispose() on the _value_ (when it's there). Option could be seen as a totally transparent proxy to the _value_. This _feels_ reasonable.

If we then take the thought experiment further, what does the code look like if we have a non-disposable Option, but we need to dispose of the bound _value_.
```c#
static Option OpenStream(StreamDesc desc) => ...;

static Unit Test(StreamDesc desc)
{
var stream = OpenStream(desc);

try
{
    return UseStream(stream);
}
finally
{
    stream.Iter(s => s.Dispose());
}

}

This looks and feels pretty bad.  It's all statement based and can't lend itself to being an expression at all.

It could be made slightly less bad by providing an extension method for disposing:
```c#
public static Unit Dispose<A>(this Option<A> ma) where A : IDisposable =>
    ma.Iter(x => x.Dispose());

It's improved, but still not pretty:
```c#
static Unit Test(StreamDesc desc)
{
var stream = OpenStream(desc);

try
{
    return UseStream(stream);
}
finally
{
    stream.Dispose();
}

}

Now let's imagine `Option` is disposable and try it with the `using` statement:
```c#
static Unit Test(StreamDesc desc)
{
    using(var stream = OpenStream(desc))
    {
        return UseStream(stream);
    }
}

This is already significantly better. But it should also work with the use function of lang-ext:
c# static Unit Test(StreamDesc desc) => use(OpenStream(desc), UseStream);
Suddenly we're in _concise functional expression land_. This makes me happy.

If we go further an consider Either then that feels right like Option too, because it's still a _value_. Or to be more precise _either one value_ - Left(x) or _another value_ - Right(y).

What about collections. Why shouldn't I be able to dispose all items in a list? If we agree that nobody really owns the items and it's the programmer that ultimately makes the decision. Surely we can give the programmer the most efficient disposable mechanism going - so they don't have to repeat the same code over and over?

We start to run into problems when we encounter Try, TryOption, etc. that are delegates. But lang-ext already provides overloads for use that work with Try (which needs expanding to the other delegate types btw). So, we have precedent too.

I have pretty much talked myself into it by writing this message. But I'm open to other opinions.

_Please let's not turn this into a dependency injection discussion ;)_

[TysonMN]

Ultimately, it's the developer that decides when to dispose, either through calling Dispose or wrapping the instantiation with a using. In my opinion, the 'owner' is the programmer, not some mystical object graph.
...
If we agree that nobody really owns the items and it's the programmer that ultimately makes the decision. Surely we can give the programmer the most efficient disposable mechanism going - so they don't have to repeat the same code over and over?

Sure, the programmer can pick any design they want, including a bad one.

“I have the right to do anything,” you say—but not everything is beneficial. “I have the right to do anything”—but not everything is constructive. ~1 Corinthians 10:23 NIV

This is what I like so much about functional programming. It's proponents claim it is a more beneficial and constructive way to write programs. I am very convinced by these claims, which is why I am so eager to learn more about functional programming.

I prefer to approach these kinds of questions from a pragmatic point-of-view...

I want to consider a pragmatic approach while also considering a theoretical one. I think to myself, "how would the design look in a perfect world?"...or at least as perfect as I can imagine it to be. Then I consider the gap between theory and practice and consciously choose a point on the interval between them. I feel like this gives be a better appreciation for compromises that I am making, both from a pragmatic perspective as well as from a theoretical one.

In theory, I want code written within small scopes so that I can look at each piece of code (along with its entire scope), see the whole thing on my screen, assume that all dependencies are correct, and immediately be convinced of the correctness of this piece of code. I think of this as the manual code-review equivalent of a automated unit tests.

In the case we are discussing, I want to be immediately convinced that any object that is disposable is disposed. It must be disposed at the right time. Too early will causes an ObjectDisposedException to be thrown. Too late we hinder performance by withholding resources from others that needs them, or worse, deadlock by never releasing the resource. Furthermore, some objects throw an exception if Dispose is called a second time. So we need find a unique space and the right time in which to call Dispose on each disposable.

To that end, consider that some disposable object is instantiated, and consider the set of objects that have the ability to call Dispose on it (either directly because the object has (or even had at one point) a reference to it or indirectly by calling some function that would eventually cause this invocation). We must pick one (the space requirement) to be the one to call Dispose. Even in such an abstract setting where it seems that we know almost nothing about the code, there is a natural choice that always works: the object that instantiated the disposable object.

For example, this code adheres to this theoretical ideal.

public T Execute<T>(Func<SomeDisposableClass, T> f) {
    var dependencies = this.GetDependencies();
   using (var someDisposableClass = new SomeDisposableClass(dependencies)) {
      return f(someDisposableClass);
   }
}

I see two downsides to this code.

1. It is not functional.
2. We would be repeating code by copy-pasting so that one can reuse it for SomeOtherDisposableClass.

So think of the theoretical ideal as the above code after these abstractions to be more functional and not duplicate code when "repeated".

In contrast, I imagine that you would implement Dispose on Option<> with something like (obj as IDisposable)?.Dispose(). This is not convincing me of correctness at all. How do I know that it wasn't disposed too early? How do I know that it couldn't have been disposed of sooner? How do I know that this is the first time that this method was called on this object? How do I know that some other object won't call this method on this object later?

I imagine the counter argument to go like this.

But the user of Option<> has an instance with identifier opt and has called opt.Dispose(). So from the perspective of Option<>, we know that it is the right time because we get to assume it is the case because the user has declared it to be.

My rebuttal is that this user should just call something like opt.IfSome(t => t.Dispose()) instead. In particular, they are more likely to "know" if t implements IDisposable, thus not needing to cast. By applying this argument recursively, we terminate at the base case in which the method calling Dispose is the same one that instantiated it.

Be aware, the whole dependency injection framework thing is something that starts me twitching like Chief Inspector Dreyfus from the Pink Panther.
...
Please let's not turn this into a dependency injection discussion ;)

I too want to keep this discussion on topic :) As such, notice that my argument has nothing to do with dependency injection. I am very interested know why you feel that way, but we can save that conversation for another time.

[TysonMN]

Now the hard part is picking the point between the pragmatic and the theoretical.

For the pragmatic side, I will say that in the programs that I write at work, I strive to achieve code like in my example, and I think the programs are better for it. When I joined my current project at work, one issue with the program was that it would max out CPU and memory slowly over the course of 24 hours. My advice to the client was that we should ignore this problem for now in the hopes that it is disappear as I continued to add new features and fix smaller bugs while simultaneously cleaning the code in the vicinity of those changes.

My hypothesis for the this issue is that the program was not disposing disposables, which caused all the memory to be allocated, which caused frequent garbage collections from the runtime to a desperate attempt to free more memory, which caused the maxed out CPU utilization.

The latest report is that this issue no longer exists (or, at least, is significantly better). I attribute this improvement to places in the code where I guarded access to IDisposable`s like in my example code in my previous comment. This leads me to believe that my theoretical code is also practical.

[StefanBertels]

I opened this issue and I wasn't sure whether it is really a good idea. But after todays comments I think @louthy wrote some good reasons why Option can be IDisposable. ACK, so far. Comments by @bender2k14 didn't show me a good reason against this.

But maybe I'm missing a point -- and I'm still interested whether there could be real practical problems with this and I'm open for any theoretical discussion how things should be.

I have to say that I avoid dependency injection and try to solve this by writing side-effect free code (static pure functions) where possible. But currently I don't see the point where Option.Dispose() would really harm.

It might be a good idea for you (and others) to have some IDisposable "graph" and "ownership" pattern, but how is this exactly related to Option.Dispose()?

Yes, everyone having a ref can call Dispose() and this might be a problem in some scenarios. But if you want to enforce this you might have to add some wrapper (something like decorator reversed / adapter) and have the outside object NOT implement IDisposable while giving some special access to the inner IDisposable object (e.g. to your parent object). If you just wrap some IDisposable with Some(...) everyone having a ref to this Option can dispose the inner object anyway...

And there are other patterns to Dispose correctly, see for example: https://msdn.microsoft.com/en-us/library/system.reactive.disposables

I really have use cases for at least an disposable Option. It's not only making using/use work again with Option allowing for cleaner code but about every code that creates Options with contained Disposables (e.g. echo library actor setup function).

(Just for the record: I don't think IDisposable and all those finalizer things are good design, but it's the way it is in C#/.NET)

[TysonMN]

I have to say that I avoid dependency injection and try to solve this by writing side-effect free code (static pure functions) where possible.

I am interested to discuss dependency injection with you as well, but I will also refrain from doing so now. The issue at hand is rather orthogonal to dependency injection. My acknowledgment of Mark as the source for my argument has had quite the opposite effect compared to what I had intended, and I think it might have presented somewhat of a straw man argument in both of your eyes because of it.

From a theoretical (or maybe hypothetical) perspective, let's consider the slippery slope argument. If Option<> should implement IDisposable, then why not have every type implement IDisposable?

[faeriedust]

using(var x = Some(disposableThing))
    {
      ...
    }

This doesn't make sense at all to me. Why turn disposableThing into an Option<> type when it clearly is never None here? This seems to be arbitrarily/randomly throwing away information for no reason. I can't think of hardly any possible use case to say: var x = Some(...);

Also, I think in general the creator of an object should be responsible for disposing it, unless it's a factory method. So I guess the main issue here is the case where you're using factory methods that return an Option<>.

[faeriedust]

I'm looking at the code:

static Option<Stream> OpenStream(StreamDesc desc) =>  ...;
static Unit Test(StreamDesc desc)
{
    var stream = OpenStream(desc);
    try
    {
        return UseStream(stream);
    }
    finally
    {
        stream.Iter(s => s.Dispose());
    }
}

My first reaction is for UseStream to not take an Option cause wth is UseStream(None) supposed to do..

static Option<Stream> OpenStream(StreamDesc desc) =>  ...;
static Unit Test(StreamDesc desc)
{
    var stream = OpenStream(desc);
    try
    {
        return stream.Map(UseStream).IfNone(unit);
    }
    finally
    {
        stream.Iter(s => s.Dispose());
    }
}

The syntax is still ugly, so we want to use the use method, right?

static Option<Stream> OpenStream(StreamDesc desc) =>  ...;

static Unit Test(StreamDesc desc) =>
    OpenStream(desc).Map(stream => use(stream, UseStream)).IfNone(unit);

Or something similar? Just a thought. I dislike that I need to do the first refactor to do this, though certainly in this case it appears to make sense.

One could also consider adding an overload of use that takes an Option<> and behaves appropriately? It wouldn't even have to be in LanguageExt. It seems simple enough to add a method like this in the client application's code if they really need such a thing.

Another argument against it is, along the lines of the slippery slope that @bender2k14 mentioned, I don't want to write code to dispose all of my Option<>s, only the ones that wrap IDisposables, so it seems to make a lot more sense to me to handle this in a method that only takes Option<IDisposable> than on the Option<> itself.

[TysonMN]

What about this argument?

The documentation of IDisposable says that Option<> (in its current form) should not implement this interface.

Implement IDisposable only if you are using unmanaged resources directly. If your app simply uses an object that implements IDisposable, don't provide an IDisposable implementation.

My arguments before were trying to explain "why" this requirement from the interface itself exists.

[StefanBertels]

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern says:

implement the Basic Dispose Pattern on types containing instances of disposable types

[TysonMN]

@StefanBertels, your quote ends just as an explanation of it beings. Here is how it continues.

If a type is responsible for the lifetime of other disposable objects, developers need a way to dispose of them, too.

The essence of our entire thread here is to decide, for each disposable object, who is responsible for its disposable. We have yet to come to an agreement about this.

The responsibility of Option<T> is to represent the concept of the existence or absence of an instance of T that it was _provided_. To also take on the responsibility of lifetime management of this provided instance is a violation of the single responsibility principle.

It cannot be the case that every class with a reference to the same instance of a disposable object are all responsible for its disposable. There must be exactly one class with that responsibility. Otherwise, it is impossible to avoid early disposal, late disposal (including never being disposed), or double disposal.

So "containing" in your quote doesn't mean "has an reference". Conceptually it means "instantiated".
Practically though, "instantiated" is too strict. A factory that only creates instances of a disposable type is not responsible for disposing it. How could it be? If it did, then the entire class is a big no-op. Instead of the factory being responsible for disposable then, the responsibility of disposable falls to the class utilizing the factory.

So an alternative to my previous example

public T Execute<T>(Func<SomeDisposableClass, T> f) {
    var dependencies = this.GetDependencies();
   using (var someDisposableClass = new SomeDisposableClass(dependencies)) {
      return f(someDisposableClass);
   }
}

is

public T Execute<T>(Func<SomeDisposableClass, T> f) {
   using (var someDisposableClass = SomeDisposableClassFactory.Create()) {
      return f(someDisposableClass);
   }
}

The authors of this documentation are trying to account of cases like this. As programmers, we know very well how much the requirement to properly handle edge cases increases the complexity of the code. Writing prose is no different.

[StefanBertels]

IMO the point is:

Does Option contain the inner object or does it just refer to it. It depends.

Making Option IDisposable will allow outside code to dispose it in case it contains inner object. You don't have to call Dispose if you don't want to.

Besides the fact that use/using code will be possible, I see two more advantages:

If you have some factory it's straight forward that the factory returns an IDisposable (or compatible) if someone should call Dispose later. Having Option non-IDisposable will hide this fact for optional return values. This is a problem for generic code. Look at echo actor setup (example): echo calls factory (setup) and therefore owns the returned object. If your setup returns Option you have a problem.

Another (minor) point is that Option itself could throw errors if you call some method after Dispose().

I don't see a practical problem with IDisposable for those cases where your inner object isn't IDisposable or should not be Disposed.

[TysonMN]

Does Option contain the inner object or does it just refer to it. It depends.

Except the interface is called IDisposable, not IMaybeIDisposable. A class is supposed to implement IDisposable if it _does_ have something that _it alone_ is responsible for disposing.

Making Option [implement] IDisposable will _allow_ outside code to dispose it in case it _contains_ inner object.

Two responses to this. First, Option<> already allows outside code to dispose. For example, opt.IfSome(t => { t.Dispose(); return unit; }). Second, whether or not Option<> "contains" (according to the previously quoted documentation) is a decision to be made by its creators, not by its users.

You don't have to call Dispose if you don't want to.

No, this is the entire problem. The contract of a class that implements IDisposable requires from its users that Dispose is called at the proper time and in the proper way. The users of Option<> are going to trust this contract and be inclined to write code like this

public static OptionAsync<T> ToAsyncUsing<T>(this Option<T> opt) {
    using(opt) {
        return opt.ToAsync();
    } 
}

because they believe this is what is required of them in order to properly use Option<T> for any type T and instance thereof. If the desires of a user are in conflict with the behavior of a class as specified by its contract, then the user needs to find a different class satisfy their desire.

_Even if_ it were ok to implement IDisposable in such a way that sometimes it is a no-op, how is the user supposed to know when this is the case? How are they supposed to know when You and I know when it would be a no-op but that is only because you and I are aware of the proposed implementation. It is unreasonable to expect users to inspect the implementation. Of course one could attempt to explain this in documentation. However, it is my understanding that closely aligned with functional programming is the desire to use strong types; that the semantics of a program are dictated by its (strong) types instead of the documentation of (weaker) types.

Look at echo actor setup (example): echo calls factory (setup) and therefore owns the returned object. If your setup returns Option you have a problem.

I think I found the line of code that are referring to. I opened this echo-process issue to document this.

If you have some factory it's straight forward that the factory returns an IDisposable (or compatible) if someone should call Dispose later. Having Option non-IDisposable will hide this fact for optional return values. This is a problem for generic code.

The shortest response that I can give to that is this. I think Actor specifically (and thus echo-process generally) is not managing object lifetimes properly. Furthermore, I think making Option<> implement IDisposable is the wrong solution to this problem. If your type of your state was instead IEnumerable<T> where T : IDisposable, would you request that Microsoft have IEnumerable implement IDisposable? I wouldn't think so.

I have more to say about Actor. I will make a post at the echo-process issue shortly.

I don't see a practical problem with IDisposable for those cases where your inner object isn't IDisposable or should not be Disposed.

The problem is that the intent it communicates to the user. The user will think that they have to write code like ToAsyncUsing above.

[StefanBertels]

Why should a user write code like your ToAsyncUsing?

This isn't limited to Option. If you decorate some Stream ref or do other transformation on any IDisposable obj you won't dispose it in that operation, either.

[TysonMN]

Why should a user write code like your ToAsyncUsing ?

Because the contract of Option<>, should it implement 'IDisposable`, states that each instance should be disposed.

This isn't limited to Option. If you decorate some Stream ref or do other transformation on any IDisposable obj you won't dispose it in that operation, either.

I don't know what you mean. Can you be more specific?

[TysonMN]

From issue #328

One thing that has crossed my mind is that tooling like ReSharper might complain about values not being disposed properly, so that would need to be considered. I don't use it, so I'm not sure if it would start throwing out warnings, but from what I've seen of it before, I wouldn't be surprised.

I don't know about ReSharper, but I have considered writing my own static analysis tool that attempts to identify undisposed disposables. I hear that Roslyn makes this type of static analysis rather easy.

[StefanBertels]

Static analysis cannot be perfect, I think proof for this is similar to that of Halt problem. Just to add some theory pence here ;-) It's not impossible to limit analyzer so it only warns about relevant missing Dispose calls (i.e. Disposing container might not be necessary if inner object is disposed in some other way). But I admit this might be a workaround...

Yes, IDisposable has it's problems, in practise, too. Think about a StreamWriter that (by default) owns a MemoryStream. Disposing MemoryStream is not necessary (no unmanaged ressource). If StreamWriter does not own Stream, why do we need to Dispose it? (Yes, Flush, but: Autoflush) Might be a bad example but in C# we probably will end in similar situations because of abstract types.

Anyway: I see demand for Option.Dispose() just because Option should be usable like a container that really "owns" the value AND generic code that does rely on is IDisposable shouldn't break.

Anyway: I value your arguments because I think this library shouldn't go into some "wrong" direction. I'm sure Paul takes all issues into account to make this library as useful as possible.

Everyone have a good start into 2018!

[TysonMN]

Static analysis cannot be perfect, I think proof for this is similar to that of Halt problem. Just to add some theory pence here ;-)

You are probably thinking of Rice's Theorem.

It's not impossible to limit analyzer so it only warns about relevant missing Dispose calls (i.e. Disposing container might not be necessary if inner object is disposed in some other way). But I admit this might be a workaround...

My approach would be to err on true positives and provide some level of warning in all other cases. The idea is that warnings are always useful because they identify locations in the code that are too complicated for automated static analysis. The point is that we should avoid writing programs like those conceived of by Turning in order to show the impossibility of the halting problem.

My understanding of functional programming is that it is fundamentally about avoiding side effects.
Theoreticians like Turning and Rice can exploit this feature of a language to show that bad things can happen when state is modified. That is why pure static functions are so wonderful. They lend so well to static analysis, whether manual or automated.

Both of you are more knowable about functional programming than myself. It has taken a long time to reach my current understanding of monads. I was confused for a rather long time.

One of my overall impressions about monads is that they go to great lengths to formalize and isolate side effects. I have almost exclusively used the option, list, promise, and whatever the monad name for C#'s Func<A> would be. I hear that some monads help to control modification of state are the reader, writer, and state monads. But even so the monads themselves do not have side effects.

If Option<> were to implement IDisposable, in what sense would Option<> still adhere to this core design principle of monads that is the avoidance of state mutation? My current understanding of Option<> is that it can be viewed as an immutable data structure. Is this benefit lost if Option<> were to implement IDisposable?

Anyway: I value your arguments because I think this library shouldn't go into some "wrong" direction. I'm sure Paul takes all issues into account to make this library as useful as possible.

Everyone have a good start into 2018!

Thanks Stefan, I appreciate that. I do want the best for language-ext. It has helped us greatly improve our code at work. This has motivated me to improved my ability to do functional programming. Getting involved with language-ext seems like an excellent way to do that. It is very helpful to process my programming thoughts here by sharing them with you and @louthy in these issues. The least I can do in return is to contribute improvements via pull requests.

Happy new year everyone :)

[faeriedust]

So should all of the monads implement IDisposable? (I still think they should not, for already mentioned reasons.)

If you want a disposable Option type, simply wrap the existing one. All problems can be solved by adding a layer of abstraction after all. :)

Also what if the inner object is owned by someone else? Then how can we say that the Option<> owns it as well?

[faeriedust]

public class OuterService : IDisposable {
  public InnerService Inner {get; set;}
  public void Dispose() => this.Inner.Dispose();
}

public Option<InnerService> _MyMethod(Option<OuterService> outer) =>
  outer.Map(x => x.Inner);

Option<> here is pretty clearly not the owner of the inner service... OuterService is

[StefanBertels]

@bender2k14
I don't think you need state or side-effects for Halt Problem... ;-)

IMO: Immutability is not a problem. The problem is we have copies of references to the same object. Which reference is "master"? There is no such ownership flag in C#. Would be great if we had something like this because then compiler could call Dispose automatically if scope of "master" reference ends.

@faeriedust
We don't know whether an arbitrary Option is owner of inner disposable or not. Depends on context.
The issue here is: If Option is owner of some Disposable, then Option itself should be IDisposable.
There is some practical value because if Option is not IDisposable, you cannot use it in code like echo actor (return type of setupFn = state). And of course using/use in combination with such Option.

I don't see a problem with @faeriedust example because it depends on users code whether Dispose on Option will be called. You only call Dispose() on Options that are owner.

Option<> here is pretty clearly not the owner of the inner service

gets wrong if you add this line of code:

var x = _MyMethod(Some(new OuterService { InnerService = CreateSomeDisposable() }));

Admittedly the drawback of IDisposable Option is that we will create objects of type Option where we won't call Dispose() because this option is not or is no more owner of the inner object of type T.

[TysonMN]

IMO: Immutability is not a problem. The problem is we have copies of references to the same object. Which reference is "master"? There is no such ownership flag in C#. Would be great if we had something like this because then compiler could call Dispose automatically if scope of "master" reference ends.

I guess the part that I fundamentally don't understand is why you want to solve your specific mutability problem by changing a core functional data type to make it mutable as well. Sure, the way in which this non-pure behavior is achieved is through a copy of a shared reference, but this fact is just an implementation detail of the desired non-pure specification.

There is some practical value because if Option is not IDisposable, you cannot use it in code like echo actor (return type of setupFn = state)

What do you think about using a class like this as your state?

public class StefanOptionWrapper<T> : IDisposable {
  public Option<T> Value { get; }
  public StefanOptionWrapper(Option<T> value) => Value = value;
  public void Dispose() => Value .IfSome(x => x.Dispose());
}

[kdblocher]

@louthy, I strongly advise against this course of action. It "feels" dirty, both from FP and OOP perspectives. We've had some internal discussion at our company about this issue (full disclosure: @faeriedust and @bender2k14 work for me), and we are all flat out against it. We would never use such a kluge. A class should NEVER dispose objects passed to it via the constructor.

But don't take it from me, take it from another expert quite knowledgeable about the subject: https://github.com/louthy/language-ext/issues/205#issuecomment-289091607

If I haven't convinced you yet that you were correct the first time, please consider that this is a violation of referential transparency (aka SRP), immutability, and DIP. Holy hell, I could treat these unholy beasts as IDisposables and let others dispose them, opening up a whole host of runtime errors. As professional functional programmers, aren't we in the business of using the type system to catch as many errors as possible, because runtime errors don't exist?

Lastly, because C# doesn't allow us non-nullable nor immutable references yet, we should be using Option<> anywhere we want to enforce them (or at least the first). If it's disposable as well, anywhere we construct an Option internally, we've constructed a resource, and best practice says we should call .Dispose() on it. I can't wait to tell everyone who is still learning higher-order programming (aka "cuz monads") to ignore the IDisposable warnings in VS that will be going off everywhere because of this.

[kdblocher]

Now, to actually solve the problem: @StefanBertels, I would suggest writing a ResourceOption monad, adding some nice extension methods off of Option<> to get one for free, and using a latch to prevent multiple calls to Dispose inside a la a State or I/O monad. Such a beast should probably go into the Actor library and not this one, since it sounds like that's the use case you want it for.

[StefanBertels]

Yes, my one line call to _MyMethod is bad style -- was just an example for something where the reference to some IDisposable is passed/wrapped. Yes, there are workarounds for e.g. echo actor. I already have some workaround. Thank you for trying to find solutions for me, but this point is more about a generic solution in LanguageExt.

I don't think this is an actor issue or an issue with my very special use case. I personally use Option as replacement for refs that can be null (None), so I would need more workarounds when making more heavy use of LanguageExt. How about using/use (see Pauls writing above)?

IMO there is no mutability or security issue: Option does not ensure that the inner object is immutable.
Option does not disable possibility to dispose inner object. Having Option.Dispose() won't change something here. I'm interested in examples showing I missed a point.

I still see the drawback for analyzers / about principle to dispose every IDisposable somewhere.
But why do you think a ResourceOption (or better DisposableOption) is something bad?

The point to put this directly into Option is just a question whether this is more practical. Probably it would be better to have different types (Option and DisposableOption) and ideally being able to shift between them / shifting the "master" flag of refs between them.

[TysonMN]

But why do you think a ResourceOption (or better DisposableOption) is something bad?

Our main point is not that DisposableOption<> is a bad idea. Our main point is that changing Option<> by having it implement IDisposable is a bad idea. The title that you picked for this issue makes us think that you want to change Option<> by having it implement IDisposable.

Can you clarify this for us @StefanBertels? Would you like to see Option<> changed by having it implement IDisposable or would you like to see a new data type added to language-ext called (say) DisposableOption<> that works exactly like the existing Option<> data type except it also implements IDisposable?

I would not be against adding this new data type.

[StefanBertels]

The issue is about the request to have Option IDisposable. It was clear from beginning and written multiple times here that this will make things better in some way and worse in others. @louthy pointed at some positive effects. We discuss and bring arguments together. IMO: This issue is not an easy one to decide.

I'm really open to arguments, but we should discuss whether they hold or not. Those arguments about principle / analyzers hold, but Paul already mentioned that there is no real concept (or at least a broken one) for IDisposable at all. Lifetime management has to be done by developer and there are different methods to manage ownership.

Side note: If you compare C# 8 (?) solution for nullable references: LanguageExt provides a very nice way to deal with null (hey, Option again) and we hopefully benefit by new default that refs shouldn't be null at all (or have to be explicity marked). Compare this to how this is solved in C# by static analysis that will be imperfect (missing warning) and even produce false positives. I like ReSharper and all those analyzer help, but I don't see it as a primary tool. It's similar with testing: Good design might reduce need for testing.

I feel that you want to just stop this request / do not respect positive outputs. But IMO some arguments are wrong (like mutability) and others depend on coding style / design. Regarding dependency injection: This is some solution to a specific problem in OOP. But we can change design to avoid this. I don't suggest to drop DI (maybe you should think about it ;-)). Anyway I currently cannot see a specific problem there. If you give some example to show problems with disposable Option we could evaluate pros and cons.

Probably @louthy has a good overview whether this change (or some DisposableOption) would in summary be an improvement for his library and code using it. I'm grateful for this library and have a lot of confidence in his wise design decisions.

IMHO there is still a lot of value with disposable Option because we could make code easier to write and to read with using/use and reduce overhead we already put into our C# code by using the library.

It will be interesting to read what Paul says about DisposableOption. Would this integrate into LanguageExt or cause even more overloading problems?

[TysonMN]

The issue is about the request to have Option IDisposable. It was clear from beginning...

Ok, good. Thanks for clarifying. That is what I thought, but your previous comment made me think that you instead just wanted to add an additional data type with the desired behavior.

I feel that you want to just stop this request / do not respect positive outputs.

Changing Option<> to implement IDisposable is probably the smallest change (aka diff size) to fix the problem you had in your use of echo-process. That weighs in favor of making this change. But such small changes also directly correlate with being a hack.

This change wouldn't affect just you and your code; it would affect everyone that uses Option<>, including me and our client's projects. You believe that your actor project will benefit from this change, and I believe that our client's projects would suffer for it. For example, Microsoft's documentations warns that this would be a breaking change.

Warning
It is a breaking change to add the IDisposable interface to an existing class. Because pre-existing consumers of your type cannot call Dispose, you cannot be certain that unmanaged resources held by your type will be released.

I can imagine a counter-argument being that this won't break any pre-existing code because not calling Dispose in those cases is the correct thing to do. There are two issues with this. First, this shows that the proposed use of IDisoposable it incorrect because it would not match this specification. Second, it could break pre-existing code because what used to be a no-op (obj as IDisposable)?.Dispose() no longer is and the correctness could depend on that fact.

I do respect the positive outputs while also thinking that they do not out weight the negative outputs in this case. It is for that reason that I oppose this change.

there is no real concept (or at least a broken one) for IDisposable at all

Just because a window is broken doesn't mean it is ok to break the one next to it. It is not wise to throw the baby out with the bath water. Of course the IDisposable interface is not ideal and the concept of ownership is unclear. Nevertheless, we still have a responsibility to our clients, our companies, and the .Net community at large to do the best we can with the IDisposable interface.

Lifetime management has to be done by developer and there are different methods to manage ownership.

There are multiple ways to do anything in life. Some ways are better, some ways are worse, and the usefulness of some ways depends on the situation. In general, the comparison of ways will only be a partial order, not a total one, so some ways are best in some situations but terrible in others. Just because the proposed change would benefit you doesn't mean it would benefit everyone.

I have tried to explain my approach to lifetime management above. Here is some more insight into my understanding that I can share on that topic. It has only been within the last year that I have realized how useful it is, in all aspects of life, to delay decisions. In the past, my personality lead me to try and solve a problem as soon as it arised. I now have a better appreciation for the fact that by delaying a decision, I will often have more information about the problem, which allows me to make a better decision. This delay is _not_ procrastination, which is waiting too long. Instead you should delay (as others have said) "until the last responsible moment".

The connection is that I want to delay choices about lifetime until the last responsible moment. I think we can always responsibly delay the choice to dispose the instance given to the constructor of Option<> to somewhere outside of Option<>. Conceptually, instead of actor.Dispose() also disposing your IObservable<> subscription, it would be more like actor.Dispose(); actorState.Map(x => x.Dispose());.

Robert Martin discusses the benefit that delaying decisions has on your architecture. Here is a blog post that nicely summarizes what I have heard Robert say on the subject. And two exellent quotes from that post are

good architecture is less about the decisions you make and more about the decisions you defer making

and

The purpose of a good architecture is to defer decisions, delay decisions. The job of an architect is not to make decisions, the job of an architect is to build a structure that allows decisions to be delayed as long as possible.

I can imagine a counter-argument to this being that the proposed change allows for the delay of disposing the value inside the Option<> instance because the choice to call Dispose on the Option<> instance can be delayed. The problem though is that this doesn't obey the specification of the IDisposable interface. The intent of this interface is that any instance of a class that implements this interface should have Dispose called on _that_ instance as soon as it is no longer needed, not when some object given in its constructor is no longer needed.

If you give some example to show problems with disposable Option we could evaluate pros and cons.

Is the following specific enough?

Because of this, if the proposed change were accepted, I think the following will inevitably happen. Some developer will create a class that uses an unmanaged resource, and they will have their class implement IDisposable because they were told that this is the proper way to free an unmanaged resource. Locally speaking, this developer is correct to do so. Then they will wrap an instance of their class with language-ext's Option<> because they were told that this is the proper way to model the possibility that their reference doesn't exist. Locally speaking, this developer is correct to do so. Then they will notice that Option<> implements IDisposable and recall that they were told that each disposable instance should be disposed of as soon as possible. And, again, locally speaking, this developer is correct to do so. But now they will have a bug in their code when later they attempt to use their disposable instance and a DisposedObjectException is thrown.

My favorite part about strongly typed languages and functional programming is how it forces the developer to do the right thing. For example, if you want extract the value from Option<int>, you can't just call .Value like you can with C#'s int?. Instead you have to call something like Match or IfNone, which forces the developer to consider the possibility that the value doesn't exist.

My opposition to this change is an attempt to protect this developer. I want the data types in language-ext to force them to do the right thing. The problem with the proposed change is that it doesn't force the developer to do the right thing. It does the opposite by giving them more ways in which to do the wrong thing.

But IMO some arguments are wrong (like mutability)

You probably know better than me. Is there any example of a monad with a non-pure method? I have only learned about and understand a few monads so far, and all of them only have pure methods.

Regarding dependency injection: This is some solution to a specific problem in OOP. But we can change design to avoid this. I don't suggest to drop DI (maybe you should think about it ;-)).

I think we have different understandings of DI.

I would say that Paul is using DI in language-ext when he passes a type parameter T into a method, constrains T to be a struct and to implement some interface with method Foo, calls default(T) to obtain an instance of T, and then calls Foo on that instance.

The implementation of this method "depends" on some method Foo and Paul "injects" that dependency into the method via a type parameter. Here of one example where Paul does this. One doesn't have to get so fancy to do DI in FP though. A more traditional approach is to just pass in a delegate as a parameter, which is what I did in my recent refactor to the parseT family of methods. Now of all the parseT functions that I touched, only this one includes branching. The others only exist to provide an unambiguous function name for each type.

Maybe you equate DI with the use of a DI container (such Simple Injector)? I would agree with you then; it is my current understand that existing DI containers are rather specific to OOP. Even in OOP though, you don't have to use a DI container to do the same injection of dependencies. Instead you can use pure DI.

Another place I like to go to learn more about functional programming the site F# for fun and profit by Scott Wlaschin. He has a nice blog post about what he thinks DI looks like in FP.

IMHO there is still a lot of value with disposable Option

Again, I am _not_ primarily against having a disposable Option. I am primarily against _not_ having a non-disposable Option<>; I am against changing the existing non-disposable Option<> and making it a disposable one.

[StefanBertels]

I hope you accept that I respond on your general points about DI for now -- as long as there is no special issue with IDisposable here. If there is, please just add example here. I'm not interested in breaking code of people using DI. (And yes, thanks for the links, I already tinker with functional solutions for dependency problems like Free Monads...)

Let's go back to this specific issue. I really hope nobody is annoyed by this lengthy discussion but at least we address this from different perspectives and we learn more than just what Option should be.

To address your argument

Implement IDisposable only if you are using unmanaged resources directly

regarding

IDisposable specification

I think IDisposable is used (and can be used) for managed resources, too. If you have unmanaged ressources, you probably need Dispose(bool disposing).
Resharper's way to build Dispose pattern is this one: https://www.jetbrains.com/help/resharper/Code_Generation_Dispose_Pattern.html
(Seems this pattern does not implement Dispose(bool disposing) for unmanaged-only case...)
Anyway my use case here is something that might be constrained (exclusive lock on file, opening exclusive tcp port, making sure only one instance is running ...) but I don't have to deal with low level objects (GC related unmanaged code like win32 calls). Compare e.g. Reactive Extensions (Observable subscriptions).

Summary: IDisposable is a solution for lifetime management for anything that explicitly needs to be disposed.

To get back to specific issue about Option<T> is IDisposable being a good or bad idea: Do you really need to call Dispose() on any disposable object?

https://stackoverflow.com/questions/13459447/do-i-need-to-consider-disposing-of-any-ienumerablet-i-use

(This is related to https://github.com/louthy/echo-process/issues/27 a little bit).

Regarding static analysis. Every line creates some IDisposable object:

            var bla1 = new[] {12, 23, 34}.Select(identity);
            var bla2 = new MemoryStream();
            var bla3 = bla2.GetBuffer().Select(i => (int) i).GetEnumerator();
            var bla4 = new FileStream(@"c:\tmp.tst", FileMode.Open);

Resharper only warns about bla3. Only bla1 has to be casted to IDisposable to call Dispose().

https://stackoverflow.com/questions/101664/can-resharper-be-set-to-warn-if-idisposable-not-handled-correctly

I really would regret this change request (Option implementing IDisposable) if it practically would result in inspection warnings that are active by default in Visual Studio and/or ReSharper. (Yes I care about other analyzers, too, but that's more about appreciation of values.) And I'm sure even though LanguageExt "misuses" some C# features for the good, Paul wouldn't like that, either.

[TysonMN]

I think IDisposable is used (and can be used) for managed resources, too.

Yes, this seems common to me, even to the point that Visual Studio now includes a snippet similar to ReSharper's.

Using IDisposable.Dispose to free managed resources is just an optimization. As such, I don't have a problem with it in general. I do remind people though not to prematurely optimize.

Summary: IDisposable is a solution for lifetime management for anything that explicitly needs to be disposed.

I agree, but I don't see how this follows from your discussion about the dispose pattern.

To get back to specific issue about Option is IDisposable being a good or bad idea: Do you really need to call Dispose() on any disposable object?

~In practice, no, but I would prefer to live in a world where the answer was yes.~

I slightly misunderstood the linked SO question. Just because a type could be cast to IDisposable doesn't mean that it should be. Ideally casting is never needed. If some piece of code doesn't know at compile time that some object's type implements IDisposable, then this is a strong indication that this piece of code should not be responsible for managements of this object's lifetime.

My original response (which now follows) addresses a slightly different question, which is

Do you really need to call Dispose() on any ~disposable~ object _whose type at compile type is known to implement IDisposable_?

In practice, no, but I would prefer to live in a world where the answer was yes. That world is easier to understand. Instead, I think we live in this more complicated world because IDisposable is misused, including by Microsoft by having Stream implement it even though this class doesn't use any unmanaged resources. I don't think I am the first one to say that Microsoft has done something wrong. ;)

So, in practice, no, but I try to make the answer "yes" for applications in which I am involved. I believe that this makes it easier to write correct code.

@StefanBertels, what do you think about this code? Do you think it should compile? If not, why not? If so, what do you think should be the state of the object referenced by the variable myOptionAsyncDisposable?

Option<IDisposable> myOptionDisposable = new System.Reactive.Disposables.CompositeDisposable();
OptionAsync<IDisposable> myOptionAsyncDisposable = myOptionDisposable;

[StefanBertels]

Using IDisposable.Dispose to free managed resources is just an optimization

I disagree. Objects might be "active" (pushing messages) and you need to stop/unsubscribe (e.g. observables).

I slightly misunderstood the linked SO question. Just because a type could be cast to IDisposable doesn't mean that it should be. Ideally casting is never needed. If some piece of code doesn't know at compile time that some object's type implements IDisposable, then this is a strong indication that this piece of code should not be responsible for managements of this object's lifetime.

Doesn't that contradict what you suggested in https://github.com/louthy/echo-process/issues/27?

IMO this is an appreciation of values (as this Option.Dispose() request): Is it better to have the ref your in your code IDisposable or not when it (or something it contains...) might have to be disposed.

@louthy uses the is IDisposable check for state in echo -- probably because this is the only way making this work without limiting state ref type to implement IDisposable.

Stream is similar to some Observable subscription: Maybe there is a need to Dispose (FileStream), maybe GC is enough. You don't know in general by the ref type you use in your code.

For Option: I really think it is clear that you don't have to call Dispose on this (or use using) just if you create or copy some option (passing the inner value around). It's IMO quite obvious that Dispose() always targets the inner value. If your inner object is IDisposable you have to care of it's lifetime and Option.Dispose() is just a proxy call.

It's nice if some technical inspection could verify correctness, so I agree to your "preferred world" in some way. But it's nicer if code is easy to read and e.g. making Option available for using would help there. And finally it's unclear what analyzers can do today or in future and IDisposable concept is unsatisfactory in some ways we cannot fix. I currently don't see that we would make that worse in reality.

@louthy: Just a thought -- would it make things better (or worse) to allow use to be called on types that are not constrained to be IDisposable and check for is IDisposable on runtime? Might be some mitigation...

@bender2k14 Regarding your last question: it does not compile for the same reason

            Option<int> myOption = 5;
            OptionAsync<int> myOptionAsync = myOption;

does not compile. I don't see your point. Despite casting error: no line in your code disposes the disposable, regardless of the change this issue requests. So there might be leak but luckily your CompositeDisposable is empty .... ;-)

What do _you_ think should be the state here?

[TysonMN]

I disagree. Objects might be "active" (pushing messages) and you need to stop/unsubscribe (e.g. observables).

I don't understand. An observable subscription is not a managed resource. That is why it implements IDisposable.

Doesn't that contradict what you suggested in louthy/echo-process#27?

I don't think so. Did you have something specific in mind?

IMO this is an appreciation of values (as this Option.Dispose() request): Is it better to have the ref your in your code IDisposable or not when it (or something it contains...) might have to be disposed.

I am having a hard time understanding what you are saying here. Could you rephrase it?

..ref your in your code IDisposable...
...ref type to implement IDisposable...
...ref type you use in your code...

By "ref [type]", you mean compile-time type, right?

Regarding your last question: it does not compile for the same reason...does not compile.

I didn't ask you if it compiles. I know it doesn't compile. I asked you if you _think_ it should compile. I asked you for your opinion. I want to know what you think. I am trying to better understand your perspective.

What do you think should be the state here?

I already know if I want that code to compile (and if not, why, and if so, what the state is). I will tell you, but I don't want to bias you my telling you what I think first.