Generate SafeHandle overloads for non-Span methods

[jeremy-visionaid]

Is your feature request related to a problem? Please describe.

Looks like v0.3.236 added a bunch of span overloads, but it also took away the non-span overloads that accepted safe handles. That creates a lot of code for me to update to the new projections... Especially when the previous methods were more both convenient and efficient in some cases.

Describe the solution you'd like

e.g. I'd like projections like the following to still exist:

internal static unsafe winmdroot.Foundation.BOOL DeviceIoControl(SafeHandle hDevice, uint dwIoControlCode, void* lpInBuffer, uint nInBufferSize, void* lpOutBuffer, uint nOutBufferSize, uint* lpBytesReturned, global::System.Threading.NativeOverlapped* lpOverlapped)

Describe alternatives you've considered

I would have to copy/modify a fair few of the old projections and/or update a lot of code to the new ones.

Additional context

e.g. Only the following SafeHandle overloads exist in v0.3.236 for DeviceIoControl:

		internal static unsafe winmdroot.Foundation.BOOL DeviceIoControl(SafeHandle hDevice, uint dwIoControlCode, ReadOnlySpan<byte> lpInBuffer, Span<byte> lpOutBuffer, out uint lpBytesReturned, ref global::System.Threading.NativeOverlapped lpOverlapped)

		internal static unsafe winmdroot.Foundation.BOOL DeviceIoControl(SafeHandle hDevice, uint dwIoControlCode, ReadOnlySpan<byte> lpInBuffer, Span<byte> lpOutBuffer)

[jevansaks]

If you can call the "unfriendly" method with the SafeHandle via implicit cast, does that solve your scenario? I've been thinking that all the friendly methods should also just take HANDLE (either via implicit cast or other extension method on SafeHandle)

[jevansaks]

Actually @AArnott reminds me that it isn't safe to do that generally but you could do that in your code with an extension method to call the p/invoke with HANDLE parameter.

[jevansaks]

It seems odd to me that you want SafeHandle but not the other safe-things that the friendly overrides are providing with Span. The other thing Span is giving you is the safety that the length of the buffer is the same length being passed in the length parameters.

I would encourage you to switch to Span -- it's just a ref readonly struct, it's not adding any overhead in these cases.

But if you don't want that, then switch to using the fully native P/invoke instead of any of the friendly methods.

[RFBomb]

I agree with @jeremy-visionaid that the overloads should accept SafeHandle.

upgrading my nuget package and getting all sorts of compiler errors because SafeHandle is no longer accepted feels wrong.
In addition to this, I noticed that some generated methods refuse to compile due to the doing something like this:

bool x = false;
fixed (void* x = y)
{
    Method(ref x, /* other args */) ;
}

Given the inability to compile the generated code (which I'll make a ticket about when I have the time), as well as the change to overloads, I've reverted to the prior pkg.

I think that the span parameters would be easier to use for some people if the generated structs were more easily convertible. For example,

SP_DEVICE_INTERFACE_DATA
SP_DEVINFO_DATA
SP_DEVICE_INTERFACE_DETAIL_DATA_W

must be converted to a Span<bytes> to use with the relevant APIs.

[jevansaks]

CsWin32 does have a path which never generates Span. Currently it's checking just if Span<> is available as a type but we could add it as a nativemethods.json option if you never want to see a Span in the projection anywhere. But, again, if that's your mode I would ask why you like SafeHandle and not Span. :)

I think that the span parameters would be easier to use for some people if the generated structs were more easily convertible. For example,

Can you give examples of the APIs and the code you have today? Maybe there's a bug or perhaps I can show how we intend for you to safely (and still efficiently without copying) call them.

[RFBomb]

SafeHandle is returned by the CreateFileW API. Any other API methods that rely on the handle and don't accept SafeHandle are inherently obtuse. Its like you letting someone borrow a dollar but only accepting gold ingots to pay you back.

I can find some time tomorrow to post the code that didn't compile in the latest nuget, as well as how I got it working. (I'm doing USB Ejection)

[jevansaks]

