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

Support disposable pattern for ref structs #1229

Open
4 of 5 tasks
auduchinok opened this issue Jan 4, 2023 · 13 comments
Open
4 of 5 tasks

Support disposable pattern for ref structs #1229

auduchinok opened this issue Jan 4, 2023 · 13 comments

Comments

@auduchinok
Copy link

auduchinok commented Jan 4, 2023

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

  • being compatible with a recent C# feature that may be used in referenced libraries/projects; otherwise F# usages may get broken

The disadvantages of making this adjustment to F# are

  • it needs work

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:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this

For Readers

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

@auduchinok
Copy link
Author

auduchinok commented Jan 6, 2023

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: JetBrains/resharper-fsharp@705cc50, JetBrains/resharper-fsharp@f63b536

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

@Xyncgas
Copy link

Xyncgas commented Feb 11, 2023

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
Copy link

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
Copy link
Member

@dsyme, @vzarytovskii Any thoughts on this suggestion ?

@Xyncgas
Copy link

Xyncgas commented Apr 1, 2024

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
Copy link

@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: dotnet/fsharp#16967

@auduchinok
Copy link
Author

auduchinok commented Apr 3, 2024

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
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link

Xyncgas commented May 16, 2024

We can always browse compiler source code to be completely sure

@abelbraaksma
Copy link
Member

abelbraaksma commented May 16, 2024

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: dotnet/roslyn@3f4d93d (PR: dotnet/roslyn#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).

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

7 participants