Add RequiresUnreferencedCode to DefinePInvokeMethod #56537
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to 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. |
| @@ -355,7 +355,9 @@ public partial class ModuleBuilder : System.Reflection.Module | |||
| public System.Reflection.Emit.MethodBuilder DefineGlobalMethod(string name, System.Reflection.MethodAttributes attributes, System.Reflection.CallingConventions callingConvention, System.Type? returnType, System.Type[]? requiredReturnTypeCustomModifiers, System.Type[]? optionalReturnTypeCustomModifiers, System.Type[]? parameterTypes, System.Type[][]? requiredParameterTypeCustomModifiers, System.Type[][]? optionalParameterTypeCustomModifiers) { throw null; } | |||
| public System.Reflection.Emit.MethodBuilder DefineGlobalMethod(string name, System.Reflection.MethodAttributes attributes, System.Type? returnType, System.Type[]? parameterTypes) { throw null; } | |||
| public System.Reflection.Emit.FieldBuilder DefineInitializedData(string name, byte[] data, System.Reflection.FieldAttributes attributes) { throw null; } | |||
| [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Constructors of SafeHandle or blittable classes passed as arguments or returned can be trimmed if not referenced directly.")] | |||
There was a problem hiding this comment.
@MichalStrehovsky @vitek-karas @sbomer @AaronRobinsonMSFT - Is it only constructors that could be an issue? Or are there other things we should let the user know about?
If it is only constructors that are the concern, it is a bit confusing to say "passed as arguments", because the marshalling code shouldn't call the ctors of SafeHandle or blittable classes on the way "in" to a P/Invoke. Only on the way "out".
There was a problem hiding this comment.
Only on the way "out".
Yep. The general guidance here any type used in by-refs (i.e., ref, out) or as return arguments should have a non-parameterized ctor. I've no opinion on the message in this case. I will say the DllImport source generator is coming up with guidance in this area as well. The interop team has also updated the official documentation too - https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.safehandle#notes-to-implementers.
/cc @jkoritzinsky
There was a problem hiding this comment.
Could we make the message more like "P/Invokes may use reflection to access a parameterless constructor of any return values or out parameters"?
There was a problem hiding this comment.
Or "P/invoke marshalling may dynamically access members that could be trimmed". I don't know if it helps being specific and listing parameterless constructors - I'm not sure if we're covering all the possible cases with this message. E.g. there's also COM marshalling that this could emit.
ghost
locked as resolved and limited conversation to collaborators
Fixes: #45703