LoadLibraryEx missing critical [Optional] attribute on reserved parameter

[AArnott]

The docs for LoadLibraryEx dictate that the hFile parameter is reserved and must be NULL. But the metadata does not attribute the parameter with [Optional], so the C# projection throws when null is passed in.

[DllImport("KERNEL32.dll", ExactSpelling = true, PreserveSig = false, SetLastError = true)]
[SupportedOSPlatform("windows5.1.2600")]
public static extern HINSTANCE LoadLibraryExW([In][Const] PWSTR lpLibFileName, [In][Reserved] HANDLE hFile, [In] LOAD_LIBRARY_FLAGS dwFlags);

Contrast this with CreateFile, which does attribute its hTemplateFile parameter as [Optional]:

[DllImport("KERNEL32.dll", ExactSpelling = true, PreserveSig = false, SetLastError = true)]
[SupportedOSPlatform("windows5.1.2600")]
public unsafe static extern HANDLE CreateFileW([In][Const] PWSTR lpFileName, [In] FILE_ACCESS_FLAGS dwDesiredAccess, [In] FILE_SHARE_MODE dwShareMode, [Optional][In] SECURITY_ATTRIBUTES* lpSecurityAttributes, [In] FILE_CREATION_DISPOSITION dwCreationDisposition, [In] FILE_FLAGS_AND_ATTRIBUTES dwFlagsAndAttributes, [Optional][In] HANDLE hTemplateFile);

This was caught by @Arebusf21 as reported in microsoft/CsWin32#839.

[kennykerr]

For what it's worth, the Rust projection just treats Optional and Reserved the same way - both end up as optional in Rust.

[AArnott]

Is it safe to assume that all reserved fields/parameters are documented as requiring 0 for their value? If so, that might open doors to simply hide such fields/parameters in the projections.

[AArnott]

To @kennykerr's point, maybe the metadata should always add Optional where it specifies Reserved.

[kennykerr]

Yep, reserved parameters are always zero. And yes, I'd prefer a single way to describe these rather than having to check for both optional and reserved.

[riverar]

Doesn't look like there's a bug here. Instead, the C# projection was not looking at the [Reserved] attribute. @AArnott Does that sound right? Can we close this?

[mikebattista]

Agreed. The metadata matches the headers and C#/Win32 should add support for [Reserved].

[AArnott]

Maybe we should start documenting these things then. The fact that [Optional] is not complete by itself is not intuitive. A projection has to take the union of optional and reserved attributes to determine whether a given value is allowed to be null.

[AArnott]

Just a note: [Optional] is only a pseudo-attribute. It isn't even a real attribute. It's a flag in the CLI metadata. Only [Reserved] is a bona-fide attribute. Given that, it seems even more strange IMO that the optional flag doesn't always appear on values that are allowed to be null.

[mikebattista]

What do you mean by that? Optional is an attribute: https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.optionalattribute?view=net-7.0.

[kennykerr]

That's just a synthesized attribute that .NET uses to represent the Optional flag (bit) in the Param table in ECMA-335.

[AArnott]

@mikebattista it appears in source code and in decompiled source, but ya, it's not really there in the compiled form, as @kennykerr says. The attribute is just there to help C# source code to indicate to the compiler that they want the flag set.

I have hit a snag though. It seems the metadata includes the Reserved attribute on out parameters. How should I think about that?

One example is the IEnumNetCfgComponent::Clone method. It is documented as "do not use", so maybe the reserved flag on the parameter is better applied to the whole method?

Another example is IEnumScript::Clone which is not documented as "do not use".

Both of these methods are interesting because they have exactly one simple function, which is entirely dependent on the caller consuming the result of its out parameter, so having that out parameter be marked reserved seems bonkers.

[mikebattista]

The metadata is following the _Reserved_ SAL annotations in the headers for these methods.

[AArnott]

Understood. IMO though, as this repo is responsible for translating headers to a ECMA representation, it should reliably set the optional flag based on whether parameters accept null values. If getting this from the headers requires considering both the reserved and optional SAL annotations, that seems like it should be the job of this repo instead of the projections.

