Rework ReturnValueValidator/ExecutionValidator

Author: YegorStepanov

src/BenchmarkDotNet/Validators/ExecutionValidator.cs

@@ -1,6 +1,5 @@
 using System;
 using System.Collections.Generic;
-using BenchmarkDotNet.Running;
 
 namespace BenchmarkDotNet.Validators
 {
@@ -12,20 +11,20 @@ public class ExecutionValidator : ExecutionValidatorBase
         private ExecutionValidator(bool failOnError)
             : base(failOnError) { }
 
-        protected override void ExecuteBenchmarks(object benchmarkTypeInstance, IEnumerable<BenchmarkCase> benchmarks, List<ValidationError> errors)
+        protected override void ExecuteBenchmarks(IEnumerable<BenchmarkExecutor> executors, List<ValidationError> errors)
         {
-            foreach (var benchmark in benchmarks)
+            foreach (var executor in executors)
             {
                 try
                 {
-                    benchmark.Descriptor.WorkloadMethod.Invoke(benchmarkTypeInstance, null);
+                    executor.Invoke();
                 }
                 catch (Exception ex)
                 {
                     errors.Add(new ValidationError(
                         TreatsWarningsAsErrors,
-                        $"Failed to execute benchmark '{benchmark.DisplayInfo}', exception was: '{GetDisplayExceptionMessage(ex)}'",
-                        benchmark));
+                        $"Failed to execute benchmark '{executor.BenchmarkCase.DisplayInfo}', exception was: '{GetDisplayExceptionMessage(ex)}'",
+                        executor.BenchmarkCase));
                 }
             }
         }

src/BenchmarkDotNet/Validators/ExecutionValidatorBase.cs

@@ -6,6 +6,7 @@
 using BenchmarkDotNet.Attributes;
 using BenchmarkDotNet.Extensions;
 using BenchmarkDotNet.Running;
