Annotate members that return failing HRESULTs in success paths

[AArnott]

When APIs are declared in the metadata without preservesig, some projections (e.g. CsWin32) will produce an API that must throw an exception when a failing HRESULT is returned. This actually provides a good C# coding experience most of the time. But when an API regularly returns failing HRESULTs, even in 'success' paths (e.g. reaching the end of a list), this raises runtime cost and coding complexity due to the need to throw and handle exceptions where a simple return value check would have been preferable.

In #1295, we learned that Rust has a counter-interest where they want to see preservesig only for methods that return non-S_OK success codes. They want the metadata to omit preservesig even if a method returns a failure code regularly. This makes sense, since rust has no exceptions.

We need to define an annotation for the metadata to apply to methods that return error codes in success paths so that it doesn't disrupt rust, but gives CsWin32 the hint it needs to apply PreserveSig semantics to the C# declaration.

In the meantime, this issue will aggregate the list of all APIs that we intend to apply this annotation to when it is invented.

APIs follow:

• DXGIFactory::EnumAdapters
• IDXGIFactory1::EnumAdapters1
• IDXGIAdapter::EnumOutputs
• ID2D1RenderTarget::EndDraw

If you have an API to add to this list, edit this description to add it, or comment below.

Also note that CsWin32 has a user-applicable workaround for any of these APIs that can unblock them. The NativeMethods.json file can include a preservesig property where you can list any particular API that you want declared with PreserveSig:

  "comInterop": {
    "preserveSigMethods": [ "ID2D1DCRenderTarget.EndDraw" ]
  }

[riverar]

Summary

I read over the various issues; It's my understanding:

• Rust (@kennykerr) wants a flag to note when an API can return multiple success values, so he knows when to drop the HRESULT->Result<T> transformation during code generation. (Result has storage for all sorts of error codes, so returning various error values is not a concern.)

• C# (@AArnott) wants a flag to note when an API during regular operation may need to return error values, so he can follow .NET PreserveSig=true semantics during code generation. (That is, let users work with expected HRESULTs without having to handle exceptions.)

• ECMA-335 does not provide any useful mediation; it is only documented as a Microsoft implementation defined bit.

Suggested way forward

1. APIs that only have alternative success values (e.g. S_FALSE, S_OK) are marked with [CanReturnMultipleSuccessValues].
2. APIs that may return commonly expected errors (e.g. EnumAdapters returning DXGI_ERROR_NOT_FOUND to signify the end of the adapter list) are marked with [CanReturnErrorsAsSuccess].

These names are subject to change, of course.

Concerns

I believe the C# ask boils down to the annotation of functions where exception handling may be annoying during normal usage. Application of this attribute feels subjective though. Do we apply the [CanReturnErrorsAsSuccess] attribute equally across all APIs that return errors in all success paths? Or do we consider rarity of such an event in the determination? And if so, who makes that call, the community? How do we resolve disagreements?

Edit: Removed [expensive] qualifier on exceptions.

[AArnott]

And if so, who makes that call, the community? How do we resolve disagreements?

Yes, there is a judgment call to be made. My proposal is that it has to be a strong majority of folks who think it's a good idea (say, 80% if I had to put a number on it) before we default to preservesig in C# for an API. Folks can always opt into preservesig, so it's only a big problem if preservesig is thrust on them (at least for now, because we don't offer a way to opt out).

Your proposal of using two attributes certainly makes it balanced, but what about the preservesig attribute in the metadata? When would we use that? Maybe only when something about the API actually requires it? (when might that be?) Or do we always apply it?

[riverar]

I'd suggest eliminating PreserveSig usage in favor of the two attributes above. (Or in other words, rename it to match the behavior of one of the suggested attributes above, then add another one.)

[AArnott]

The identification of the two scenarios is the critical part. We'd use preservesig except it can only handle (either) one of them. Abandoning it and inventing two more mechanisms doesn't make sense to me. I'm leaning toward a vote to preserve its current use (CanReturnMultipleSuccessValues) and add the CanReturnErrorsAsSuccess attribute.

[riverar]

Sounds ok to me. My intention was to create language-neutral attributes with additional clarity in the name. Re-using PreserveSig taints metadata a bit with .NET-only attributes and binds us to .NET implementation-compatible usage. (In this case, it's what we want so not really an issue.)

[AArnott]

I see your point. If the ECMA standard doesn't define its meaning than we are weighting the .NET behavior by applying its meaning to the metadata. In that case we could go either way. Although the projections already are interpreting this bit, so removing its use in favor of two more attributes would be a breaking change, whereas just adding an attribute to capture the CanReturnErrorsAsSuccess use case would enable CsWin32 to move forward without the breaking change.

[mikebattista]

@kennykerr

So if we switch the meaning of PreserveSig to match the .NET scenario and introduce AlternateSuccessCodes like we had before, does that address all the concerns?

[kennykerr]

I like Andrew's suggestion here to avoid more breaking changes: #1315 (comment)

But either way I can live with it.

[mikebattista]

I don't like that PreserveSig is being used in a way today that doesn't align with its original semantics, and that was an artifact of Rust's specific interpretation of it at the time it was introduced.

I'd rather either align with its original semantics and add an attribute for the other scenario or use 2 separate attributes that are distinct from PreserveSig.

[kennykerr]

Sure, happy with an explicit attribute - just let me know and I'll update the Rust parser.

[AArnott]

@mikebattista, I think we must make the preservesig flag align with whether the return type on the method is the real, actual return type. This is what .NET interprets it as. It is more of a convention that folks declare methods with preservesig based on either multiple success codes or error codes in success paths. AFAIK the metadata is already honoring the preservesig flag meaning the return type is literally meant as the return type.
Though I guess the metadata has already been customized somewhat so that preservesig (both the flag and the adjustment to the method signature) is applied to some methods based on one of the two reasons we would use it in .NET.

[mikebattista]

PreserveSig so far has only been applied for the AlternateSuccessCodes scenario. We can migrate those over to a new attribute. Are you saying using PreserveSig for the "error codes in success paths" is also not a good use of PreserveSig? If so, then we'd need a second attribute.

Is there a set of scenarios that we would just bucket under PreserveSig and encode that in the metadata? Or do we expect that each scenario will have different enough semantics and interpretations across languages that we should define explicit attributes for new scenarios going forward?

[AArnott]

I'm saying that whatever we use PreserveSig for must match the signature (as it does, and no one is proposing anything differently) or else projections would malfunction all over the place. By that I mean:

[PreserveSig]
HRESULT SomeMethod();
// OR
void SomeMethod();

When PreserveSig is absent, projections must assume that the return type is actually HRESULT though the signature does not show it.
The above is non-negotiable.

What is usually negotiable is whether the metadata declares COM methods as the first or second option. As long as the method actually returns HRESULT, we can declare it either way in the metadata. But if a COM interface method returns something other than HRESULT, then it must be declared with PreserveSig.

So, that leaves the question for the HRESULT-returning COM methods: Should they use PreserveSig always, or never, or sometimes?

Right now, that policy question lands where Rust finds useful, which is 'multiple success codes => preservesig'.
Changing that policy should serve some Good, and the only possibility there that we've identified so far is to match what CsWin32 would find useful, which would be 'multiple success codes or error codes is success paths => preservesig'.
Helping .NET would hurt Rust. No net win there. Just switching favorites. It would make CsWin32 "just work" and Rust would have a change to make.

Either way, rust and cswin32 have diverging interests, so preservesig alone cannot capture everything for everyone. We need to represent 3 states, not just 2. A new attribute would suffice. Two new attributes would also suffice, and leave PreserveSig to be used strictly where it is absolutely needed (non-HRESULT returning COM methods).

So is two attributes and a 'pure' use of PreserveSig be better than one attribute with an expanded use of preservesig? Possibly. It would mean a bit more work for rust (and of course for CsWin32) but @kennykerr has graciously said he's willing, and I'm happy to deal with either for CsWin32 as well.

[mikebattista]

So is two attributes and a 'pure' use of PreserveSig be better than one attribute with an expanded use of preservesig? Possibly. It would mean a bit more work for rust (and of course for CsWin32) but @kennykerr has graciously said he's willing, and I'm happy to deal with either for CsWin32 as well.

Yes I would prefer 2 attributes + 'pure' use of PreserveSig based on everything that's been said.

[mikebattista]

Are we ok with #1315 (comment) for the attribute definitions?

[AArnott]

Yes. Sounds good.

[riverar]

I'm confused how a third attribute appeared. PreserveSig can fulfill the role of one of the two proposed attributes right? (How PreserveSig is handled in .NET is not relevant to metadata. I did try to rename this...)

[AArnott]

The reason I gathered that Mike wanted to use two attributes is that the semantics behind each of them are not the same as the effect that preservesig has. There are some methods in the metadata that must be preservesig, not because of its return values but because of its return type not being HRESULT.
Now, we could make every method in the metadata be preservesig, but I don't see that serving any purpose, and would probably lead to a worse experience... somewhere. And that wouldn't help reduce the number of attributes from 2 to 1 either.

The only way to get to just one attribute is to overlay preservesig with responsibility for one of the two attributes in order to avoid that attribute. Mike's position is that we shouldn't conflate these. While I could go either way, I'm happy to support that as I see the merit of it.

[kennykerr]

Yes, I lean toward the two attributes as well. PreserveSig implies an action while the attributes merely describe a characteristic.

[mikebattista]

Yes to all of the above. Self-descriptive attributes will better communicate usage and semantics to all projections and is more future-proof.