Fixed: Add validation warning for sealed classes containing benchmarks (dotnet#2660)

* Add validation for sealed classes containing benchmarks

* Moved public class validation to CompilationValidator

Co-authored-by: Tim Cassell <[email protected]>

Author: ketanpkolte

src/BenchmarkDotNet/Extensions/ReflectionExtensions.cs

@@ -155,10 +155,7 @@ internal static bool ContainsRunnableBenchmarks(this Type type)
         {
             var typeInfo = type.GetTypeInfo();
 
-            if (typeInfo.IsAbstract
-                || typeInfo.IsSealed
-                || typeInfo.IsNotPublic
-                || typeInfo.IsGenericType && !IsRunnableGenericType(typeInfo))
+            if (typeInfo.IsAbstract || typeInfo.IsGenericType && !IsRunnableGenericType(typeInfo))
                 return false;
 
             return typeInfo.GetBenchmarks().Any();

src/BenchmarkDotNet/Running/BenchmarkConverter.cs

@@ -263,16 +263,7 @@ private static void AssertMethodIsAccessible(string methodType, MethodInfo metho
         {
             if (!methodInfo.IsPublic)
                 throw new InvalidBenchmarkDeclarationException($"{methodType} method {methodInfo.Name} has incorrect access modifiers.\nMethod must be public.");
-
-            var declaringType = methodInfo.DeclaringType;
-
-            while (declaringType != null)
-            {
-                if (!declaringType.GetTypeInfo().IsPublic && !declaringType.GetTypeInfo().IsNestedPublic)
-                    throw new InvalidBenchmarkDeclarationException($"{declaringType.FullName} containing {methodType} method {methodInfo.Name} has incorrect access modifiers.\nDeclaring type must be public.");
-
-                declaringType = declaringType.DeclaringType;
-            }
+            /* Moved the code that verifies if DeclaringType of a given MethodInfo (a method) is publicly accessible to CompilationValidator */
         }
 
         private static void AssertMethodIsNotGeneric(string methodType, MethodInfo methodInfo)

src/BenchmarkDotNet/Validators/CompilationValidator.cs

@@ -7,6 +7,7 @@
 using BenchmarkDotNet.Running;
 using BenchmarkDotNet.Toolchains;
 using Microsoft.CodeAnalysis.CSharp;
+using BenchmarkDotNet.Attributes;
 
 namespace BenchmarkDotNet.Validators
 {
@@ -25,8 +26,38 @@ private CompilationValidator() { }
         public IEnumerable<ValidationError> Validate(ValidationParameters validationParameters)
             => ValidateCSharpNaming(validationParameters.Benchmarks)
                     .Union(ValidateNamingConflicts(validationParameters.Benchmarks))
+                    .Union(ValidateClassModifiers((validationParameters.Benchmarks))
                     .Union(ValidateAccessModifiers(validationParameters.Benchmarks))
-                    .Union(ValidateBindingModifiers(validationParameters.Benchmarks));
+                    .Union(ValidateBindingModifiers(validationParameters.Benchmarks))
+                );
+
+        private static IEnumerable<ValidationError> ValidateClassModifiers(IEnumerable<BenchmarkCase> benchmarks)
+        {
+            return benchmarks
+                .Distinct(BenchmarkMethodEqualityComparer.Instance)
+                .SelectMany(benchmark =>
+                {
+                    var type = benchmark.Descriptor.Type;
+                    var errors = new List<ValidationError>();
+
+                    if (type.IsSealed)
+                    {
+                        errors.Add(new ValidationError(
+                           true,
+                           $"Benchmarked method `{benchmark.Descriptor.WorkloadMethod.Name}` is within a sealed class, Declaring type must be unsealed.",
+                           benchmark));
+                    }
+                    if (!type.IsVisible)
+                    {
+                        errors.Add(new ValidationError(
+                            true,
+                            $"Benchmarked method `{benchmark.Descriptor.WorkloadMethod.Name}` is within a non-visible class, all declaring types must be public.",
+                            benchmark));
+                    }
+                    // TODO: Generics validation
+                    return errors;
+                });
+        }
 
         private static IEnumerable<ValidationError> ValidateCSharpNaming(IEnumerable<BenchmarkCase> benchmarks)
             => benchmarks

tests/BenchmarkDotNet.Tests/Validators/CompilationValidatorTests.cs

@@ -7,6 +7,7 @@
 using BenchmarkDotNet.Attributes;
 using BenchmarkDotNet.Validators;
 using Xunit;
+using System.Reflection;
 
 namespace BenchmarkDotNet.Tests.Validators
 {
@@ -59,6 +60,29 @@ public void BenchmarkedMethodNameMustNotUseCsharpKeywords()
                     "Benchmarked method `typeof` contains illegal character(s) or uses C# keyword. Please use `[<Benchmark(Description = \"Custom name\")>]` to set custom display name."));
         }
 
+        [Theory]
+        /* BenchmarkDotNet can only benchmark public unsealed classes*/
+        [InlineData(typeof(BenchMarkPublicClass), false)]
+        [InlineData(typeof(BenchMarkPublicClass.PublicNestedClass), false)]
+        [InlineData(typeof(SealedClass.PublicNestedClass), false)]
+        [InlineData(typeof(OuterClass.PublicNestedClass), true)]
+        [InlineData(typeof(SealedClass), true)]
+        [InlineData(typeof(MyPrivateClass), true)]
+        [InlineData(typeof(MyPublicProtectedClass), true)]
+        [InlineData(typeof(MyPrivateProtectedClass), true)]
+        [InlineData(typeof(MyProtectedInternalClass), true)]
+        [InlineData(typeof(MyInternalClass), true)]
+        [InlineData(typeof(OuterClass), true)]
+        [InlineData(typeof(OuterClass.InternalNestedClass), true)]
+        [InlineData(typeof(BenchMarkPublicClass.InternalNestedClass), true)]
+        /* Generics Remaining */
+        public void Benchmark_Class_Modifers_Must_Be_Public(Type type, bool hasErrors)
+        {
+            var validationErrors = CompilationValidator.FailOnError.Validate(BenchmarkConverter.TypeToBenchmarks(type));
+
+            Assert.Equal(hasErrors, validationErrors.Any());
+        }
+
         [Theory]
         [InlineData(typeof(BenchmarkClassWithStaticMethod), true)]
         [InlineData(typeof(BenchmarkClass<PublicClass>), false)]
@@ -115,7 +139,17 @@ protected internal class ProtectedInternalClass
         {
             protected internal class ProtectedInternalNestedClass { }
         }
-    }
+
+        private class MyPrivateClass{ [Benchmark] public void PublicMethod(){} }
+
+        protected class MyPublicProtectedClass{ [Benchmark] public void PublicMethod(){} }
+
+        private protected class MyPrivateProtectedClass{ [Benchmark] public void PublicMethod(){} }
+
+        internal class MyInternalClass{ [Benchmark] public void PublicMethod(){} }
+
+        protected internal class MyProtectedInternalClass{ [Benchmark] public void PublicMethod() { } }
+        }
 
     public class BenchmarkClassWithStaticMethod
     {
@@ -138,4 +172,29 @@ internal class InternalClass
     {
         internal class InternalNestedClass { }
     }
+
+    public sealed class SealedClass
+    {
+        [Benchmark] public void PublicMethod() { }
+
+        public class PublicNestedClass { [Benchmark] public void PublicMethod() { } }
+    }
+
+    internal class OuterClass
+    {
+        [Benchmark] public void PublicMethod(){}
+
+        internal class InternalNestedClass { [Benchmark] public void PublicMethod() { } }
+
+        public class PublicNestedClass { [Benchmark] public void PublicMethod() { } }
+    }
+
+    public class BenchMarkPublicClass
+    {
+        [Benchmark] public void PublicMethod(){}
+
+        public class PublicNestedClass { [Benchmark] public void PublicMethod() { } }
+
+        internal class InternalNestedClass { [Benchmark] public void PublicMethod() { } }
+    }
 }