[mikebattista]

If we added [Optional] in addition to [Reserved] would that work? Are [Reserved] parameters always semantically equivalent to [Optional]? Or does [Reserved] in the metadata never have any value other than [Optional], and we should remove it entirely?

[AArnott]

The two are not equivalent. [Optional] indicates that null is allowed. @kennykerr claims that [Reserved] mandates that it be 0/null¹. It therefore seems to follow that all reserved parameters must be optional, but not all optional parameters are reserved.

We should not remove Reserved because it's useful for projections to drop the parameter since the user can't do anything useful with it anyway. CsWin32 just started doing this today.

¹ Out parameters with [Reserved] on them such as the examples I gave in an earlier comment brings this into question, however. Maybe these APIs are inappropriately tagged as reserved and the metadata should manually remove them.

[mikebattista]

Aside from your footnote, are you saying projections should just drop parameters marked with Reserved? If so, what's the value of adding Optional?

If that's not correct or not all projections would do that, do you agree that adding [Optional] in addition to [Reserved] is the right thing?

[AArnott]

Dropping reserved parameters would be a decision to be made by the projection. They can't really drop them, since it's part of the binary interface, but in CsWin32's case we can drop them from the friendly overloads. At that point, CsWin32 no longer cares about the Optional attribute being present as well. But for any projection that doesn't want to or can't produce overloads, some (e.g. Rust) would still want to know when a value is allowed to be null.

do you agree that adding [Optional] in addition to [Reserved] is the right thing?

Yes, primarily because Reserved is an attribute that IMO ought to be safe for a projection to ignore.

[mikebattista]

@kennykerr are you ok with this? Adding [Optional] to all [Reserved] parameters?

[riverar]

We should consider that reserved parameters are reserved by Microsoft for future use. This future may change the optional nature of the parameter for some future scenario, which may require projections to emit this parameter.

[riverar]

Another example is IEnumScript::Clone which is not documented as "do not use".

Both of these methods are interesting because they have exactly one simple function, which is entirely dependent on the caller consuming the result of its out parameter, so having that out parameter be marked reserved seems bonkers.

Cloning the script enumerator does not appear to be implemented, which makes the reserved attribute make sense.

use windows::Win32::{
    Globalization::{
        CMultiLanguage, IMultiLanguage3, SCRIPTCONTF_SCRIPT_SYSTEM, SCRIPTCONTF_SCRIPT_USER,
    },
    System::Com::{
        CoCreateInstance, CoInitializeEx, CLSCTX_INPROC_SERVER, COINIT_MULTITHREADED,
    },
};

fn main() -> windows::core::Result<()> {
    unsafe {
        CoInitializeEx(None, COINIT_MULTITHREADED)?;

        let language: IMultiLanguage3 =
            CoCreateInstance(&CMultiLanguage, None, CLSCTX_INPROC_SERVER)?;
        let scripts = language.EnumScripts(
            SCRIPTCONTF_SCRIPT_USER.0 as u32 | SCRIPTCONTF_SCRIPT_SYSTEM.0 as u32,
            1033,
        )?;
        dbg!(scripts.Clone());
    }

    Ok(())
}

produces

[src\main.rs:20] scripts.Clone() = Err(
    Error {
        code: HRESULT(0x80004001),
        message: "Not implemented",
    },
)

[kennykerr]

@kennykerr are you ok with this? Adding [Optional] to all [Reserved] parameters?

Sure that's fine but if you're not going to drop reserved, I don't really see the point.

I agree with @riverar that reserved parameters are just optional parameters that currently have no other supported values, other than zero, but may in future.

Anyway, if you're going to slap optional on all reserved parameters let's just make sure it's done reliably so that tools that don't distinguish between them don't have to check for both.

[riverar]

I believe @AArnott wants to hide reserved parameters from the developer and only provide overloaded methods w/ optional parameters (if any). He can't do that if we semantically merge the Optional and Reserved attributes, so I think that needs to stay.

I think for projections that can't use or don't need this distinction, Optional on all Reserved items is workable.