-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Respect ISupportErrorInfo
for HRESULT
to Exception
throwing conversion
#117690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Tagging subscribers to this area: @dotnet/interop-contrib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements support for respecting the ISupportErrorInfo
interface when converting HRESULT values to exceptions in COM interop scenarios. The changes add a new overload to Marshal.GetExceptionForHR
and Marshal.ThrowExceptionForHR
that takes an interface ID and COM object pointer to check if the interface supports error information before using the error details.
Key changes include:
- Adding new public API overloads that check
ISupportErrorInfo
before using error information - Updating both built-in COM and source-generated COM to use the enhanced error handling
- Removing legacy native code implementations in favor of managed implementations
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
System.Runtime.InteropServices.cs | Adds new public API overloads for GetExceptionForHR and ThrowExceptionForHR with ISupportErrorInfo support |
Marshal.cs | Implements the core logic for checking ISupportErrorInfo and calling appropriate error handling |
StubHelpers.cs | Updates COM stub helpers to use new managed implementation instead of native QCall |
ManagedHResultExceptionGeneratorResolver.cs | Updates source generator to pass interface ID and object pointer to new API |
ThrowExceptionForHRTests.cs | Adds comprehensive tests for the new ISupportErrorInfo functionality |
Multiple native files | Removes obsolete native QCall implementations for COM exception handling |
Comments suppressed due to low confidence (1)
src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/Marshal/ThrowExceptionForHRTests.cs:114
- This test assertion checks that an InvalidOperationException is returned when using an ArgumentException's HResult. This seems inconsistent - if the error info contains an InvalidOperationException but we're passing an ArgumentException's HResult, the test should verify that the error info takes precedence over the HResult when ISupportErrorInfo returns S_OK.
Assert.IsType<InvalidOperationException>(Marshal.GetExceptionForHR(new ArgumentException().HResult, iid, pUnk));
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
...opServices/gen/ComInterfaceGenerator/Marshallers/ManagedHResultExceptionGeneratorResolver.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs
Outdated
Show resolved
Hide resolved
...InteropServices.UnitTests/System/Runtime/InteropServices/Marshal/ThrowExceptionForHRTests.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should leave #116917 open until we have tests for the generator or file a new issue to track adding tests.
This has added testing for the API. I don't think we need testing for the COM source generator in this case. The fact that we use the API should be sufficient. |
I'm saying this is okay because we already have error related testing. I don't think specifically testing the |
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
…pServices/Marshal.cs Co-authored-by: Aaron Robinson <arobins@microsoft.com>
/ba-g wasm timeouts for legs that don't run these tests. |
Add the API proposed in #117438
Update built-in COM to use it.
Update source-generated COM to use it.
Fixes #117438
Fixes #116917