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

[C# feature request] Add IDisposable adapter for any type for using statement. #10361

Closed
bradphelan opened this issue Apr 6, 2016 · 15 comments
Closed

Comments

@bradphelan
Copy link

Currently it is painful to adapt classes to the using statement. If I have a class

public class MyFile {
    public string Path { get; };
    public void Close();
}

I can't use it in a using statement because it isn't IDisposable but the Close method is probably the one I am interested in. Normally I would do

var file = new MyFile("c:\temp\foo.txt");
using(Disposable.Create(()=>file.Close()){
    // Do stuff with file
}

There are two problems above. file is scoped too wide. I would have to add another layer of noisy brackets. The other is the noisy Disposable.Create. My proposal is that for objects that do not implement IDisposable we add an overload for using.

using(var file = new MyFile("c:\temp\foo.txt"), ()=> file.Close()){
    // Do stuff with file
}

or just pass the method group

using(var file = new MyFile("c:\temp\foo.txt"), file.Close){
    // Do stuff with file
}
@bradphelan bradphelan changed the title Add IDisposable adapter for any type for using statement. [C# feature request] Add IDisposable adapter for any type for using statement. Apr 6, 2016
@svick
Copy link
Contributor

svick commented Apr 6, 2016

This doesn't look very useful to me, since types like your MyFile should be very rare. Or do you have some specific situation where they are common?

@GeirGrusom
Copy link

You save no characters typing this out, so it seems like a performance optimization?

Also related issue #8115

@DavidArno
Copy link

Like @svick, I'm sceptical of the real need for this. How often do you really encounter such classes without being able to access the source and thus fix them by making them implement IDisposable?

Having said that, if there is a real-world case for having to do what you describe, then your proposal seems a neat way of dealing with it by both controlling the scope of var and by making the syntax less noisy.

@matthid
Copy link

matthid commented Apr 9, 2016

There are already some language features that help:

public class MyFile
{
    public string Path => "Path";
    public void Close() => Console.WriteLine("closed");
}
public class Disposable<T> : IDisposable
{
    public T Item { get; }
    private readonly Action<T> cleanup;
    public Disposable(T item, Action<T> cleanup)
    {
        this.Item = item;
        this.cleanup = cleanup;
    }
    public void Dispose()
     => this.cleanup(this.Item);
}
public static class Disposable
{
    public static Disposable<T> Create<T>(T item, Action<T> cleanup)
      => new Disposable<T>(item, cleanup);
}
using (var d = Disposable.Create(new MyFile(), f => f.Close())) {
    Console.WriteLine($"Path: {d.Item.Path}");
}

using static Disposable;
using (var d = Create(new MyFile(), f => f.Close())) {
    Console.WriteLine($"Path: {d.Item.Path}");
}

IMHO there is no need to bake that into the language. Of course you can come up with a better name than Create but both of your problems seem to be solved with this approach?

@GSPP
Copy link

GSPP commented Apr 10, 2016

Maybe using could make use of extension methods called Dispose.

Otherwise I'm voting down because this is super rare in my experience. The existence of a Close method is often an anti-pattern anyway. Almost all the Close methods in the BCL are API design errors from the .NET 1.0 days. There's a story somewhere on the web on why they even exist.

It also leads to people writing

using (x) { x.Close(); x.Dispose(); }

@bradphelan
Copy link
Author

This gives me an idea. The 'using' statement is too rigidly coupled to the
'IDisposable' interface. Let's create an interface that describes exactly
what using does. It introduces a variable into scope and it disposes
something. No reason that the thing disposed and the thing introduced into
scope are the same object. So ...

interface IUsing : IDisposable {
T Item { get; }
}

And a helper class

public class Using {
public T Item {get;}
private readonly IDisposable Cleanup;
public Using(T item, IDisposable Cleanup){
Item = item;
this.Cleanup = cleanup;
}
public void Dispose() => Cleanup();
}

public static class Using {
public static IUsing MakeUsable( T item, IDisposable Cleanup) =>
new Using(item,cleanup);
public static IUsing MakeUsable ( T item, Action Cleanup) =>
MakeUsable(item, Disposable.Create(Cleanup);
public static IUsing MakeUsable (T item, Action Cleanup) =>
MakeUsable(item, () => Cleanup(item));

}

and a simple if naive use case

using static Using;

using (var file = MakeUsable(new MyFile(), f => f.Close()){
Console.WriteLine($"Path: {f.Path}");
}

it would be more useful to be done like

IUsing OpenFile(string path) =>
MakeUsable(new MyFile(), f=>f.Close());

and

using (var myFile = OpenFile("c:\passwords")){
Console.WriteLine($"Path: {f.Path}");
}

And for those that say this is an anti pattern. Often with windowing
toolkits you can show a windows. You can hide it or close it. Ie there are
two methods, Hide() and Close(). Which one of those should be the
disposable one. Depends on what you are trying to do.

Now this is very similar to what you suggested but the using statement
would be made aware of the interface *IItemInterface *

On Sat, Apr 9, 2016 at 4:17 PM Matthias Dittrich notifications@github.com
wrote:

There are already some language features that help:

public class MyFile
{
public string Path => "Path";
public void Close() => Console.WriteLine("closed");
}public class Disposable : IDisposable
{
public T Item { get; }
private readonly Action cleanup;
public Disposable(T item, Action cleanup)
{
this.Item = item;
this.cleanup = cleanup;
}
public void Dispose()
=> this.cleanup(this.Item);
}public static class Disposable
{
public static Disposable Create(T item, Action cleanup)
=> new Disposable(item, cleanup);
}using (var d = Disposable.Create(new MyFile(), f => f.Close())) {
Console.WriteLine($"Path: {d.Item.Path}");
}
using static Disposable;using (var d = Create(new MyFile(), f => f.Close())) {
Console.WriteLine($"Path: {d.Item.Path}");
}

IMHO there is no need to bake that into the language. Of course you can
come up with a better name than Create but both of your problems seem to
be solved with this approach?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#10361 (comment)

@GSPP
Copy link

GSPP commented Apr 10, 2016

This windowing Close method is not about disposal. It is appropriately named. Close in order to dispose of resources as a synonym for Dispose is the anti-pattern.

Also whether it's Close or Dispose does not matter much. It should only be one of them, though.

@bradphelan
Copy link
Author

What is a "resource" ?

On Sun, 10 Apr 2016 19:17 GSPP, notifications@github.com wrote:

This windowing Close method is not about disposal. It is appropriately
named. Close in order to dispose of resources as a synonym for Dispose is
the anti-pattern.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#10361 (comment)

@GSPP
Copy link

GSPP commented Apr 10, 2016

Something that is a significant cost to the system while it is not disposed. When disposed it permanently transitions to a destroyed state and stops being a cost. I think this is what everyone intuitively understands to be a resource.

A window that can be reopened is not a resource. It's a two state system that can be transitioned arbitrarily.

Destroying a window is a permanent relief of resources. That's disposal.

@bradphelan
Copy link
Author

No. A "resource" at least as far as the "using" concept is concerned is just a simple state machine for a common pattern. The pattern is that the code performs action "A", enters a scope and must execute action "B" when it exits the scope to maintain correctness when it exits. When it enters the scope some data is provided that is only valid within that scope. Within that definition you are free to do what you want. There is no requirement that anything must occupy "significant" resources. What does significant mean anyway?

That it automatically respects the IDisposable interface is an implementation detail. There is no requirement that the "using" statement cannot infer the same start -> execute_with_data ->stop pattern from other contextual information.

I don't think TransactionScope fits your definition of resource. It is more about correctness rather than disposing of something taking up significant resources.

@GeirGrusom
Copy link

I don't think TransactionScope fits your definition of resource

TransactionScope's resource is the transaction it's working with which must be committed when the scope exits.

@bradphelan
Copy link
Author

My point exactly. It has nothing to do with system resources or memory or
stuff that takes up "significant" something or other. Deciding to use
"Using" is about maintaining program correctness not about deciding if some
concept of resource meets an abitrary level of significant or not.

On Mon, 11 Apr 2016 07:52 Henning Moe, notifications@github.com wrote:

I don't think TransactionScope fits your definition of resource

TransactionScope's resource is the transaction it's working with which
must be committed when the scope exits.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#10361 (comment)

@DavidArno
Copy link

@GSPP,

This windowing Close method is not about disposal. It is appropriately named. Close in order to dispose of resources as a synonym for Dispose is the anti-pattern.

This is circular reasoning. using requires a IDisposable, not a IPostUsingActionable, therefore if one wishes to eg use using to simplify the code around opening and closing a window, one it is only an anti-pattern because of the design of using. So if there's an anti-pattern in there, it's using itself.

@bradphelan,
I'm still unconvinced of the need to change the behaviour of using though. Your Using<T> solution solves it without a compiler change. Why not package that code up as a NuGet package for all to use and accept it as "job done"?

@GSPP
Copy link

GSPP commented Apr 11, 2016

I think I misunderstood a bit what was being talked about, and I was misunderstood as well. It does not matter, moving on.

I think the issues brought up in this ticket are totally valid but so far no solution is known that should actually make it into the language, IMHO.

If you really care about scoping you can apply this solution (which I would not do because it decreases code quality I think):

{
 var f = new File();
 using (Disp.Create(_ => f.Close())) {
 }
}

Can we add a Disposable.Create helper to the framework? I have always been missing that and needed to write it myself. I think that gets us 50% of the benefits already at minimal cost.

@HaloFour
Copy link

@GSPP

Rx has some great Disposable helpers, including Disposable.Create().

@bradphelan

I proposed #11420 which would allow for the C# compiler to consider instance methods if the target type does not implement IDisposable. This would allow for you to write an extension method called Dispose for that type which would adapt the type to the disposable pattern:

public class MyFile {
    public string Path { get; };
    public void Close();
}

public static class MyFileExtensions {
    public static void Dispose(this MyFile file) {
        file.Close();
    }
}

using(var file = new MyFile("c:\temp\foo.txt")){
    // Do stuff with file
}

@gafter
Copy link
Member

gafter commented Mar 24, 2017

We are now taking language feature discussion in other repositories:

Features that are under active design or development, or which are "championed" by someone on the language design team, have already been moved either as issues or as checked-in design documents. For example, the proposal in this repo "Proposal: Partial interface implementation a.k.a. Traits" (issue 16139 and a few other issues that request the same thing) are now tracked by the language team at issue 52 in https://github.com/dotnet/csharplang/issues, and there is a draft spec at https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md and further discussion at issue 288 in https://github.com/dotnet/csharplang/issues. Prototyping of the compiler portion of language features is still tracked here; see, for example, https://github.com/dotnet/roslyn/tree/features/DefaultInterfaceImplementation and issue 17952.

In order to facilitate that transition, we have started closing language design discussions from the roslyn repo with a note briefly explaining why. When we are aware of an existing discussion for the feature already in the new repo, we are adding a link to that. But we're not adding new issues to the new repos for existing discussions in this repo that the language design team does not currently envision taking on. Our intent is to eventually close the language design issues in the Roslyn repo and encourage discussion in one of the new repos instead.

Our intent is not to shut down discussion on language design - you can still continue discussion on the closed issues if you want - but rather we would like to encourage people to move discussion to where we are more likely to be paying attention (the new repo), or to abandon discussions that are no longer of interest to you.

If you happen to notice that one of the closed issues has a relevant issue in the new repo, and we have not added a link to the new issue, we would appreciate you providing a link from the old to the new discussion. That way people who are still interested in the discussion can start paying attention to the new issue.

Also, we'd welcome any ideas you might have on how we could better manage the transition. Comments and discussion about closing and/or moving issues should be directed to #18002. Comments and discussion about this issue can take place here or on an issue in the relevant repo.


I am not moving this particular issue because I don't have confidence that the LDM would likely consider doing this.

@gafter gafter closed this as completed Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants