Skip to content

Add custom exception handlers to generated COM wrappers #114828

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Add custom exception handlers to generated COM wrappers #114828

wants to merge 7 commits into from

Conversation

dongle-the-gadget
Copy link

Fixes #109522

@dongle-the-gadget
Add custom exception handler to COM Wrappers
08f039e 
This is an initial iteration to add custom exception handlers to
generated COM wrappers, through a new ExceptionToUnmanagedMarshaller
property in GeneratedComWrappersAttribute

Fix dotnet#109522
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 19, 2025
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

@dongle-the-gadget
Copy link
Author

@dotnet-policy-service agree

@dongle-the-gadget dongle-the-gadget changed the title Add ExceptionToUnmanagedMarshaller to GeneratedComInterfaceAttribute Add custom exception handlers to generated COM wrappers Apr 19, 2025
@dongle-the-gadget
Copy VTable Index exception logic over
1a66148 
Resolves an issue with previous implementation, highlighted by Jeremy.
The previous implementation is overcomplicated and hooks into the
default exception logic, which is not ideal. This commit copies the
existing custom exception marshalling logic from the VTable stub
generator.
@dongle-the-gadget
Remove redundant marshaller in interface data
d5cef7e
@dongle-the-gadget
Remove redundant metadata name logic
5319da6
jkoritzinsky
jkoritzinsky reviewed Apr 19, 2025
View reviewed changes
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nits, but looking much better!

dongle-the-gadget and others added 2 commits April 19, 2025 11:25
@dongle-the-gadget @jkoritzinsky
Remove leftover System.Text namespace
e86c58f 
Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
@dongle-the-gadget @jkoritzinsky
Remove leftover null pattern matching
cf6a5bf 
Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
This was referenced Apr 19, 2025
Open
Open
Open
Open
Closed
AaronRobinsonMSFT
AaronRobinsonMSFT reviewed Apr 19, 2025
View reviewed changes
...stem.Runtime.InteropServices/gen/ComInterfaceGenerator/GeneratedComInterfaceAttributeData.cs
{
if (exceptionToUnmanagedMarshaller.Value is not INamedTypeSymbol)
{
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally don't use null for logical flow. This would mean the return type should be updated. I don't think we should be failing in this case as it will silent and difficult to reason about. A diagnostic would be more appropriate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was based on InteropAttributeDataExtensions.WithValuesFromNamedArguments, which returns null if an invalid StringMarshalling type is passed. I'm not sure how this is handled particularly by GeneratedComInterfaceAttributeData.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is another one of the cases where we use null as our "unable to parse" sentinel (and in the caller we check for null and emit a diagnostic and replace with a sentinel). Can you confirm?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it looks like there's no regards to null-checking here. Even today if you pass in an invalid type for StringMarshallingCustomType, i.e.

[GeneratedComInterface(StringMarshalling = StringMarshalling.Custom, StringMarshallingCustomType = typeof(string*))]
public partial interface Foo;

the generator fails with a generic error message from Roslyn:

Generator 'ComInterfaceGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'NullReferenceException' with message 'Object reference not set to an instance of an object.'

@dongle-the-gadget @AaronRobinsonMSFT
Remove extra colon
d9804bb 
Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@build-analysis build-analysis bot mentioned this pull request Apr 20, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkoritzinsky jkoritzinsky jkoritzinsky left review comments

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT left review comments

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area-System.Runtime.InteropServices community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
No milestone
3 participants
@dongle-the-gadget @jkoritzinsky @AaronRobinsonMSFT