[DllImportGenerator] Stop flagging methods with well-known unsupported types for...

* Make analyzer stop flagging methods with well-known unsupported types for conversion
* Make fixer put attribute arguments in preferred order

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 589935e..224d51b 100644 (file)
--- 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 @@ namespace Microsoft.Interop.Analyzers
     {
         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 @@ namespace Microsoft.Interop.Analyzers
                     if (generatedDllImportAttrType == null)
                         return;
 
-                    INamedTypeSymbol? dllImportAttrType = compilationContext.Compilation.GetTypeByMetadataName(typeof(DllImportAttribute).FullName);
-                    if (dllImportAttrType == null)
-                        return;
+                    var knownUnsupportedTypes = new List<ITypeSymbol>(s_unsupportedTypeNames.Length);
+                    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<ITypeSymbol> knownUnsupportedTypes)
         {
             var method = (IMethodSymbol)context.Symbol;
 
@@ -64,6 +75,19 @@ namespace Microsoft.Interop.Analyzers
             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/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportFixer.cs b/src/libraries/System.Runtime.InteropServices/gen/DllImportGenerator/Analyzers/ConvertToGeneratedDllImportFixer.cs
index 4b4a719..c99f64f 100644 (file)
--- 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 @@ namespace Microsoft.Interop.Analyzers
         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 @@ namespace Microsoft.Interop.Analyzers
                             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 @@ namespace Microsoft.Interop.Analyzers
                 }
             }
 
-            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/ConvertToGeneratedDllImportAnalyzerTests.cs b/src/libraries/System.Runtime.InteropServices/tests/DllImportGenerator.UnitTests/ConvertToGeneratedDllImportAnalyzerTests.cs
index 8d99a82..2524bcd 100644 (file)
--- 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 @@ namespace DllImportGenerator.UnitTests
             new object[] { typeof(ConsoleKey) }, // enum
         };
 
+        public static IEnumerable<object[]> UnsupportedTypes() => new[]
+        {
+            new object[] { typeof(System.Runtime.InteropServices.CriticalHandle) },
+            new object[] { typeof(System.Runtime.InteropServices.HandleRef) },
+            new object[] { typeof(System.Text.StringBuilder) },
+        };
+
         [ConditionalTheory]
         [MemberData(nameof(MarshallingRequiredTypes))]
         [MemberData(nameof(NoMarshallingRequiredTypes))]
@@ -134,6 +141,14 @@ partial class Test
                     .WithArguments("Method2"));
         }
 
+        [ConditionalTheory]
+        [MemberData(nameof(UnsupportedTypes))]
+        public async Task UnsupportedType_NoDiagnostic(Type type)
+        {
+            string source = DllImportWithType(type.FullName!);
+            await VerifyCS.VerifyAnalyzerAsync(source);
+        }
+
         [ConditionalFact]
         public async Task NotDllImport_NoDiagnostic()
         {

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 c6c61fe..9f8528c 100644 (file)
--- 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
 }}" : @$"
@@ -367,5 +367,43 @@ partial class Test
                 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,
+                fixedSource,
+                usePreprocessorDefines ? WithPreprocessorDefinesKey : NoPreprocessorDefinesKey);
+        }
     }
 }