Ref readonly support (#1389)

* add tests

* bump the language version to be able to compile in parameters

* emit the "in" modifier when needed

* add more test cases to make sure we differentiate readonly ref return and readonly struct return and their combination

* implement ref readonly returning benchmarks support

* fix a bug with nested byref type names

* set LangVersion when needed (and hopefully fix Travis failure)

Author: adamsitnik

src/BenchmarkDotNet/Code/CodeGenerator.cs

@@ -47,6 +47,7 @@ internal static string Generate(BuildPartition buildPartition)
                     .Replace("$WorkloadTypeName$", provider.WorkloadTypeName)
                     .Replace("$WorkloadMethodDelegate$", provider.WorkloadMethodDelegate(passArguments))
                     .Replace("$WorkloadMethodReturnType$", provider.WorkloadMethodReturnTypeName)
+                    .Replace("$WorkloadMethodReturnTypeModifiers$", provider.WorkloadMethodReturnTypeModifiers)
                     .Replace("$OverheadMethodReturnTypeName$", provider.OverheadMethodReturnTypeName)
                     .Replace("$GlobalSetupMethodName$", provider.GlobalSetupMethodName)
                     .Replace("$GlobalCleanupMethodName$", provider.GlobalCleanupMethodName)
@@ -61,7 +62,6 @@ internal static string Generate(BuildPartition buildPartition)
                     .Replace("$InitializeArgumentFields$", GetInitializeArgumentFields(benchmark)).Replace("$LoadArguments$", GetLoadArguments(benchmark))
                     .Replace("$PassArguments$", passArguments)
                     .Replace("$EngineFactoryType$", GetEngineFactoryTypeName(benchmark))
-                    .Replace("$Ref$", provider.UseRefKeyword ? "ref" : null)
                     .Replace("$MeasureExtraStats$", buildInfo.Config.HasExtraStatsDiagnoser() ? "true" : "false")
                     .Replace("$DisassemblerEntryMethodName$", DisassemblerConstants.DisassemblerEntryMethodName)
                     .Replace("$WorkloadMethodCall$", provider.GetWorkloadMethodCall(passArguments)).ToString();
@@ -174,7 +174,11 @@ private static DeclarationsProvider GetDeclarationsProvider(Descriptor descripto
 
             if (method.ReturnType.IsByRef)
             {
-                return new ByRefDeclarationsProvider(descriptor);
+                // System.Runtime.CompilerServices.IsReadOnlyAttribute is part of .NET Standard 2.1, we can't use it here..
+                if (method.ReturnParameter.GetCustomAttributes().Any(attribute => attribute.GetType().Name == "IsReadOnlyAttribute"))
+                    return new ByReadOnlyRefDeclarationsProvider(descriptor);
+                else
+                    return new ByRefDeclarationsProvider(descriptor);
             }
 
             return new NonVoidDeclarationsProvider(descriptor);
@@ -234,11 +238,20 @@ private static string GetEngineFactoryTypeName(BenchmarkCase benchmarkCase)
         }
 
         private static string GetParameterModifier(ParameterInfo parameterInfo)
-            => parameterInfo.ParameterType.IsByRef
-                ? "ref"
-                : parameterInfo.IsOut
-                    ? "out"
-                    : string.Empty;
+        {
+            if (!parameterInfo.ParameterType.IsByRef)
+                return string.Empty;
+
+            // From https://stackoverflow.com/a/38110036/5852046 :
+            // "If you don't do the IsByRef check for out parameters, then you'll incorrectly get members decorated with the
+            // [Out] attribute from System.Runtime.InteropServices but which aren't actually C# out parameters."
+            if (parameterInfo.IsOut)
+                return "out";
+            else if (parameterInfo.IsIn)
+                return "in";
+            else
+                return "ref";
+        }
 
         /// <summary>
         /// for CoreRT we can't use reflection to load type and run a method, so we simply generate a switch for all types..

src/BenchmarkDotNet/Code/DeclarationsProvider.cs

@@ -38,6 +38,8 @@ internal abstract class DeclarationsProvider
 
         public virtual string WorkloadMethodDelegate(string passArguments) => Descriptor.WorkloadMethod.Name;
 
+        public virtual string WorkloadMethodReturnTypeModifiers => null;
+
         public virtual string GetWorkloadMethodCall(string passArguments) => $"{Descriptor.WorkloadMethod.Name}({passArguments})";
 
         public virtual string ConsumeField => null;
@@ -48,8 +50,6 @@ internal abstract class DeclarationsProvider
 
         public abstract string OverheadImplementation { get; }
 
-        public virtual bool UseRefKeyword => false;
-
         private string GetMethodName(MethodInfo method)
         {
             if (method == null)
@@ -133,7 +133,16 @@ public ByRefDeclarationsProvider(Descriptor descriptor) : base(descriptor) { }
 
         public override string ExtraDefines => "#define RETURNS_BYREF";
 
-        public override bool UseRefKeyword => true;
+        public override string WorkloadMethodReturnTypeModifiers => "ref";
+    }
+
+    internal class ByReadOnlyRefDeclarationsProvider : ByRefDeclarationsProvider
+    {
+        public ByReadOnlyRefDeclarationsProvider(Descriptor descriptor) : base(descriptor) { }
+
+        public override string ExtraDefines => "#define RETURNS_BYREF_READONLY";
+
+        public override string WorkloadMethodReturnTypeModifiers => "ref readonly";
     }
 
     internal class TaskDeclarationsProvider : VoidDeclarationsProvider

src/BenchmarkDotNet/Engines/DeadCodeEliminationHelper.cs

@@ -20,5 +20,14 @@ public static void KeepAliveWithoutBoxing<T>(T value) { }
         [MethodImpl(MethodImplOptions.NoInlining)]
         [UsedImplicitly] // Used in generated benchmarks
         public static void KeepAliveWithoutBoxing<T>(ref T value) { }
+
+        /// <summary>
+        /// This method can't get inlined, so any value send to it
+        /// will not get eliminated by the dead code elimination
+        /// it's not called KeepAliveWithoutBoxing because compiler would not be able to diff `ref` and `in`
+        /// </summary>
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        [UsedImplicitly] // Used in generated benchmarks
+        public static void KeepAliveWithoutBoxingReadonly<T>(in T value) { }
     }
 }

