-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Use the same method to generate DllImportAttribute in local methods and P/Invoke forwarders #103973
Use the same method to generate DllImportAttribute in local methods and P/Invoke forwarders #103973
Conversation
Tagging subscribers to this area: @dotnet/interop-contrib |
Can we add a test for this? |
...ibraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs
Outdated
Show resolved
Hide resolved
...ibraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs
Outdated
Show resolved
Hide resolved
Perhaps I'm missing some nuance here, but how do we get into the valid situation where we need this set on the generated local p/invoke and not just the forwarded one? From what I can tell, we must:
Shouldn't we have errored on (2)? Or determined that we didn't have marshalling info in (1), so should forward? |
This is only applicable on the dotnet/runtime-only downlevel support, where we'll generate a source-generated stub for whatever parameters we can and forward whatever parameters we can't marshal (technically we also do this in main, but we emit errors in that case). |
Oh, I thought the dotnet/runtime-only down-level support switched to the direct forwarder if any parameters were ones we can't marshal. And that we always emitted the error if we ended up in the state of generating a stub with parameters we can't marshal. |
Is that true? It looks like we have tests that expect a forwarder stub if we can't marshal all the parameters: Lines 524 to 531 in c5e8f83
|
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.
LGTM, but I defer to @elinor-fung and @jkoritzinsky for final sign-off.
@@ -724,7 +785,7 @@ public class Basic { } | |||
await test.RunAsync(); | |||
} | |||
|
|||
|
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.
@@ -438,37 +438,24 @@ private static MemberDeclarationSyntax PrintForwarderStub(ContainingSyntax userD | |||
{ | |||
Debug.Assert(!options.GenerateForwarders, "GenerateForwarders should have already been handled to use a forwarder stub"); | |||
|
|||
// StringMarshalling.Utf16 is the only value that has a supported analogue in DllImportAttribute.CharSet. If it's not Utf16, consider StringMarshalling unset |
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.
Unsure if this is worth quibbling about, but CharSet.Ansi
on non-Windows is Utf8 if I recall correctly. Based on certain reg keys on Windows it can also be Utf8. I don't know if this makes a difference in this case, but it is something to note.
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.
The change looks good to me. I do want to make sure we understand the down-level support and dotnet/runtime-only downlevel support behaviour/expectations previously commented on though:
This is only applicable on the dotnet/runtime-only downlevel support, where we'll generate a source-generated stub for whatever parameters we can and forward whatever parameters we can't marshal (technically we also do this in main, but we emit errors in that case).
Is that true? It looks like we have tests that expect a forwarder stub if we can't marshal all the parameters:
I think it'd be good to have documentation and tests (or just doc for dotnet/runtime-only, since I don't think we have a way to test that) for the expected behaviour when not all parameters can be marshalled:
- when do we print a simple forwarder stub versus marshal what we can and forward the rest
- do we emit a diagnostic on what we can't marshal or just forward silently
- what is the difference in behaviour for this scenario in down-level and runtime-only down-level support (also non-down-level / current version, but I think we have a decent handle on and coverage of that)
The case of 'we generate a stub that marshals the parameters we can and forwards the remainder, without erroring' was surprising to me.
@@ -438,37 +438,24 @@ private static MemberDeclarationSyntax PrintForwarderStub(ContainingSyntax userD | |||
{ | |||
Debug.Assert(!options.GenerateForwarders, "GenerateForwarders should have already been handled to use a forwarder stub"); | |||
|
|||
// StringMarshalling.Utf16 is the only value that has a supported analogue in DllImportAttribute.CharSet. If it's not Utf16, consider StringMarshalling unset |
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.
If it's not Utf16, consider StringMarshalling unset
Shouldn't it have errored in this case? I know we have a test around down-level forwarding of StringMarshalling.Utf8
or StringMarshalling.Custom
.
Line 242 in 0015867
public async Task StringMarshallingForwardingNotSupported_ReportsDiagnostic() |
Or am I missing something about dotnet/runtime-only down-level versus down-level support?
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.
It should error in down-level forwarding of UTF-8 and StringMarshalling.Custom, but this code path is taken in the normal path of LibraryImport in net7+, so we don't want to unconditionally warn here. Ideally this should warn when we hit this path for down-level support with partial marshalling, but we don't right now either.
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.
LGTM once @elinor-fung's comments are resolved.
@jtschuster Are we waiting on something specific for this PR to go in? |
Was this superceded by #104416? |
Yes. Closing. |
We should forward the LibraryImpot.StringMarshalling as DllImport.CharSet to local P/Invokes too, not just forwarded DllImport's.