NET-1253 Fix S1699 FN: Adds support for complex inheritance

Author: sebastien-marichal

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

@@ -14,61 +14,44 @@
  * along with this program; if not, see https://sonarsource.com/license/ssal/
  */
 
-namespace SonarAnalyzer.CSharp.Rules
-{
-    [DiagnosticAnalyzer(LanguageNames.CSharp)]
-    public sealed class ConstructorOverridableCall : SonarDiagnosticAnalyzer
-    {
-        internal const string DiagnosticId = "S1699";
-        private const string MessageFormat = "Remove this call from a constructor to the overridable '{0}' method.";
+namespace SonarAnalyzer.CSharp.Rules;
 
-        private static readonly DiagnosticDescriptor rule =
-            DescriptorFactory.Create(DiagnosticId, MessageFormat);
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class ConstructorOverridableCall : SonarDiagnosticAnalyzer
+{
+    private const string DiagnosticId = "S1699";
+    private const string MessageFormat = "Remove this call from a constructor to the overridable '{0}' method.";
 
-        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(rule);
+    private static readonly DiagnosticDescriptor Rule =
+        DescriptorFactory.Create(DiagnosticId, MessageFormat);
 
-        protected override void Initialize(SonarAnalysisContext context)
-        {
-            context.RegisterNodeAction(
-                CheckOverridableCallInConstructor,
-                SyntaxKind.InvocationExpression);
-        }
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);
 
-        private static void CheckOverridableCallInConstructor(SonarSyntaxNodeReportingContext context)
+    protected override void Initialize(SonarAnalysisContext context) =>
+        context.RegisterSymbolStartAction(c =>
         {
-            var invocationExpression = (InvocationExpressionSyntax)context.Node;
-
-            var calledOn = (invocationExpression.Expression as MemberAccessExpressionSyntax)?.Expression;
-            var isCalledOnThis = calledOn == null || calledOn is ThisExpressionSyntax;
-            if (!isCalledOnThis)
-            {
-                return;
-            }
-
-            var enclosingSymbol = context.Model.GetEnclosingSymbol(invocationExpression.SpanStart) as IMethodSymbol;
-            if (!IsMethodConstructor(enclosingSymbol))
+            if (c.Symbol is IMethodSymbol { MethodKind: MethodKind.Constructor, ContainingType.IsSealed: false } constructor
+                && !constructor.ContainingType.DerivesFrom(KnownType.Nancy_NancyModule))
             {
-                return;
+                c.RegisterSyntaxNodeAction(cc => CheckOverridableCallInConstructor(cc, constructor), SyntaxKind.InvocationExpression);
             }
+        }, SymbolKind.Method);
 
+    private static void CheckOverridableCallInConstructor(SonarSyntaxNodeReportingContext context, IMethodSymbol constructor)
+    {
+        var invocationExpression = (InvocationExpressionSyntax)context.Node;
 
-            if (context.Model.GetSymbolInfo(invocationExpression.Expression).Symbol is IMethodSymbol methodSymbol &&
-                IsMethodOverridable(methodSymbol) &&
-                enclosingSymbol.IsInType(methodSymbol.ContainingType))
-            {
-                context.ReportIssue(rule, invocationExpression.Expression, methodSymbol.Name);
-            }
-        }
-
-        private static bool IsMethodOverridable(IMethodSymbol methodSymbol)
-        {
-            return methodSymbol.IsVirtual || methodSymbol.IsAbstract;
-        }
-
-        private static bool IsMethodConstructor(IMethodSymbol methodSymbol)
+        if (invocationExpression.Expression is IdentifierNameSyntax or MemberAccessExpressionSyntax { Expression: ThisExpressionSyntax }
+            && context.Model.GetEnclosingSymbol(invocationExpression.SpanStart).Equals(constructor)
+            && context.Model.GetSymbolInfo(invocationExpression.Expression).Symbol is IMethodSymbol methodSymbol
+            && IsMethodOverridable(methodSymbol))
         {
-            return methodSymbol != null &&
-                methodSymbol.MethodKind == MethodKind.Constructor;
+            context.ReportIssue(Rule, invocationExpression.Expression, methodSymbol.Name);
         }
     }
+
+    private static bool IsMethodOverridable(IMethodSymbol methodSymbol) =>
+        methodSymbol.IsVirtual
+        || methodSymbol.IsAbstract
+        || (methodSymbol.IsOverride && !methodSymbol.IsSealed);
 }

analyzers/src/SonarAnalyzer.Core/AnalysisContext/SonarAnalysisContext.cs

