Annotate methods that return handles that are not meant to be closed

[AArnott]

Methods such as GetStdHandle and GetCurrentProcess return handles that are not expected to ever be closed by the caller, and doing so may cause malfunctions.

While tagging such methods may be a manual process, we should have such a means so that projections that offer enhanced handle closing APIs can avoid causing problems for such APIs.

[sotteson1]

I'm having them return a native typdef called NonClosableHandle which has the attribute that says it's also usable for HANDLE.

[AArnott]

I think I'd rather see the return value attributed so that it can work for any handle type -- not just HANDLE, and without losing the type information for the user. Any chance we could solve it that way?

[kennykerr]

Would this be in addition to the AlsoUsableFor attribute? Struggling to keep all of these things straight. 🤔

[AArnott]

I see AlsoUsableFor as orthogonal, ideally. Many methods in Win32 headers return HANDLE or other handle types, and some subset of these return handles that are not meant to be released by the caller, but they retain their "types" anyway. I'm advocating that we do the same, and use [return: DoNotRelease] attribution on the extern method declaration to indicate when the caller should not release the handle.

[kennykerr]

I like [return: DoNotRelease] and maps well to the languages I care about, but then I don't see why GetProcessHeap has to return the made-up ProcessHeapHandle type.

[sotteson1]

I like [return: DoNotRelease] and maps well to the languages I care about, but then I don't see why GetProcessHeap has to return the made-up ProcessHeapHandle type.

HANDLE is marked as closable by CloseHandle, but that's not how you close a heap handle. How would I have it return HANDLE and the projection then know how to dispose of it?

[sotteson1]

I see AlsoUsableFor as orthogonal, ideally. Many methods in Win32 headers return HANDLE or other handle types, and some subset of these return handles that are not meant to be released by the caller, but they retain their "types" anyway. I'm advocating that we do the same, and use [return: DoNotRelease] attribution on the extern method declaration to indicate when the caller should not release the handle.

I thought about that, but the way I've done it doesn't require any extra work for you (or for me). The type it returns doesn't have a close function, but it's usable in the same places where the closeable is used. If we think it's too unwieldy I can do it the other way as an attribute on return and out parameters.

[kennykerr]

HANDLE is marked as closable by CloseHandle, but that's not how you close a heap handle. How would I have it return HANDLE and the projection then know how to dispose of it?

I didn't say anything about HANDLE. The API usage requires me to call GetProcessHeap and pass the resulting value to HeapAlloc and HeapFree. Those functions expect a HeapHandle. Based on Andrew's suggestion, I would thus expect GetProcessHeap to return a HeapHandle where the return value is attributed with DoNotRelease. That ensures that the handle returned by GetProcessHeap is type-safe and can't easily be used with the wrong functions but gives the language a clue that the result of GetProcessHeap should not be passed to HeapDestroy.

[sotteson1]

Sorry, I thought your complaint was that it's not returning HANDLE. Like I said, I can do Andrew's suggestion but it will be more work for projection writers. If you've already implemented AlsoUsableAs it should work the way it is.

[kennykerr]

Yes, I don't really mind either way - I just don't see the point of having both.

[AArnott]

I just don't see the point of having both.

The AlsoUsableAs is necessary to bridge from an API that returns an HICON to an API that accepts an HGDIOBJ. Since these are all structs and there is no automatic polymorphism, in C# we have to define implicit conversion operators to allow HICON where HGDIOBJ is expected. AlsoUsableAs tells us when to generate those converters.
Having [return: DoNotRelease] allows us to keep using those same typedefs as we were before and that are found in native code, with all the type safety that affords.

In contrast, simply returning NonClosableHandle with AlsoUsableAs applied to it means:

1. NonClosableHandle hides the original handle type, making it harder for customers to discover that they can use it as an argument that takes HANDLE, HGDIOBJ, etc.
2. NonClosableHandle may end up with many AlsoUsableAs attributes on it (as we label more APIs that return handles that are not intended to be closed), further eroding type safety and clarity since it could potentially be used anywhere.

That all said, I'm ok with going with @sotteson1's original plan based on it requiring no additional work for anyone and wait to hear from customers to see if my above two concerns are warranted.

[sotteson1]

1. NonClosableHandle hides the original handle type, making it harder for customers to discover that they can use it as an argument that takes HANDLE, HGDIOBJ, etc.
2. NonClosableHandle may end up with many AlsoUsableAs attributes on it (as we label more APIs that return handles that are not intended to be closed), further eroding type safety and clarity since it could potentially be used anywhere.

That's true. I like how using the DoNotRelease would keep from hiding the original handle type.

[kennykerr]

Nice. I also like preserving the original handle types.

[tim-weis]

What possible solutions are there for annotating a function like GetModuleHandleExW? This is a special case where the semantics of the return value depend on the input: If GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT is specified, the returned value must not be passed to FreeLibrary. Otherwise it needs to be released using FreeLibrary.

[AArnott]

@tim-weis: oooh, good one. I imagine the very specific cases like this one may lead us to places that we may never be able to express in the metadata. In such cases the user may need to be careful and read the docs and code their app in such a way as to conform with the docs, possibly without the nuanced help from the metadata or projection. The projection should (IMO) allow the app/library author to control releasing handles enough to always get it right for cases like this even if the metadata or projection do not know about these special cases.

[kennykerr]

I would avoid using a handle type that includes the RAIIFree attribute with such APIs.

[AArnott]

That's one idea. But I wonder whether the 99% common use case of the function should win out in cases where the no-free flag is the 1% case.

[kennykerr]

That's a fair point, I guess the 1% case could just write their own import/extern declaration.

[AArnott]

Agreed, if that's really the % breakdown. I don't have data for this particular case. In CsWin32, we offer two overloads so the 99% and the 1% case can self-serve without having to declare the extern method.