fixes(configuration): Not unique exporter for exporter type (#1796)

* fixes(configuration): Not unique exporter for exporter type

* fixes(configuration): Assembly Config Attribute Priority

Assembly-level configuration attributes must have a lower priority than class to allow overriding their behavior.

* feat(configuration): Add property ConfigAnalyse to IConfig

* feat(configuration):  Implements IConfig.ConfigAnalyse

* feat: Display ConfigAnalyse on Benchmark Summary

* feat(configuration):  Implement test

* feat: omit 'Config Issues' header if there are no any issues.

* fixes: Renamed ConfigAnalyse in ConfigAnalysisConclusion

Author: workgroupengineering

src/BenchmarkDotNet/Analysers/ConclusionHelper.cs

@@ -7,14 +7,14 @@ namespace BenchmarkDotNet.Analysers
 {
     public static class ConclusionHelper
     {
-        public static void Print(ILogger logger, List<Conclusion> conclusions)
+        public static void Print(ILogger logger, IEnumerable<Conclusion> conclusions)
         {
             PrintFiltered(conclusions, ConclusionKind.Error, "Errors", logger.WriteLineError);
             PrintFiltered(conclusions, ConclusionKind.Warning, "Warnings", logger.WriteLineError);
             PrintFiltered(conclusions, ConclusionKind.Hint, "Hints", logger.WriteLineHint);
         }
 
-        private static void PrintFiltered(List<Conclusion> conclusions, ConclusionKind kind, string title, Action<string> printLine)
+        private static void PrintFiltered(IEnumerable<Conclusion> conclusions, ConclusionKind kind, string title, Action<string> printLine)
         {
             var filtered = conclusions.Where(c => c.Kind == kind).ToArray();
             if (filtered.Any())

src/BenchmarkDotNet/Configs/DebugConfig.cs

@@ -52,6 +52,7 @@ public override IEnumerable<Job> GetJobs()
 
     public abstract class DebugConfig : IConfig
     {
+        private readonly static Conclusion[] emptyConclusion = Array.Empty<Conclusion>();
         public abstract IEnumerable<Job> GetJobs();
 
         public IEnumerable<IValidator> GetValidators() => Array.Empty<IValidator>();
@@ -83,5 +84,7 @@ public string ArtifactsPath
         public IEnumerable<BenchmarkLogicalGroupRule> GetLogicalGroupRules() => Array.Empty<BenchmarkLogicalGroupRule>();
 
         public ConfigOptions Options => ConfigOptions.KeepBenchmarkFiles | ConfigOptions.DisableOptimizationsValidator;
+
+        public IReadOnlyList<Conclusion> ConfigAnalysisConclusion => emptyConclusion;
     }
 }

src/BenchmarkDotNet/Configs/DefaultConfig.cs

@@ -20,6 +20,7 @@ namespace BenchmarkDotNet.Configs
     public class DefaultConfig : IConfig
     {
         public static readonly IConfig Instance = new DefaultConfig();
+        private readonly static Conclusion[] emptyConclusion = Array.Empty<Conclusion>();
 
         private DefaultConfig()
         {
@@ -91,6 +92,8 @@ public string ArtifactsPath
             }
         }
 
+        public IReadOnlyList<Conclusion> ConfigAnalysisConclusion => emptyConclusion;
+
         public IEnumerable<Job> GetJobs() => Array.Empty<Job>();
 
         public IEnumerable<BenchmarkLogicalGroupRule> GetLogicalGroupRules() => Array.Empty<BenchmarkLogicalGroupRule>();

src/BenchmarkDotNet/Configs/IConfig.cs

@@ -51,5 +51,10 @@ public interface IConfig
         /// the auto-generated project build timeout
         /// </summary>
         TimeSpan BuildTimeout { get; }
+        
+        /// <summary>
+        /// Collect any errors or warnings when composing the configuration
+        /// </summary>
+        IReadOnlyList<Conclusion> ConfigAnalysisConclusion { get; }
     }
-}
+}

src/BenchmarkDotNet/Configs/ImmutableConfig.cs

@@ -50,7 +50,8 @@ internal ImmutableConfig(
             IOrderer orderer,
             SummaryStyle summaryStyle,
             ConfigOptions options,
-            TimeSpan buildTimeout)
+            TimeSpan buildTimeout,
+            IReadOnlyList<Conclusion> configAnalysisConclusion)
         {
             columnProviders = uniqueColumnProviders;
             loggers = uniqueLoggers;
@@ -69,6 +70,7 @@ internal ImmutableConfig(
             SummaryStyle = summaryStyle;
             Options = options;
             BuildTimeout = buildTimeout;
+            ConfigAnalysisConclusion = configAnalysisConclusion;
         }
 
         public ConfigUnionRule UnionRule { get; }
@@ -108,5 +110,7 @@ public IDiagnoser GetCompositeDiagnoser(BenchmarkCase benchmarkCase, RunMode run
 
             return diagnosersForGivenMode.Any() ? new CompositeDiagnoser(diagnosersForGivenMode) : null;
         }
+
+        public IReadOnlyList<Conclusion> ConfigAnalysisConclusion { get; private set; }
     }
