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 using statement to structurally match IDisposable on ref structs (16.3, Core 3) #1623

Open
5 tasks done
Tracked by #9
agocke opened this issue Jun 12, 2018 · 39 comments
Open
5 tasks done
Tracked by #9
Assignees
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Milestone

Comments

@agocke
Copy link
Member

agocke commented Jun 12, 2018

Currently the language states that any type used in an using statement must have an implicit conversion to IDisposable. This proposal is allow any type which structurally matches IDisposable (has a public void-returning non-generic instance method taking no arguments) to be used in a using statement.

The semantics of this construct are the same as a regular using, except that if the type does not have an implicit conversion to IDisposable, there is a check to see if it structurally matches IDisposable. If so, instead of the IDisposable.Dispose call, there is a call to the structural Dispose method.

At the moment, extension methods are not considered, like foreach statements. If we were to add extension method support to GetEnumerator for foreach, I propose we also add it for using.

[jcouv update:] This feature was restricted to ref structs due to backwards compatibility reasons. Also, the part about considering extension methods was split out of this feature, and it was not implemented in dev16.0 milestone.

Full proposal in https://github.com/dotnet/csharplang/blob/master/proposals/csharp-8.0/using.md

LDM history:

@agocke
Copy link
Member Author

agocke commented Jun 12, 2018

cc @gafter @jaredpar @fayrose

@CyrusNajmabadi
Copy link
Member

This proposal is allow any type which structurally matches IDisposable (has a public void-returning non-generic instance method taking no arguments) to be used in a using statement.

I would also like it if we could allow an appropriately matching extension method. It seems reasonable to be able to embueue a type you don't own with RAII semantics if that makes sense for your domain.

Just my 2c :)

@gafter
Copy link
Member

gafter commented Jun 12, 2018

I like it, but I would suggest that perhaps we should consider extension methods right from the start (no matter what, if anything, we do about foreach).

@CyrusNajmabadi
Copy link
Member

Ha! Beat you @gafter :)

@gafter
Copy link
Member

gafter commented Jun 12, 2018

Not by much; we both submitted at 3:47.

@HaloFour
Copy link
Contributor

Dupe of #93 😉

@gafter
Copy link
Member

gafter commented Jun 12, 2018

I think we have too many special rules inherited from the days before we had extension methods, params, etc. I think we should just do ordinary overload resolution, and permit any method that can be called with the arguments to be used. Accessibility checked as usual, not requiring public. Allow extensions. In general, treat it just like any method call that happens to have zero arguments. We would need LDM buy-in for it.

@HaloFour
Copy link
Contributor

@agocke

The semantics of this construct are the same as a regular using, except that if the type does have an implicit conversion to IDisposable, there is a check to see if it structurally matches IDisposable. If so, instead of the IDisposable.Dispose call, there is a call to the structural Dispose method.

That should read:

"except that if the type not does have an implicit conversion to IDisposable"

Correct? The compiler should only apply the structural typing if the type isn't IDisposable in order to not change the behavior of existing code, right?

@yaakov-h
Copy link
Member

I think so.

Does "structually matches" also include extension methods, or do we need to explicitly mention that?

@Korporal
Copy link

Just my thoughts but the existing dispose pattern(s) are already pretty bewildering for many developers and this proposal effectively makes this even more abstruse. There are several scenarios to be considered when incorporating dispose support like "does the class inherit from a base class that already has dispose" or "do I actually 'own' the field that I want to ultimately dispose" and so on.

Perhaps the entire mechanics of dispose is ripe for a review and a simpler alternative mechanism be introduced...

@agocke
Copy link
Member Author

agocke commented Jun 13, 2018

@HaloFour Correct.

@andre-ss6
Copy link

Just a question: what would be the benefit of doing this if extensions methods were to be ignored? I'm curious.

@jnm2
Copy link
Contributor

jnm2 commented Jun 17, 2018

@andre-ss6 Besides consistency with the rest of the language, it would also be a stepping stone to enable using await to await a Dispose method that returns a custom awaitable type.

@jaredpar
Copy link
Member

@andre-ss6

Just a question: what would be the benefit of doing this if extensions methods were to be ignored? I'm curious.

One concrete benefit: allows ref struct to participate in using statements. There is no way for them to do this today as they can't implement interfaces by design.

@rvhuang
Copy link

rvhuang commented Jul 8, 2018

Asking a stupid question: if this proposal comes true, what is the point of implementing IDisposable then?

@yaakov-h
Copy link
Member

yaakov-h commented Jul 8, 2018

So that third-party code can safely dispose of your object... at least until shapes and SDisposable comes along.

@markrendle
Copy link

What if there are two extension classes in scope that both provide public static void Dispose(this Thing thing)? Currently when you get extension method clash, you get the Call is ambiguous... error; how would that be surfaced where the extension method invocation is masked by the using syntax?

@jnm2
Copy link
Contributor

jnm2 commented Jul 9, 2018

@markrendle The same thing that happens with conflicting GetAwaiter() extension methods and await, I suppose?

In particular: sharplab

@HaloFour
Copy link
Contributor

HaloFour commented Jul 9, 2018

@markrendle

