Using PreserveSig by default for IDXGIFactory::EnumAdapters and similar functions

[atcarter714]

Hopefully I understood Mr. @AArnott correctly and I'm posting this in the right place, if not forgive me.

I've been consuming the win32metadata and getting my bearings with CsWin32, and I came across something I think could be an improvement. In particular, I'm referring to variants of IDXGIFactoryX::EnumAdaptersX, IDXGIAdapterX::EnumOutputsX and similar functions (where "X" is the number or lackthereof of the name of COM interface versions).

We expect these function calls to return a DXGI_ERROR_NOT_FOUND when we pass up the last index / device ordinal on the local machine, so enumerating adapters will generally fail with that HRESULT at least one in 3 or 4 times (assuming most gaming systems are generally going to have a CPU with embedded graphics, a dedicated GPU and everything has the Microsoft Basic Render Adapter, the software renderer, and some users may have dual GPUs although not as commonly). It's a similar case with enumerating outputs per adapter: most of the time you have one output, some psychopaths like me may have 3 or 4 but then we expect that DXGI_ERROR_NOT_FOUND return code to come.

Throwing a ComException in this case just doesn't seem quite appropriate ... and it made me write some pretty ugly C# code with a try/catch block to enumerate devices and break my loop when an exception is thrown. I would propose that this be PreserveSig and more closely mirror the original DXGI function signatures to return HRESULT.

Currently, CsWin32 is generating this signature:
new void EnumAdapters1(uint Adapter, out IDXGIAdapter1 ppAdapter)

I think it should instead return HRESULT so client code can check for the expected DXGI_ERROR_NOT_FOUND when devices are fully enumerated through to break their loop, and they can throw their own ComException if a different, exceptional HRESULT comes back. Would definitely make consuming this a bit cleaner.

[mikebattista]

Could you clarify the full set of APIs that need PreserveSig? Is it just EnumAdaptersX and EnumOutputsX? If not, what are the other "similar functions" that require this?

[atcarter714]

Could you clarify the full set of APIs that need PreserveSig? Is it just EnumAdaptersX and EnumOutputsX? If not, what are the other "similar functions" that require this?

Those are the ones I know for sure, I will be hunting for any others which have a similar usage pattern but can't think of them off the top of my head. I'm working on a little wrapper library so I encounter this stuff as I work my way through the API. I will definitely provide any specific signatures with this same issue when I encounter them though.

[mikebattista]

Windows.Win32.Graphics.Dxgi.IDXGIAdapter.EnumOutputs : => [PreserveSig]
Windows.Win32.Graphics.Dxgi.IDXGIFactory.EnumAdapters : => [PreserveSig]
Windows.Win32.Graphics.Dxgi.IDXGIFactory1.EnumAdapters1 : => [PreserveSig]

[mikebattista]

I fixed the ones above. Feel free to reopen or file a new issue if you find others.

[atcarter714]

Thanks @mikebattista I will be on the hunt through the DirectX APIs (11 and 12, at least) for any more, and of course any other Win32/COM issues similar to this. Loving the stuff being done here! <3

[kennykerr]

I just came across this change while trying to update the Rust project to the use the latest metadata. This is not an appropriate application of the PreserveSig attribute. This attribute is meant to be used on functions or methods that may return alternative success codes, such as S_FALSE. Without this attribute, it is impossible to retrieve and test against such alternative success codes.

But the PreserveSig attribute is specifically not for functions like this where S_OK is the only possible success code as it signficantly degrades the calling patterns for some languages that rely on the usual transformations. It may be unfortunate that some languages choose to turn failure error codes into exceptions, but that is a language-specific decision that should not be "fixed" by changing the metadata to incorrectly imply that alternative success codes may be used.

Reopening as this blocks Rust adoption.

[atcarter714]

This is not an appropriate application of the PreserveSig attribute. This attribute is meant to be used on functions or methods that may return alternative success codes, such as S_FALSE.

Yikes, I thought it had a more general application for simply keeping the original native API signatures and that the S_FALSE situation was just an example of its usefulness. Didn't understand that it was very specific in nature and had further-reaching implications. Appreciate the explanation.

I guess maybe we were attempting to implement my suggestions at the wrong level in this stack of projects then? If that is the case, would that mean we'd need to deal with this within the context of the CsWin32 code generator project? 🤔

[KalleOlaviNiemitalo]

Could perhaps define FrequentlyFailsAttribute for this purpose and make the C# projection use it.

Meaning: This function frequently returns a failure code in normal use and the projection should make such failures efficient even if that needs more work from callers.

[kennykerr]

I guess maybe we were attempting to implement my suggestions at the wrong level in this stack of projects then? If that is the case, would that mean we'd need to deal with this within the context of the CsWin32 code generator project?

👍

[mikebattista]

I reverted the changes for now.

[kennykerr]

Thanks, I'll give the next release a try.

[AArnott]

it signficantly degrades the calling patterns for some languages that rely on the usual transformations

Well, what's bad for rust is good for C#, and vice versa.

I was not aware of the policy @kennykerr references that PreserveSig should only be used for non-S_OK success codes. IIRC I was the one that evangelized caring about the preservesig flag in the metadata in the first place, and gave the use cases as both S_FALSE and common error codes.
CsWin32 can live with the policy Kenny suggests, provided we find another way to annotate these methods as returning failure codes in success paths. @KalleOlaviNiemitalo's suggestion of an attribute would be fine.

[kennykerr]

We really need to get to a point where we actually document the Win32 metadata to avoid such confusion or ambiguity. Then we can wrestle through these definitions and move on with clarity.

[kennykerr]

I'll just add that I don't mind having two different concepts/attributes but they need to be distinguishable. My understanding of PreserveSig is fairly universal in the sense that it completely hides or exposes alternative success codes in Rust/C#/C++ whereas alternative error codes are always available and its more about the programming model that a particular language favors for exposing error information.

[atcarter714]

What if we had an attribute like "ExpectedErrorsAttribute" or "ExpectedHResultsAttribute" .... usage for this situation might be something like:

[ExpectedHResults( 0x887A002 )]

That's a signal to code gens and other tools that they can expect the call to somewhat frequently result in HRESULT: 0x887A002 and the code gen can then take the appropriate actions in its language (C# in my case, Rust may choose to ignore it if there's another way of dealing with it -- idk any Rust yet). In this case, it would tell CsWin32 that 0x887A002 aka DXGI_ERROR_NOT_FOUND is not "exceptional" and we shouldn't throw anything but instead return that information to the caller ...

[AArnott]

@atcarter714 Are you suggesting that the method carry an HRESULT return type but still throw for failing HRESULTs except where the value of that HRESULT matches something in a list? That's too inconsistent with existing patterns for my taste. I think it won't match user expectations and thereby lead to too many bugs.

The strong precedent that .NET gave us is that HRESULT methods should always return, otherwise they should throw for failure cases. So in CsWin32 we seek to default to throwing, unless we know that failure codes will be typical, in which case we declare the method as returning HRESULT and then we never throw, no matter what the HRESULT ends up being.

[atcarter714]

@atcarter714 Are you suggesting that the method carry an HRESULT return type but still throw for failing HRESULTs except where the value of that HRESULT matches something in a list? That's too inconsistent with existing patterns for my taste. I think it won't match user expectations and thereby lead to too many bugs.

The strong precedent that .NET gave us is that HRESULT methods should always return, otherwise they should throw for failure cases. So in CsWin32 we seek to default to throwing, unless we know that failure codes will be typical, in which case we declare the method as returning HRESULT and then we never throw, no matter what the HRESULT ends up being.

Not necessarily, more along the lines of telling consumers ahead of time to expect this and deal with it how your language and conventions needs to deal with it. I get your point though, and you're right, that would be an inconsistent behavior. But I think the attribute itself is a good idea and pretty clearly conveys the information a code generator needs to handle the debacle appropriately.

Edit: And it can simply be ignored by languages which don't support exceptions or have a different way of addressing these situations.

[atcarter714]

