Log TaskParameterEvent for scalar parameters (#9908)

Fixes #9827 ### Context `TaskParameterEvent` with `TaskParameterMessageKind.TaskInput` is currently used only for parameters that are lists. Parameters that are simple strings are logged as a specially formatted low-importance message. The binlog viewer contains logic to recognize this special message and recover the Name and Value to be rendered in the viewer UI. Since we will use this event for analyzers, it would be unfortunate to add one more place with this suboptimal processing. ### Changes Made Unified the logic in `TaskExecutionHost` to log all parameters as `TaskParameterEvent` with `TaskParameterMessageKind.TaskInput`. The change is under a change wave check. ### Testing - Added a new unit test. - Compared diagnostic-level output with task parameter logging enabled with and without the change. No differences were found when passing null, empty, false, true, numeric, string, or stringified item list parameters. - Compared the appearance in binlog for the same sample values as above. No differences were observed. - Compared OrchardCore binlogs with and without the change. They're the same size and the only difference I found was in rendering the `SolutionConfigurationContents` parameter which is a string but the content _looks_ formatted so it was rendered incorrectly as a list of items. ### Notes As @KirillOsenkov pointed out in the issue, we don't really depend on the textual messages so we don't have to do any double-logging. The viewer can remove parsing the textual messages on its own schedule.

Author: ladipro

documentation/wiki/ChangeWaves.md

@@ -23,6 +23,9 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
 
 ## Current Rotation of Change Waves
 
+### 17.12
+- [Log TaskParameterEvent for scalar parameters](https://github.com/dotnet/msbuild/pull/9908)
+
 ### 17.10
 - [AppDomain configuration is serialized without using BinFmt](https://github.com/dotnet/msbuild/pull/9320) - feature can be opted out only if [BinaryFormatter](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.formatters.binary.binaryformatter) is allowed at runtime by editing `MSBuild.runtimeconfig.json`
 - [Warning on serialization custom events by default in .NET framework](https://github.com/dotnet/msbuild/pull/9318)
@@ -37,14 +40,12 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
 - [Keep the encoding of standard output & error consistent with the console code page for ToolTask](https://github.com/dotnet/msbuild/pull/9539)
 - [Convert.ToString during a property evaluation uses the InvariantCulture for all types](https://github.com/dotnet/msbuild/pull/9874)
 
-
 ### 17.8
 - [[RAR] Don't do I/O on SDK-provided references](https://github.com/dotnet/msbuild/pull/8688)
 - [Delete destination file before copy](https://github.com/dotnet/msbuild/pull/8685)
 - [Moving from SHA1 to SHA256 for Hash task](https://github.com/dotnet/msbuild/pull/8812)
 - [Deprecating custom derived BuildEventArgs](https://github.com/dotnet/msbuild/pull/8917) - feature can be opted out only if [BinaryFormatter](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.formatters.binary.binaryformatter) is allowed at runtime by editing `MSBuild.runtimeconfig.json`
 
-
 ### 17.6
 - [Parse invalid property under target](https://github.com/dotnet/msbuild/pull/8190)
 - [Eliminate project string cache](https://github.com/dotnet/msbuild/pull/7965)

src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs

@@ -18,6 +18,7 @@
 using Microsoft.Build.Execution;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Shared;
+using Shouldly;
 using Xunit;
 using InvalidProjectFileException = Microsoft.Build.Exceptions.InvalidProjectFileException;
 using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem;
@@ -1048,6 +1049,36 @@ public void TestTaskDictionaryOutputItems()
  """);
  ml.AssertLogContains("a=b");
  }
+
+ [Fact]
+ public void TestTaskParameterLogging()
+ {
+ string customTaskPath = Assembly.GetExecutingAssembly().Location;
+ MockLogger ml = ObjectModelHelpers.BuildProjectExpectSuccess($"""
+ <Project>
+ <UsingTask TaskName=`TaskThatReturnsDictionaryTaskItem` AssemblyFile=`{customTaskPath}`/>
+ <ItemGroup>
+ <MyItem Include="item1"/>
+ <MyItem Include="item2"/>
+ </ItemGroup>
+ <Target Name=`Build`>
+ <TaskThatReturnsDictionaryTaskItem Key="a" Value="b" AdditionalParameters="@(MyItem)" />
+ </Target>
+ </Project>
+ """);
+
+ // Each parameter should be logged as TaskParameterEvent.
+ ml.TaskParameterEvents.Count.ShouldBe(3);
+ IList<string> messages = ml.TaskParameterEvents.Select(e => e.Message).ToList();
+ messages.ShouldContain($"{ItemGroupLoggingHelper.TaskParameterPrefix}Key=a");
+ messages.ShouldContain($"{ItemGroupLoggingHelper.TaskParameterPrefix}Value=b");
+ messages.ShouldContain($"{ItemGroupLoggingHelper.TaskParameterPrefix}\n AdditionalParameters=\n item1\n item2");
+
+ // Parameters should not be logged as messages.
+ messages = ml.BuildMessageEvents.Select(e => e.Message).ToList();
+ messages.ShouldNotContain(m => m.StartsWith(ItemGroupLoggingHelper.TaskParameterPrefix));
+ }
+
  #endregion
 
  #region ITestTaskHost Members

src/Build.UnitTests/BackEnd/TaskThatReturnsDictionaryTaskItem.cs

@@ -17,6 +17,8 @@ public sealed class TaskThatReturnsDictionaryTaskItem : Utilities.Task
  public string Key { get; set; }
  public string Value { get; set; }
 
+ public ITaskItem[] AdditionalParameters { get; set; }
+
  public override bool Execute()
  {
  var metaValue = new MinimalDictionary<string, string>

src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs

@@ -1266,30 +1266,6 @@ private bool InitializeTaskVectorParameter(
  return success;
  }
 
- /// <summary>
- /// Variation to handle arrays, to help with logging the parameters.
- /// </summary>
- /// <remarks>
- /// Logging currently enabled only by an env var.
- /// </remarks>
- private bool InternalSetTaskParameter(TaskPropertyInfo parameter, IList parameterValue)
- {
- if (LogTaskInputs &&
- !_taskLoggingContext.LoggingService.OnlyLogCriticalEvents &&
- parameterValue.Count > 0 &&
- parameter.Log)
- {
- ItemGroupLoggingHelper.LogTaskParameter(
- _taskLoggingContext,
- TaskParameterMessageKind.TaskInput,
- parameter.Name,
- parameterValue,
- parameter.LogItemMetadata);
- }
-
- return InternalSetTaskParameter(parameter, (object)parameterValue);
- }
-
  private static readonly string TaskParameterFormatString = ItemGroupLoggingHelper.TaskParameterPrefix + "{0}={1}";
 
  /// <summary>
@@ -1303,14 +1279,31 @@ private bool InternalSetTaskParameter(
 
  if (LogTaskInputs && !_taskLoggingContext.LoggingService.OnlyLogCriticalEvents)
  {
- // If the type is a list, we already logged the parameters
- if (!(parameterValue is IList))
+ IList parameterValueAsList = parameterValue as IList;
+ bool legacyBehavior = !ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_12);
+
+ // Legacy textual logging for parameters that are not lists.
+ if (legacyBehavior && parameterValueAsList == null)
  {
  _taskLoggingContext.LogCommentFromText(
- MessageImportance.Low,
- TaskParameterFormatString,
- parameter.Name,
- ItemGroupLoggingHelper.GetStringFromParameterValue(parameterValue));
+ MessageImportance.Low,
+ TaskParameterFormatString,
+ parameter.Name,
+ ItemGroupLoggingHelper.GetStringFromParameterValue(parameterValue));
+ }
+
+ if (parameter.Log)
+ {
+ // Structured logging for all parameters that have logging enabled and are not empty lists.
+ if (parameterValueAsList?.Count > 0 || (parameterValueAsList == null && !legacyBehavior))
+ {
+ ItemGroupLoggingHelper.LogTaskParameter(
+ _taskLoggingContext,
+ TaskParameterMessageKind.TaskInput,
+ parameter.Name,
+ parameterValueAsList ?? new object[] { parameterValue },
+ parameter.LogItemMetadata);
+ }
  }
  }
 

src/Framework/ChangeWaves.cs

@@ -29,7 +29,8 @@ internal static class ChangeWaves
  internal static readonly Version Wave17_6 = new Version(17, 6);
  internal static readonly Version Wave17_8 = new Version(17, 8);
  internal static readonly Version Wave17_10 = new Version(17, 10);
- internal static readonly Version[] AllWaves = { Wave17_4, Wave17_6, Wave17_8, Wave17_10 };
+ internal static readonly Version Wave17_12 = new Version(17, 12);
+ internal static readonly Version[] AllWaves = { Wave17_4, Wave17_6, Wave17_8, Wave17_10, Wave17_12 };
 
  /// <summary>
  /// Special value indicating that all features behind all Change Waves should be enabled.

src/UnitTests.Shared/MockLogger.cs

@@ -123,6 +123,11 @@ public sealed class MockLogger : ILogger
  /// </summary>
  public List<TaskFinishedEventArgs> TaskFinishedEvents { get; } = new List<TaskFinishedEventArgs>();
 
+ /// <summary>
+ /// List of TaskParameter events
+ /// </summary>
+ public List<TaskParameterEventArgs> TaskParameterEvents { get; } = new List<TaskParameterEventArgs>();
+
  /// <summary>
  /// List of BuildMessage events
  /// </summary>
@@ -362,6 +367,11 @@ public void LoggerEventHandler(object sender, BuildEventArgs eventArgs)
  TaskFinishedEvents.Add(taskFinishedEventArgs);
  break;
  }
+ case TaskParameterEventArgs taskParameterEventArgs:
+ {
+ TaskParameterEvents.Add(taskParameterEventArgs);
+ break;
+ }
  case BuildMessageEventArgs buildMessageEventArgs:
  {
  BuildMessageEvents.Add(buildMessageEventArgs);