Repository navigation
LibraryImportAttribute for p/invoke source generation #46822
Description
Background and Motivation
The DllImportAttribute used in P/Invoke scenarios relies on a built-in system of dynamic IL generation to support marshalling of arguments to to a native environment. A new attribute that would permit the static IL generation
is proposed. Scenario details and overall design can be found here.
Proposed API
The proposed shape for LibraryImportAttribute is based on the existing DllImportAttribute, but looks to remove the inconsistent or platform-specific aspects.
This attribute is intended to be emitted by the p/invoke source generator rather than added to the runtime libraries.
Related proposal: #46838
namespace System.Runtime.InteropServices
{
/// <summary>
/// Specifies how strings should be marshalled
/// </summary>
public enum StringMarshalling
{
None = 0,
Utf8, // UTF-8
Utf16, // UTF-16, machine-endian
}
/// <summary>
/// Attribute used to indicate a Source Generator should create a function for marshaling
/// arguments instead of relying on the CLR to generate an IL Stub at runtime.
/// </summary>
[AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = false)]
public sealed class LibraryImportAttribute : Attribute
{
/// <summary>
/// Constructor.
/// </summary>
/// <param name="libraryName">Name of the library containing the import</param>
public LibraryImportAttribute(string libraryName);
/// <summary>
/// Library to load.
/// </summary>
public string LibraryName { get; }
/// <summary>
/// Indicates the name or ordinal of the entry point to be called.
/// </summary>
/// <see cref="System.Runtime.InteropServices.DllImportAttribute.EntryPoint"/>
public string? EntryPoint { get; set; }
/// <summary>
/// Indicates how to marshal string parameters to the method.
/// </summary>
/// <remarks>
/// If this field is specified, <see cref="MarshalStringsUsing" /> must not be specified.
/// </remarks>
public StringMarshalling StringMarshalling { get; set; }
/// <summary>
/// Indicates how to marshal string parameters to the method.
/// </summary>
/// <remarks>
/// If this field is specified, <see cref="StringMarshalling" /> must not be specified.
/// The type should be one that conforms to usage with the attributes:
/// <see cref="System.Runtime.InteropServices.MarshalUsingAttribute"/>
/// <see cref="System.Runtime.InteropServices.NativeMarshallingAttribute"/>
/// </remarks>
public Type? MarshalStringsUsing { get; set; }
/// <summary>
/// Indicates whether the callee sents an error (SetLastError on Windows or errorno
/// on other platforms) before returning from the attributed method.
/// </summary>
/// <see cref="System.Runtime.InteropServices.DllImportAttribute.SetLastError"/>
public bool SetLastError { get; set; }
}
}Alternate names for the attribute:
ForeignFunctionAttributeGeneratedDllImportAttributeInteropImportAttributeNativeLibraryImportAttributePInvokeImportAttribute
...
Usage Examples
Usage of LibraryImportAttribute would be similar to that of DllImportAttribute, but would also require a source generator be referenced during build. The attributed method and its containing types must also be partial. The source generator can be found in DllImportGenerator.
[LibraryImport("NativeBinary")]
internal static partial int Sum(int a, int b);
[LibraryImport("NativeBinary", StringMarshalling = StringMarshalling.Utf16)]
internal static partial string GetValue(string name);
[LibraryImport("NativeBinary", MarshalStringsUsing = typeof(MyStringMarshal.Wtf8String))]
internal static partial string GetValue(string name);
namespace MyStringMarshal
{
struct Wtf8String { ... }
}Analyzer/Generator diagnostics
- Fixer up for converting supported signatures from
DllImportAttributetoLibraryImportAttribute. - Generator will emit errors for invalid usage or missing support - prototyped diagnostics can be found here.
- Guidance for the Type to use with
MarshalStringsUsingis covered by the diagnostics in Attributes and Analyzer Diagnostics to support custom struct marshalling in the DllImport Source Generator #46838
Risks
Attributes are typically static metadata so this will increase the amount of metadata for a compiled assembly. The DllImportAttribute is actually a pseudo-attribute and does not have the same metadata cost as a typical .NET Attribute like the one proposed - note that the resulting generated code will vastly out-weigh the cost of this new metadata.
In terms of UX, a user would hit a build failure if one uses the attribute but doesn't reference a source code generator. There are also differences in compatibility/support between the built-in system and the proposed source generator - documented here - which users may hit. Users will get a generator diagnostic if trying to use the generator in these cases.
Activity
AaronRobinsonMSFT commentedon Jan 11, 2021
/cc @stephentoub @jkoritzinsky @elinor-fung @jkotas @tannergooding
jkotas commentedon Jan 11, 2021
We will need number of other APIs to support source generated interop. Have you filled this one because you need it to get in sooner than the rest, or can it wait for the rest of the APIs to support source generated interop?
I think it is ok to add this attribute before API review as internal to CoreLib if you would like to get the source generated interop (partially) enabled for shipping bits to prove that it works well and to make it easier to fine tune the shape of the APIs required to support it.
Does it also need the constructor (and maybe the Value property) so that
GeneratedDllImportAttribute("kernel32.dll")works?I am not following how the compatibility differences relate to analyzer rules. I would expect that we are going to have:
stephentoub commentedon Jan 11, 2021
That certainly helps with migration, which is great. On the flip side, DllImport has been around since the beginning of .NET, and for such things, it's rare we don't find something about it we wish we could have done differently. Are there any things here it would be valuable to redesign, as we now have that chance?
stephentoub commentedon Jan 11, 2021
I'd have thought a much bigger cost would be all of the code actually spit into the compiled assembly by the generator...?
rseanhall commentedon Jan 11, 2021
Was #33742 (comment) addressed somewhere else? Why does this attribute need to be part of .NET instead of the source generator?
AaronRobinsonMSFT commentedon Jan 11, 2021
@jkotas This is the first of several API issues - see "New .NET APIs" in #43060.
Oops. Copied the API from the wrong document - updated above.
That is my mistake, added.
I was including both of the above as compatibility between source gen and the built-in. I agree both should do that. I will update the proposal to clarify.
@stephentoub This v1 proposal is for specifically for providing what is needed in .NETCoreApp. We have some ideas on additional support but those could be added in an update. I don't know if we should add them here at the moment. I believe the design is flexible enough to expand.
Absolutely, but I was interpreting this as the specific API itself and not how other tools respond to it. The result of the source generator will very much impact the size and is planned to be investigated see "Prototype exit criteria" in #43060.
@rseanhall Adding this to .NETCoreApp permits its use in .NETCoreApp. Providing this as a NuGet would make a cumbersome library dependency to remove during compilation - which isn't possible in source generators and would therefore need to be a post-process step.
jkotas commentedon Jan 11, 2021
I think question is about what to drop from the initial proposal, not about what to add to it. For example, do we like
PreserveSig? It is very COM specific and it is not a very good name either. Would it make sense to drop it for now and replace it with something more appropriate in future iterations? Run a similar thought process for each of thebools.AaronRobinsonMSFT commentedon Jan 11, 2021
Ah. If I recall the desire was to have the same API surface area. I could have missed the rational for that suggestion, but when it was originally proposed most of the above properties were removed since they didn't have a lot of value. We can definitely remove some of them if keeping the surface area the same isn't a goal.
/cc @jkoritzinsky @elinor-fung
AaronRobinsonMSFT commentedon Jan 11, 2021
@stephentoub @jkotas At present the following two fields are explicit errors in the prototype and have been removed from above. The
PreserveSigis still being used, but I could see an argument for changing the name. The others all seemed reasonable to us and were implemented. All semantic differences and limitations on the other fields are fully documented here.jkotas commentedon Jan 11, 2021
I see a very few uses of it in our own libraries.
HRESULT marshaling always had a questionable behavior around IErrorInfo. It assumes that whatever thread-local IErrorInfo* happens to be there is valid for failing HRESULT that is not always true, and tends to lead to bogus exception to be thrown intermittently on error. At least some (if not all) uses of it in our own libraries seem to suffer from this problem. If we keep it as is, we will be rolling this problem into future.
We are introducing analyzers to prefer
ExactSpelling=true(#35695). Should we makeExactSpelling=trueto be the default? Or maybe even drop it completely fromGeneratedDllImportAttribute?25 remaining items
bartonjs commentedon Feb 1, 2022
Video
The main thing that came up in discussion is that the string encoding being an open
Typeparameter is not very good ease-of-use/user-experience. We probably want something with the convenience of theCharSetenum, but perhaps a new one specific to this use case. (So this would be a sibling property to theMarshalStringsUsingproperty, just accelerating the common cases)teo-tsirpanis commentedon Feb 1, 2022
@bartonjs in case you don't know, the livestream is down.
bartonjs commentedon Feb 1, 2022
@teo-tsirpanis Yeah. @terrajobst's connection dropped and so I just stepped in and wrote the notes to wrap up the meeting.
elinor-fung commentedon Feb 1, 2022
Current thoughts on the string marshalling:
Not promoting ANSI as first-class via the enum - users can go through
MarshalStringsUsing.bartonjs commentedon Feb 15, 2022
We renamed the MarshalStringsUsing property to StringMarshallingCustomType, and renamed the 0 value of StringMarshalling from
NonetoCustomto better describe the relationship of the two properties. Otherwise, looks good as proposed.naine commentedon Feb 15, 2022
Customseems a bit inconsistent with the mutually exclusive relationship of the two properties, since it implies you should doStringMarshalling = Custom, StringMarshallingCustomType = typeof(...).IMO, the attribute descriptions should be updated as follows if that name will be used: