Skip to content
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

Update the ConvertToGeneratedDllImport code-fix to support stripping out PreserveSig and transforming the signature when PreserveSig was set to true #65169

Merged
merged 2 commits into from
Feb 16, 2022

Conversation

jkoritzinsky
Copy link
Member

This PR updates the code-fix to add the existing return type as an out parameter at the end of the signature and update callers in the same document to use the new signature (and throw an exception with a corresponding failing HRESULT).

The design follows the "Extract Local Function" and "Make `DynamicInterfaceCastable method static" code fixers in dotnet/roslyn-analyzers

Open questions:

  • Do we want to search for callers in the whole solution, or do we want to only search for them in the current document? Solution-wide is more difficult, but doable if we really want to.
  • Prior to Remove GeneratedDllImportAttribute.PreserveSig #65164, we supported PreserveSig = false by throwing before unmarshalling. Is it more desirable to replicate that behavior, or is throwing after unmarshalling okay?

…out PreserveSig and transforming the signature when PreserveSig was set to true.
@jkoritzinsky jkoritzinsky added area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature labels Feb 11, 2022
@ghost ghost assigned jkoritzinsky Feb 11, 2022
@ghost
Copy link

ghost commented Feb 11, 2022

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

Issue Details

This PR updates the code-fix to add the existing return type as an out parameter at the end of the signature and update callers in the same document to use the new signature (and throw an exception with a corresponding failing HRESULT).

The design follows the "Extract Local Function" and "Make `DynamicInterfaceCastable method static" code fixers in dotnet/roslyn-analyzers

Open questions:

  • Do we want to search for callers in the whole solution, or do we want to only search for them in the current document? Solution-wide is more difficult, but doable if we really want to.
  • Prior to Remove GeneratedDllImportAttribute.PreserveSig #65164, we supported PreserveSig = false by throwing before unmarshalling. Is it more desirable to replicate that behavior, or is throwing after unmarshalling okay?
Author: jkoritzinsky
Assignees: -
Labels:

area-System.Runtime.InteropServices, source-generator

Milestone: -

@AaronRobinsonMSFT
Copy link
Member

Do we want to search for callers in the whole solution, or do we want to only search for them in the current document? Solution-wide is more difficult, but doable if we really want to.

Ugh. I'd assume this is going to need to be solution wide. I personally don't think we should be altering call-sites all that much but that isn't a popular opinion.

is throwing after unmarshalling okay?

That can get tricky. This requires the state of the outs to be valid and that isn't always the case for out params of native functions. I can recall hitting older Win32 APIs that left outs in a dubious state, but I doubt that is any issue any longer.

@jkoritzinsky
Copy link
Member Author

Do we want to search for callers in the whole solution, or do we want to only search for them in the current document? Solution-wide is more difficult, but doable if we really want to.

Ugh. I'd assume this is going to need to be solution wide. I personally don't think we should be altering call-sites all that much but that isn't a popular opinion.

We do warn the user if we can't change all of the call-sites, so the experience isn't terrible. I'm fine holding off on this work until we get feedback that document-wide isn't enough.

is throwing after unmarshalling okay?

That can get tricky. This requires the state of the outs to be valid and that isn't always the case for out params of native functions. I can recall hitting older Win32 APIs that left outs in a dubious state, but I doubt that is any issue any longer.

I think we'll hold off on adding a custom marshaller then until we get feedback that there's APIs that don't work correctly with the new ordering.

@jkoritzinsky jkoritzinsky merged commit 8cd701a into dotnet:main Feb 16, 2022
@jkoritzinsky jkoritzinsky deleted the preservesig-transform branch February 16, 2022 00:49
@ghost ghost locked as resolved and limited conversation to collaborators Mar 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants