Add an attribute for identifying methods and functions that return alternative success codes

[knopp]

For example DoDragDrop returns DRAGDROP_S_DROP on successful drop. However there doesn't seem to be any way to retrieve this value, since windows-rs immediately calls ok() on the HRESULT. And given that DRAGDROP_S_DROP doesn't have the error bit set it is not passed as error in the Result.

Is this an oversight?

[fowenix]

If you check here, and place the value of DRAGDROP_S_DROP(262400) where self.0 is, it returns true. but so does DRAGDROP_S_CANCEL(262401). So you want identify which one you got?

[knopp]

That's not really the point. The problem is that you can not get the original hresult, because if positive it never leaves DoDragDrop(). This is what generated DoDragDrop() code looks like:

pub unsafe fn DoDragDrop<'a>(
    pdataobj: impl ::windows::IntoParam<'a, IDataObject>,
    pdropsource: impl ::windows::IntoParam<'a, IDropSource>,
    dwokeffects: u32,
    pdweffect: *mut u32,
) -> ::windows::Result<()> {
    #[cfg(windows)]
    {
        #[link(name = "OLE32")]
        extern "system" {
            fn DoDragDrop(
                pdataobj: ::windows::RawPtr,
                pdropsource: ::windows::RawPtr,
                dwokeffects: u32,
                pdweffect: *mut u32,
            ) -> ::windows::HRESULT;
        }
        DoDragDrop(
            pdataobj.into_param().abi(),
            pdropsource.into_param().abi(),
            ::std::mem::transmute(dwokeffects),
            ::std::mem::transmute(pdweffect),
        )
        // Here a positive HRESULT gets swallowed.
        .ok()
    }
    #[cfg(not(windows))]
    unimplemented!("Unsupported target OS");
}

[tim-weis]

This potential issue has been acknowledged. I don't know whether this has been missed in the implementation, or OLE just being odd. Either way it's an issue that would need to be addressed.

[kennykerr]

Yes, I'm considering a few options for dealing with this. One is to preserve the original signature as a fallback (#737) or a custom Result<T> that retains the HRESULT even in the success case. I'd prefer the latter because the former would lead to an explosion of the API surface or two different flavors of the Windows API, which I'd like to avoid.

[knopp]

For me custom result object would definitely solve the issue. The object could also implement Deref/DerefMut for convenient access to actual value.

[rylev]

I'm unsure which way to achieve this is best. Result is used a lot in the ecosystem, and people will expect any type named Result to behave like std::result::Result does (with its many helper methods).

Two variants I see:

• Methods return std::result::Result<SomeWrapper<T>, window::Error>. The SomeWrapper derefs to the inner T so the inner T's methods can still be used. Passing this value to methods which expect T will require a call to some method like into() or using deref doing *wrapper.
• Methods return windows::Result<T> which derefs to std::result::Result<T, windows::Error> but also contains the success HRESULT. This makes pattern matching more difficult though as the user would either have to deref the value to cast it to std::result::Result or use whatever structure windows::Result<T> uses.

[damyanp]

The vast majority of APIs behave nicely and only ever return S_OK on success. For those that return S_FALSE or any of the other non-error result codes, we should get those tagged in the metadata so that projections can behave differently. I don't think that this problem will be unique to rust.

[kennykerr]

I love @damyanp's suggestion. He's absolutely right of course. The vast majority of APIs don't need this and penalizing every API to accommodate the handful that use odd success codes doesn't make sense. Transferring to the win32 metadata repo - hopefully they can add an attribute to such functions/methods and then languages like Rust can simply project their signatures accordingly.

[knopp]

Didn't even know that was an option :) Sounds great, though I'm not clear on how this would work. I assume every call like this would need to be tagged manually, since there's no way to infer this from headers?

[mikebattista]

You want an attribute that indicates that a function doesn't return S_OK on success?

[damyanp]

This is for functions that return success HRESULTs that aren't S_OK. So it might return S_OK, but they may also return S_FALSE, or some other >0 HRESULT to include additional data about the result.

Generally in projections we'd like to map HRESULTs to exceptions or whatever the idiomatic error handling scheme is for a language. Most (if not all) of these assume that there only one possible way to indicate "success" and don't have a natural way to return "I succeeded, but....".

We can build these things, but it's pretty unnatural and we think that for the vast majority of Win32/COM APIs it would be unnecessary, so we're looking for a way to only do this for the cases where we know that applications want to check for non-S_OK success codes.

