microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
1.99k stars 84 forks source link

Generate overload that sets `dwLength`? #1142

Closed chucker closed 4 months ago

chucker commented 4 months ago

Is your feature request related to a problem? Please describe. I'm trying to replace custom PInvoke wrappers with this library to reduce brittle code I have to maintain. As an example call, I picked GlobalMemoryStatusEx, and found that it didn't initially work as expected.

It turned out that, similarly to here, the root cause was that the metadata was incorrect. By explicitly (rather than transitively) referencing a newer Microsoft.Windows.SDK.Win32Metadata, the source generator created a slightly different method. Before, it took an out parameter, suggesting I didn't have to pass a struct; now, it takes a ref.

However, this now means I have to externally initialize the struct's dwLength:

var status = new Windows.Win32.System.SystemInformation.MEMORYSTATUSEX
{
    dwLength = (uint)Marshal.SizeOf(typeof(Windows.Win32.System.SystemInformation.MEMORYSTATUSEX))
};

if (Windows.Win32.PInvoke.GlobalMemoryStatusEx(ref status))
    return status.ullTotalPhys;

This strikes me as unnecessary boilerplate. Windows.Win32.System.SystemInformation.MEMORYSTATUSEX is a source generator-emitted type, and Windows.Win32.PInvoke.GlobalMemoryStatusEx a source generator-emitted method, so why pass it information it can already know?

Describe the solution you'd like With friendlyOverloads enabled, an overload should be offered that skips this boilerplate. In that case, the parameter should revert to being out, since there's really no useful information to pass in, something like:

/// <inheritdoc cref="GlobalMemoryStatusEx(winmdroot.System.SystemInformation.MEMORYSTATUSEX*)"/>
[SupportedOSPlatform("windows5.1.2600")]
internal static unsafe winmdroot.Foundation.BOOL GlobalMemoryStatusEx(out winmdroot.System.SystemInformation.MEMORYSTATUSEX lpBuffer)
{
        var status = new winmdroot.System.SystemInformation.MEMORYSTATUSEX
        {
            dwLength = (uint)Marshal.SizeOf(typeof(Windows.Win32.System.SystemInformation.MEMORYSTATUSEX))
        };

    fixed (winmdroot.System.SystemInformation.MEMORYSTATUSEX* lpBufferLocal = &lpBuffer)
    {
        winmdroot.Foundation.BOOL __result = PInvoke.GlobalMemoryStatusEx(lpBufferLocal);
        return __result;
    }
}

…so that I can do:

if (Windows.Win32.PInvoke.GlobalMemoryStatusEx(out var status))
    return status.ullTotalPhys;

More than that, given that Windows.Win32.System.SystemInformation.MEMORYSTATUSEX is emitted by the source generator in the first place, I'm not sure the call to Marshal.SizeOf should be even necessary.

/// <inheritdoc cref="GlobalMemoryStatusEx(winmdroot.System.SystemInformation.MEMORYSTATUSEX*)"/>
[SupportedOSPlatform("windows5.1.2600")]
internal static unsafe winmdroot.Foundation.BOOL GlobalMemoryStatusEx(out winmdroot.System.SystemInformation.MEMORYSTATUSEX lpBuffer)
{
        var status = new winmdroot.System.SystemInformation.MEMORYSTATUSEX
        {
            dwLength = 64u // we can just determine this at source generation time, no?
        };

    fixed (winmdroot.System.SystemInformation.MEMORYSTATUSEX* lpBufferLocal = &lpBuffer)
    {
        winmdroot.Foundation.BOOL __result = PInvoke.GlobalMemoryStatusEx(lpBufferLocal);
        return __result;
    }
}

I apologize if this has been discussed before; I did try to look for similar issues.

AArnott commented 4 months ago

Thanks for your feedback.

108 seems to capture the potentially actionable part of what you're asking for.

The only safe time to initialize that field is when the struct is created. Various methods may take pointers to those structs, at which point those methods don't actually know the length of the struct and that's why win32 needs you to specify the size of it.

Furthermore, based on the header files / metadata that CsWin32 has to work with, there is just no way to know that a ref parameter only needed one size-related field to be initialized and thus turn it to out and initialize it itself.

chucker commented 4 months ago

Thanks for explaining!

https://github.com/microsoft/CsWin32/issues/108 seems to capture the potentially actionable part of what you're asking for.

That does look like an improvement. :)