@AArnott To elaborate on that thought a bit more, the sort of behavior you described (don't throw for 0x887A002 but throw for unexpected HRESULTs) is how I would handle things at the "higher", application level, not in the generator itself. And I agree it would be weird to generate interop code that returns HRESULT but sometimes throws and sometimes returns (breaks conventions, as you pointed out).

Like I originally suggested, I just want us to be able to generate a PInvoke function for EnumAdapters that returns HRESULT like the original/native one, so I can use a straightforward while( (hr = factory.EnumAdapters( i++, out var pAdapter)) != DXGI_ERROR_NOT_FOUND ) sort of pattern to enumerate graphics adapters, rather than being forced to have this odd sort of loop wrapped in try with a catch( COMException ex ) block that then has to check the HRESULT of ex to see if it's the expected 0x887A002 value meaning we've successfully iterated through all adapters and reached the end, in which case the job is done. That ugly try/catch pattern also causes a line of red warning text in the Output window informing you an exception just occurred, even though it's really not a true "error" per say, and that can be a little confusing ... when I first saw that in the output I was concerned and had to go confirm that it was coming from this and not something else.

I think this sort of attribute I'm suggesting is a pretty expressive and straight-forward way to let all languages and code generators know exactly what the native function is designed to do and what to expect from it so they can respond appropriately. And I'm wondering if we could carry over that attribute into generated C# code too so that user code can reflect on it and have more valuable information about it? Or is there some other way an application would have to go about obtaining that? Because I can imagine that at some point in the future I might make my own tools that will do some cool stuff with the generated code and it'd be useful to have that data available through reflection!

[kennykerr]

An attribute seems fine as long as it doesn't call out specific error codes. Windows APIs aren't generally guaranteed to return a closed set of error codes.

[atcarter714]

An attribute seems fine as long as it doesn't call out specific error codes. Windows APIs aren't generally guaranteed to return a closed set of error codes.

Not sure if I follow you there, what I'm suggesting is that it's merely a bit of additional information about the API call and what you can expect it to sometimes return. It's not an explicit guarantee of any particular result(s), just a heads up that there are possible non-zero values to think about which aren't necessarily "bad".

[kennykerr]

I'm not sure this is something that belongs in metadata as I can't imagine how language projections would do anything useful with it. That rightly belongs in API documentation.

I'll just leave a pointer back to the original issue where support for alternative success code was originally discussed and adopted:

#559 (comment)

This was originally an [AlternateSuccessCodes] attribute that was later replaced with the PreserveSig bit - happy to go back to using an [AlternateSuccessCodes] attribute on these methods so that the PreserveSig bit can have the broader meaning that @AArnott was expecting. I would then simply ignore the PreserveSig bit. I think that should address everyone's concerns.

[atcarter714]

@kennykerr ah ok, I get what you're saying now, I think: it seems a bit out of scope for winmd to you. You could indeed be right, as I'm just getting into these projects and learning my way around them right now. But it still seems to me like it would be pretty benign and non-intrusive to any code which doesn't care about it but at the same time could be valuable to those who do (including user's software and tools that consume winmd). If it seems out of scope to everyone or is asking too much though I will chill and forget about it though, haha! 😁

[AArnott]

The metadata is primarily populated by scraping the header files. Adding anything beyond that is super labor intensive, debatable, and unlikely to ever be complete. So while we do go beyond the header files in some cases, I think we should be very thoughtful and choose carefully where our effort goes. For example, getting enums right tends to be a highly requested and impactful feature. As is whether we return HRESULT or void.
While I can think of some cool things we could do in CsWin32 if the metadata included the likely error codes (e.g. generate the HRESULT constants you'll likely need without you having to ask for each one), listing those error codes for each method is likely to be so spotty in coverage that it confuse and mislead users more than help them.

I don't care too much whether we use the attribute to indicate what Rust needs or what C# needs. But we need an attribute, since the preservesig bit alone isn't descriptive enough. I'd support whoever sends the PR to introduce the attribute and apply it to the first APIs in making the decision to go either way on this.

[AArnott]

Closing as a duplicate of #1315.