From 12d1f96acb3aae4b96c7b052bfaff890e7f2d8be Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Fri, 22 Oct 2021 15:03:19 -0700 Subject: [PATCH 1/3] Stop flagging methods with well-known unsupported types for conversion --- .../ConvertToGeneratedDllImportAnalyzer.cs | 38 +++++++++++++++---- ...onvertToGeneratedDllImportAnalyzerTests.cs | 15 ++++++++ 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportAnalyzer.cs b/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportAnalyzer.cs index 589935ea5c31dd..ee77d9a0e2f80a 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportAnalyzer.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportAnalyzer.cs @@ -3,8 +3,6 @@ using System.Collections.Generic; using System.Collections.Immutable; -using System.Diagnostics; -using System.Runtime.InteropServices; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; @@ -18,6 +16,13 @@ public class ConvertToGeneratedDllImportAnalyzer : DiagnosticAnalyzer { private const string Category = "Interoperability"; + private static readonly string[] s_unsupportedTypeNames = new string[] + { + "System.Runtime.InteropServices.CriticalHandle", + "System.Runtime.InteropServices.HandleRef", + "System.Text.StringBuilder" + }; + public static readonly DiagnosticDescriptor ConvertToGeneratedDllImport = new DiagnosticDescriptor( Ids.ConvertToGeneratedDllImport, @@ -43,15 +48,21 @@ public override void Initialize(AnalysisContext context) if (generatedDllImportAttrType == null) return; - INamedTypeSymbol? dllImportAttrType = compilationContext.Compilation.GetTypeByMetadataName(typeof(DllImportAttribute).FullName); - if (dllImportAttrType == null) - return; + List knownUnsupportedTypes = new List(); + foreach (string typeName in s_unsupportedTypeNames) + { + INamedTypeSymbol? unsupportedType = compilationContext.Compilation.GetTypeByMetadataName(typeName); + if (unsupportedType != null) + { + knownUnsupportedTypes.Add(unsupportedType); + } + } - compilationContext.RegisterSymbolAction(symbolContext => AnalyzeSymbol(symbolContext, dllImportAttrType), SymbolKind.Method); + compilationContext.RegisterSymbolAction(symbolContext => AnalyzeSymbol(symbolContext, knownUnsupportedTypes), SymbolKind.Method); }); } - private static void AnalyzeSymbol(SymbolAnalysisContext context, INamedTypeSymbol dllImportAttrType) + private static void AnalyzeSymbol(SymbolAnalysisContext context, List knownUnsupportedTypes) { var method = (IMethodSymbol)context.Symbol; @@ -64,6 +75,19 @@ private static void AnalyzeSymbol(SymbolAnalysisContext context, INamedTypeSymbo if (dllImportData.ModuleName == "QCall") return; + // Ignore methods with unsupported parameters + foreach (IParameterSymbol parameter in method.Parameters) + { + if (knownUnsupportedTypes.Contains(parameter.Type)) + { + return; + } + } + + // Ignore methods with unsupported returns + if (method.ReturnsByRef || method.ReturnsByRefReadonly || knownUnsupportedTypes.Contains(method.ReturnType)) + return; + context.ReportDiagnostic(method.CreateDiagnostic(ConvertToGeneratedDllImport, method.Name)); } } diff --git a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/ConvertToGeneratedDllImportAnalyzerTests.cs b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/ConvertToGeneratedDllImportAnalyzerTests.cs index 2329c3d221b77b..8e4b4cb9f44aae 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/ConvertToGeneratedDllImportAnalyzerTests.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/ConvertToGeneratedDllImportAnalyzerTests.cs @@ -37,6 +37,13 @@ public static IEnumerable NoMarshallingRequiredTypes() => new[] new object[] { typeof(ConsoleKey) }, // enum }; + public static IEnumerable UnsupportedTypes() => new[] + { + new object[] { typeof(System.Runtime.InteropServices.CriticalHandle) }, + new object[] { typeof(System.Runtime.InteropServices.HandleRef) }, + new object[] { typeof(System.Text.StringBuilder) }, + }; + [Theory] [MemberData(nameof(MarshallingRequiredTypes))] [MemberData(nameof(NoMarshallingRequiredTypes))] @@ -134,6 +141,14 @@ await VerifyCS.VerifyAnalyzerAsync( .WithArguments("Method2")); } + [Theory] + [MemberData(nameof(UnsupportedTypes))] + public async Task UnsupportedType_NoDiagnostic(Type type) + { + string source = DllImportWithType(type.FullName!); + await VerifyCS.VerifyAnalyzerAsync(source); + } + [Fact] public async Task NotDllImport_NoDiagnostic() { From 36e5edf4d1a3c645b196167c596807decc4fbd52 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Mon, 25 Oct 2021 12:00:11 -0700 Subject: [PATCH 2/3] Make fixer put attribute arguments in preferred order --- .../ConvertToGeneratedDllImportFixer.cs | 39 +++++++++++++++- .../ConvertToGeneratedDllImportFixerTests.cs | 44 +++++++++++++++++-- 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportFixer.cs b/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportFixer.cs index 4b4a719057b5d7..c99f64fcc4829d 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportFixer.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportFixer.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Collections.Immutable; +using System.Linq; using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; @@ -28,6 +29,18 @@ public sealed class ConvertToGeneratedDllImportFixer : CodeFixProvider public const string NoPreprocessorDefinesKey = "ConvertToGeneratedDllImport"; public const string WithPreprocessorDefinesKey = "ConvertToGeneratedDllImportPreprocessor"; + private static readonly string[] s_preferredAttributeArgumentOrder = + { + nameof(DllImportAttribute.EntryPoint), + nameof(DllImportAttribute.BestFitMapping), + nameof(DllImportAttribute.CallingConvention), + nameof(DllImportAttribute.CharSet), + nameof(DllImportAttribute.ExactSpelling), + nameof(DllImportAttribute.PreserveSig), + nameof(DllImportAttribute.SetLastError), + nameof(DllImportAttribute.ThrowOnUnmappableChar) + }; + public override async Task RegisterCodeFixesAsync(CodeFixContext context) { // Get the syntax root and semantic model @@ -152,8 +165,11 @@ private async Task ConvertToGeneratedDllImport( SyntaxFactory.ElasticMarker })); + // Sort attribute arguments so that GeneratedDllImport and DllImport match + MethodDeclarationSyntax updatedDeclaration = (MethodDeclarationSyntax)generator.ReplaceNode(methodSyntax, dllImportSyntax, SortDllImportAttributeArguments(dllImportSyntax, generator)); + // Remove existing leading trivia - it will be on the GeneratedDllImport method - MethodDeclarationSyntax updatedDeclaration = methodSyntax.WithLeadingTrivia(); + updatedDeclaration = updatedDeclaration.WithLeadingTrivia(); // #endif updatedDeclaration = updatedDeclaration.WithTrailingTrivia( @@ -225,7 +241,26 @@ private SyntaxNode GetGeneratedDllImportAttribute( } } - return generator.RemoveNodes(generatedDllImportSyntax, argumentsToRemove); + generatedDllImportSyntax = generator.RemoveNodes(generatedDllImportSyntax, argumentsToRemove); + return SortDllImportAttributeArguments((AttributeSyntax)generatedDllImportSyntax, generator); + } + + private static SyntaxNode SortDllImportAttributeArguments(AttributeSyntax attribute, SyntaxGenerator generator) + { + AttributeArgumentListSyntax updatedArgList = attribute.ArgumentList.WithArguments( + SyntaxFactory.SeparatedList( + attribute.ArgumentList.Arguments.OrderBy(arg => + { + // Unnamed arguments first + if (arg.NameEquals == null) + return -1; + + // Named arguments in specified order, followed by any named arguments with no preferred order + string name = arg.NameEquals.Name.Identifier.Text; + int index = System.Array.IndexOf(s_preferredAttributeArgumentOrder, name); + return index == -1 ? int.MaxValue : index; + }))); + return generator.ReplaceNode(attribute, attribute.ArgumentList, updatedArgList); } private bool TryCreateUnmanagedCallConvAttributeToEmit( diff --git a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/ConvertToGeneratedDllImportFixerTests.cs b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/ConvertToGeneratedDllImportFixerTests.cs index 15519be05f4a29..0a3523e257f6e4 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/ConvertToGeneratedDllImportFixerTests.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/ConvertToGeneratedDllImportFixerTests.cs @@ -257,7 +257,7 @@ partial class Test [GeneratedDllImport(""DoesNotExist"", EntryPoint = ""Entry"")] public static partial int {{|CS8795:Method1|}}(out int ret); #else - [DllImport(""DoesNotExist"", BestFitMapping = false, EntryPoint = ""Entry"")] + [DllImport(""DoesNotExist"", EntryPoint = ""Entry"", BestFitMapping = false)] public static extern int Method1(out int ret); #endif @@ -306,7 +306,7 @@ partial class Test [GeneratedDllImport(""DoesNotExist"", EntryPoint = ""Entry"")] public static partial int {{|CS8795:Method1|}}(out int ret); #else - [DllImport(""DoesNotExist"", CallingConvention = CallingConvention.Winapi, EntryPoint = ""Entry"")] + [DllImport(""DoesNotExist"", EntryPoint = ""Entry"", CallingConvention = CallingConvention.Winapi)] public static extern int Method1(out int ret); #endif }}" : @$" @@ -351,7 +351,7 @@ partial class Test [UnmanagedCallConv(CallConvs = new System.Type[] {{ typeof({callConvType.FullName}) }})] public static partial int {{|CS8795:Method1|}}(out int ret); #else - [DllImport(""DoesNotExist"", CallingConvention = CallingConvention.{callConv}, EntryPoint = ""Entry"")] + [DllImport(""DoesNotExist"", EntryPoint = ""Entry"", CallingConvention = CallingConvention.{callConv})] public static extern int Method1(out int ret); #endif }}" : @$" @@ -361,6 +361,44 @@ partial class Test [GeneratedDllImport(""DoesNotExist"", EntryPoint = ""Entry"")] [UnmanagedCallConv(CallConvs = new System.Type[] {{ typeof({callConvType.FullName}) }})] public static partial int {{|CS8795:Method1|}}(out int ret); +}}"; + await VerifyCS.VerifyCodeFixAsync( + source, + fixedSource, + usePreprocessorDefines ? WithPreprocessorDefinesKey : NoPreprocessorDefinesKey); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task PreferredAttributeOrder(bool usePreprocessorDefines) + { + string source = @$" +using System.Runtime.InteropServices; +partial class Test +{{ + [DllImport(""DoesNotExist"", SetLastError = true, EntryPoint = ""Entry"", ExactSpelling = true, CharSet = CharSet.Unicode)] + public static extern int [|Method|](out int ret); +}}"; + // Fixed source will have CS8795 (Partial method must have an implementation) without generator run + string fixedSource = usePreprocessorDefines + ? @$" +using System.Runtime.InteropServices; +partial class Test +{{ +#if DLLIMPORTGENERATOR_ENABLED + [GeneratedDllImport(""DoesNotExist"", EntryPoint = ""Entry"", CharSet = CharSet.Unicode, ExactSpelling = true, SetLastError = true)] + public static partial int {{|CS8795:Method|}}(out int ret); +#else + [DllImport(""DoesNotExist"", EntryPoint = ""Entry"", CharSet = CharSet.Unicode, ExactSpelling = true, SetLastError = true)] + public static extern int Method(out int ret); +#endif +}}" : @$" +using System.Runtime.InteropServices; +partial class Test +{{ + [GeneratedDllImport(""DoesNotExist"", EntryPoint = ""Entry"", CharSet = CharSet.Unicode, ExactSpelling = true, SetLastError = true)] + public static partial int {{|CS8795:Method|}}(out int ret); }}"; await VerifyCS.VerifyCodeFixAsync( source, From dde7663230381ef54f06495ae3e4f34d3c4800c8 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Mon, 25 Oct 2021 20:16:25 -0700 Subject: [PATCH 3/3] Update src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportAnalyzer.cs Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com> --- .../Analyzers/ConvertToGeneratedDllImportAnalyzer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportAnalyzer.cs b/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportAnalyzer.cs index ee77d9a0e2f80a..224d51bc99384e 100644 --- a/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportAnalyzer.cs +++ b/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportAnalyzer.cs @@ -48,7 +48,7 @@ public override void Initialize(AnalysisContext context) if (generatedDllImportAttrType == null) return; - List knownUnsupportedTypes = new List(); + var knownUnsupportedTypes = new List(s_unsupportedTypeNames.Length); foreach (string typeName in s_unsupportedTypeNames) { INamedTypeSymbol? unsupportedType = compilationContext.Compilation.GetTypeByMetadataName(typeName);