fsharp / fslang-suggestions

The place to make suggestions, discuss and vote on F# language and core library features
345 stars 21 forks source link

Support disposable pattern for `ref` structs #1229

Open auduchinok opened 1 year ago

auduchinok commented 1 year ago

I propose we support consuming disposable pattern for ref structs:

You can define a disposable ref struct. To do that, ensure that a ref struct fits the disposable pattern. That is, it has an instance or extension Dispose method, which is accessible, parameterless and has a void return type.

In recent C# versions this pattern can be used instead of implementing IDisposable in a struct type. If such a change to a struct type is done in a C# project/library, then this type can't be used in a use binding in F# anymore, i.e this code:

use x = Library.Method()
...

has to be rewritten into this:

let x = Library.Method()
...
x.Dispose()

or preferably even this:

let x = Library.Method()
try
    ...
finally
    x.Dispose()

The type change doesn't need any changes in referencing C# projects, so library authors may break usages in F# code without realising it.

Pros and Cons

The advantages of making this adjustment to F# are

The disadvantages of making this adjustment to F# are

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S-M

Affidavit (please submit!)

Please tick this by placing a cross in the box:

Please tick all that apply:

For Readers

If you would like to see this issue implemented, please click the :+1: emoji on this issue. These counts are used to generally order the suggestions by engagement.

auduchinok commented 1 year ago

Just in case, this was what actually happened to us when a referenced C# library had changed a value type like described above, and that broke our F# code while C# code had no issues at all. The required fixes were just like described: https://github.com/JetBrains/resharper-fsharp/commit/705cc5015a7bddf83c5e48b2f4cfa56fc06e9543, https://github.com/JetBrains/resharper-fsharp/commit/f63b5360e617875138d2bb7b47f8b9be909ecfdb

The main issue is such changes are not seen as breaking from C# side.

Xyncgas commented 1 year ago

what if we just automatically call dispose when these value go out of scope. Unlike objects they don't live longer than where they are

vzarytovskii commented 1 year ago

what if we just automatically call dispose when these value go out of scope. Unlike objects they don't live longer than where they are

It is too implicit.

edgarfgp commented 6 months ago

@dsyme, @vzarytovskii Any thoughts on this suggestion ?

Xyncgas commented 6 months ago

It is too implicit

ref struct is going to be expiring where they are created, it's a feature people chose to use, so maybe it wouldn't be too implicit but rather natural to see ref struct's dispose being called (especially for people who came from C/C++)?

vzarytovskii commented 6 months ago

@dsyme, @vzarytovskii Any thoughts on this suggestion ?

It feels a bit weird tbh, it's a very ad-hoc solution from the C# side, ducktyped dispose, but only for ref structs.

Personally I think that we either should fully support it on any type (like we do with fixed) or wait until ref structs support interfaces (https://github.com/dotnet/csharplang/blob/main/proposals/ref-struct-interfaces.md).

I would personally vote for the latter, since it makes more sense from the design of the IL types and how disposables work already.

Tracking item for it (and other .NET 9 features) is here: https://github.com/dotnet/fsharp/issues/16967

auduchinok commented 6 months ago

It feels a bit weird tbh, it's a very ad-hoc solution from the C# side, ducktyped dispose, but only for ref structs.

I agree with this, but, unfortunately, we see people using this approach in libraries, and it becomes harder to consume these libraries from F# side.

abelbraaksma commented 5 months ago

I tend to agree with @auduchinok that we shouldn't fail when it comes to consuming other libraries.

@vzarytovskii, it wouldn't be too hard to implement this through use x = someRefType, where we dispose it if it contains these methods (but not the interface), as described in the C# proposal. Doing so gives us better feature parity.

vzarytovskii commented 5 months ago

I tend to agree with @auduchinok that we shouldn't fail when it comes to consuming other libraries.

@vzarytovskii, it wouldn't be too hard to implement this through use x = someRefType, where we dispose it if it contains these methods (but not the interface), as described in the C# proposal. Doing so gives us better feature parity.

My point was that we should either support all or only interface based disposal. Supporting it just for refs feels weird and very ad-hoc-y

abelbraaksma commented 5 months ago

The referenced document isn't very clear on whether having the Dispose() method present on any type is enough, or whether it must be a ref type.

I agree, though, that it makes sense to allow this pattern for any type that exposes a public instance member named Dispose, of type unit -> unit.

brianrourkeboll commented 5 months ago

The referenced document isn't very clear on whether having the Dispose() method present on any type is enough, or whether it must be a ref type.

C# does indeed only seem to do it for ref structs or types implementing IDisposable:

Xyncgas commented 5 months ago

We can always browse compiler source code to be completely sure

abelbraaksma commented 5 months ago

We can also just go one step further and essentially do it the way it would work as if SRTP were conditionally applied. That is, if the Dispose -> unit -> unit is present as an accessible member, call it.

It won't lead to any backward compat issues (as such types currently cannot be bound with use), though it may lead to people being less diligent in implementing IDisposable (which may have been the motivation for the narrower approach in C#).

We can always browse compiler source code to be completely sure

Indeed :).

In fact, that appears to be true. It was a conscious change during implementing to limit it to ref struct only: https://github.com/dotnet/roslyn/commit/3f4d93d4026ef9a9eb7b858f7afa22e480a6d3ce (PR: https://github.com/dotnet/roslyn/pull/32052).

EDIT: I found the original speclet, which we can use as a guide: https://github.com/dotnet/roslyn/blob/f3dccaa8f00ecdf64ba32c1d6999a94c83eef8c5/docs/features/enhanced-using.md (interestingly, pattern-based dispose is supported in async await scenarios if an accessible member DisposeAsync is available. Curious that they didn't do the same for synchronous Dispose as well).