Fix S4200 FN: Add support for LibraryImportAttribute; run only on non…

…-generated code (#6603)

analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/Net7/S4200.cs

@@ -0,0 +1,13 @@
+using System.Runtime.InteropServices;
+
+namespace Net7;
+
+public static partial class S4200
+{
+    // Use Goto definition in VS on the methods to see the code generated by the Microsoft.Interop.LibraryImportGenerator
+    [LibraryImport("foo.dll")]
+    public static partial void DllImportAttributeAppliedToThisFunction(); // Noncompliant (S4200): Make this native method private and provide a wrapper.
+
+    [LibraryImport("foo.dll", StringMarshalling = StringMarshalling.Utf8)]
+    public static partial void DllImportAttributeAppliedToGeneratedLocalFunction(string p); // Noncompliant
+}

analyzers/src/SonarAnalyzer.CSharp/Rules/NativeMethodsShouldBeWrapped.cs

@@ -34,20 +34,28 @@ public sealed class NativeMethodsShouldBeWrapped : SonarDiagnosticAnalyzer
         protected override void Initialize(SonarAnalysisContext context)
         {
             context.RegisterSymbolAction(ReportPublicExternalMethods, SymbolKind.Method);
-            context.RegisterSyntaxNodeAction(ReportTrivialWrappers, SyntaxKind.MethodDeclaration);
+            context.RegisterSyntaxNodeActionInNonGenerated(ReportTrivialWrappers, SyntaxKind.MethodDeclaration);
         }
 
         private static void ReportPublicExternalMethods(SymbolAnalysisContext c)
         {
             var methodSymbol = (IMethodSymbol)c.Symbol;
-            if (methodSymbol.IsExtern
-                && methodSymbol.IsPubliclyAccessible()
-                && methodSymbol.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is MethodDeclarationSyntax methodDeclaration)
+            if (IsExternMethod(methodSymbol)
+                && methodSymbol.IsPubliclyAccessible())
             {
-                c.ReportIssue(Diagnostic.Create(Rule, methodDeclaration.Identifier.GetLocation(), MakeThisMethodPrivateMessage));
+                foreach (var methodDeclaration in methodSymbol.DeclaringSyntaxReferences
+                    .Where(x => !x.SyntaxTree.IsGenerated(CSharpGeneratedCodeRecognizer.Instance, c.Compilation))
+                    .Select(x => x.GetSyntax())
+                    .OfType<MethodDeclarationSyntax>())
+                {
+                    c.ReportIssue(Diagnostic.Create(Rule, methodDeclaration.Identifier.GetLocation(), MakeThisMethodPrivateMessage));
+                }
             }
         }
 
+        private static bool IsExternMethod(IMethodSymbol methodSymbol) =>
+            methodSymbol.IsExtern || methodSymbol.HasAttribute(KnownType.System_Runtime_InteropServices_LibraryImportAttribute);
+
         private static void ReportTrivialWrappers(SyntaxNodeAnalysisContext c)
         {
             var methodDeclaration = (MethodDeclarationSyntax)c.Node;
@@ -97,7 +105,7 @@ a.Expression is LiteralExpressionSyntax
         private static ISet<IMethodSymbol> GetExternalMethods(IMethodSymbol methodSymbol) =>
             methodSymbol.ContainingType.GetMembers()
                 .OfType<IMethodSymbol>()
-                .Where(m => m.IsExtern)
+                .Where(IsExternMethod)
                 .ToHashSet();
 
         private static IEnumerable<SyntaxNode> GetBodyDescendants(MethodDeclarationSyntax methodDeclaration) =>

analyzers/tests/SonarAnalyzer.UnitTest/Rules/NativeMethodsShouldBeWrappedTest.cs

@@ -41,9 +41,19 @@ public void NativeMethodsShouldBeWrapped_CSharp9() =>
         public void NativeMethodsShouldBeWrapped_CSharp10() =>
             builder.AddPaths("NativeMethodsShouldBeWrapped.CSharp10.cs").WithOptions(ParseOptionsHelper.FromCSharp10).Verify();
 
+        // NativeMethodsShouldBeWrapped.CSharp11.SourceGenerator.cs contains the code as generated by the SourceGenerator. To regenerate it:
+        // * Take the code from NativeMethodsShouldBeWrapped.CSharp11.cs
+        // * Copy it to a new .Net 7 project
+        // * Press F12 on any of the partial methods
+        // * Copy the result to NativeMethodsShouldBeWrapped.CSharp11.SourceGenerator.cs
         [TestMethod]
         public void NativeMethodsShouldBeWrapped_CSharp11() =>
-            builder.AddPaths("NativeMethodsShouldBeWrapped.CSharp11.cs").WithOptions(ParseOptionsHelper.FromCSharp11).Verify();
+            builder
+                .AddPaths("NativeMethodsShouldBeWrapped.CSharp11.cs")
+                .AddPaths("NativeMethodsShouldBeWrapped.CSharp11.SourceGenerator.cs")
+                .WithOptions(ParseOptionsHelper.FromCSharp11)
+                .WithConcurrentAnalysis(false)
+                .Verify();
 
 #endif
 

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/NativeMethodsShouldBeWrapped.CSharp11.SourceGenerator.cs

@@ -0,0 +1,86 @@
+// <auto-generated/>
+namespace LibraryImportAttributeTests
+{
+    public static unsafe partial class ExternMethods
+    {
+        [System.Runtime.InteropServices.DllImportAttribute("foo.dll", EntryPoint = "DllImportAttributeAppliedToThisFunction", ExactSpelling = true)]
+        public static extern partial void DllImportAttributeAppliedToThisFunction();
+    }
+}
+namespace LibraryImportAttributeTests
+{
+    public static unsafe partial class ExternMethods
+    {
+        [System.Runtime.InteropServices.DllImportAttribute("foo.dll", EntryPoint = "CompliantDllImportAttributeAppliedToThisFunction", ExactSpelling = true)]
+        private static extern partial void CompliantDllImportAttributeAppliedToThisFunction(int i);
+    }
+}
+namespace LibraryImportAttributeTests
+{
+    public static unsafe partial class ExternMethods
+    {
+        [System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.LibraryImportGenerator", "7.0.7.6804")]
+        [System.Runtime.CompilerServices.SkipLocalsInitAttribute]
+        public static partial void DllImportAttributeAppliedToGeneratedLocalFunction(string p)
+        {
+            byte* __p_native = default;
+            // Setup - Perform required setup.
+            global::System.Runtime.InteropServices.Marshalling.Utf8StringMarshaller.ManagedToUnmanagedIn __p_native__marshaller = new();
+            try
+            {
+                // Marshal - Convert managed data to native data.
+                byte* __p_native__stackptr = stackalloc byte[global::System.Runtime.InteropServices.Marshalling.Utf8StringMarshaller.ManagedToUnmanagedIn.BufferSize];
+                __p_native__marshaller.FromManaged(p, new System.Span<byte>(__p_native__stackptr, global::System.Runtime.InteropServices.Marshalling.Utf8StringMarshaller.ManagedToUnmanagedIn.BufferSize));
+                {
+                    // PinnedMarshal - Convert managed data to native data that requires the managed data to be pinned.
+                    __p_native = __p_native__marshaller.ToUnmanaged();
+                    __PInvoke(__p_native);
+                }
+            }
+            finally
+            {
+                // Cleanup - Perform required cleanup.
+                __p_native__marshaller.Free();
+            }
+
+            // Local P/Invoke
+            [System.Runtime.InteropServices.DllImportAttribute("foo.dll", EntryPoint = "DllImportAttributeAppliedToGeneratedLocalFunction", ExactSpelling = true)]
+            static extern unsafe void __PInvoke(byte* p);
+        }
+    }
+}
+namespace LibraryImportAttributeTests
+{
+    public static unsafe partial class ExternMethods
+    {
+        [System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Interop.LibraryImportGenerator", "7.0.7.6804")]
+        [System.Runtime.CompilerServices.SkipLocalsInitAttribute]
+        private static partial void CompliantDllImportAttributeAppliedToGeneratedLocalFunction(string p)
+        {
+            byte* __p_native = default;
+            // Setup - Perform required setup.
+            global::System.Runtime.InteropServices.Marshalling.Utf8StringMarshaller.ManagedToUnmanagedIn __p_native__marshaller = new();
+            try
+            {
+                // Marshal - Convert managed data to native data.
+                byte* __p_native__stackptr = stackalloc byte[global::System.Runtime.InteropServices.Marshalling.Utf8StringMarshaller.ManagedToUnmanagedIn.BufferSize];
+                __p_native__marshaller.FromManaged(p, new System.Span<byte>(__p_native__stackptr, global::System.Runtime.InteropServices.Marshalling.Utf8StringMarshaller.ManagedToUnmanagedIn.BufferSize));
+                {
+                    // PinnedMarshal - Convert managed data to native data that requires the managed data to be pinned.
+                    __p_native = __p_native__marshaller.ToUnmanaged();
+                    __PInvoke(__p_native);
+                }
+            }
+            finally
+            {
+                // Cleanup - Perform required cleanup.
+                __p_native__marshaller.Free();
+            }
+
+            // Local P/Invoke
+            [System.Runtime.InteropServices.DllImportAttribute("foo.dll", EntryPoint = "CompliantDllImportAttributeAppliedToGeneratedLocalFunction", ExactSpelling = true)]
+            static extern unsafe void __PInvoke(byte* p);
+        }
+    }
+}
+

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/NativeMethodsShouldBeWrapped.CSharp11.cs

@@ -8,3 +8,40 @@ public interface IInterface
     [DllImport("kernel32.dll", CharSet = CharSet.Unicode)]
     public static extern virtual bool RemoveDirectory2(string name);  // Noncompliant
 }
+
+namespace LibraryImportAttributeTests
+{
+    // Copy the class to a .Net 7 project in VS and use "goto definition" on the methods to extract the code generated by the Microsoft.Interop.LibraryImportGenerator
+    public static partial class ExternMethods
+    {
+        [LibraryImport("foo.dll")]
+        public static partial void DllImportAttributeAppliedToThisFunction(); // Noncompliant
+
+        [LibraryImport("foo.dll")]
+        private static partial void CompliantDllImportAttributeAppliedToThisFunction(int i); // Compliant
+
+        [LibraryImport("foo.dll", StringMarshalling = StringMarshalling.Utf8)]
+        public static partial void DllImportAttributeAppliedToGeneratedLocalFunction(string p); // Noncompliant
+
+        [LibraryImport("foo.dll", StringMarshalling = StringMarshalling.Utf8)]
+        private static partial void CompliantDllImportAttributeAppliedToGeneratedLocalFunction(string p); // Compliant
+
+        // Wrapper tests
+
+        public static void CompliantDllImportAttributeAppliedToThisFunctionNoncompliantWrapper(int i) // Noncompliant {{Make this wrapper for native method 'CompliantDllImportAttributeAppliedToThisFunction' less trivial.}}
+        {
+            CompliantDllImportAttributeAppliedToThisFunction(i);
+        }
+
+        public static void CompliantDllImportAttributeAppliedToThisFunctionCompliantWrapper(int i) // Compliant
+        {
+            if (i > 0)
+            {
+                CompliantDllImportAttributeAppliedToThisFunction(i);
+            }
+        }
+
+        public static void CompliantDllImportAttributeAppliedToGeneratedLocalFunctionWrapper(string p) // Noncompliant
+            => CompliantDllImportAttributeAppliedToGeneratedLocalFunction(p);
+    }
+}