-}
+}

src/BenchmarkDotNet/Configs/ImmutableConfigBuilder.cs

@@ -36,10 +36,11 @@ public static ImmutableConfig Create(IConfig source)
         {
             var uniqueColumnProviders = source.GetColumnProviders().Distinct().ToImmutableArray();
             var uniqueLoggers = source.GetLoggers().ToImmutableHashSet();
+            var configAnalyse = new List<Conclusion>();
 
             var uniqueHardwareCounters = source.GetHardwareCounters().ToImmutableHashSet();
             var uniqueDiagnosers = GetDiagnosers(source.GetDiagnosers(), uniqueHardwareCounters);
-            var uniqueExporters = GetExporters(source.GetExporters(), uniqueDiagnosers);
+            var uniqueExporters = GetExporters(source.GetExporters(), uniqueDiagnosers, configAnalyse);
             var uniqueAnalyzers = GetAnalysers(source.GetAnalysers(), uniqueDiagnosers);
 
             var uniqueValidators = GetValidators(source.GetValidators(), MandatoryValidators, source.Options);
@@ -66,7 +67,8 @@ public static ImmutableConfig Create(IConfig source)
                 source.Orderer ?? DefaultOrderer.Instance,
                 source.SummaryStyle ?? SummaryStyle.Default,
                 source.Options,
-                source.BuildTimeout
+                source.BuildTimeout,
+                configAnalyse.AsReadOnly()
             );
         }
 
@@ -90,18 +92,43 @@ private static ImmutableHashSet<IDiagnoser> GetDiagnosers(IEnumerable<IDiagnoser
             return builder.ToImmutable();
         }
 
-        private static ImmutableArray<IExporter> GetExporters(IEnumerable<IExporter> exporters, ImmutableHashSet<IDiagnoser> uniqueDiagnosers)
+        private static ImmutableArray<IExporter> GetExporters(IEnumerable<IExporter> exporters,
+            ImmutableHashSet<IDiagnoser> uniqueDiagnosers,
+            IList<Conclusion> configAnalyse)
         {
-            var result = new List<IExporter>();
+
+            void AddWarning(string message)
+            {
+                var conclusion = Conclusion.CreateWarning("Configuration", message);
+                configAnalyse.Add(conclusion);
+            }
+
+            var mergeDictionary = new Dictionary<System.Type, IExporter>();
 
             foreach (var exporter in exporters)
-                if (!result.Contains(exporter))
-                    result.Add(exporter);
+            {
+                var exporterType = exporter.GetType();
+                if (mergeDictionary.ContainsKey(exporterType))
+                {
+                    AddWarning($"The exporter {exporterType} is already present in configuration. There may be unexpected results.");
+                }
+                mergeDictionary[exporterType] = exporter;
+            }
+
 
             foreach (var diagnoser in uniqueDiagnosers)
-            foreach (var exporter in diagnoser.Exporters)
-                if (!result.Contains(exporter))
-                    result.Add(exporter);
+                foreach (var exporter in diagnoser.Exporters)
+                {
+                    var exporterType = exporter.GetType();
+                    if (mergeDictionary.ContainsKey(exporterType))
+                    {
+                        AddWarning($"The exporter {exporterType} of {diagnoser.GetType().Name} is already present in configuration. There may be unexpected results.");
+                    }
+                    mergeDictionary[exporterType] = exporter;
+                };
+
+            var result = mergeDictionary.Values.ToList();
+
 
             var hardwareCounterDiagnoser = uniqueDiagnosers.OfType<IHardwareCountersDiagnoser>().SingleOrDefault();
             var disassemblyDiagnoser = uniqueDiagnosers.OfType<DisassemblyDiagnoser>().SingleOrDefault();
@@ -113,8 +140,31 @@ private static ImmutableArray<IExporter> GetExporters(IEnumerable<IExporter> exp
             for (int i = result.Count - 1; i >=0; i--)
                 if (result[i] is IExporterDependencies exporterDependencies)
                     foreach (var dependency in exporterDependencies.Dependencies)
-                        if (!result.Contains(dependency))
+                        /*
+                         *  When exporter that depends on an other already present in the configuration print warning.
+                         *
+                         *  Example:
+                         *
+                         *  // Global Current Culture separator is Semicolon;
+                         *  [CsvMeasurementsExporter(CsvSeparator.Comma)] // force use Comma
+                         *  [RPlotExporter]
+                         *  public class MyBanch
+                         *  {
+                         *
+                         *  }
+                         *
+                         *  RPlotExporter is depend from CsvMeasurementsExporter.Default
+                         *
+                         *  On active logger will by print:
+                         *  "The CsvMeasurementsExporter is already present in the configuration. There may be unexpected results of RPlotExporter.
+                         *
+                         */
+                        if (!result.Any(exporter=> exporter.GetType() == dependency.GetType()))
                             result.Insert(i, dependency); // All the exporter dependencies should be added before the exporter
+                        else
+                        {
+                            AddWarning($"The {dependency.Name} is already present in the configuration. There may be unexpected results of {result[i].GetType().Name}.");
+                        }
 
             result.Sort((left, right) => (left is IExporterDependencies).CompareTo(right is IExporterDependencies)); // the case when they were defined by user in wrong order ;)
 

src/BenchmarkDotNet/Configs/ManualConfig.cs

@@ -29,7 +29,8 @@ public class ManualConfig : IConfig
         private readonly List<Job> jobs = new List<Job>();
         private readonly HashSet<HardwareCounter> hardwareCounters = new HashSet<HardwareCounter>();
         private readonly List<IFilter> filters = new List<IFilter>();
-        private readonly List<BenchmarkLogicalGroupRule> logicalGroupRules = new List<BenchmarkLogicalGroupRule>();
+        private readonly HashSet<BenchmarkLogicalGroupRule> logicalGroupRules = new HashSet<BenchmarkLogicalGroupRule>();
+        private readonly static Conclusion[] emptyConclusion = Array.Empty<Conclusion>();
 
         public IEnumerable<IColumnProvider> GetColumnProviders() => columnProviders;
         public IEnumerable<IExporter> GetExporters() => exporters;
@@ -50,6 +51,8 @@ public class ManualConfig : IConfig
         [PublicAPI] public SummaryStyle SummaryStyle { get; set; }
         [PublicAPI] public TimeSpan BuildTimeout { get; set; } = DefaultConfig.Instance.BuildTimeout;
 
+        public IReadOnlyList<Conclusion> ConfigAnalysisConclusion => emptyConclusion;
+
         public ManualConfig WithOption(ConfigOptions option, bool value)
         {
             Options = Options.Set(value, option);
@@ -277,4 +280,4 @@ private static TimeSpan GetBuildTimeout(TimeSpan current, TimeSpan other)
                 ? other
                 : TimeSpan.FromMilliseconds(Math.Max(current.TotalMilliseconds, other.TotalMilliseconds));
     }
-}
+}

