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

nativeptr<'T> should support comparison #1194

Closed
5 tasks done
roboz0r opened this issue Oct 19, 2022 · 14 comments
Closed
5 tasks done

nativeptr<'T> should support comparison #1194

roboz0r opened this issue Oct 19, 2022 · 14 comments

Comments

@roboz0r
Copy link

roboz0r commented Oct 19, 2022

Suggestion is to have nativeptr<'T> support comparison. Probably it should also support all other interfaces of nativeint

The current workaround is to cast nativeptr<'T> to nativeint when storing in e.g. a Set.

Pros and Cons

The advantages of making this adjustment to F# are you aren't required to throw away type information to store pointers in a collection.

The disadvantages of making this adjustment to F# are it's work.

Extra information

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

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.

@abelbraaksma
Copy link

abelbraaksma commented Oct 20, 2022

It seems that C# supports these out of the box, so I think it makes sense to support this. See: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/pointer-related-operators.

Though I think that many of the other interfaces may be problematic in F#, at least until we have a way to deal with all these static interfaces. Though I guess we could add a bunch of them in a functional way at least by extending the surface area of the NativeInterop.NativePtr functions? Just linking the original docs here for comparison.

@roboz0r
Copy link
Author

roboz0r commented Oct 20, 2022

Probably this should be split into two parts.

  1. The .NET 6 interfaces
  2. The .NET 7 interfaces

@jl0pd
Copy link

jl0pd commented Oct 20, 2022

Allowing nativeptr<T> to satisfy comparison constraint still won't allow it to be used inside Set<T> because pointers cannot be used as generic arguments (limitation of runtime, see ECMA-335 II.9.4)

@roboz0r
Copy link
Author

roboz0r commented Oct 20, 2022

@jl0pd This is very surprising to me as I'm running code right now which stores pointers in a HashSet<nativeptr<myStruct>>. A previous version of the code was also working with Set<nativeint>.

@jl0pd
Copy link

jl0pd commented Oct 20, 2022

Surprisingly F# erases nativeptr<T> when it's used inside generic, but keeps pointer type when it's used as plain type:

// return type is HashSet<IntPtr>
let set () : System.Collections.Generic.HashSet<nativeptr<int>> = Unchecked.defaultof<_>

// return type is int*
let ptr () : nativeptr<int> = Unchecked.defaultof<_>

@abelbraaksma
Copy link

abelbraaksma commented Oct 20, 2022

That looks like a bug to me. Having pointers in a collection seems wrong to me and extremely unstable. If pinned, it's gonna be disastrous, if not pinned even more so. They have no place in heap-allocated objects. If you do need them (let's say store some pointer to an unmanaged object) you can always store it as a native int.

ECMA-335 II.9.4

Exactly.

@abelbraaksma
Copy link

Probably this should be split into two parts.

Implementing the interfaces isn't going to be easy, as afaik, F# Core has to remain compatible with .NET Standard 2.0. And, unless I'm mistaken, these interfaces aren't available there, and there's strong reluctance to move to a multi-versioned F# Core.

(I'm just repeating what I recall from a previous discussion a similar proposal I made myself, but things may have changed meanwhile)

@vzarytovskii
Copy link

Probably this should be split into two parts.

  1. The .NET 6 interfaces
  2. The .NET 7 interfaces

We won't do it, likely, since we are targeting netstandard2.0

@roboz0r
Copy link
Author

roboz0r commented Oct 20, 2022

We won't do it, likely, since we are targeting netstandard2.0

Fair enough. I was mostly trying to avoid doing the cast and wasn't aware of all this background. I thought it would be a straight-forward addition.

Having pointers in a collection seems wrong to me and extremely unstable.

My use case is to support xlAsyncReturn via an unmanaged delegate.

[<UnmanagedFunctionPointer(CallingConvention.StdCall)>]
type internal Excel12vDelegate = delegate of xlfn:int * count:int * ppOpers:nativeptr<nativeptr<xlOper12>> * pOperRes:nativeptr<xlOper12> -> xlRetCode

I'm keeping the pointers in a HashSet as Excel may cancel the computation at any time. My async code checks that the pointer is still in the HashSet before returning via the delegate. In this case the pointer originates from unmanaged memory but there is a mix of unmanaged and .NET pointers crossing that boundary. I'm making extensive use of pinned arrays so the heap doesn't get corrupted.

@abelbraaksma
Copy link

Thanks for explaining the use case. I’d probably try to capture it through closures, but that may not be feasible. Sounds like rather scary code to try to manage, but maybe there’s simply no other way.

@roboz0r
Copy link
Author

roboz0r commented Oct 20, 2022

capture it through closures

This is exactly what I'm doing. The consumer side interface reduces to nativeptr<_> -> (Async<_> -> unit). It seems to be stable with 10k+ parallel calls so I'm going to call that good enough.

@dsyme
Copy link
Collaborator

dsyme commented Oct 28, 2022

Can you use NativePtr.toNativeInt and compare the integers?

@dsyme
Copy link
Collaborator

dsyme commented Oct 28, 2022

The current workaround is to cast nativeptr<'T> to nativeint when storing in e.g. a Set.

I see, you've listed that workaround. To me this is adequate.

@dsyme
Copy link
Collaborator

dsyme commented Oct 28, 2022

Closing this out as the workaround is adequate.

@dsyme dsyme closed this as completed Oct 28, 2022
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