What if there are two extension classes in scope that both provide public static void Dispose(this Thing thing)? Currently when you get extension method clash, you get the Call is ambiguous... error; how would that be surfaced where the extension method invocation is masked by the using syntax?

This same situation can arise with await which may use extension methods to resolve GetAwaiter and the result is that same compilation error due to calls being ambiguous.

@scalablecory
Copy link

Correct? The compiler should only apply the structural typing if the type isn't IDisposable in order to not change the behavior of existing code, right?

I'd be curious to see what code would be affected by this that wasn't already buggy. Sounds like a big code smell.

@markrendle
Copy link

@jnm2 @HaloFour Right, yes, I forgot that await was structural already.

@LYP951018
Copy link

We just need Shapes.

@andre-ss6
Copy link

And Higher Kinded Polymorphism. Just imagine what a beautiful world it would be where c# supports HKTs 😝

@jnm2
Copy link
Contributor

jnm2 commented Dec 21, 2018

Now that await using has advanced, how soon until the structural matching allows await using on any type with a public or interface-implemented DisposeAsync() method with an awaitable return type with a void result? (I.e., not just Task.)

@jcouv jcouv changed the title Proposal: Allow using statement to structurally match IDisposable Proposal: Allow using statement to structurally match IDisposable on ref structs Jan 4, 2019
@bartdesmet
Copy link

Also, given the following extension method

public static ConfiguredCancelableAsyncEnumerable<T>.Enumerator ConfigureAwait<T>(this IAsyncEnumerator<T> enumerator, bool continueOnCapturedContext)

using the ConfiguredCancelableAsyncEnumerable<T>.Enumerator type that has shipped recently in .NET Core 3.0 Preview 2, to aid in writing code like

await using (var e = source.GetAsyncEnumerator(cancellationToken).ConfigureAwait(false))
{
    // ...
}

when await foreach can't be used (e.g. because the logic requires direct control over MoveNextAsync calls, as is the case for many LINQ operators and aggregates), currently results in error

error CS8410: 'ConfiguredCancelableAsyncEnumerable<TSource>.Enumerator': type used in an async using statement must be implicitly convertible to 'System.IAsyncDisposable'

It'd be a pity to have to resort to try...finally... here in order to apply ConfigureAwait for purposes of "configured disposal".

For this to work as intended, await using would need to support binding to any DisposeAsync method, using an empty argument list (possibly filling in optional parameters or empty params arrays?), whose return type is awaitable. That way, we can achieve the intended effect of awaiting on the "configured" awaitable.

Also tagging @stephentoub given this discussion refers to the corefx types introduced for configuring async enumerables.

@stephentoub
Copy link
Member

await using would need to support binding to any DisposeAsync method

Isn't that the plan now / already implemented @jcouv?

@jcouv
Copy link
Member

jcouv commented Feb 14, 2019

@bartdesmet Yes, we've arrived at that same conclusion (when @stephentoub implemented methods like ConfigureAwait).
Pattern-based disposal should work already (as of dev16 preview3). Both using and foreach (as well as await using and await foreach) will bind a .Dispose() (or .DisposeAsync()) call with no arguments. This recognizes methods with params or optional parameters.
The only caveat is that extension methods will no contribute at the moment (ie. dev16 preview4). This may be revised by the time C# 8.0 ships.
Let me know if you run into any unexpected behavior.

For reference (if you want to look at some test cases), here's the PR for doing that in async constructs.

@bartdesmet
Copy link

Thanks, this works now as expected, and I've converted the LINQ code to use await using (with a "configured" enumerator) where necessary.

@gafter gafter added this to 8.0 Candidate in Language Version Planning Mar 6, 2019
@gafter gafter moved this from 8.0 Candidate to 8.0 Candidate (part 2) in Language Version Planning Mar 6, 2019
@gafter gafter moved this from 8.0 Candidate (part 2) to 8.0 Candidate in Language Version Planning Mar 6, 2019
@jcouv jcouv changed the title Proposal: Allow using statement to structurally match IDisposable on ref structs Proposal: Allow using statement to structurally match IDisposable on ref structs (16.3, Core 3) Jul 18, 2019
@IanKemp
Copy link

IanKemp commented Aug 6, 2019

So, is this done and dusted? If so, can it be closed?

@agocke
Copy link
Member Author

agocke commented Aug 6, 2019

C# 8 hasn't shipped yet, so this should remain open until it has

@alrz
Copy link
Member

alrz commented Oct 8, 2019

At the moment, extension methods are not considered, like foreach statements. If we were to add extension method support to GetEnumerator for foreach, I propose we also add it for using.

Is the "extension-based foreach" rejected indefinitely or it's just cut from this release?

@Eli-Black-Work
Copy link

Can this be closed, now that C# 8 has shipped?

@333fred
Copy link
Member

333fred commented Jul 6, 2020

No, we keep issues open until a spec has been merged.

@andre-ss6
Copy link

But hasn't it?

@yaakov-h
Copy link
Member

yaakov-h commented Jul 7, 2020

No. That adds a proposal (proposals/ subfolder), but the spec itself (spec/ subfolder) has not been updated to incorporate these (or any recent) changes.

@333fred 333fred added the Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification label Oct 16, 2020
@333fred 333fred removed this from C# 8.0 in Language Version Planning Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Projects
None yet
Development

No branches or pull requests