Review feedback

Add test to validate check for blittability for assembly reference

docs/design/libraries/LibraryImportGenerator/StructMarshalling.md

@@ -246,13 +246,15 @@ As an alternative design, we can use a different definition of "does not require
 
 For the auto-layout clause, we have one small issue; today, our ref-assemblies do not expose if a value type is marked as `[StructLayout(LayoutKind.Auto)]`, so we'd still have some cases where we might have runtime failures. However, we can update the tooling used in dotnet/runtime, GenAPI, to expose this information if we so desire. Once that case is handled, we have a mechanism that we can safely use to determine, at compile time, which types will not require marshalling. If we decide to not cover this case (as cases where users mark types as `LayoutKind.Auto` manually are exceptionally rare), we still have a solid design as Roslyn will automatically determine for us if a type is `unmanaged`, so we don't need to do any additional work.
 
-Users are permitted to rely upon the built-in notion of blittable if the type to marshal adheres to the following constraints, termed "strictly blittable" in code:
+As `unmanaged` is a C# language concept, we can use Roslyn's APIs to determine if a type is `unmanaged` to determine if it does not require marshalling without needing to define any new attributes and reshape the ecosystem. However, to enable this work, the LibraryImportGenerator, as well as any other source generators that generate calls to native code using the interop team's infrastructure, will need to require that the user applies the `DisableRuntimeMarshallingAttribute` to their assembly when non-trivial custom user-defined types are used. To help support users in this case, the interop team will provide a code-fix that will generate the `DisableRuntimeMarshallingAttribute` for users when they use the source generator. New codebases that adopt the source-generated interop world should immediately apply `DisableRuntimeMarshallingAttribute`.
+
+Applying `DisableRuntimeMarshallingAttribute` can impact existing codebases depending on what interop APIs are used. Along with the potential difficultly in tracking down these places there is also an argument to made around the UX of the source-generator. Users are permitted to rely upon the runtime's definition of blittable and not apply `DisableRuntimeMarshallingAttribute` if all the types to be marshalled adhere to the following constraints, termed "strictly blittable" in code:
 
 1) Is always blittable (for example, `int` or `double`, but not `char`).
 2) Is a value type defined in the source project the current source generator is running on.
 3) Is a type composed of types adhering to (1) and (2).
 
-The "strictly blittable" notion is being introduced to ease adoption. The impact of `DisableRuntimeMarshallingAttribute` is that some scenarios require effort that is much larger relative to the need to marshal an internal transfer type that has historically been blittable. For example, the following value type would require the consumer of the source generator to apply `DisableRuntimeMarshallingAttribute` if the above was not permitted.
+For example, the following value type would require the consumer of the source generator to apply `DisableRuntimeMarshallingAttribute` if the above was not permitted.
 
 ```csharp
 struct S
@@ -262,8 +264,6 @@ struct S
 }
 ```
 
-As `unmanaged` is a C# language concept, we can use Roslyn's APIs to determine if a type is `unmanaged` to determine if it does not require marshalling without needing to define any new attributes and reshape the ecosystem. However, to enable this work, the LibraryImportGenerator, as well as any other source generators that generate calls to native code using the interop team's infrastructure, will need to require that the user applies the `DisableRuntimeMarshallingAttribute` to their assembly when custom user-defined types are used. As we believe that users should be able to move over their assemblies to the new source-generated interop world as a whole assembly, we do not believe that this will cause any serious issues in adoption. To help support users in this case, the interop team will provide a code-fix that will generate the `DisableRuntimeMarshallingAttribute` for users when they use the source generator.
-
 ### Usage
 
 There are 2 usage mechanisms of these attributes.

src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/CodeSnippets.cs

@@ -635,28 +635,6 @@ partial class Test
         [MarshalUsing(typeof({nativeTypeName}))] out {typeName} pOut);
 }}
 ";
-
-        private static string NonBlittableUserDefinedTypeWithNativeType() => $@"
-struct S
-{{
-    public string s;
-}}
-
-[CustomTypeMarshaller(typeof(S))]
-struct Native
-{{
-    private System.IntPtr p;
-    public Native(S s)
-    {{
-        p = System.IntPtr.Zero;
-    }}
-
-    public S ToManaged() => new S {{ s = string.Empty }};
-}}
-";
-
-        public static string NonBlittableUserDefinedTypeToBlittableNativeType(string attr) => MarshalUsingParametersAndModifiers("S", "Native", attr) + NonBlittableUserDefinedTypeWithNativeType();
-
         public static string BasicNonBlittableUserDefinedType(bool defineNativeMarshalling = true) => $@"
 {(defineNativeMarshalling ? "[NativeMarshalling(typeof(Native))]" : string.Empty)}
 struct S
@@ -1546,5 +1524,28 @@ partial struct Basic
 }
 ";
 
+        public static class ValidateDisableRuntimeMarshalling
+        {
+            public static string NonBlittableUserDefinedTypeWithNativeType = $@"
+public struct S
+{{
+    public string s;
+}}
+
+[CustomTypeMarshaller(typeof(S))]
+public struct Native
+{{
+    private System.IntPtr p;
+    public Native(S s)
+    {{
+        p = System.IntPtr.Zero;
+    }}
+
+    public S ToManaged() => new S {{ s = string.Empty }};
+}}
+";
+
+            public static string TypeUsage(string attr) => MarshalUsingParametersAndModifiers("S", "Native", attr);
+        }
     }
 }

src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/CompileFails.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.Immutable;
+using System.IO;
 using System.Linq;
 using System.Runtime.InteropServices;
 using System.Threading.Tasks;
@@ -170,5 +171,32 @@ public async Task ValidateSnippets_InvalidCodeGracefulFailure(string source, int
             int compilerErrors = newComp.GetDiagnostics().Count(d => d.Severity == DiagnosticSeverity.Error);
             Assert.Equal(expectedCompilerErrors, compilerErrors);
         }
+
+        [Fact]
+        public async Task ValidateDisableRuntimeMarshallingForBlittabilityCheckFromAssemblyReference()
+        {
+            string assemblySource = $@"
+using System.Runtime.InteropServices;
+using System.Runtime.InteropServices.Marshalling;
+{CodeSnippets.ValidateDisableRuntimeMarshalling.NonBlittableUserDefinedTypeWithNativeType}
+";
+            Compilation assemblyComp = await TestUtils.CreateCompilation(assemblySource);
+            TestUtils.AssertPreSourceGeneratorCompilation(assemblyComp);
+
+            var ms = new MemoryStream();
+            Assert.True(assemblyComp.Emit(ms).Success);
+
+            string testSource = CodeSnippets.ValidateDisableRuntimeMarshalling.TypeUsage(string.Empty);
+
+            Compilation testComp = await TestUtils.CreateCompilation(testSource, refs: new[] { MetadataReference.CreateFromImage(ms.ToArray()) });
+            TestUtils.AssertPreSourceGeneratorCompilation(testComp);
+
+            var newComp = TestUtils.RunGenerators(testComp, out var generatorDiags, new Microsoft.Interop.LibraryImportGenerator());
+
+            // The errors should indicate the DisableRuntimeMarshalling is required.
+            Assert.True(generatorDiags.All(d => d.Id == "SYSLIB1051"));
+
+            TestUtils.AssertPostSourceGeneratorCompilation(newComp);
+        }
     }
 }

src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Compiles.cs

@@ -151,8 +151,10 @@ public static IEnumerable<object[]> CodeSnippetsToCompile()
             // Structs
             yield return new[] { CodeSnippets.BlittableStructParametersAndModifiers(string.Empty) };
             yield return new[] { CodeSnippets.BlittableStructParametersAndModifiers(CodeSnippets.DisableRuntimeMarshalling) };
-            yield return new[] { CodeSnippets.NonBlittableUserDefinedTypeToBlittableNativeType(string.Empty) };
-            yield return new[] { CodeSnippets.NonBlittableUserDefinedTypeToBlittableNativeType(CodeSnippets.DisableRuntimeMarshalling) };
+            yield return new[] { CodeSnippets.ValidateDisableRuntimeMarshalling.TypeUsage(string.Empty)
+                + CodeSnippets.ValidateDisableRuntimeMarshalling.NonBlittableUserDefinedTypeWithNativeType };
+            yield return new[] { CodeSnippets.ValidateDisableRuntimeMarshalling.TypeUsage(CodeSnippets.DisableRuntimeMarshalling)
+                + CodeSnippets.ValidateDisableRuntimeMarshalling.NonBlittableUserDefinedTypeWithNativeType };
 
             // SafeHandle
             yield return new[] { CodeSnippets.BasicParametersAndModifiers("Microsoft.Win32.SafeHandles.SafeFileHandle") };

