From cf1c9d9816f3a85c3651bd6b9b846ffe684cb65d Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Wed, 3 Jul 2024 20:52:43 -0700 Subject: [PATCH 1/4] [LibraryImportGenerator] Use basic forwarder in down-level support if any parameters can't be marshalled --- .../gen/LibraryImportGenerator/LibraryImportGenerator.cs | 6 ++++++ .../LibraryImportGenerator/PInvokeStubCodeGenerator.cs | 8 ++++++++ 2 files changed, 14 insertions(+) diff --git a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs index 8b4c4de4d7f047..2b8212d80f4efe 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs @@ -347,6 +347,12 @@ private static (MemberDeclarationSyntax, ImmutableArray) Generat LibraryImportGeneratorHelpers.CreateGeneratorResolver(pinvokeStub.TargetFramework, pinvokeStub.Options, pinvokeStub.EnvironmentFlags), new CodeEmitOptions(SkipInit: pinvokeStub.TargetFramework is (TargetFramework.Net, _))); + // For down-level support, if some parameters cannot be marshalled, consider the target framework as not supported + if (stubGenerator.HasForwardedTypes && (pinvokeStub.TargetFramework.TargetFramework != TargetFramework.Net || pinvokeStub.TargetFramework.Version.Major < 7)) + { + supportsTargetFramework = false; + } + // Check if the generator should produce a forwarder stub - regular DllImport. // This is done if the signature is blittable or the target framework is not supported. if (stubGenerator.StubIsBasicForwarder diff --git a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/PInvokeStubCodeGenerator.cs b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/PInvokeStubCodeGenerator.cs index bd2d5cb73075d0..2edb79187ad648 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/PInvokeStubCodeGenerator.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/PInvokeStubCodeGenerator.cs @@ -30,6 +30,8 @@ internal sealed class PInvokeStubCodeGenerator { public bool StubIsBasicForwarder { get; } + public bool HasForwardedTypes { get; } + /// /// Identifier for managed return value /// @@ -74,6 +76,12 @@ public PInvokeStubCodeGenerator( // Check if generator is either blittable or just a forwarder. noMarshallingNeeded &= generator is { Generator: BlittableMarshaller, TypeInfo.IsByRef: false } or { Generator: Forwarder }; + + // Track if any generators are just forwarders - for types other than void, this indicates + // types that can't be marshalled by the source generated + // In .NET 7+ support, we would have emitted a diagnostic error about lack of support + // In down-level support, we do not error - tracking this allows us to switch to generating a basic forwarder stub + HasForwardedTypes |= generator is { Generator: Forwarder, TypeInfo.ManagedType: not SpecialTypeInfo { SpecialType: Microsoft.CodeAnalysis.SpecialType.System_Void } }; } StubIsBasicForwarder = !setLastError From 92d619a25f20fa6805eff90756b2c0ec65ecf1d6 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Wed, 3 Jul 2024 22:14:18 -0700 Subject: [PATCH 2/4] Add tests --- .../CodeSnippets.cs | 7 +++- .../Compiles.cs | 42 +++++++++++++++++-- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/CodeSnippets.cs b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/CodeSnippets.cs index 6204b100ce3252..56eb1d52dbe1ee 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/CodeSnippets.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/CodeSnippets.cs @@ -813,13 +813,16 @@ partial class Test } """; - public static string BasicReturnAndParameterByValue(string returnType, string parameterType, string preDeclaration = "") => $$""" + /// + /// Declaration with a non-blittable parameter that is always supported for marshalling + /// + public static string BasicReturnAndParameterWithAlwaysSupportedParameter(string returnType, string parameterType, string preDeclaration = "") => $$""" using System.Runtime.InteropServices; {{preDeclaration}} partial class Test { [LibraryImport("DoesNotExist")] - public static partial {{returnType}} Method({{parameterType}} p); + public static partial {{returnType}} Method({{parameterType}} p, out int i); } """; diff --git a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Compiles.cs b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Compiles.cs index b7d672d45493ed..7b7e14b6e92bb1 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Compiles.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Compiles.cs @@ -521,9 +521,46 @@ public static IEnumerable CodeSnippetsToValidateFallbackForwarder() yield return new object[] { ID(), code, TestTargetFramework.Framework, true }; } - // Confirm that if support is missing for any type (like arrays), we fall back to a forwarder even if other types are supported. + // Confirm that if support is missing for a type with an ITypeBasedMarshallingInfoProvider (like arrays and SafeHandles), we fall back to a forwarder even if other types are supported. { - string code = CodeSnippets.BasicReturnAndParameterByValue("System.Runtime.InteropServices.SafeHandle", "int[]", CodeSnippets.LibraryImportAttributeDeclaration); + string code = CodeSnippets.BasicReturnAndParameterWithAlwaysSupportedParameter("void", "System.Runtime.InteropServices.SafeHandle", CodeSnippets.LibraryImportAttributeDeclaration); + yield return new object[] { ID(), code, TestTargetFramework.Net6, true }; + yield return new object[] { ID(), code, TestTargetFramework.Core, true }; + yield return new object[] { ID(), code, TestTargetFramework.Standard, true }; + yield return new object[] { ID(), code, TestTargetFramework.Framework, true }; + } + { + string code = CodeSnippets.BasicReturnAndParameterWithAlwaysSupportedParameter("System.Runtime.InteropServices.SafeHandle", "int", CodeSnippets.LibraryImportAttributeDeclaration); + yield return new object[] { ID(), code, TestTargetFramework.Net6, true }; + yield return new object[] { ID(), code, TestTargetFramework.Core, true }; + yield return new object[] { ID(), code, TestTargetFramework.Standard, true }; + yield return new object[] { ID(), code, TestTargetFramework.Framework, true }; + } + { + string code = CodeSnippets.BasicReturnAndParameterWithAlwaysSupportedParameter("void", "int[]", CodeSnippets.LibraryImportAttributeDeclaration); + yield return new object[] { ID(), code, TestTargetFramework.Net6, true }; + yield return new object[] { ID(), code, TestTargetFramework.Core, true }; + yield return new object[] { ID(), code, TestTargetFramework.Standard, true }; + yield return new object[] { ID(), code, TestTargetFramework.Framework, true }; + } + { + string code = CodeSnippets.BasicReturnAndParameterWithAlwaysSupportedParameter("int", "int[]", CodeSnippets.LibraryImportAttributeDeclaration); + yield return new object[] { ID(), code, TestTargetFramework.Net6, true }; + yield return new object[] { ID(), code, TestTargetFramework.Core, true }; + yield return new object[] { ID(), code, TestTargetFramework.Standard, true }; + yield return new object[] { ID(), code, TestTargetFramework.Framework, true }; + } + + // Confirm that if support is missing for a type without an ITypeBasedMarshallingInfoProvider (like StringBuilder), we fall back to a forwarder even if other types are supported. + { + string code = CodeSnippets.BasicReturnAndParameterWithAlwaysSupportedParameter("void", "System.Text.StringBuilder", CodeSnippets.LibraryImportAttributeDeclaration); + yield return new object[] { ID(), code, TestTargetFramework.Net6, true }; + yield return new object[] { ID(), code, TestTargetFramework.Core, true }; + yield return new object[] { ID(), code, TestTargetFramework.Standard, true }; + yield return new object[] { ID(), code, TestTargetFramework.Framework, true }; + } + { + string code = CodeSnippets.BasicReturnAndParameterWithAlwaysSupportedParameter("int", "System.Text.StringBuilder", CodeSnippets.LibraryImportAttributeDeclaration); yield return new object[] { ID(), code, TestTargetFramework.Net6, true }; yield return new object[] { ID(), code, TestTargetFramework.Core, true }; yield return new object[] { ID(), code, TestTargetFramework.Standard, true }; @@ -724,7 +761,6 @@ public class Basic { } await test.RunAsync(); } - [OuterLoop("Uses the network for downlevel ref packs")] [InlineData(TestTargetFramework.Standard)] [InlineData(TestTargetFramework.Framework)] From 458a486c70322e66dea142bbf1eef50dac18e0a6 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Wed, 3 Jul 2024 23:27:04 -0700 Subject: [PATCH 3/4] Remove tests using partial forwarding --- .../AttributeForwarding.cs | 74 ------------------- .../Diagnostics.cs | 14 +++- 2 files changed, 10 insertions(+), 78 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/AttributeForwarding.cs b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/AttributeForwarding.cs index 09ba899af027c7..f2c79633d6e8f9 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/AttributeForwarding.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/AttributeForwarding.cs @@ -331,80 +331,6 @@ await VerifySourceGeneratorAsync( }); } - [Fact] - [OuterLoop("Uses the network for downlevel ref packs")] - public async Task InOutAttributes_Forwarded_To_ForwardedParameter() - { - // This code is invalid configuration from the source generator's perspective. - // We just use it as validation for forwarding the In and Out attributes. - string source = """ - using System.Runtime.CompilerServices; - using System.Runtime.InteropServices; - partial class C - { - [LibraryImportAttribute("DoesNotExist")] - [return: MarshalAs(UnmanagedType.Bool)] - public static partial bool Method1([In, Out] int {|SYSLIB1051:a|}); - } - """ + CodeSnippets.LibraryImportAttributeDeclaration; - - await VerifySourceGeneratorAsync( - source, - (targetMethod, newComp) => - { - INamedTypeSymbol marshalAsAttribute = newComp.GetTypeByMetadataName(TypeNames.System_Runtime_InteropServices_MarshalAsAttribute)!; - INamedTypeSymbol inAttribute = newComp.GetTypeByMetadataName(TypeNames.System_Runtime_InteropServices_InAttribute)!; - INamedTypeSymbol outAttribute = newComp.GetTypeByMetadataName(TypeNames.System_Runtime_InteropServices_OutAttribute)!; - Assert.Collection(targetMethod.Parameters, - param => Assert.Collection(param.GetAttributes(), - attr => - { - Assert.Equal(inAttribute, attr.AttributeClass, SymbolEqualityComparer.Default); - Assert.Empty(attr.ConstructorArguments); - Assert.Empty(attr.NamedArguments); - }, - attr => - { - Assert.Equal(outAttribute, attr.AttributeClass, SymbolEqualityComparer.Default); - Assert.Empty(attr.ConstructorArguments); - Assert.Empty(attr.NamedArguments); - })); - }, - TestTargetFramework.Standard); - } - - [Fact] - [OuterLoop("Uses the network for downlevel ref packs")] - public async Task MarshalAsAttribute_Forwarded_To_ForwardedParameter() - { - string source = """ - using System.Runtime.CompilerServices; - using System.Runtime.InteropServices; - partial class C - { - [LibraryImportAttribute("DoesNotExist")] - [return: MarshalAs(UnmanagedType.Bool)] - public static partial bool Method1([MarshalAs(UnmanagedType.I2)] int a); - } - """ + CodeSnippets.LibraryImportAttributeDeclaration; - - await VerifySourceGeneratorAsync( - source, - (targetMethod, newComp) => - { - INamedTypeSymbol marshalAsAttribute = newComp.GetTypeByMetadataName(TypeNames.System_Runtime_InteropServices_MarshalAsAttribute)!; - Assert.Collection(targetMethod.Parameters, - param => Assert.Collection(param.GetAttributes(), - attr => - { - Assert.Equal(marshalAsAttribute, attr.AttributeClass, SymbolEqualityComparer.Default); - Assert.Equal(UnmanagedType.I2, (UnmanagedType)attr.ConstructorArguments[0].Value!); - Assert.Empty(attr.NamedArguments); - })); - }, - TestTargetFramework.Standard); - } - private static Task VerifySourceGeneratorAsync(string source, Action targetPInvokeAssertion, TestTargetFramework targetFramework = TestTargetFramework.Net) { var test = new GeneratedTargetPInvokeTest(targetPInvokeAssertion, targetFramework) diff --git a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Diagnostics.cs b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Diagnostics.cs index aeb467e4bc77c3..646021b0b1adfc 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Diagnostics.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/LibraryImportGenerator.UnitTests/Diagnostics.cs @@ -250,7 +250,7 @@ partial class Test public static partial void {|#0:Method1|}(string s); [LibraryImport("DoesNotExist", StringMarshalling = StringMarshalling.Custom, StringMarshallingCustomType = typeof(Native))] - public static partial void Method2(string {|#1:s|}); + public static partial void {|#2:Method2|}(string {|#1:s|}); struct Native { @@ -266,7 +266,13 @@ public Native(string s) { } .WithArguments($"{nameof(TypeNames.LibraryImportAttribute)}{Type.Delimiter}{nameof(StringMarshalling)}={nameof(StringMarshalling)}{Type.Delimiter}{nameof(StringMarshalling.Utf8)}"), VerifyCS.Diagnostic(GeneratorDiagnostics.ParameterTypeNotSupportedWithDetails) .WithLocation(1) - .WithArguments("Marshalling string or char without explicit marshalling information is not supported. Specify 'LibraryImportAttribute.StringMarshalling', 'LibraryImportAttribute.StringMarshallingCustomType', 'MarshalUsingAttribute' or 'MarshalAsAttribute'.", "s") + .WithArguments("Marshalling string or char without explicit marshalling information is not supported. Specify 'LibraryImportAttribute.StringMarshalling', 'LibraryImportAttribute.StringMarshallingCustomType', 'MarshalUsingAttribute' or 'MarshalAsAttribute'.", "s"), + VerifyCS.Diagnostic(GeneratorDiagnostics.CannotForwardToDllImport) + .WithLocation(2) + .WithArguments($"{nameof(TypeNames.LibraryImportAttribute)}{Type.Delimiter}{nameof(StringMarshalling)}={nameof(StringMarshalling)}{Type.Delimiter}{nameof(StringMarshalling.Custom)}"), + VerifyCS.Diagnostic(GeneratorDiagnostics.CannotForwardToDllImport) + .WithLocation(2) + .WithArguments($"{nameof(TypeNames.LibraryImportAttribute)}{Type.Delimiter}{nameof(LibraryImportAttribute.StringMarshallingCustomType)}") }; var test = new VerifyCS.Test(TestTargetFramework.Standard) @@ -289,10 +295,10 @@ partial class Test { [{|#0:LibraryImport("DoesNotExist", StringMarshalling = StringMarshalling.Custom)|}] public static partial void Method1(out int i); - + [{|#1:LibraryImport("DoesNotExist", StringMarshalling = StringMarshalling.Utf8, StringMarshallingCustomType = typeof(Native))|}] public static partial void Method2(out int i); - + struct Native { public Native(string s) { } From 365fd8e9d866cb5cb0d7e91b0f8e85d1f5507a9e Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Mon, 8 Jul 2024 08:23:13 -0700 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Aaron Robinson --- .../gen/LibraryImportGenerator/LibraryImportGenerator.cs | 3 ++- .../gen/LibraryImportGenerator/PInvokeStubCodeGenerator.cs | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs index 2b8212d80f4efe..0c85cda3cbaf14 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs @@ -348,7 +348,8 @@ private static (MemberDeclarationSyntax, ImmutableArray) Generat new CodeEmitOptions(SkipInit: pinvokeStub.TargetFramework is (TargetFramework.Net, _))); // For down-level support, if some parameters cannot be marshalled, consider the target framework as not supported - if (stubGenerator.HasForwardedTypes && (pinvokeStub.TargetFramework.TargetFramework != TargetFramework.Net || pinvokeStub.TargetFramework.Version.Major < 7)) + if (stubGenerator.HasForwardedTypes + && (pinvokeStub.TargetFramework.TargetFramework != TargetFramework.Net || pinvokeStub.TargetFramework.Version.Major < 7)) { supportsTargetFramework = false; } diff --git a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/PInvokeStubCodeGenerator.cs b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/PInvokeStubCodeGenerator.cs index 2edb79187ad648..0d5c8a06d14f0a 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/PInvokeStubCodeGenerator.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/PInvokeStubCodeGenerator.cs @@ -78,9 +78,9 @@ public PInvokeStubCodeGenerator( or { Generator: Forwarder }; // Track if any generators are just forwarders - for types other than void, this indicates - // types that can't be marshalled by the source generated + // types that can't be marshalled by the source generated. // In .NET 7+ support, we would have emitted a diagnostic error about lack of support - // In down-level support, we do not error - tracking this allows us to switch to generating a basic forwarder stub + // In down-level support, we do not error - tracking this allows us to switch to generating a basic forwarder (DllImport declaration) HasForwardedTypes |= generator is { Generator: Forwarder, TypeInfo.ManagedType: not SpecialTypeInfo { SpecialType: Microsoft.CodeAnalysis.SpecialType.System_Void } }; }