PR feedback.

docs/design/libraries/LibraryImportGenerator/SpanMarshallers.md

@@ -24,11 +24,11 @@ We have decided to match the managed semantics of `(ReadOnly)Span<T>` to provide
 
 As part of this design, we would also want to include some in-box marshallers that follow the design laid out in the [Struct Marshalling design doc](./StructMarshalling.md) to support some additional scenarios:
 
-- A marshaler that marshals an empty span as a non-null pointer.
+- A marshaller that marshals an empty span as a non-null pointer.
   - This marshaller would only support empty spans as it cannot correctly represent non-empty spans of non-blittable types.
-- A marshaler that marshals out a pointer to the native memory as a Span instead of copying the data into a managed array.
+- A marshaller that marshals out a pointer to the native memory as a Span instead of copying the data into a managed array.
   - This marshaller would only support blittable spans by design.
-  - This marshaler will require the user to manually release the memory. Since this will be an opt-in marshaler, this scenario is already advanced and that additional requirement should be understandable to users who use this marshaler.
+  - This marshaller will require the user to manually release the memory. Since this will be an opt-in marshaller, this scenario is already advanced and that additional requirement should be understandable to users who use this marshaller.
   - Since there is no mechansim to provide a collection length, the question of how to provide the span's length in this case is still unresolved. One option would be to always provide a length 1 span and require the user to create a new span with the correct size, but that feels like a bad design.
 
 ### Pros/Cons of Design 1
@@ -40,7 +40,7 @@ Pros:
 
 Cons:
 
-- Defining custom marshalers for non-empty spans of non-blittable types generically is impossible since the marshalling rules of the element's type cannot be known.
+- Defining custom marshallers for non-empty spans of non-blittable types generically is impossible since the marshalling rules of the element's type cannot be known.
 - Custom non-default marshalling of the span element types is impossible for non-built-in types.
 - Inlining the span marshalling fully into the stub increases on-disk IL size.
 - This design does not enable developers to easily define custom marshalling support for their own collection types, which may be desireable.
@@ -83,30 +83,55 @@ namespace System.Runtime.InteropServices
 The attribute would be used with a collection type like `Span<T>` as follows:
 
 ```csharp
-[NativeTypeMarshalling(typeof(DefaultSpanMarshaler<>))]
+[NativeTypeMarshalling(typeof(DefaultSpanMarshaller<>))]
 public ref struct Span<T>
 {
   ...
 }
 
 [CustomTypeMarshaller(typeof(Span<>), CustomTypeMarshallerKind.LinearCollection)]
-public ref struct DefaultSpanMarshaler<T>
+public ref struct DefaultSpanMarshaller<T>
 {
   ...
 }
 ```
 
-The `CustomTypeMarshallerKind.LinearCollection` kind is applied to a generic marshaler type with the "LinearCollection marshaller shape" described below.
+The `CustomTypeMarshallerKind.LinearCollection` kind is applied to a generic marshaller type with the "LinearCollection marshaller shape" described below.
 
 #### Supporting generics
 
-Since generic parameters cannot be used in attributes, open generic types will be permitted in the `NativeTypeMarshallingAttribute` and the `CustomTypeMarshallerAttribute` as long as they have the same arity as the type the attribute is applied to and generic parameters provided to the applied-to type can also be used to construct the type passed as a parameter.
+Since generic parameters cannot be used in attributes, open generic types will be permitted in the `NativeTypeMarshallingAttribute` and the `CustomTypeMarshallerAttribute` as long as they have the same arity as the type with the attribute and generic parameters provided to the type with the attribute can also be used to construct the type passed as a parameter.
 
 If a `CustomTypeMarshaller`-attributed type is a marshaller for a type for a pointer, an array, or a combination of pointers and arrays, the `CustomTypeMarshallerAttribute.GenericPlaceholder` type can be used in the place of the first generic parameter of the marshaller type.
 
+For example:
+
+```csharp
+[CustomTypeMarshaller(typeof(CustomTypeMarshallerAttribute.GenericPlaceholder), Direction = CustomTypeMarshallerDirection.In)]
+struct Marshaller<T>
+{
+    public Marshaller(T managed);
+}
+[CustomTypeMarshaller(typeof(CustomTypeMarshallerAttribute.GenericPlaceholder[]), Direction = CustomTypeMarshallerDirection.In)]
+struct Marshaller<T>
+{
+    public Marshaller(T[] managed);
+}
+[CustomTypeMarshaller(typeof(CustomTypeMarshallerAttribute.GenericPlaceholder*), Direction = CustomTypeMarshallerDirection.In)]
+struct Marshaller<T> where T : unmanaged
+{
+    public Marshaller(T* managed);
+}
+[CustomTypeMarshaller(typeof(CustomTypeMarshallerAttribute.GenericPlaceholder*[]), Direction = CustomTypeMarshallerDirection.In)]
+struct Marshaller<T> where T : unmanaged
+{
+    public Marshaller(T*[] managed);
+}
+```
+
 #### LinearCollection marshaller shape
 
-A generic collection marshaller would be required to have the following shape, in addition to the requirements for marshaler types used with the `CustomTypeMarshallerKind.Value` shape, excluding the constructors.
+A generic collection marshaller would be required to have the following shape, in addition to the requirements for marshaller types used with the `CustomTypeMarshallerKind.Value` shape, excluding the constructors.
 
 ```csharp
 [CustomTypeMarshaller(typeof(GenericCollection<, , ,...>), CustomTypeMarshallerKind.LinearCollection)]
@@ -118,8 +143,6 @@ public struct GenericContiguousCollectionMarshallerImpl<T, U, V,...>
     public GenericContiguousCollectionMarshallerImpl(GenericCollection<T, U, V, ...> collection, int nativeSizeOfElement);
     public GenericContiguousCollectionMarshallerImpl(GenericCollection<T, U, V, ...> collection, Span<byte> stackSpace, int nativeSizeOfElement); // optional
 
-    public const int StackBufferSize = /* */; // required if the span-based constructor is supplied.
-
     /// <summary>
     /// A span that points to the memory where the managed values of the collection are stored (in the marshalling case) or should be stored (in the unmarshalling case).
     /// </summary>
@@ -180,7 +203,7 @@ To support supplying information about collection element counts, a parameterles
 The `ElementIndirectionLevel` property is added to support supplying marshalling info for element types in a collection. For example, if the user is passing a `List<List<Foo>>` from managed to native code, they could provide the following attributes to specify marshalling rules for the outer and inner lists and `Foo` separately:
 
 ```csharp
-private static partial void Bar([MarshalUsing(typeof(ListAsArrayMarshaller<List<Foo>>), CountElementName = nameof(count)), MarshalUsing(ConstantElementCount = 10, ElementIndirectionLevel = 1), MarshalUsing(typeof(FooMarshaler), ElementIndirectionLevel = 2)] List<List<Foo>> foos, int count);
+private static partial void Bar([MarshalUsing(typeof(ListAsArrayMarshaller<List<Foo>>), CountElementName = nameof(count)), MarshalUsing(ConstantElementCount = 10, ElementIndirectionLevel = 1), MarshalUsing(typeof(FooMarshaller), ElementIndirectionLevel = 2)] List<List<Foo>> foos, int count);
 ```
 
 Multiple `MarshalUsing` attributes can only be supplied on the same parameter or return value if the `ElementIndirectionLevel` property is set to distinct values. One `MarshalUsing` attribute per parameter or return value can leave the `ElementIndirectionLevel` property unset. This attribute controls the marshalling of the collection object passed in as the parameter. The sequence of managed types for `ElementIndirectionLevel` is based on the elements of the `ManagedValues` span on the collection marshaller of the previous indirection level. For example, for the marshalling info for `ElementIndirectionLevel = 1` above, the managed type is the type of the following C# expression: `ListAsArrayMarshaller<List<Foo>>.ManagedValues[0]`.
@@ -193,13 +216,13 @@ This design could be used to provide a default marshaller for spans and arrays.
 
 ```csharp
 [CustomTypeMarshaller(typeof(Span<>), CustomTypeMarshallerKind.LinearCollection)]
