Addressing code review suggestions (#1975)

* remove redundant ctor

* use rootAllApplicationAssemblies  to determine whether TrimmerDefaultAction should be set to link or not

* set TrimMode to link in explicit way as for older versions it might have different default value

* remove NativeAot50, nobody should be using it

Author: adamsitnik

src/BenchmarkDotNet.Annotations/Attributes/GenericTypeArgumentsAttribute.cs

@@ -1,5 +1,4 @@
-using JetBrains.Annotations;
-using System;
+using System;
 using System.Diagnostics.CodeAnalysis;
 
 namespace BenchmarkDotNet.Attributes
@@ -9,9 +8,6 @@ public class GenericTypeArgumentsAttribute : Attribute
     {
         public Type[] GenericTypeArguments { get; }
 
-        // CLS-Compliant Code requires a constructor without an array in the argument list
-        [PublicAPI] public GenericTypeArgumentsAttribute() => GenericTypeArguments = new Type[0];
-
         public GenericTypeArgumentsAttribute([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type)
             => GenericTypeArguments = new Type[] { type };
 

src/BenchmarkDotNet.Annotations/Jobs/RuntimeMoniker.cs

@@ -95,11 +95,6 @@ public enum RuntimeMoniker
         /// </summary>
         Net70,
 
-        /// <summary>
-        /// NativeAOT compiled as net5.0
-        /// </summary>
-        NativeAot50,
-
         /// <summary>
         /// NativeAOT compiled as net6.0
         /// </summary>

src/BenchmarkDotNet/ConsoleArguments/ConfigParser.cs

@@ -363,7 +363,6 @@ private static Job CreateJobForGivenRuntime(Job baseJob, string runtimeId, Comma
                         .WithToolchain(CsProjCoreToolchain.From(new NetCoreAppSettings(runtimeId, null, runtimeId, options.CliPath?.FullName, options.RestorePath?.FullName)));
                 case RuntimeMoniker.Mono:
                     return baseJob.WithRuntime(new MonoRuntime("Mono", options.MonoPath?.FullName));
-                case RuntimeMoniker.NativeAot50:
                 case RuntimeMoniker.NativeAot60:
                     return CreateAotJob(baseJob, options, runtimeMoniker, "6.0.0-*", "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental/nuget/v3/index.json");
                 case RuntimeMoniker.NativeAot70:

src/BenchmarkDotNet/Environments/Runtimes/NativeAotRuntime.cs

@@ -6,10 +6,6 @@ namespace BenchmarkDotNet.Environments
 {
     public class NativeAotRuntime : Runtime
     {
-        /// <summary>
-        /// NativeAOT compiled as net5.0
-        /// </summary>
-        public static readonly NativeAotRuntime Net50 = new NativeAotRuntime(RuntimeMoniker.NativeAot50, "net5.0", "NativeAOT 5.0");
         /// <summary>
         /// NativeAOT compiled as net6.0
         /// </summary>
@@ -40,7 +36,6 @@ public static NativeAotRuntime GetCurrentVersion()
 
             switch (version)
             {
-                case Version v when v.Major == 5 && v.Minor == 0: return Net50;
                 case Version v when v.Major == 6 && v.Minor == 0: return Net60;
                 case Version v when v.Major == 7 && v.Minor == 0: return Net70;
                 default:

src/BenchmarkDotNet/Extensions/RuntimeMonikerExtensions.cs

@@ -43,8 +43,6 @@ internal static Runtime GetRuntime(this RuntimeMoniker runtimeMoniker)
                     return CoreRuntime.Core70;
                 case RuntimeMoniker.Mono:
                     return MonoRuntime.Default;
-                case RuntimeMoniker.NativeAot50:
-                    return NativeAotRuntime.Net50;
                 case RuntimeMoniker.NativeAot60:
                     return NativeAotRuntime.Net60;
                 case RuntimeMoniker.NativeAot70:

src/BenchmarkDotNet/Toolchains/NativeAot/Generator.cs

@@ -26,7 +26,7 @@ internal Generator(string ilCompilerVersion,
             string runtimeFrameworkVersion, string targetFrameworkMoniker, string cliPath,
             string runtimeIdentifier, IReadOnlyDictionary<string, string> feeds, bool useNuGetClearTag,
             bool useTempFolderForRestore, string packagesRestorePath,
-            bool rootAllApplicationAssemblies, bool ilcGenerateCompleteTypeMetadata, bool ilcGenerateStackTraceData, string trimmerDefaultAction)
+            bool rootAllApplicationAssemblies, bool ilcGenerateCompleteTypeMetadata, bool ilcGenerateStackTraceData)
             : base(targetFrameworkMoniker, cliPath, GetPackagesDirectoryPath(useTempFolderForRestore, packagesRestorePath), runtimeFrameworkVersion)
         {
             this.ilCompilerVersion = ilCompilerVersion;
@@ -38,7 +38,6 @@ internal Generator(string ilCompilerVersion,
             this.rootAllApplicationAssemblies = rootAllApplicationAssemblies;
             this.ilcGenerateCompleteTypeMetadata = ilcGenerateCompleteTypeMetadata;
             this.ilcGenerateStackTraceData = ilcGenerateStackTraceData;
-            this.trimmerDefaultAction = trimmerDefaultAction;
         }
 
         private readonly string ilCompilerVersion;
@@ -51,7 +50,6 @@ internal Generator(string ilCompilerVersion,
         private readonly bool rootAllApplicationAssemblies;
         private readonly bool ilcGenerateCompleteTypeMetadata;
         private readonly bool ilcGenerateStackTraceData;
-        private readonly string trimmerDefaultAction;
 
         private bool IsNuGet => feeds.ContainsKey(NativeAotNuGetFeed) && !string.IsNullOrWhiteSpace(ilCompilerVersion);
 
@@ -189,24 +187,17 @@ private string GenerateProjectForLocalBuild(BuildPartition buildPartition, Artif
 </Project>";
 
         private string GetTrimmingSettings()
-        {
-            StringBuilder sb = new StringBuilder();
-
-            sb.AppendLine(rootAllApplicationAssemblies ? "<PublishTrimmed>false</PublishTrimmed>" : "<TrimMode>link</TrimMode>");
-
-            if (!string.IsNullOrEmpty(trimmerDefaultAction))
-            {
-                sb.Append($"<TrimmerDefaultAction>{trimmerDefaultAction}</TrimmerDefaultAction>");
-            }
-
-            return sb.ToString();
-        }
+            => rootAllApplicationAssemblies
+                ? "" // use the defaults
+                // TrimMode is set in explicit way as for older versions it might have different default value
+                : "<TrimMode>link</TrimMode><TrimmerDefaultAction>link</TrimmerDefaultAction>";
 
         public IEnumerable<string> GetRdXmlFiles(Type benchmarkTarget, ILogger logger)
         {
+            yield return GeneratedRdXmlFileName;
+
             var projectFile = GetProjectFilePath(benchmarkTarget, logger);
             var projectFileFolder = projectFile.DirectoryName;
-            yield return GeneratedRdXmlFileName;
             var rdXml = Path.Combine(projectFileFolder, "rd.xml");
             if (File.Exists(rdXml))
             {

src/BenchmarkDotNet/Toolchains/NativeAot/NativeAotToolchain.cs

@@ -7,32 +7,32 @@ namespace BenchmarkDotNet.Toolchains.NativeAot
 {
     public class NativeAotToolchain : Toolchain
     {
-        /// <summary>
-        /// compiled as net5.0, targets experimental 6.0.0-* NativeAOT build from the https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental/nuget/v3/index.json
-        /// </summary>
-        public static readonly IToolchain Net50 = GetBuilderForOldExperimentalFeed().TargetFrameworkMoniker("net5.0").ToToolchain();
         /// <summary>
         /// compiled as net6.0, targets experimental 6.0.0-* NativeAOT build from the https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental/nuget/v3/index.json
         /// </summary>
-        public static readonly IToolchain Net60 = GetBuilderForOldExperimentalFeed().TargetFrameworkMoniker("net6.0").ToToolchain();
+        public static readonly IToolchain Net60 = CreateBuilder()
+            .UseNuGet("6.0.0-*", "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental/nuget/v3/index.json")
+            .TargetFrameworkMoniker("net6.0")
+            .ToToolchain();
+
         /// <summary>
         /// compiled as net7.0, targets latest (7.0.0-*) NativeAOT build from the .NET 7 feed: https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet7/nuget/v3/index.json
         /// </summary>
-        public static readonly IToolchain Net70 = CreateBuilder().UseNuGet().TargetFrameworkMoniker("net7.0").ToToolchain();
-
-        private static NativeAotToolchainBuilder GetBuilderForOldExperimentalFeed()
-            => CreateBuilder().UseNuGet("6.0.0-*", "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental/nuget/v3/index.json");
+        public static readonly IToolchain Net70 = CreateBuilder()
+            .UseNuGet("7.0.0-*", "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet7/nuget/v3/index.json")
+            .TargetFrameworkMoniker("net7.0")
+            .ToToolchain();
 
         internal NativeAotToolchain(string displayName,
             string ilCompilerVersion, string ilcPath,
             string runtimeFrameworkVersion, string targetFrameworkMoniker, string runtimeIdentifier,
             string customDotNetCliPath, string packagesRestorePath,
             Dictionary<string, string> feeds, bool useNuGetClearTag, bool useTempFolderForRestore,
-            bool rootAllApplicationAssemblies, bool ilcGenerateCompleteTypeMetadata, bool ilcGenerateStackTraceData, string trimmerDefaultAction)
+            bool rootAllApplicationAssemblies, bool ilcGenerateCompleteTypeMetadata, bool ilcGenerateStackTraceData)
             : base(displayName,
                 new Generator(ilCompilerVersion, runtimeFrameworkVersion, targetFrameworkMoniker, customDotNetCliPath,
                     runtimeIdentifier, feeds, useNuGetClearTag, useTempFolderForRestore, packagesRestorePath,
-                    rootAllApplicationAssemblies, ilcGenerateCompleteTypeMetadata, ilcGenerateStackTraceData, trimmerDefaultAction),
+                    rootAllApplicationAssemblies, ilcGenerateCompleteTypeMetadata, ilcGenerateStackTraceData),
                 new DotNetCliPublisher(customDotNetCliPath, GetExtraArguments(runtimeIdentifier), GetEnvironmentVariables(ilcPath)),
                 new Executor())
         {
@@ -43,8 +43,7 @@ internal NativeAotToolchain(string displayName,
 
         public static NativeAotToolchainBuilder CreateBuilder() => NativeAotToolchainBuilder.Create();
 
-        public static string GetExtraArguments(string runtimeIdentifier)
-            => $"-r {runtimeIdentifier}";
+        public static string GetExtraArguments(string runtimeIdentifier) => $"-r {runtimeIdentifier}";
 
         // https://github.com/dotnet/corert/blob/7f902d4d8b1c3280e60f5e06c71951a60da173fb/Documentation/how-to-build-and-run-ilcompiler-in-console-shell-prompt.md#compiling-source-to-native-code-using-the-ilcompiler-you-built
         // we have to pass IlcPath env var to get it working

src/BenchmarkDotNet/Toolchains/NativeAot/NativeAotToolchainBuilder.cs

@@ -19,7 +19,6 @@ public class NativeAotToolchainBuilder : CustomDotNetCliToolchainBuilder
         private bool ilcGenerateStackTraceData = true;
 
         private bool isIlCompilerConfigured;
-        private string trimmerDefaultAction = "link";
 
         /// <summary>
         /// creates a NativeAOT toolchain targeting NuGet build of Microsoft.DotNet.ILCompiler
@@ -111,19 +110,6 @@ public NativeAotToolchainBuilder IlcGenerateStackTraceData(bool value)
             return this;
         }
 
-        /// <summary>
-        /// By using the default value ("link") this ensures that the trimmer only analyzes the parts of the library's dependencies that are used.
-        /// It tells the trimmer that any code that is not part of a "root" can be trimmed if it is unused.
-        /// </summary>
-        /// <remarks>Pass null or empty string to NOT set TrimmerDefaultAction to any value.</remarks>
-        [PublicAPI]
-        public NativeAotToolchainBuilder SeTrimmerDefaultAction(string value = "link")
-        {
-            trimmerDefaultAction = value;
-
-            return this;
-        }
-
         [PublicAPI]
         public override IToolchain ToToolchain()
         {
@@ -144,8 +130,7 @@ public override IToolchain ToToolchain()
                 useTempFolderForRestore: useTempFolderForRestore,
                 rootAllApplicationAssemblies: rootAllApplicationAssemblies,
                 ilcGenerateCompleteTypeMetadata: ilcGenerateCompleteTypeMetadata,
-                ilcGenerateStackTraceData: ilcGenerateStackTraceData,
-                trimmerDefaultAction: trimmerDefaultAction
+                ilcGenerateStackTraceData: ilcGenerateStackTraceData
             );
         }
     }

src/BenchmarkDotNet/Toolchains/ToolchainExtensions.cs

@@ -108,8 +108,6 @@ private static IToolchain GetToolchain(RuntimeMoniker runtimeMoniker)
                     return CsProjCoreToolchain.NetCoreApp60;
                 case RuntimeMoniker.Net70:
                     return CsProjCoreToolchain.NetCoreApp70;
-                case RuntimeMoniker.NativeAot50:
-                    return NativeAotToolchain.Net50;
                 case RuntimeMoniker.NativeAot60:
                     return NativeAotToolchain.Net60;
                 case RuntimeMoniker.NativeAot70:

tests/BenchmarkDotNet.IntegrationTests/BuildTimeoutTests.cs

@@ -29,7 +29,7 @@ public void WhenBuildTakesMoreTimeThanTheTimeoutTheBuildIsCancelled()
             var config = ManualConfig.CreateEmpty()
                 .WithBuildTimeout(timeout)
                 .AddJob(Job.Dry
-                    .WithRuntime(NativeAotRuntime.Net50)
+                    .WithRuntime(NativeAotRuntime.Net60)
                     .WithToolchain(NativeAotToolchain.CreateBuilder()
                         .UseNuGet(
                             "6.0.0-rc.1.21420.1", // we test against specific version to keep this test stable