Well, technically you're calling the friendly overload of CreateFile there. If you want the one that returns HANDLE you can call it with the raw pointer parameters.

As I'm working on adopting this version into a larger codebase I am seeing that some AsSpan() and AsSpanBytes() helpers would be helpful, so I agree with that part of the feedback.

[jeremy-visionaid]

It's not so much a matter that I don't want to use the "safer" overloads (I'm already using the APIs that create SafeHandles becase I do want that). It's more that this change seems a lot more invasive than the usual changes to keep up with the current release. Usually it's just a few lines here and there, but this time it looks like hundreds and some of it requires careful consideration to make sure I don't cause regressions. It's not something I could do quickly by hand or trust Copilot with.

Am I missing something with the implicit cast? I don't see any implicit operators on the SafeHandles/HANDLE that would convert that way (and it seems like if it were implicit then the it would compile as is already?). Also, like you say, I assume that wouldn'be safe since GC could potentially kick in mid-way through the P/Invoke. So I'd really need the DangerousAddRef/DangerousGetHandle/DangerousRelease or some other keep alive that the previous projections did for me.

Adding AsSpan<byte>() for structs would definitely make adoption a bit easier, though I note that wouldn't always apply for things like DeviceIoControl since you often need to call it twice to figure out how much memory you need e.g. To allocate for things like variable length in-line arrays. It would still be a lot of code to update too.

I think the Span overloads are a good addition for sure, I would definitely use them for new code, but I'd also like to have my cake and eat it and have my old code to still compile 😄

[jevansaks]

I hear you. I was trying to avoid an explosion of overloads but we could generate both to help with the transition?

The case I've found so far where the Span<byte> is a harder to work with is things like ReadProcessMemory where one of the parameters is a void* but it literally is just a number. Or like VerQueryValue has void** out parameter and it's giving you just a pointer into some other memory. That's where out void* would be easier. But any time it's reading or filling out a buffer/struct for you, those are turning out much safer with the Span approach.

Am I missing something with the implicit cast?

No, that suggestion was wrong. Sorry about that.

Can you give some examples for the DeviceIoControl calls that you're finding difficult? Or are you saying you are on board with the Span versions but it's hard to move to the latest because it's a more invasive change?

[jeremy-visionaid]

Yeah, the Span overloads are good for most cases I think - now that they exist they would be my first choice. And I appreciate the desire to keep the number of overloads under control, but there are still cases where the old projections are more convenient. It's not so much there are cases that I can't update - it's simply that it's a lot of work to do so. It's the first time where I'm put off updating to the latest version - it would have to wait for a rainy day!

[jevansaks]

I understand the difficulty of trying to absorb the upgrade all-or-nothing. I'll look into adding a knob to generate both overloads. Then folks can opt-in but new projects don't get cluttered with the less friendly pointer-based overloads.

[RFBomb]

This is the block that cannot compile in the latest package :
NativeMethods.txt :

SetupDiGetDeviceInterfaceDetail

Generated Code

		/// <inheritdoc cref="SetupDiGetDeviceInterfaceDetail(winmdroot.Devices.DeviceAndDriverInstallation.HDEVINFO, winmdroot.Devices.DeviceAndDriverInstallation.SP_DEVICE_INTERFACE_DATA*, winmdroot.Devices.DeviceAndDriverInstallation.SP_DEVICE_INTERFACE_DETAIL_DATA_W*, uint, uint*, winmdroot.Devices.DeviceAndDriverInstallation.SP_DEVINFO_DATA*)"/>
		[SupportedOSPlatform("windows5.0")]
		internal static unsafe winmdroot.Foundation.BOOL SetupDiGetDeviceInterfaceDetail(SafeHandle DeviceInfoSet, in winmdroot.Devices.DeviceAndDriverInstallation.SP_DEVICE_INTERFACE_DATA DeviceInterfaceData, out winmdroot.Devices.DeviceAndDriverInstallation.SP_DEVICE_INTERFACE_DETAIL_DATA_W DeviceInterfaceDetailData, out uint RequiredSize, ref winmdroot.Devices.DeviceAndDriverInstallation.SP_DEVINFO_DATA DeviceInfoData)
		{
			DeviceInterfaceDetailData = default(winmdroot.Devices.DeviceAndDriverInstallation.SP_DEVICE_INTERFACE_DETAIL_DATA_W);
			return SetupDiGetDeviceInterfaceDetail(DeviceInfoSet, in DeviceInterfaceData, global::System.Runtime.InteropServices.MemoryMarshal.AsBytes(new Span<winmdroot.Devices.DeviceAndDriverInstallation.SP_DEVICE_INTERFACE_DETAIL_DATA_W>(ref DeviceInterfaceDetailData)), out RequiredSize, ref DeviceInfoData);
		}

An example of an overload that was problematic due to removal:

        /// <summary>
        /// Sets or Unsets the 'Prevent Removal' flag
        /// </summary>
        private static unsafe bool PreventRemoval(SafeHandle handle, bool prevent)
        {
            var pmr = new PREVENT_MEDIA_REMOVAL() { PreventMediaRemoval = prevent };
            bool success;
#if CSWIN32_0_3_236
              // Note that I do like this, as it is much cleaner
              ReadOnlySpan<byte> inputBuffer = new(&pmr, Marshal.SizeOf<PREVENT_MEDIA_REMOVAL>());
              success = PInvoke.DeviceIoControl(handle, PInvoke.IOCTL_STORAGE_MEDIA_REMOVAL, inputBuffer, null);
#elif CSWIN32_0_3_235
            // !! Overload was removed in 0_3_236
            success = PInvoke.DeviceIoControl(handle, PInvoke.IOCTL_STORAGE_MEDIA_REMOVAL, &pmr, (uint)Marshal.SizeOf<PREVENT_MEDIA_REMOVAL>(), null, 0u, null, null);
#endif
            return success;
        }

To me, this would be ideal :

        private static unsafe bool PreventRemoval(SafeHandle handle, bool prevent)
        {
            var pmr = new PREVENT_MEDIA_REMOVAL() { PreventMediaRemoval = prevent };
            bool success = PInvoke.DeviceIoControl(handle, PInvoke.IOCTL_STORAGE_MEDIA_REMOVAL, pmr.AsReadOnlySpanBytes(), null);
            // throw or react if false
            return success;
        }

[jevansaks]

To me, this would be ideal :

Agreed! I couldn't convince the other maintainers that we should provide a "AsReadOnlySpanBytes" extension but we could do a .AsSpan() and .AsReadOnlySpan() extension on all the native structs we generate. Then the call would be

PInvoke.DeviceIoControl(handle, PInvoke.IOCTL_STORAGE_MEDIA_REMOVAL, MemoryMarshal.AsBytes(pmr.AsReadOnlySpan()), null);

Which is a few more characters but makes clear we're doing some unsafe interop in this call. But then in your codebase if you wanted to add a .AsBytes() extension method like this:

    public static Span<byte> AsBytes<T>(this Span<T> span)
        where T : struct
    {
        return MemoryMarshal.AsBytes(span);
    }

Then it could be:

PInvoke.DeviceIoControl(handle, PInvoke.IOCTL_STORAGE_MEDIA_REMOVAL, pmr.AsReadOnlySpan().AsBytes(), null);

[RFBomb]

Its also easy enough to define our own static extension, but having it built in would be a convenience.

#if NETFRAMEWORK
        public static unsafe ReadOnlySpan<byte> AsReadOnlySpanBytes<T>(this ref T obj) where T: struct
        {
            ReadOnlySpan<byte> ret;
            fixed (void* ptr = &obj)
            {
                ret = new ReadOnlySpan<byte>(ptr, Marshal.SizeOf<T>());
            }
            return ret;
        }
#else
        public static ReadOnlySpan<byte> AsReadOnlySpanBytes<T>(this ref T obj) where T: struct
        {
            return MemoryMarshal.AsBytes(MemoryMarshal.CreateReadOnlySpan(ref obj, Marshal.SizeOf<T>()));
        }
#endif