-public ref struct SpanMarshaler<T>
+public ref struct SpanMarshaller<T>
 {
     private Span<T> managedCollection;
 
     private int nativeElementSize;
 
-    public SpanMarshaler(Span<T> collection, int nativeSizeOfElement)
+    public SpanMarshaller(Span<T> collection, int nativeSizeOfElement)
     {
        managedCollection = collection;
        Value = Marshal.AllocCoTaskMem(collection.Length * nativeSizeOfElement);
@@ -290,8 +313,6 @@ public struct GenericContiguousCollectionMarshallerImpl<T, U, V,...>
     public GenericContiguousCollectionMarshallerImpl(GenericCollection<T, U, V, ...> collection, int nativeSizeOfElements);
     public GenericContiguousCollectionMarshallerImpl(GenericCollection<T, U, V, ...> collection, Span<byte> stackSpace, int nativeSizeOfElements); // optional
 
-    public const int StackBufferSize = /* */; // required if the span-based constructor is supplied.
-
 -    public Span<TCollectionElement> ManagedValues { get; }
 
 -    public void SetUnmarshalledCollectionLength(int length);

docs/design/libraries/LibraryImportGenerator/StructMarshalling.md

@@ -51,7 +51,6 @@ public sealed class CustomTypeMarshallerAttribute : Attribute
      public Type ManagedType { get; }
      public CustomTypeMarshallerKind MarshallerKind { get; }
      public int BufferSize { get; set; }
-     public bool RequiresStackBuffer { get; set; }
 }
 
 public enum CustomTypeMarshallerKind
@@ -85,16 +84,16 @@ If a `Value` property is provided, the developer may also provide a ref-returnin
 A `ref` or `ref readonly` typed `Value` property is unsupported. If a ref-return is required, the type author can supply a `GetPinnableReference` method on the native type to pin the desired `ref` to return and then use `System.Runtime.CompilerServices.Unsafe.AsPointer` to get a pointer from the `ref` that will have already been pinned by the time the `Value` getter is called.
 
 ```csharp
-[NativeMarshalling(typeof(TMarshaler))]
+[NativeMarshalling(typeof(TMarshaller))]
 public struct TManaged
 {
      // ...
 }
 
 [CustomTypeMarshaller(typeof(TManaged))]
-public struct TMarshaler
+public struct TMarshaller
 {
-     public TMarshaler(TManaged managed) {}
+     public TMarshaller(TManaged managed) {}
      public TManaged ToManaged() {}
 
      public void FreeNative() {}
@@ -117,14 +116,14 @@ Since C# 7.3 added a feature to enable custom pinning logic for user types, we s
 Custom marshalers of collection-like types or custom string encodings (such as UTF-32) may want to use stack space for extra storage for additional performance when possible. If the `TNative` type provides additional constructor with the following signature and sets the `BufferSize` field on the `CustomTypeMarshallerAttribute`, then it will opt in to using a caller-allocated buffer:
 
 ```csharp
-[CustomTypeMarshaller(typeof(TManaged), BufferSize = /* */, RequiresStackBuffer = /* */)]
+[CustomTypeMarshaller(typeof(TManaged), BufferSize = /* */)]
 partial struct TNative
 {
      public TNative(TManaged managed, Span<byte> buffer) {}
 }
 ```
 
-When these members are present, the source generator will call the two-parameter constructor with a possibly stack-allocated buffer of `BufferSize` bytes when a stack-allocated buffer is usable. If a stack-allocated buffer is a requirement, the `RequiresStackBuffer` field should be set to `true` and the `buffer` will be guaranteed to be allocated on the stack. Setting the `RequiresStackBuffer` field to `false` is the same as not specifying the value in the attribute. Since a dynamically allocated buffer is not usable in all scenarios, for example Reverse P/Invoke and struct marshalling, a one-parameter constructor must also be provided for usage in those scenarios. This may also be provided by providing a two-parameter constructor with a default value for the second parameter.
+When these members are present, the source generator will call the two-parameter constructor with a possibly stack-allocated buffer of `BufferSize` bytes when a stack-allocated buffer is usable. Since a dynamically allocated buffer is not usable in all scenarios, for example Reverse P/Invoke and struct marshalling, a one-parameter constructor must also be provided for usage in those scenarios.
 
 Type authors can pass down the `buffer` pointer to native code by defining a `Value` property that returns a pointer to the first element, generally through code using `MemoryMarshal.GetReference()` and `Unsafe.AsPointer`. If `RequiresStackBuffer` is not provided or set to `false`, the `buffer` span must be pinned to be used safely. The `buffer` span can be pinned by defining a `GetPinnableReference()` method on the native type that returns a reference to the first element of the span.
 

docs/project/list-of-diagnostics.md

@@ -158,18 +158,21 @@ The diagnostic id values reserved for .NET Libraries analyzer warnings are `SYSL
 |  __`SYSLIB1052`__ | Specified configuration is not supported by source-generated P/Invokes |
 |  __`SYSLIB1053`__ | Current target framework is not supported by source-generated P/Invokes |
 |  __`SYSLIB1054`__ | Specified LibraryImportAttribute arguments cannot be forwarded to DllImportAttribute |
-|  __`SYSLIB1055`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1056`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1057`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1058`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1059`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1060`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1061`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1062`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1063`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1064`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1065`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1066`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1067`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1068`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
-|  __`SYSLIB1069`__ | *_`SYSLIB1055`-`SYSLIB1069` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1055`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1056`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1057`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1058`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1059`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1060`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1061`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1062`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1063`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1064`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1065`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1066`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1067`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1068`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1069`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1070`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1071`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |
+|  __`SYSLIB1072`__ | *_`SYSLIB1055`-`SYSLIB1072` reserved for Microsoft.Interop.LibraryImportGenerator._* |

src/libraries/Common/src/System/Runtime/InteropServices/CustomTypeMarshallerDirection.cs

@@ -1,17 +1,18 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-#nullable enable
-
 #if !TEST_CORELIB
 using System.ComponentModel;
 #endif
 
-//
-// Types in this file are used for generated p/invokes (docs/design/features/source-generator-pinvokes.md).
-//
 namespace System.Runtime.InteropServices
 {
+    /// <summary>
+    /// A direction of marshalling data into or out of the managed environment
+    /// </summary>
+    /// <remarks>
+    /// <seealso cref="CustomTypeMarshallerAttribute.Direction"/>
+    /// </remarks>
     [Flags]
 #if LIBRARYIMPORT_GENERATOR_TEST
     public
@@ -20,12 +21,24 @@ namespace System.Runtime.InteropServices
 #endif
     enum CustomTypeMarshallerDirection
     {
+        /// <summary>
+        /// No marshalling direction
+        /// </summary>
 #if !TEST_CORELIB
         [EditorBrowsable(EditorBrowsableState.Never)]
 #endif
         None = 0,
+        /// <summary>
+        /// Marshalling from a managed environment to an unmanaged environment
+        /// </summary>
         In = 0x1,
+        /// <summary>
+        /// Marshalling from an unmanaged environment to a managed environment
+        /// </summary>
         Out = 0x2,
+        /// <summary>
+        /// Marshalling to and from managed and unmanaged environments
+        /// </summary>
         Ref = In | Out,
     }
 }

src/libraries/Common/src/System/Runtime/InteropServices/CustomTypeMarshallerFeatures.cs

@@ -1,13 +1,14 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-#nullable enable
-
-//
-// Types in this file are used for generated p/invokes (docs/design/features/source-generator-pinvokes.md).
-//
 namespace System.Runtime.InteropServices
 {
+    /// <summary>
+    /// Optional features supported by custom type marshallers.
+    /// </summary>
+    /// <remarks>
+    /// <seealso cref="CustomTypeMarshallerAttribute.Features"/>
+    /// </remarks>
     [Flags]
 #if LIBRARYIMPORT_GENERATOR_TEST
     public
@@ -16,9 +17,21 @@ namespace System.Runtime.InteropServices
 #endif
     enum CustomTypeMarshallerFeatures
     {
+        /// <summary>
+        /// No optional features supported
+        /// </summary>
         None = 0,
+        /// <summary>
+        /// The marshaller owns unmanaged resources that must be freed
+        /// </summary>
         UnmanagedResources = 0x1,
+        /// <summary>
+        /// The marshaller can use a caller-allocated buffer instead of allocating in some scenarios
+        /// </summary>
         CallerAllocatedBuffer = 0x2,
+        /// <summary>
+        /// The marshaller uses the two-stage marshalling design for its <see cref="CustomTypeMarshallerKind"/> instead of the one-stage design.
+        /// </summary>
         TwoStageMarshalling = 0x4
     }
 }