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

Reconsider taking SafeHandle-derived types as input parameters #125

Open
jnm2 opened this issue Feb 18, 2021 · 10 comments
Open

Reconsider taking SafeHandle-derived types as input parameters #125

jnm2 opened this issue Feb 18, 2021 · 10 comments

Comments

@jnm2
Copy link
Contributor

jnm2 commented Feb 18, 2021

No semantically correct SafeHandle type is provided by CsWin32 when using GetCursorInfo with GetIconInfo and DrawIconEx, now that all handle parameters are SafeHandle:

var cursorInfo = new CURSORINFO { cbSize = Marshal.SizeOf(typeof(CURSORINFO)) };

if (!PInvoke.GetCursorInfo(ref cursorInfo))
    throw new Win32Exception();

//                           ↓ Options: abuse an existing SafeHandle type or declare one myself
if (!PInvoke.GetIconInfo(new WhatDoIWriteHere(cursorInfo.hCursor, ownsHandle: false), out var iconInfo))
    throw new Win32Exception();
    
// Calling DrawIconEx has the same issue.

Prior to everything becoming abstract SafeHandle, the following code worked. It's not as ergonomic as passing HCURSOR, but it is nicer than the two new options:

if (!PInvoke.GetIconInfo(new(cursorInfo.hCursor, ownsHandle: false), out var iconInfo))
    throw new Win32Exception();

I'm also not a fan of throwing away the parameter type information in this way. GetObject has parameter SafeHandle h now, for instance. A parameter like that doesn't instantly tell you what kinds of handle you can pass or how to obtain them.

@jnm2 jnm2 changed the title All handle parameters being abstract SafeHandle punishes the use of GetCursorInfo and GetIconInfo All handle parameters being abstract SafeHandle punishes the use of GetCursorInfo and GetIconInfo/DrawIconEx Feb 18, 2021
@jnm2
Copy link
Contributor Author

jnm2 commented Feb 18, 2021

new UnownedHandle(cursorHandle) is not so bad, syntax-wise, though I'm still uneasy about the loss of signature information.

@AArnott
Copy link
Member

AArnott commented Feb 18, 2021

Yes, the type-specificity is valuable, I agree. And we may bring it back, when we get the majority of the APIs "correct" and can circle back to bring back the niceties. We can leave this issue open to track considering bringing those back.
We may offer non-SafeHandle overloads as well, since that sounds like something that will help in some cases.

@AArnott AArnott changed the title All handle parameters being abstract SafeHandle punishes the use of GetCursorInfo and GetIconInfo/DrawIconEx Reconsider taking SafeHandle-derived types as input parameters Feb 18, 2021
@AraHaan
Copy link
Contributor

AraHaan commented Mar 3, 2021

I would also like overloads generated to generate overloads for HandleRef as well too.

@AArnott
Copy link
Member

AArnott commented Mar 3, 2021

@AraHaan: HandleRef has been deprecated since .NET 2.0. We have to keep the number of overloads we generate to a very small number to maintain an overall good experience, so they have to appeal to a broad audience. I think such a very old type won't meet the bar.
But if you'd like to explain why this is important to you, I'd like to hear it.

@AraHaan
Copy link
Contributor

AraHaan commented Mar 3, 2021

HandleRef is not only used by my application (for referencing HWND handles to winforms controls to pass them into p/invokes).

They are also used in the runtime itself (if I remember right they are used a lot in the winforms repository, maybe also the wpf repository in the dotnet org).

Possibly used inside of System.Drawing, System.Drawing.Common as well too. One way to find out is to search HandleRef in github under the dotnet org scope to see where it all is used.

This is where my code uses it: https://github.com/Elskom/Els_kom_new/search?q=HandleRef&type=code (it seems to not show every usage of it for some reason in my NativeMethods.cs file.)

Got 616 results for it as well too https://github.com/search?q=org%3Adotnet+HandleRef&type=code, a lot of commits, and issues regarding it as well.

@jnm2
Copy link
Contributor Author

jnm2 commented Mar 3, 2021

HandleRef is useful for when you don't own the handle, but you do have a managed object (e.g. Form) that owns the handle, and you need to keep that object from being collected and freeing the handle concurrently during the native call. The alternative is to call GC.KeepAlive(wrapper); after the native call.

Since HandleRef is a struct, it won't ever introduce an overload resolution ambiguity when passing the argument null. default, yes.

@AArnott
Copy link
Member

AArnott commented Mar 3, 2021

Alright. I guess it may be worth considering HandleRef overloads. But please file an issue to track that because it's decidedly off-topic here.

@AraHaan
Copy link
Contributor

AraHaan commented Mar 17, 2021

I think generating SafeHandle overloads should be opt-in by default but have the user opt-in for them if they specifically ask for said overload to be generated within the configuration for specific members only (opt-in per p/invoke function name) within the text file that controls the source generator.

An idea for this to for example pretend that GetTickCount accepted an HANDLE of some type, and then one could tell the source generator to only generate methods that take in SafeHandle by doing this in the NativeMethods.txt file:

GetTickCount (use SafeHandle)

However if they wanted to use both for example SafeHandle and then also HandleRef, they could then have:

GetTickCount (use SafeHandle and HandleRef)

That way we keep the cleanliness of not source generating to much code (only generate what is really used), as well as allow the user to opt in to only what they need based on their type of application or code.

@AArnott
Copy link
Member

AArnott commented Mar 17, 2021

@AraHaan I tend to disagree since most APIs that produce handles expect the caller to close those handles, and .NET has a very strong precedent for using SafeHandle here, and most users want that. Our defaults should match the expectations of most folks.
We might expose an option to suppress or alter friendly overload generation though, which may give you what you want.

@AArnott
Copy link
Member

AArnott commented May 13, 2024

One issue with declaring input parameters with specific SafeHandle-derived types is the existence of multiple derived types that should all be valid inputs. .NET itself defines multiple SafeHandle-derived types that close with CloseHandle. By declaring parameters as taking a SafeHandle, all these are acceptable. If we forced them to take SafeFileHandle (which is just one such type that closes with CloseHandle), then all the other handle types wouldn't work as arguments, even though they were legitimately acquired from other APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants