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

[Proposal]: Allow Dispose by Convention #11420

Closed
HaloFour opened this issue May 19, 2016 · 15 comments
Closed

[Proposal]: Allow Dispose by Convention #11420

HaloFour opened this issue May 19, 2016 · 15 comments

Comments

@HaloFour
Copy link

Currently the using statement is tied specifically to implementation of the IDisposable interface. This makes it impossible to use resource-like objects from other assemblies within using statements.

I propose that the compiler allow resolving an accessible void Dispose() method in the case that the type does not implement IDisposable. That would allow for a developer to extend an existing class through the use of an extension method which would contain the logic for "disposing" of that resource:

public class SomeResource {
    public int Id { get; }
}

public static class SomeResourceExtensions {
    public static void Dispose(this SomeResource resource) {
        Console.WriteLine($"Disposing {resource.Id}");
    }
}

static class Program {
    static void Main() {
        using (var resource = new SomeResource() { Id = 123 }) {
            Console.WriteLine($"Using ${resource.Id}");
        }
    }
}

// output:
// Using 123
// Disposing 123

This brings using in line with the conventions also established by foreach, LINQ and await where the compiler can resort to resolving instance members of a specific name/shape rather than require implementation of an interface or base class.

@alrz
Copy link
Contributor

alrz commented May 19, 2016

How is this any better than defer #8115?

var resource = new SomeResource() { Id = 123 };
defer Console.WriteLine($"Using ${resource.Id}");

I think I'd prefer an assignment even when we have an actual IDisposable via RAII (#185),

using resource = new SomeResource() { Id = 123 };

@HaloFour
Copy link
Author

HaloFour commented May 19, 2016

@alrz

It builds onto the existing C# pattern for resource disposal rather than adding completely new syntax that just happens to have the primary use case of resource disposal. It also brings using in line with how the C# compiler treats many similar features like foreach, LINQ and await.

I won't argue that it won't be "abused" because it certainly will be, just as using is today through helper methods like Disposable.Create(), but I do believe that using will help to keep the conversation on the context of resources rather than arbitrary actions that happen to occur when the scope exists like defer.

@HaloFour
Copy link
Author

@alrz

Also, with defer you have to include the adaptation code in-situ which could lead to a lot of repetition, particularly if the "disposal" is more complicated than a quick one-liner. You could write a helper method but if you're going to that length you might as well write an extension method and have using do the work for you.

The defer statement does have one advantage in that it doesn't impact the scope/indentation, but I think that #181 solves that issue in this case as well as with normal using statements:

static class Program {
    static void Main() {
        using var resource = new SomeResource() { Id = 123 };
        Console.WriteLine($"Using ${resource.Id}");
    }
}

@alrz
Copy link
Contributor

alrz commented May 19, 2016

@HaloFour The good thing about defer statement compared to an extension Dispose method is that the actual code is visible at the call-site, so it can't be really "abused" in any way except for flow control which can't be prevented overall. On the other hand, an extention Dispose method is literally invisible in a using statement so you'd have no idea that what happens when you get out of the using statement. Also, unlike defer, this needs a whole class and a method to satisfy said "patterns". The point of being able to "dispose" a non-disposable object is to run custom logic not just being able to use it in a using statement. Other patterns like Deconstruct, GetAwaitable etc have their own definition and they don't imply running custom code. The only positive aspect is reusability, which again, due the invisibility of the call, I don't think that it would overcome disadvantages.

@HaloFour
Copy link
Author

@alrz

he good thing about defer statement compared to an extension Dispose method is that the actual code is visible at the call-site, so it can't be really "abused" in any way except for flow control which can't be prevented overall.

Which is great until you need to refactor it or someone fails a copypasta. I like adapting the disposal in one place.

On the other hand, an extention Dispose method is literally invisible in a using statement so you'd have no idea that what happens when you get out of the using statement.

Same with an extension GetEnumerator, GetAwaiter or any of the dozen or so LINQ methods.

The point of being able to dispose a non-disposable object is to run custom logic not just being able to use it in a using statement.

But you want to dispose it, so why not use what has been established as the appropriate C# pattern for disposing it rather than invent completely new syntax for these special cases?

Other patterns like Deconstruct, GetAwaitable etc have their own definition and they don't imply running custom code.

Considering that you can define any of the above as extension methods, yes, they absolutely imply running custom code. That's the entire point of being able to write them as extension methods. You can adapt literally any type to an awaitable through custom code.

Frankly, I despise defer. It creates duplicity in the language given that it's primary use case is resource disposal, and it creates a pit of failure by encouraging the very anti-patterns people claim are abuse of using today. If the main purpose of defer is to adapt resource disposal to circumstances in which using cannot be used then why not fix using?

@alrz
Copy link
Contributor

alrz commented May 19, 2016

@HaloFour

Same with an extension GetEnumerator, GetAwaiter or any of the dozen or so LINQ methods.

You don't expect any of these to do anything other than what they are meant to be used for.

If the main purpose of defer is to adapt resource disposal to circumstances in which using cannot be used