src/BenchmarkDotNet/Extensions/ReflectionExtensions.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using System.Reflection;
 using BenchmarkDotNet.Attributes;
@@ -36,12 +37,11 @@ internal static bool HasAttribute<T>(this MethodInfo methodInfo) where T : Attri
         /// </summary>
         internal static string GetCorrectCSharpTypeName(this Type type, bool includeNamespace = true, bool includeGenericArgumentsNamespace = true)
         {
-            if (!type.Name.EndsWith("&"))
-                while (!(type.IsPublic || type.IsNestedPublic) && type.BaseType != null)
-                    type = type.BaseType;
+            while (!(type.IsPublic || type.IsNestedPublic) && type.BaseType != null)
+                type = type.BaseType;
 
             // the reflection is missing information about types passed by ref (ie ref ValueTuple<int> is reported as NON generic type)
-            if (type.IsByRef && !type.IsGenericType && type.Name.Contains('`'))
+            if (type.IsByRef && !type.IsGenericType)
                 type = type.GetElementType() ?? throw new NullReferenceException(nameof(type.GetElementType)); // https://github.com/dotnet/corefx/issues/29975#issuecomment-393134330
 
             if (type == typeof(void))

src/BenchmarkDotNet/Templates/BenchmarkType.txt

@@ -53,7 +53,7 @@
 
         public delegate $OverheadMethodReturnTypeName$ OverheadDelegate($ArgumentsDefinition$);
 
-        public delegate $Ref$ $WorkloadMethodReturnType$ WorkloadDelegate($ArgumentsDefinition$);
+        public delegate $WorkloadMethodReturnTypeModifiers$ $WorkloadMethodReturnType$ WorkloadDelegate($ArgumentsDefinition$);
 
         public Runnable_$ID$()
         {
@@ -281,6 +281,65 @@
                 return ref $WorkloadMethodCall$;
             }
 
+            return ref workloadDefaultValueHolder;
+        }
+#elif RETURNS_BYREF_READONLY_$ID$
+
+        private void OverheadActionUnroll(System.Int64 invokeCount)
+        {
+            $LoadArguments$
+            $OverheadMethodReturnTypeName$ value = default($OverheadMethodReturnTypeName$);
+            for (System.Int64 i = 0; i < invokeCount; i++)
+            {
+                value = overheadDelegate($PassArguments$);@Unroll@
+            }
+            BenchmarkDotNet.Engines.DeadCodeEliminationHelper.KeepAliveWithoutBoxing(value);
+        }
+
+        private void OverheadActionNoUnroll(System.Int64 invokeCount)
+        {
+            $LoadArguments$
+            $OverheadMethodReturnTypeName$ value = default($OverheadMethodReturnTypeName$);
+            for (System.Int64 i = 0; i < invokeCount; i++)
+            {
+                value = overheadDelegate($PassArguments$);
+            }
+            BenchmarkDotNet.Engines.DeadCodeEliminationHelper.KeepAliveWithoutBoxing(value);
+        }
+
+        private $WorkloadMethodReturnType$ workloadDefaultValueHolder = default($WorkloadMethodReturnType$);
+
+        private void WorkloadActionUnroll(System.Int64 invokeCount)
+        {
+            $LoadArguments$
+            ref $WorkloadMethodReturnType$ alias = ref workloadDefaultValueHolder;
+            for (System.Int64 i = 0; i < invokeCount; i++)
+            {
+                alias = workloadDelegate($PassArguments$);@Unroll@
+            }
+            BenchmarkDotNet.Engines.DeadCodeEliminationHelper.KeepAliveWithoutBoxingReadonly(alias);
+        }
+        
+        private void WorkloadActionNoUnroll(System.Int64 invokeCount)
+        {
+            $LoadArguments$
+            ref $WorkloadMethodReturnType$ alias = ref workloadDefaultValueHolder;
+            for (System.Int64 i = 0; i < invokeCount; i++)
+            {
+                alias = workloadDelegate($PassArguments$);
+            }
+            BenchmarkDotNet.Engines.DeadCodeEliminationHelper.KeepAliveWithoutBoxingReadonly(alias);
+        }
+
+        [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoOptimization | System.Runtime.CompilerServices.MethodImplOptions.NoInlining)]
+        public ref readonly $WorkloadMethodReturnType$ $DisassemblerEntryMethodName$()
+        {
+            if (NotEleven == 11)
+            {
+                $LoadArguments$
+                return ref $WorkloadMethodCall$;
+            }
+            
             return ref workloadDefaultValueHolder;
         }
 #elif RETURNS_VOID_$ID$