src/BenchmarkDotNet/Running/BenchmarkConverter.cs

@@ -89,7 +89,7 @@ private static ImmutableConfig GetFullTypeConfig(Type type, IConfig config)
             var typeAttributes = type.GetCustomAttributes(true).OfType<IConfigSource>();
             var assemblyAttributes = type.Assembly.GetCustomAttributes().OfType<IConfigSource>();
 
-            foreach (var configFromAttribute in typeAttributes.Concat(assemblyAttributes))
+            foreach (var configFromAttribute in assemblyAttributes.Concat(typeAttributes))
                 config = ManualConfig.Union(config, configFromAttribute.Config);
 
             return ImmutableConfigBuilder.Create(config);

src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs

@@ -255,6 +255,12 @@ private static void PrintSummary(ILogger logger, ImmutableConfig config, Summary
             // TODO: make exporter
             ConclusionHelper.Print(logger, config.GetCompositeAnalyser().Analyse(summary).Distinct().ToList());
 
+            if (config.ConfigAnalysisConclusion.Any())
+            {
+                logger.WriteLineHeader("// * Config Issues *");
+                ConclusionHelper.Print(logger, config.ConfigAnalysisConclusion);
+            }
+
             // TODO: move to conclusions
             var columnWithLegends = summary.Table.Columns.Select(c => c.OriginalColumn).Where(c => !string.IsNullOrEmpty(c.Legend)).ToList();
             var effectiveTimeUnit = summary.Table.EffectiveSummaryStyle.TimeUnit;

tests/BenchmarkDotNet.Tests/Configs/ImmutableConfigTests.cs

@@ -376,5 +376,37 @@ public class TestExporterDependency : IExporter
             public string Name => nameof(TestExporterDependency);
             public void ExportToLog(Summary summary, ILogger logger) { }
         }
+
+        [Fact]
+        public void GenerateWarningWhenExporterDependencyAlreadyExistInConfig()
+        {
+            System.Globalization.CultureInfo currentCulture = default;
+            System.Globalization.CultureInfo currentUICulture = default;
+            {
+                var ct = System.Threading.Thread.CurrentThread;
+                currentCulture = ct.CurrentCulture;
+                currentUICulture = ct.CurrentUICulture;
+                ct.CurrentCulture = System.Globalization.CultureInfo.InvariantCulture;
+                ct.CurrentUICulture = System.Globalization.CultureInfo.InvariantCulture;
+            }
+            try
+            {
+                var mutable = ManualConfig.CreateEmpty();
+                mutable.AddExporter(new BenchmarkDotNet.Exporters.Csv.CsvMeasurementsExporter(BenchmarkDotNet.Exporters.Csv.CsvSeparator.Comma));
+                mutable.AddExporter(RPlotExporter.Default);
+
+                var final = ImmutableConfigBuilder.Create(mutable);
+
+                Assert.Equal(1, final.ConfigAnalysisConclusion.Count);
+            }
+            finally
+            {
+                var ct = System.Threading.Thread.CurrentThread;
+                ct.CurrentCulture = currentCulture;
+                ct.CurrentUICulture = currentUICulture;
+
+            }
+
+        }
     }
 }