I think defer would be used for "deferring" a block of code and it doesn't have to be resource disposal. In fact, it might not even have a relationship to a particular object. So it is a new construct and is not necessarily a replacement for using. I'm trying to say that defer and Dispose should not be used interchangeably.

@HaloFour
Copy link
Author

@alrz

I also think that even if defer is implemented that this change is still valuable. It fits with the existing conventions of the language and makes it simple to adapt existing classes to resources. It does reduce much of the primary use cases of defer but that keyword might still have its uses with more complicated resource cleanup scenarios such as having to include argumentsome beyond just the target resource or having to call some instance method on some other instance, like a pool.

@dsaf
Copy link

dsaf commented May 19, 2016

@HaloFour
Copy link
Author

HaloFour commented May 19, 2016

@alrz

You don't expect any of these to do anything other than what they are meant to be used for.

Nor do I expect that in this case. The primary use case is certainly to adapt non-IDisposable resources to be disposable. The fact that some people might take the opportunity to use it for other purposes doesn't invalidate that primary use case. They're already doing it with IDisposable. You could argue that Task.Yield() is also an example of using a convention to abuse the await language feature since you're not awaiting the completion of a task, but there it is in the BCL.

I think defer would be used for "deferring" a block of code and it doesn't have to be resource disposal.

Indeed, but other than the contrived examples of writing console messages in reverse order which is all it seems that Swift and other documentation can provide, what are the use cases and are they so common that they justify a new language feature? I personally doubt it. The prototypical use case of defer in all of those languages is to do exactly what using already does.

I really don't want this to devolve into a using v. defer debate. I believe that this proposal stands on its own regardless of whether or not defer is implemented.

@dsaf

It's interesting but while I respect @jskeet it's not really authoritative. 😀

That post does mention another place where conventions are used over explicit implementations, collection initializers.

@alrz
Copy link
Contributor

alrz commented May 19, 2016

out ot curiosity, "The primary use case is certainly to adapt non-IDisposable resources to be disposable." why they didn't implement IDisposable in the first place? If the reason is to avoid an interface call (which turns to a double invocation with the extension method) or give it a name other than Dispose, well, the extension method wouldn't help with that.

PS: VB supports both of these without losing Using.

Class C : Implements IDisposable
    Public Sub Close() Implements IDisposable.Dispose
    End Sub
End Class

@HaloFour
Copy link
Author

@alrz

why they didn't implement IDisposable in the first place?

Usually because you have to use a third-party closed-source library where someone not familiar with .NET messed up. At least those are the most common cases where I've run into this.

If the reason is to avoid an interface call (which turns to a double invocation with the extension method)

Depending on how that resource is disposed, yes, you are adding another static method call into the equation. If that static method invocation just wraps an instance method invocation then the JIT would definitely inline it away. I doubt performance is generally a consideration here, though.

or give it a name other than Dispose, well, the extension method wouldn't help with that.

Actually that's probably the most common scenario. In my experience I've seen multiple examples of a class representing a resource that should be disposed that did not implement IDisposable but provided a separate instance method like Destroy or Close (and yes, I recognize that Close isn't always the same thing). As such the types could not be used with using. An extension Dispose method to serve as an adapter would allow it to be.

@dsaf
Copy link

dsaf commented May 20, 2016

@HaloFour

That post does mention another place where conventions are used over explicit implementations, collection initializers.

Yes, I linked it because of this. If the rule was violated once, surely it can be violated twice :).

why they didn't implement IDisposable in the first place?

Usually because you have to use a third-party closed-source library where someone not familiar with .NET messed up. At least those are the most common cases where I've run into this.

This is rare enough for you to just implement and use a custom hack:

public static void Using<T>(Func<T> factory, Action<T> action) where T : class
{
    T instance = null;
    try
    {
        instance = factory();
        action(instance);
    }
    finally
    {
        if (instance != null)
        {
            //TODO: invoke Dispose via reflection.
        }
    }
}

@HaloFour
Copy link
Author

@dsaf

This is rare enough for you to just implement and use a custom hack:

Indeed, it's not that difficult of a problem to work around. But it is annoying. And given that it's seemingly the odd case where the language absolutely requires implementation of an interface over convention to support a language feature I thought it was worth a mention.

The two other places (that I can think of) where the language does require an interface implementation is INotifyCompletion or ICriticalNotifyCompletion for the OnCompleted method on awaiters (every other member in by convention), and the requirement of IEnumerable<T> for collection initializers. There was a proposal to remove the latter on CodePlex, which @jskeet relied would be a welcome change. 😀 There are probably other cases that I can't think of at the moment.

This seems like a big enough of a deal to have warranted the #8115 proposal, over which I will make no bones about expressing my disapproval. I think that this proposal, combined with #115, fully eliminates the resource cleanup use cases for defer.

@dsaf
Copy link

dsaf commented May 20, 2016

@HaloFour Well, I definitely dislike the defer thing.

@gafter
Copy link
Member

gafter commented Sep 25, 2017

Moved to dotnet/csharplang#93

@gafter gafter closed this as completed Sep 25, 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

5 participants