diff --git a/src/BenchmarkDotNet/Extensions/ReflectionExtensions.cs b/src/BenchmarkDotNet/Extensions/ReflectionExtensions.cs index cf1f71a166..0a1a7c22fc 100644 --- a/src/BenchmarkDotNet/Extensions/ReflectionExtensions.cs +++ b/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(); diff --git a/src/BenchmarkDotNet/Running/BenchmarkConverter.cs b/src/BenchmarkDotNet/Running/BenchmarkConverter.cs index dbe350b2c8..69837feec8 100644 --- a/src/BenchmarkDotNet/Running/BenchmarkConverter.cs +++ b/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) diff --git a/src/BenchmarkDotNet/Validators/CompilationValidator.cs b/src/BenchmarkDotNet/Validators/CompilationValidator.cs index d8c2ec6c5b..b0bb87681d 100644 --- a/src/BenchmarkDotNet/Validators/CompilationValidator.cs +++ b/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 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 ValidateClassModifiers(IEnumerable benchmarks) + { + return benchmarks + .Distinct(BenchmarkMethodEqualityComparer.Instance) + .SelectMany(benchmark => + { + var type = benchmark.Descriptor.Type; + var errors = new List(); + + 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 ValidateCSharpNaming(IEnumerable benchmarks) => benchmarks diff --git a/tests/BenchmarkDotNet.Tests/Validators/CompilationValidatorTests.cs b/tests/BenchmarkDotNet.Tests/Validators/CompilationValidatorTests.cs index 632875d52b..683e258bbe 100644 --- a/tests/BenchmarkDotNet.Tests/Validators/CompilationValidatorTests.cs +++ b/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 `[]` 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), 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() { } } + } }