src/BenchmarkDotNet/Templates/CsProj.txt

@@ -16,6 +16,8 @@
     <UseSharedCompilation>false</UseSharedCompilation>
     <CodeAnalysisRuleSet></CodeAnalysisRuleSet>
     $COPIEDSETTINGS$
+    <!-- we set LangVersion after $COPIEDSETTINGS$ which might contain LangVersion copied from the benchmarks project -->
+    <LangVersion Condition="'$(LangVersion)' == '' Or '$(LangVersion)' &lt; '7.3'">latest</LangVersion>
   </PropertyGroup>
 
   <ItemGroup>

src/BenchmarkDotNet/Toolchains/Roslyn/Builder.cs

@@ -29,7 +29,10 @@ public BuildResult Build(GenerateResult generateResult, BuildPartition buildPart
         {
             logger.WriteLineInfo($"BuildScript: {generateResult.ArtifactsPaths.BuildScriptFilePath}");
 
-            var syntaxTree = CSharpSyntaxTree.ParseText(File.ReadAllText(generateResult.ArtifactsPaths.ProgramCodePath));
+            var syntaxTree = CSharpSyntaxTree.ParseText(
+                text: File.ReadAllText(generateResult.ArtifactsPaths.ProgramCodePath),
+                // this version is used to parse the boilerplate code generated by BDN, so th benchmark themselves can use more recent version
+                options: new CSharpParseOptions(LanguageVersion.CSharp7_3));
 
             var compilationOptions = new CSharpCompilationOptions(
                 outputKind: OutputKind.ConsoleApplication,

tests/BenchmarkDotNet.IntegrationTests/ArgumentsTests.cs

@@ -96,6 +96,21 @@ public void Simple(ref bool boolean, ref int number)
             }
         }
 
+        [Theory, MemberData(nameof(GetToolchains))]
+        public void ArgumentsCanBePassedByReadonlyReferenceToBenchmark(IToolchain toolchain) => CanExecute<WithInArguments>(toolchain);
+
+        public class WithInArguments
+        {
+            [Benchmark]
+            [Arguments(true, 1)]
+            [Arguments(false, 2)]
+            public void Simple(in bool boolean, in int number)
+            {
+                if (boolean && number != 1 || !boolean && number != 2)
+                    throw new InvalidOperationException("Incorrect values were passed");
+            }
+        }
+
         [Theory, MemberData(nameof(GetToolchains))]
         public void NonCompileTimeConstantsCanBeReturnedFromSource(IToolchain toolchain) => CanExecute<WithComplexTypesReturnedFromSources>(toolchain);
 

tests/BenchmarkDotNet.IntegrationTests/ValuesReturnedByBenchmarkTest.cs

@@ -48,9 +48,24 @@ public class ValuesReturnedByBenchmark
             [Benchmark]
             public Jit ReturnEnum() => Jit.RyuJit;
 
-            private int field = 123;
+            private int intergerField = 123;
             [Benchmark]
-            public ref int ReturnByRef() => ref field;
+            public ref int ReturnByRef() => ref intergerField;
+
+            [Benchmark]
+            public ref readonly int ReturnByReadonlyRef() => ref intergerField;
+
+            public readonly struct ReadOnlyStruct { }
+            private ReadOnlyStruct readOnlyStructField;
+
+            [Benchmark]
+            public ReadOnlyStruct ReturnReadOnlyStruct() => new ReadOnlyStruct();
+
+            [Benchmark]
+            public ref ReadOnlyStruct ReturnReadOnlyStructByRef() => ref readOnlyStructField;
+
+            [Benchmark]
+            public ref readonly ReadOnlyStruct ReturnReadOnlyStructByReadonlyRef() => ref readOnlyStructField;
 
             [Benchmark]
             public Span<byte> ReturnStackOnlyType() => new Span<byte>(Array.Empty<byte>());

tests/BenchmarkDotNet.Tests/ReflectionTests.cs

@@ -43,6 +43,27 @@ public class GenericByRef
             public void TheMethod(ref ValueTuple<int, short> _) { }
         }
 
+        [Fact]
+        public void GetCorrectCSharpTypeNameSupportsNestedTypes()
+        {
+            var nestedType = typeof(Nested);
+
+            CheckCorrectTypeName("BenchmarkDotNet.Tests.ReflectionTests.Nested", nestedType);
+        }
+
+        [Fact]
+        public void GetCorrectCSharpTypeNameSupportsNestedTypesPassedByReference()
+        {
+            var byRefNestedType = typeof(Nested).GetMethod(nameof(Nested.TheMethod)).GetParameters().Single().ParameterType;
+
+            CheckCorrectTypeName("BenchmarkDotNet.Tests.ReflectionTests.Nested", byRefNestedType);
+        }
+
+        public class Nested
+        {
+            public void TheMethod(ref Nested _) { }
+        }
+
         [AssertionMethod]
         private static void CheckCorrectTypeName(string expectedName, Type type)
         {