From 8ffc3fe3dc15b17ecf39a289deb5cd7fb65993a0 Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Tue, 11 Apr 2023 12:36:40 -0500 Subject: [PATCH 1/2] Avoid overly-specific cast in bulk metadata copy (#8646) The return value of `ITaskItem.CloneCustomMetadata` is an `IDictionary`, which is generally (in modern MSBuild) backed by a `Dictionary`, but can be (when given an item from a net35 taskhost) a `Hashtable`. In the latter situation, casting entries to `KeyValuePair<,>` fails, because they conform only to `DictionaryEntry`. Use that less-well-typed approach--the casts were present in the pre- bulk-edit version of the code. Fixes #8645. --- eng/Versions.props | 2 +- src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/eng/Versions.props b/eng/Versions.props index 6167961d9bb..36f15aea630 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -2,7 +2,7 @@ - 17.6.0release + 17.6.1release 17.5.0 15.1.0.0 preview diff --git a/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs b/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs index b6157448cf1..0598193fabb 100644 --- a/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs +++ b/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs @@ -1391,8 +1391,8 @@ private void GatherTaskItemOutputs(bool outputTargetIsItem, string outputTargetN newItem = new ProjectItemInstance(_projectInstance, outputTargetName, EscapingUtilities.Escape(output.ItemSpec), parameterLocationEscaped); newItem.SetMetadataOnTaskOutput(output.CloneCustomMetadata() - .Cast>() - .Select(x => new KeyValuePair(x.Key, EscapingUtilities.Escape(x.Value)))); + .Cast() + .Select(x => new KeyValuePair((string)x.Key, EscapingUtilities.Escape((string)x.Value)))); } } From 78cc219f08c46ac70dba467a32d44406a1fa889f Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Tue, 11 Apr 2023 14:43:47 -0500 Subject: [PATCH 2/2] Regression test for #8645 Add a test task with properties like the failing WiX tasks to prevent regressions of #8645. --- .../BackEnd/TaskBuilder_Tests.cs | 25 ++++++++++ .../TaskThatReturnsMinimalItem.cs | 48 +++++++++++++++++++ src/Framework/ITaskItemExtensions.cs | 4 +- 3 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 src/Build.UnitTests/TaskThatReturnsMinimalItem.cs diff --git a/src/Build.UnitTests/BackEnd/TaskBuilder_Tests.cs b/src/Build.UnitTests/BackEnd/TaskBuilder_Tests.cs index c91df2ca317..f0f889eca24 100644 --- a/src/Build.UnitTests/BackEnd/TaskBuilder_Tests.cs +++ b/src/Build.UnitTests/BackEnd/TaskBuilder_Tests.cs @@ -597,6 +597,31 @@ public void NullMetadataOnLegacyOutputItems() logger.AssertLogContains("[foo: ]"); } + /// + /// If an item returned from a task has bare-minimum metadata implementation, we shouldn't crash. + /// + [Fact] + public void MinimalLegacyOutputItems() + { + string customTaskPath = Assembly.GetExecutingAssembly().Location; + + string projectContents = $""" + + + + + + + + + + + + """; + + MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess(projectContents, _testOutput, LoggerVerbosity.Diagnostic); + } + /// /// Regression test for https://github.com/dotnet/msbuild/issues/5080 /// diff --git a/src/Build.UnitTests/TaskThatReturnsMinimalItem.cs b/src/Build.UnitTests/TaskThatReturnsMinimalItem.cs new file mode 100644 index 00000000000..7f8eec32b2a --- /dev/null +++ b/src/Build.UnitTests/TaskThatReturnsMinimalItem.cs @@ -0,0 +1,48 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections; + +using Microsoft.Build.Framework; + +namespace Microsoft.Build.Engine.UnitTests; + +/// +/// Task that emulates .NET 3.5 tasks. +/// +public sealed class TaskThatReturnsMinimalItem : ITask +{ + public IBuildEngine? BuildEngine { get; set; } + public ITaskHost? HostObject { get; set; } + + [Output] + public ITaskItem MinimalTaskItemOutput { get => new MinimalTaskItem(); } + + public bool Execute() => true; + + /// + /// Minimal implementation of that uses a for metadata, + /// like MSBuild 3 did. + /// + internal sealed class MinimalTaskItem : ITaskItem + { + public string ItemSpec { get => $"{nameof(MinimalTaskItem)}spec"; set => throw new NotImplementedException(); } + + public ICollection MetadataNames => throw new NotImplementedException(); + + public int MetadataCount => throw new NotImplementedException(); + + public IDictionary CloneCustomMetadata() + { + Hashtable t = new(); + t["key"] = "value"; + + return t; + } + public void CopyMetadataTo(ITaskItem destinationItem) => throw new NotImplementedException(); + public string GetMetadata(string metadataName) => "value"; + public void RemoveMetadata(string metadataName) => throw new NotImplementedException(); + public void SetMetadata(string metadataName, string metadataValue) => throw new NotImplementedException(); + } +} diff --git a/src/Framework/ITaskItemExtensions.cs b/src/Framework/ITaskItemExtensions.cs index 7dc7dbdaf86..6ba56e1a880 100644 --- a/src/Framework/ITaskItemExtensions.cs +++ b/src/Framework/ITaskItemExtensions.cs @@ -35,7 +35,9 @@ public static IEnumerable> EnumerateMetadata(this I return enumerableMetadata; } - // In theory this should never be reachable. + // Fallback for + // * ITaskItem implementations from MSBuild 3.5 from the GAC + // * Custom ITaskItems that don't use Dictionary var list = new KeyValuePair[customMetadata.Count]; int i = 0;