+using BenchmarkDotNet.Toolchains.InProcess.NoEmit;
 
 namespace BenchmarkDotNet.Validators
 {
@@ -24,29 +25,34 @@ public IEnumerable<ValidationError> Validate(ValidationParameters validationPara
 
             foreach (var typeGroup in validationParameters.Benchmarks.GroupBy(benchmark => benchmark.Descriptor.Type))
             {
-                if (!TryCreateBenchmarkTypeInstance(typeGroup.Key, errors, out var benchmarkTypeInstance))
-                {
-                    continue;
-                }
+                var executors = new List<BenchmarkExecutor>();
 
-                if (!TryToSetParamsFields(benchmarkTypeInstance, errors))
+                foreach (var benchmark in typeGroup)
                 {
-                    continue;
-                }
+                    if (!TryCreateBenchmarkTypeInstance(typeGroup.Key, errors, out var benchmarkTypeInstance))
+                        continue;
 
-                if (!TryToSetParamsProperties(benchmarkTypeInstance, errors))
-                {
-                    continue;
-                }
+                    if (!TryToFillParameters(benchmark, benchmarkTypeInstance, errors))
+                        continue;
 
-                if (!TryToCallGlobalSetup(benchmarkTypeInstance, errors))
-                {
-                    continue;
+                    if (!TryToCallGlobalSetup(benchmarkTypeInstance, errors))
+                        continue;
+
+                    if (!TryToCallIterationSetup(benchmarkTypeInstance, errors))
+                        continue;
+
+                    executors.Add(new BenchmarkExecutor(benchmarkTypeInstance, benchmark));
                 }
 
-                ExecuteBenchmarks(benchmarkTypeInstance, typeGroup, errors);
+                ExecuteBenchmarks(executors, errors);
+
+                foreach (var executor in executors)
+                {
+                    if (!TryToCallIterationCleanup(executor.Instance, errors))
+                        continue;
 
-                TryToCallGlobalCleanup(benchmarkTypeInstance, errors);
+                    TryToCallGlobalCleanup(executor.Instance, errors);
+                }
             }
 
             return errors;
@@ -73,15 +79,25 @@ private bool TryCreateBenchmarkTypeInstance(Type type, List<ValidationError> err
 
         private bool TryToCallGlobalSetup(object benchmarkTypeInstance, List<ValidationError> errors)
         {
-            return TryToCallGlobalMethod<GlobalSetupAttribute>(benchmarkTypeInstance, errors);
+            return TryToCallGlobalMethod<GlobalSetupAttribute>(benchmarkTypeInstance, errors, true);
         }
 
         private void TryToCallGlobalCleanup(object benchmarkTypeInstance, List<ValidationError> errors)
         {
-            TryToCallGlobalMethod<GlobalCleanupAttribute>(benchmarkTypeInstance, errors);
+            TryToCallGlobalMethod<GlobalCleanupAttribute>(benchmarkTypeInstance, errors, true);
+        }
+
+        private bool TryToCallIterationSetup(object benchmarkTypeInstance, List<ValidationError> errors)
+        {
+            return TryToCallGlobalMethod<IterationSetupAttribute>(benchmarkTypeInstance, errors, false);
+        }
+
+        private bool TryToCallIterationCleanup(object benchmarkTypeInstance, List<ValidationError> errors)
+        {
+            return TryToCallGlobalMethod<IterationCleanupAttribute>(benchmarkTypeInstance, errors, false);
         }
 
-        private bool TryToCallGlobalMethod<T>(object benchmarkTypeInstance, List<ValidationError> errors)
+        private bool TryToCallGlobalMethod<T>(object benchmarkTypeInstance, List<ValidationError> errors, bool canBeAsync) where T : Attribute
         {
             var methods = benchmarkTypeInstance
                 .GetType()
@@ -107,7 +123,16 @@ private bool TryToCallGlobalMethod<T>(object benchmarkTypeInstance, List<Validat
             {
                 var result = methods.First().Invoke(benchmarkTypeInstance, null);
 
-                TryToGetTaskResult(result);
+                var isAsyncMethod = TryAwaitTask(result, out _);
+
+                if (!canBeAsync && isAsyncMethod)
+                {
+                    errors.Add(new ValidationError(
+                        TreatsWarningsAsErrors,
+                        $"[{GetAttributeName(typeof(T))}] cannot be async. Error in type {benchmarkTypeInstance.GetType().Name}"));
+
+                    return false;
+                }
             }
             catch (Exception ex)
             {
@@ -121,135 +146,115 @@ private bool TryToCallGlobalMethod<T>(object benchmarkTypeInstance, List<Validat
             return true;
         }
 
-        private static string GetAttributeName(Type type) => type.Name.Replace("Attribute", string.Empty);
-
-        private static void TryToGetTaskResult(object result)
+        private static bool TryAwaitTask(object task, out object result)
         {
-            if (result == null)
+            result = null;
+
+            if (task is null)
             {
-                return;
+                return false;
             }
 
-            var returnType = result.GetType();
+            var returnType = task.GetType();
             if (returnType.IsGenericType && returnType.GetGenericTypeDefinition() == typeof(ValueTask<>))
             {
-                var asTaskMethod = result.GetType().GetMethod("AsTask");
-                result = asTaskMethod.Invoke(result, null);
+                var asTaskMethod = task.GetType().GetMethod("AsTask");
+                task = asTaskMethod.Invoke(task, null);
             }
 
-            if (result is Task task)
-            {
-                task.GetAwaiter().GetResult();
-            }
-            else if (result is ValueTask valueTask)
+            if (task is ValueTask valueTask)
+                task = valueTask.AsTask();
+
+            if (task is Task t)
             {
-                valueTask.GetAwaiter().GetResult();
+                if (TryGetTaskResult(t, out var taskResult))
+                    result = taskResult;
+
+                return true;
             }
+
+            return false;
         }
 
-        private bool TryToSetParamsFields(object benchmarkTypeInstance, List<ValidationError> errors)
+        // https://stackoverflow.com/a/52500763
+        private static bool TryGetTaskResult(Task task, out object result)
         {
-            var paramFields = benchmarkTypeInstance
-                .GetType()
-                .GetAllFields()
-                .Where(fieldInfo => fieldInfo.GetCustomAttributes(false).OfType<ParamsAttribute>().Any())
-                .ToArray();
+            task.GetAwaiter().GetResult();
 
-            if (!paramFields.Any())
+            result = null;
+
+            var voidTaskType = typeof(Task<>).MakeGenericType(Type.GetType("System.Threading.Tasks.VoidTaskResult"));
+            if (voidTaskType.IsInstanceOfType(task))
             {
-                return true;
+                return false;
             }
 
-            foreach (var paramField in paramFields)
+            var property = task.GetType().GetProperty("Result", BindingFlags.Public | BindingFlags.Instance);
+            if (property is null)
             {
-                if (!paramField.IsPublic)
-                {
-                    errors.Add(new ValidationError(
-                        TreatsWarningsAsErrors,
-                        $"Fields marked with [Params] must be public, {paramField.Name} of {benchmarkTypeInstance.GetType().Name} is not"));
-
-                    return false;
-                }
-
-                var values = paramField.GetCustomAttributes(false).OfType<ParamsAttribute>().Single().Values;
-                if (!values.Any())
-                {
-                    errors.Add(new ValidationError(
-                        TreatsWarningsAsErrors,
-                        $"Fields marked with [Params] must have some values defined, {paramField.Name} of {benchmarkTypeInstance.GetType().Name} has none"));
-
-                    return false;
-                }
-
-                try
-                {
-                    paramField.SetValue(benchmarkTypeInstance, values.First());
-                }
-                catch (Exception ex)
-                {
-                    errors.Add(new ValidationError(
-                        TreatsWarningsAsErrors,
-                        $"Failed to set {paramField.Name} of {benchmarkTypeInstance.GetType().Name} to {values.First()}, exception was: {GetDisplayExceptionMessage(ex)}"));
-
-                    return false;
-                }
+                return false;
             }
 
+            result = property.GetValue(task);
             return true;
         }
 
-        private bool TryToSetParamsProperties(object benchmarkTypeInstance, List<ValidationError> errors)
+        private bool TryToFillParameters(BenchmarkCase benchmark, object benchmarkTypeInstance, List<ValidationError> errors)
         {
-            var paramProperties = benchmarkTypeInstance
-                .GetType()
-                .GetAllProperties()
-                .Where(propertyInfo => propertyInfo.GetCustomAttributes(false).OfType<ParamsAttribute>().Any())
-                .ToArray();
+            if (ValidateMembers<ParamsAttribute>(benchmarkTypeInstance, errors))
+                return false;
 
-            if (!paramProperties.Any())
-            {
-                return true;
-            }
+            if (ValidateMembers<ParamsSourceAttribute>(benchmarkTypeInstance, errors))
+                return false;
+
+            bool hasError = false;
 
-            foreach (var paramProperty in paramProperties)
+            foreach (var parameter in benchmark.Parameters.Items)
             {
-                var setter = paramProperty.SetMethod;
-                if (setter == null || !setter.IsPublic)
+                // InProcessNoEmitToolchain does not support arguments
+                if (!parameter.IsArgument)
                 {
-                    errors.Add(new ValidationError(
-                        TreatsWarningsAsErrors,
-                        $"Properties marked with [Params] must have public setter, {paramProperty.Name} of {benchmarkTypeInstance.GetType().Name} has not"));
-
-                    return false;
+                    try
+                    {
+                        InProcessNoEmitRunner.FillMember(benchmarkTypeInstance, benchmark, parameter);
+                    }
+                    catch (Exception ex)
+                    {
+                        hasError = true;
+                        errors.Add(new ValidationError(
+                            TreatsWarningsAsErrors,
+                            ex.Message,
+                            benchmark));
+                    }
                 }
+            }
 
-                var values = paramProperty.GetCustomAttributes(false).OfType<ParamsAttribute>().Single().Values;
-                if (!values.Any())
-                {
-                    errors.Add(new ValidationError(
-                        TreatsWarningsAsErrors,
-                        $"Properties marked with [Params] must have some values defined, {paramProperty.Name} of {benchmarkTypeInstance.GetType().Name} has not"));
+            return !hasError;
+        }
 
-                    return false;
-                }
+        private bool ValidateMembers<T>(object benchmarkTypeInstance, List<ValidationError> errors) where T : Attribute
+        {
+            const BindingFlags reflectionFlags = BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic;
 
-                try
-                {
-                    setter.Invoke(benchmarkTypeInstance, new[] { values.First() });
-                }
-                catch (Exception ex)
+            bool hasError = false;
+
+            foreach (var member in benchmarkTypeInstance.GetType().GetTypeMembersWithGivenAttribute<T>(reflectionFlags))
+            {
+                if (!member.IsPublic)
                 {
                     errors.Add(new ValidationError(
                         TreatsWarningsAsErrors,
-                        $"Failed to set {paramProperty.Name} of {benchmarkTypeInstance.GetType().Name} to {values.First()}, exception was: {GetDisplayExceptionMessage(ex)}"));
+                        $"Member \"{member.Name}\" must be public if it has the [{GetAttributeName(typeof(T))}] attribute applied to it, {member.Name} of {benchmarkTypeInstance.GetType().Name} has not"));
 
-                    return false;
+                    hasError = true;
                 }
             }
 
-            return true;
+            return hasError;
         }
 
+        private static string GetAttributeName(Type type) => type.Name.Replace("Attribute", string.Empty);
+
         protected static string GetDisplayExceptionMessage(Exception ex)
         {
             if (ex is TargetInvocationException targetInvocationException)
@@ -258,6 +263,33 @@ protected static string GetDisplayExceptionMessage(Exception ex)
             return ex?.Message ?? "Unknown error";
         }
 
-        protected abstract void ExecuteBenchmarks(object benchmarkTypeInstance, IEnumerable<BenchmarkCase> benchmarks, List<ValidationError> errors);
+        protected abstract void ExecuteBenchmarks(IEnumerable<BenchmarkExecutor> executors, List<ValidationError> errors);
+
+        protected class BenchmarkExecutor
+        {
+            public object Instance { get; }
+            public BenchmarkCase BenchmarkCase { get; }
+
+            public BenchmarkExecutor(object instance, BenchmarkCase benchmarkCase)
+            {
+                Instance = instance;
+                BenchmarkCase = benchmarkCase;
+            }
+
+            public object Invoke()
+            {
+                var arguments = BenchmarkCase.Parameters.Items
+                    .Where(parameter => parameter.IsArgument)
+                    .Select(argument => argument.Value)
+                    .ToArray();
+
+                var result = BenchmarkCase.Descriptor.WorkloadMethod.Invoke(Instance, arguments);
+
+                if (TryAwaitTask(result, out var taskResult))
+                    result = taskResult;
+
+                return result;
+            }
+        }
     }
 }