src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/TestUtils.cs

@@ -122,10 +122,12 @@ public static void AssertPostSourceGeneratorCompilation(Compilation comp, params
         /// <param name="source">Source to compile</param>
         /// <param name="targetFramework">Target framework of the compilation</param>
         /// <param name="outputKind">Output type</param>
+        /// <param name="refs">Addtional metadata references</param>
+        /// <param name="preprocessorSymbols">Prepocessor symbols</param>
         /// <returns>The resulting compilation</returns>
-        public static Task<Compilation> CreateCompilation(string source, TestTargetFramework targetFramework = TestTargetFramework.Net, OutputKind outputKind = OutputKind.DynamicallyLinkedLibrary, IEnumerable<string>? preprocessorSymbols = null)
+        public static Task<Compilation> CreateCompilation(string source, TestTargetFramework targetFramework = TestTargetFramework.Net, OutputKind outputKind = OutputKind.DynamicallyLinkedLibrary, IEnumerable<MetadataReference>? refs = null, IEnumerable<string>? preprocessorSymbols = null)
         {
-            return CreateCompilation(new[] { source }, targetFramework, outputKind, preprocessorSymbols);
+            return CreateCompilation(new[] { source }, targetFramework, outputKind, refs, preprocessorSymbols);
         }
 
         /// <summary>
@@ -134,14 +136,17 @@ public static Task<Compilation> CreateCompilation(string source, TestTargetFrame
         /// <param name="sources">Sources to compile</param>
         /// <param name="targetFramework">Target framework of the compilation</param>
         /// <param name="outputKind">Output type</param>
+        /// <param name="refs">Addtional metadata references</param>
+        /// <param name="preprocessorSymbols">Prepocessor symbols</param>
         /// <returns>The resulting compilation</returns>
-        public static Task<Compilation> CreateCompilation(string[] sources, TestTargetFramework targetFramework = TestTargetFramework.Net, OutputKind outputKind = OutputKind.DynamicallyLinkedLibrary, IEnumerable<string>? preprocessorSymbols = null)
+        public static Task<Compilation> CreateCompilation(string[] sources, TestTargetFramework targetFramework = TestTargetFramework.Net, OutputKind outputKind = OutputKind.DynamicallyLinkedLibrary, IEnumerable<MetadataReference>? refs = null, IEnumerable<string>? preprocessorSymbols = null)
         {
             return CreateCompilation(
                 sources.Select(source =>
                     CSharpSyntaxTree.ParseText(source, new CSharpParseOptions(LanguageVersion.Preview, preprocessorSymbols: preprocessorSymbols))).ToArray(),
                 targetFramework,
-                outputKind);
+                outputKind,
+                refs);
         }
 
         /// <summary>
@@ -150,8 +155,9 @@ public static Task<Compilation> CreateCompilation(string[] sources, TestTargetFr
         /// <param name="sources">Sources to compile</param>
         /// <param name="targetFramework">Target framework of the compilation</param>
         /// <param name="outputKind">Output type</param>
+        /// <param name="refs">Addtional metadata references</param>
         /// <returns>The resulting compilation</returns>
-        public static async Task<Compilation> CreateCompilation(SyntaxTree[] sources, TestTargetFramework targetFramework = TestTargetFramework.Net, OutputKind outputKind = OutputKind.DynamicallyLinkedLibrary)
+        public static async Task<Compilation> CreateCompilation(SyntaxTree[] sources, TestTargetFramework targetFramework = TestTargetFramework.Net, OutputKind outputKind = OutputKind.DynamicallyLinkedLibrary, IEnumerable<MetadataReference>? refs = null)
         {
             var referenceAssemblies = await GetReferenceAssemblies(targetFramework);
 
@@ -161,6 +167,11 @@ public static async Task<Compilation> CreateCompilation(SyntaxTree[] sources, Te
                 referenceAssemblies = referenceAssemblies.Add(GetAncillaryReference());
             }
 
+            if (refs is not null)
+            {
+                referenceAssemblies = referenceAssemblies.AddRange(refs);
+            }
+
             return CSharpCompilation.Create("compilation",
                 sources,
                 referenceAssemblies,