[mikebattista]

What would you like to see in the metadata specifically? I understand the scenario but I'm not clear how you want to model this.

[damyanp]

I was thinking of a per-function attribute that indicates that applications might want to inspect successful HRESULTs.

[mikebattista]

So just a boolean? And not a list of all possible success HRESULTs?

[damyanp]

It might be possible for projections to make use of all the HRESULTs, but in practice I suspect most will opt to just pass the HRESULT back to the caller somehow, so a boolean would be fine. @kennykerr might have thoughts on this.

[kennykerr]

Just an attribute on functions/methods that return HRESULT. The attribute doesn't need any values (not even bool). It just signals to the languages that the HRESULT has alternate status codes.

[kennykerr]

So for example:

[DllImport("OLE32", ExactSpelling = true)]
[SupportedOSPlatform("windows5.0")]
[AlternateSuccessCodes] // <-- add this
public unsafe static extern HRESULT DoDragDrop([In] IDataObject pDataObj, [In] IDropSource pDropSource, [In] uint dwOKEffects, [In][Out] uint* pdwEffect);

[kennykerr]

Yep I'm sure folks will be happy to help report/list APIs that need the attribute.

[mikebattista]

Ok. We can add support for an attribute but will likely need to crowd source where to apply it.

[kennykerr]

Great, thanks!

[AArnott]

For this particular requirement, I agree a mere attribute is enough. However if we can document which HRESULTs may be expected, then projections can automatically emit those HRESULTs as constants as well.
Same for Win32Error codes.

It bites if the user asks for one method, then has to explicitly ask individually for 8 other error codes to be generated as well where the projection could have anticipated that given info from the metadata.

[damyanp]

I was wondering if we should extend this concept to cover other error return mechanisms? An extreme example would be WaitForSingleObject. There are also various functions that use the "return bool, use GetLastError" pattern that would be nice to map to idomatic error handling.

Another example here is that many IO functions use the bool/GLE approach, but we expect developers to know that ERROR_IO_PENDING is not actually an error.

We could add a new attribute for each error reporting mechanism (in which case I think we'd need to document that some attributes are mutually exclusive) or we could have a single "error_handling" attribute that has a parameter to indicate how projections should interpret return codes.

[mikebattista]

There are also various functions that use the "return bool, use GetLastError" pattern that would be nice to map to idomatic error handling.

This is #536.

An extreme example would be WaitForSingleObject

WaitForSingleObject returns an enum today which contains all the possible return codes. Projections would need to know though that WAIT_FAILED means call GetLastError to provide a friendlier interface on top. This is similar to #536. I think one requirement is, for APIs with SetLastError = true, if the failure condition can't be inferred from the types, add an attribute with the list of return values that mean call GetLastError.

Another example here is that many IO functions use the bool/GLE approach, but we expect developers to know that ERROR_IO_PENDING is not actually an error.

This is a combination of both issues. The additional requirement from this thread is listing AlternateSuccessCodes in an attribute, ideally with a list of those codes.

Is this correct? I'm not sure we need one "error_handling" mega-attribute to handle all of these requirements. These could be separate attributes.

[damyanp]

I think your understanding matches mine. From my POV I don't have a firm requirement for a one-attribute-to-rule-them-all vs individual attributes to handle the various cases. I thinking about the potential issues with mutually exclusive attributes (eg you maybe you can't have AlternateSuccessCodes along with SetLastError). However, maybe there's extra expressiveness to be had by allowing these attributes to be combined? So maybe ReadFile would be both SetLastError and AlternateSuccessCodes(ERROR_IO_PENDING).

[mikebattista]

ReadFile already has SetLastError = true and returns false when GetLastError should be called. I view that as being able to infer the failure condition from the types and don't think another attribute is necessary for that piece.

ERROR_IO_PENDING is returned by GetLastError, not by ReadFile itself. Is this the same for the other I/O APIs you mentioned that behave like this? That seems a little different than a function that returns S_FALSE for success, which is what the original issue here is about. How would projections want to handle this? Would we need to differentiate return codes vs. GetLastError codes in the attributes?

Reading through https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-readfile#pipes, it looks like ReadFile with pipes can also return false with GetLastError() = ERROR_MORE_DATA which is similar to ERROR_IO_PENDING. Again, what would be a natural way for projections to provide a friendlier interface on top?

[damyanp]

IMO, I can see someone wanting to build a projection where for something like ReadFile it would call GetLastError if it returned false and then return a status code indicating whether or not it completed, or instead hit the IO_PENDING or MORE_DATA condition.

There may be questions about whether or not this behavior would be right for all projections, but I think for some use cases this would be the right thing to do. It's what I personally would like windows-rs to do for example, but I understand that that may not be desirable for everyone.

Maybe this can be summed up (and this might be controversial) as ideally we wouldn't have GetLastError() projected at all. Instead we would ensure that the projection did the right thing and called it as necessary, so you can eliminate the entire class of bugs where someone accidentally calls something that sets the last error before calling GetLastError to log the error. A source of the "Error: the operation completed successfully" message boxes you see from time to time.

[kennykerr]

Ok, thanks. To me it just seemed to give the impression that the attribute could somehow be applied to some other part of the function (which I don't think makes sense).

I guess I'm going to have to figure out how to read attributes on return types. 😉

[kennykerr]

The Windows crate for Rust now supports the AlternateSuccessCodes attribute as of microsoft/windows-rs#1096 - I have tests for DoDragDrop that validate the function signature is preserved.

[kennykerr]

For what its worth, I've implemented the presence of this attribute to have the exact same semantics as DllImportAttribute.PreserveSig.

Indicates whether unmanaged methods that have HRESULT return values are directly translated or whether HRESULT return values are automatically converted to exceptions.

[kennykerr]

So how do we annotate a Win32-style function for a new API so that the WinmdGenerator adds the attribute?

And any thoughts about just using the PreserveSig bit (already defined by ECMA335) rather than using a custom attribute?

[sotteson1]

So how do we annotate a Win32-style function for a new API so that the WinmdGenerator adds the attribute?

And any thoughts about just using the PreserveSig bit (already defined by ECMA335) rather than using a custom attribute?

I think using PreserveSig is a good idea. I'm reopening the issue so I can change to use PreserveSig instead of AlternateSuccessCodes. That doesn't answer your first question about how to annotate new C APIs, though. Any ideas?

[kennykerr]

We could mint a new SAL-style annotation.

#define _WIN32_METADATA_PRESERVESIG_

_WIN32_METADATA_PRESERVESIG_
STDAPI TestFunction();

That would allow us to eventually add it to the Windows SDK for in-box APIs (if desired) but also start using it today for new components and validation without waiting for a new SDK.

[riverar]

Should we also take this opportunity to move the attribute back to function-level? It doesn't really make sense to me to attach this attribute to the return type itself.

[kennykerr]

The ECMA 335 PreserveSig bit is a flag on the MethodDef table.

[riverar]

@kennykerr Ah great, fixed itself! 👍

[AArnott]

In CsWin32 we default to preserving non-preservesig signatures, but users can specifically choose PreserveSig for certain methods that return multiple success codes. Is the proposal here then that the metadata express that so users don't have to?

[kennykerr]

Yes.

[kennykerr]

Another customer blocked by this issue: microsoft/windows-rs#1371

[mikebattista]

https://task.ms/37680914

[sotteson1]

The ECMA 335 PreserveSig bit is a flag on the MethodDef table.

My fix will be to use this instead of using the attribute I was using before.

[kennykerr]

Just tried this in the latest update. I see the AlternateSuccessCodes was removed and the PreserveSig bit is set in its place.

The trouble is that hundreds of other functions also have this set. I don't know if they were set before but I can't see how I can use the new bit in place of AlternateSuccessCodes if its applied so liberally.

[riverar]

Yeah seems to be set for all APIs.

win32metadata/sources/ClangSharpSourceToWinmd/ClangSharpSourceWinmdGenerator.cs

Lines 1182 to 1187 in 27d2eb9

	var methodDef =
	this.AddMethodViaSymbol(
	symbol,
	MethodAttributes.Public | MethodAttributes.Static | MethodAttributes.HideBySig | MethodAttributes.PinvokeImpl,
	MethodImplAttributes.Managed | MethodImplAttributes.PreserveSig,
	false);

Doesn't appear using PreserveSig is appropriate and we should restore the method attribute. (Or as Kenny mentioned, remove it from the world sans the chosen few.)

[kennykerr]

Or remove it from all methods and just apply it to those that actually need it.

[sotteson1]

Or remove it from all methods and just apply it to those that actually need it.

Yes, that's the right way to fix it.