@@ -82,6 +82,10 @@ public void RegisterSymbolAction(Action<SonarSymbolReportingContext> action, par
         RegisterCompilationStartAction(
             x => x.RegisterSymbolAction(action, symbolKinds));
 
+    public void RegisterSymbolStartAction(Action<SonarSymbolStartAnalysisContext> action, SymbolKind symbolKind) =>
+        RegisterCompilationStartAction(
+            x => x.RegisterSymbolStartAction(action, symbolKind));
+
     public void RegisterNodeAction<TSyntaxKind>(GeneratedCodeRecognizer generatedCodeRecognizer, Action<SonarSyntaxNodeReportingContext> action, params TSyntaxKind[] syntaxKinds)
         where TSyntaxKind : struct =>
         RegisterCompilationStartAction(

analyzers/src/SonarAnalyzer.Core/Semantics/KnownType.cs

@@ -194,6 +194,7 @@ public sealed partial class KnownType
     public static readonly KnownType MySql_Data_MySqlClient_MySqlHelper = new("MySql.Data.MySqlClient.MySqlHelper");
     public static readonly KnownType MySql_Data_MySqlClient_MySqlScript = new("MySql.Data.MySqlClient.MySqlScript");
     public static readonly KnownType Nancy_Cookies_NancyCookie = new("Nancy.Cookies.NancyCookie");
+    public static readonly KnownType Nancy_NancyModule = new("Nancy.NancyModule");
     public static readonly KnownType NFluent_Check = new("NFluent.Check");
     public static readonly KnownType NFluent_FluentCheckException = new("NFluent.FluentCheckException");
     public static readonly KnownType NFluent_Kernel_FluentCheckException = new("NFluent.Kernel.FluentCheckException");

analyzers/tests/SonarAnalyzer.Test/Rules/ConstructorOverridableCallTest.cs

@@ -16,13 +16,21 @@
 
 using SonarAnalyzer.CSharp.Rules;
 
-namespace SonarAnalyzer.Test.Rules
+namespace SonarAnalyzer.Test.Rules;
+
+[TestClass]
+public class ConstructorOverridableCallTest
 {
-    [TestClass]
-    public class ConstructorOverridableCallTest
-    {
-        [TestMethod]
-        public void ConstructorOverridableCall() =>
-            new VerifierBuilder<ConstructorOverridableCall>().AddPaths("ConstructorOverridableCall.cs").Verify();
-    }
+    private readonly VerifierBuilder builder = new VerifierBuilder<ConstructorOverridableCall>();
+
+    [TestMethod]
+    public void ConstructorOverridableCall() =>
+        builder.AddPaths("ConstructorOverridableCall.cs").Verify();
+
+    [TestMethod]
+    public void ConstructorOverridableCall_Nancy() =>
+        builder
+            .WithConcurrentAnalysis(false)
+            .AddPaths("ConstructorOverridableCall.Nancy.cs")
+            .VerifyNoIssues();
 }

analyzers/tests/SonarAnalyzer.Test/TestCases/ConstructorOverridableCall.Nancy.cs

@@ -0,0 +1,19 @@
+using System;
+
+namespace  Nancy
+{
+    // Shim of a Nancy module
+    // https://github.com/NancyFx/Nancy/wiki/Exploring-the-nancy-module
+    public class NancyModule
+    {
+        public virtual void Get<T>(string path, Func<dynamic, T> action) { }
+    }
+}
+
+public class SampleModule : Nancy.NancyModule
+{
+    public SampleModule()
+    {
+        Get("/", _ => "Hello World!"); // Compliant
+    }
+}

analyzers/tests/SonarAnalyzer.Test/TestCases/ConstructorOverridableCall.cs

@@ -55,21 +55,58 @@ public override void DoSomething()
             Console.WriteLine(this.foo.Length);
         }
     }
+
+    public class BaseClass
+    {
+        public BaseClass(int a)
+        {
+        }
+
+        public virtual int DoSomething() { return 0; }
+        public virtual int DoSomething(int a) { return 0; }
+    }
+
+    public class InlineBaseThisCall : BaseClass
+    {
+        // Error@+1 [CS0120]
+        public InlineBaseThisCall() : this(DoSomething()) { } // Noncompliant
+        // Error@+1 [CS0120]
+        public InlineBaseThisCall(int a) : base(DoSomething(a)) { } // Noncompliant
+    }
+
+    // https://sonarsource.atlassian.net/browse/NET-1318
+    public class LocalFuncion : BaseClass
+    {
+        public LocalFuncion() : base(0)
+        {
+            Local();
+
+            void Local()
+            {
+                DoSomething(); // FN
+            }
+        }
+    }
 }
 
 // https://github.com/SonarSource/sonar-dotnet/issues/6245
 namespace Tests.Diagnostics.Repro_6245
 {
     public class Class1
     {
+        public Class1()
+        {
+            DoSomething(); // Noncompliant
+        }
+
         public virtual void DoSomething() { }
     }
 
     public class ClassA2 : Class1
     {
         public ClassA2()
         {
-            DoSomething(); // FN
+            DoSomething(); // Noncompliant
         }
     }
 
@@ -80,6 +117,7 @@ public class ClassA3 : ClassA2
         public ClassA3(string name)
         {
             Name = name;
+            DoSomething(); // Noncompliant
         }
 
         public override void DoSomething()