From 047fd1a605d0b741e45d469d2f699a45f0c01f25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Thu, 20 Aug 2026 14:14:04 +0200 Subject: [PATCH 1/7] Expand built-in metadata references in the task host Item definition metadata may reference built-in metadata, as in %(Filename). Such a value is stored unexpanded and substituted when the metadata is read, so that it tracks the item it is read from. The marshalled item used to carry items across a process boundary holds a single flat dictionary and returned the stored text verbatim, so a task running in a task host saw the literal "%(Filename)" where the same task run in-proc saw "hello". Under -mt nearly every task runs in a task host, which is how the VS repository build ended up emitting paths containing "%(Filename)". Substitute the references on read instead, which keeps the value tracking the item spec even if the task reassigns it. Values a task writes on the item are literal and are excluded, matching what the task would read back in-proc. RecursiveDir is resolved from the item's own metadata because it derives from the wildcard the item was expanded from rather than from the item spec. No serialized state changes, so a task host built from different sources behaves exactly as it does today. Fixes #14763 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6 --- .../ItemDefinitionMetadataInTaskHost_Tests.cs | 109 +++++++++++++++ .../BackEnd/MetadataObservationTask.cs | 57 ++++++++ src/Framework/BuiltInMetadataExpander.cs | 120 ++++++++++++++++ src/Shared/TaskParameter.cs | 29 +++- src/Shared/UnitTests/TaskParameter_Tests.cs | 131 ++++++++++++++++++ 5 files changed, 445 insertions(+), 1 deletion(-) create mode 100644 src/Build.UnitTests/BackEnd/ItemDefinitionMetadataInTaskHost_Tests.cs create mode 100644 src/Build.UnitTests/BackEnd/MetadataObservationTask.cs create mode 100644 src/Framework/BuiltInMetadataExpander.cs diff --git a/src/Build.UnitTests/BackEnd/ItemDefinitionMetadataInTaskHost_Tests.cs b/src/Build.UnitTests/BackEnd/ItemDefinitionMetadataInTaskHost_Tests.cs new file mode 100644 index 00000000000..e966a7d6735 --- /dev/null +++ b/src/Build.UnitTests/BackEnd/ItemDefinitionMetadataInTaskHost_Tests.cs @@ -0,0 +1,109 @@ +// 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.Diagnostics; +using System.IO; +using Microsoft.Build.Execution; +using Microsoft.Build.UnitTests; +using Shouldly; +using Xunit; + +#nullable disable + +namespace Microsoft.Build.Engine.UnitTests.BackEnd +{ + /// + /// Item definition metadata referencing built-in metadata, such as %(Filename), is stored unexpanded + /// and substituted when the metadata is read. These tests assert that a task observes the same value whether + /// it runs in-proc or in a task host. + /// Regression tests for https://github.com/dotnet/msbuild/issues/14763. + /// + public sealed class ItemDefinitionMetadataInTaskHost_Tests + { + private static string AssemblyLocation { get; } = + typeof(ItemDefinitionMetadataInTaskHost_Tests).Assembly.Location + ?? Path.Combine(AppContext.BaseDirectory, "Microsoft.Build.Engine.UnitTests.dll"); + + private readonly ITestOutputHelper _output; + + public ItemDefinitionMetadataInTaskHost_Tests(ITestOutputHelper output) => _output = output; + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void MetadataReferencingBuiltInMetadataIsExpandedForTheTask(bool useTaskHost) + { + Observe("%(Filename)", useTaskHost).ShouldBe("hello"); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void MetadataReferencingBuiltInMetadataFollowsReassignedItemSpec(bool useTaskHost) + { + Observe("%(Filename)", useTaskHost, newItemSpec: @"other\renamed.txt").ShouldBe("renamed"); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void MetadataOverriddenOnTheItemWinsOverTheItemDefinition(bool useTaskHost) + { + Observe("%(Filename)", useTaskHost, itemOverride: "explicit").ShouldBe("explicit"); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void EscapedMetadataReferenceIsNotExpanded(bool useTaskHost) + { + Observe("%25(Filename)", useTaskHost).ShouldBe("%(Filename)"); + } + + /// + /// Runs a task against an item whose definition carries and returns the + /// value the task itself observed, having asserted the task ran where the test intended. + /// + private string Observe(string definitionValue, bool useTaskHost, string newItemSpec = null, string itemOverride = null) + { + using TestEnvironment env = TestEnvironment.Create(_output); + + string project = $""" + + + + + {definitionValue} + + + + + {(itemOverride is null ? string.Empty : $"{itemOverride}")} + + + + + + + + + + """; + + ProjectInstance projectInstance = new(env.CreateFile("test.proj", project).Path); + + BuildResult result = BuildManager.DefaultBuildManager.Build( + new BuildParameters { EnableNodeReuse = false }, + new BuildRequestData(projectInstance, targetsToBuild: ["Observe"])); + + result.OverallResult.ShouldBe(BuildResultCode.Success); + + int taskProcessId = int.Parse(projectInstance.GetPropertyValue("TaskProcessId")); + bool ranOutOfProc = taskProcessId != Process.GetCurrentProcess().Id; + ranOutOfProc.ShouldBe(useTaskHost, $"the task was expected to run {(useTaskHost ? "in a task host" : "in-proc")}"); + + return projectInstance.GetPropertyValue("ObservedValue"); + } + } +} diff --git a/src/Build.UnitTests/BackEnd/MetadataObservationTask.cs b/src/Build.UnitTests/BackEnd/MetadataObservationTask.cs new file mode 100644 index 00000000000..4aa5c9507f9 --- /dev/null +++ b/src/Build.UnitTests/BackEnd/MetadataObservationTask.cs @@ -0,0 +1,57 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using Microsoft.Build.Framework; +using Microsoft.Build.Utilities; + +#nullable disable + +namespace Microsoft.Build.UnitTests +{ + /// + /// Reports the value of a metadata name as the task itself observes it, plus the id of the process the + /// task ran in, so tests can tell an in-proc execution apart from a task host one. + /// Optionally reassigns ItemSpec first, to exercise metadata that derives from it. + /// + public class MetadataObservationTask : Task + { + [Required] + public ITaskItem[] Items { get; set; } + + [Required] + public string MetadataName { get; set; } + + public string NewItemSpec { get; set; } + + [Output] + public string ObservedValue { get; set; } + + [Output] + public int TaskProcessId { get; set; } + + public override bool Execute() + { + TaskProcessId = Process.GetCurrentProcess().Id; + + if (Items.Length > 0) + { + ITaskItem item = Items[0]; + + if (!string.IsNullOrEmpty(NewItemSpec)) + { + item.ItemSpec = NewItemSpec; + } + + ObservedValue = item.GetMetadata(MetadataName); + } + else + { + ObservedValue = string.Empty; + } + + return true; + } + } +} + diff --git a/src/Framework/BuiltInMetadataExpander.cs b/src/Framework/BuiltInMetadataExpander.cs new file mode 100644 index 00000000000..daeeefef104 --- /dev/null +++ b/src/Framework/BuiltInMetadataExpander.cs @@ -0,0 +1,120 @@ +// 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 Microsoft.NET.StringTools; + +namespace Microsoft.Build.Framework; + +/// +/// Substitutes built-in metadata references, such as %(Filename), with the values they denote for a +/// given item. +/// +/// +/// +/// Item definition metadata may reference built-in metadata. Such values are stored unexpanded so that they +/// follow the item they are read from, and are substituted whenever the metadata is read. +/// +/// +/// This is a deliberately minimal substitute for the evaluation expander, which also handles properties, item +/// vectors, custom metadata, transforms and truncation. Built-in metadata references are the only expression +/// form that survives evaluation unexpanded. It lives here because the evaluation expander is internal to +/// Microsoft.Build, while this is needed by MSBuild and Microsoft.Build.Tasks as well. +/// +/// +internal static class BuiltInMetadataExpander +{ + /// + /// Expands every built-in metadata reference in against the given item. + /// Anything else, including a reference that cannot be satisfied, is left untouched. + /// + /// The escaped value to expand. + /// The escaped item spec that built-in metadata is derived from. + /// The escaped path of the project that defined the item. + /// + /// The item's RecursiveDir, which comes from the wildcard the item was expanded from rather than the item spec. + /// + /// Cache of already derived modifier values for this item. + /// The value with built-in metadata references substituted. + internal static string? Expand( + string? escapedValue, + string escapedItemSpec, + string? escapedDefiningProject, + string? escapedRecursiveDir, + ref ItemSpecModifiers.Cache cache) + { + int index = escapedValue is null ? -1 : escapedValue.IndexOf("%(", StringComparison.Ordinal); + + if (index < 0) + { + return escapedValue; + } + + SpanBasedStringBuilder? builder = null; + int copiedUpTo = 0; + + try + { + while (index >= 0) + { + int closingParenthesis = escapedValue!.IndexOf(')', index + 2); + + if (closingParenthesis < 0) + { + break; + } + + if (TryGetModifier(escapedValue, index + 2, closingParenthesis, out ItemSpecModifierKind kind)) + { + builder ??= Strings.GetSpanBasedStringBuilder(); + builder.Append(escapedValue, copiedUpTo, index - copiedUpTo); + builder.Append(kind is ItemSpecModifierKind.RecursiveDir + ? escapedRecursiveDir ?? string.Empty + : ItemSpecModifiers.GetItemSpecModifier(escapedItemSpec, kind, currentDirectory: null, escapedDefiningProject, ref cache)); + copiedUpTo = closingParenthesis + 1; + } + + index = escapedValue.IndexOf("%(", closingParenthesis + 1, StringComparison.Ordinal); + } + + if (builder is null) + { + return escapedValue; + } + + builder.Append(escapedValue!, copiedUpTo, escapedValue!.Length - copiedUpTo); + return builder.ToString(); + } + finally + { + builder?.Dispose(); + } + } + + /// + /// Reads the metadata name between %( and its closing parenthesis and resolves it to a built-in + /// metadata kind, tolerating surrounding whitespace as the evaluation expander does. A name qualified by an + /// item type is rejected, since the engine resolves built-in metadata against an untyped table and so never + /// satisfies one either. + /// + private static bool TryGetModifier(string value, int start, int end, out ItemSpecModifierKind kind) + { + while (start < end && char.IsWhiteSpace(value[start])) + { + start++; + } + + while (end > start && char.IsWhiteSpace(value[end - 1])) + { + end--; + } + + if (end <= start) + { + kind = default; + return false; + } + + return ItemSpecModifiers.TryGetModifierKind(value.Substring(start, end - start), out kind); + } +} diff --git a/src/Shared/TaskParameter.cs b/src/Shared/TaskParameter.cs index 3fb693876f6..c88fdae5560 100644 --- a/src/Shared/TaskParameter.cs +++ b/src/Shared/TaskParameter.cs @@ -575,6 +575,12 @@ private class TaskParameterTaskItem : /// private ItemSpecModifiers.Cache _cachedModifiers; + /// + /// Names of metadata written on this item after it was received. Their values are literal, so they are + /// never expanded on read. + /// + private HashSet _locallySetMetadata = null; + /// /// Constructor for serialization /// @@ -756,6 +762,9 @@ public void SetMetadata(string metadataName, string metadataValue) _customEscapedMetadata ??= new Dictionary(MSBuildNameIgnoreCaseComparer.Default); _customEscapedMetadata[metadataName] = metadataValue ?? String.Empty; + + _locallySetMetadata ??= new HashSet(MSBuildNameIgnoreCaseComparer.Default); + _locallySetMetadata.Add(metadataName); } /// @@ -773,6 +782,7 @@ public void RemoveMetadata(string metadataName) } _customEscapedMetadata.Remove(metadataName); + _locallySetMetadata?.Remove(metadataName); } /// @@ -882,7 +892,24 @@ string ITaskItem2.GetMetadataValueEscaped(string metadataName) string metadataValue = null; _customEscapedMetadata?.TryGetValue(metadataName, out metadataValue); - return metadataValue ?? string.Empty; + if (metadataValue is null) + { + return string.Empty; + } + + // Item definition metadata referencing built-in metadata is stored unexpanded and substituted on read + // so that it tracks the item it is read from. Definition and directly set metadata arrive here + // flattened into a single dictionary, so the distinction is instead carried by the value itself: + // anything expanded during evaluation has no reference left to substitute. Metadata a task sets is + // literal, so it is excluded. + if (metadataValue.IndexOf("%(", StringComparison.Ordinal) < 0 || _locallySetMetadata?.Contains(metadataName) == true) + { + return metadataValue; + } + + _customEscapedMetadata.TryGetValue(ItemSpecModifiers.RecursiveDir, out string escapedRecursiveDir); + + return BuiltInMetadataExpander.Expand(metadataValue, _escapedItemSpec, _escapedDefiningProject, escapedRecursiveDir, ref _cachedModifiers); } /// diff --git a/src/Shared/UnitTests/TaskParameter_Tests.cs b/src/Shared/UnitTests/TaskParameter_Tests.cs index b70b70169c9..d7905e704fc 100644 --- a/src/Shared/UnitTests/TaskParameter_Tests.cs +++ b/src/Shared/UnitTests/TaskParameter_Tests.cs @@ -606,6 +606,137 @@ public void ITaskItemParameter_EscapableNotEscapedMetadata() Assert.Equal("c1)d1", foo2.GetMetadataValueEscaped("b")); } + /// + /// Regression test for https://github.com/dotnet/msbuild/issues/14763 + /// Item definition metadata referencing built-in metadata is stored unexpanded and substituted on read, + /// so a task must observe the same value whether or not the item crossed the TaskHost boundary. + /// + [Theory] + [InlineData("%(Filename)", "hello")] + [InlineData("%( Filename )", "hello")] + [InlineData("%(filename)", "hello")] + [InlineData("pre-%(Filename)-post", "pre-hello-post")] + [InlineData("%(Filename)%(Extension)", "hello.txt")] + [InlineData("no expression", "no expression")] + // An escaped reference is literal text rather than an expression. + [InlineData("%25(Filename)", "%(Filename)")] + // Malformed references are not expressions either. + [InlineData("%(Filename", "%(Filename")] + [InlineData("a%(", "a%(")] + [InlineData("%()", "%()")] + public void ItemDefinitionMetadataReadsTheSameAcrossTaskHostBoundary(string definitionValue, string expected) + { + ProjectItemInstanceTaskItem item = CreateItemWithDefinitionMetadata("NameMeta", definitionValue); + + item.GetMetadata("NameMeta").ShouldBe(expected); + RoundTrip(item).GetMetadata("NameMeta").ShouldBe(expected); + } + + /// + /// A task that reassigns ItemSpec sees item definition metadata re-derived from the new spec. That must + /// hold on both sides of the TaskHost boundary, so the value cannot simply be substituted up front. + /// + [Fact] + public void ItemDefinitionMetadataFollowsReassignedItemSpecAcrossTaskHostBoundary() + { + string renamed = $"other{Path.DirectorySeparatorChar}renamed.txt"; + + ProjectItemInstanceTaskItem item = CreateItemWithDefinitionMetadata("NameMeta", "%(Filename)"); + ITaskItem marshalled = RoundTrip(item); + + item.ItemSpec = renamed; + marshalled.ItemSpec = renamed; + + item.GetMetadata("NameMeta").ShouldBe("renamed"); + marshalled.GetMetadata("NameMeta").ShouldBe("renamed"); + } + + /// + /// A value a task writes on the item is literal, matching what the same task would read back in-proc. + /// + [Fact] + public void MetadataSetByTheTaskIsNotExpanded() + { + ITaskItem marshalled = RoundTrip(CreateItemWithDefinitionMetadata("NameMeta", "%(Filename)")); + + marshalled.SetMetadata("NameMeta", "%(Filename)"); + marshalled.SetMetadata("Other", "%(Filename)"); + + marshalled.GetMetadata("NameMeta").ShouldBe("%(Filename)"); + marshalled.GetMetadata("Other").ShouldBe("%(Filename)"); + + // Removing the task's value uncovers the item definition value again. + marshalled.RemoveMetadata("NameMeta"); + marshalled.GetMetadata("NameMeta").ShouldBe(""); + } + + /// + /// Bulk metadata reads report values unexpanded, matching what an engine item reports in-proc. + /// + [Fact] + public void BulkMetadataReadsMatchEngineItemAcrossTaskHostBoundary() + { + ProjectItemInstanceTaskItem item = CreateItemWithDefinitionMetadata("NameMeta", "%(Filename)"); + ITaskItem marshalled = RoundTrip(item); + + ((IDictionary)((ITaskItem2)item).CloneCustomMetadataEscaped())["NameMeta"].ShouldBe("%(Filename)"); + ((IDictionary)((ITaskItem2)marshalled).CloneCustomMetadataEscaped())["NameMeta"].ShouldBe("%(Filename)"); + } + + /// + /// RecursiveDir comes from the wildcard the item was expanded from rather than from the item spec, so it + /// has to be resolved from the item's own metadata to keep patterns such as + /// out\%(RecursiveDir)%(Filename)%(Extension) producing the same path on both sides of the boundary. + /// + [Fact] + public void RecursiveDirReferenceIsExpandedAcrossTaskHostBoundary() + { + string sep = Path.DirectorySeparatorChar.ToString(); + string expected = $"out{sep}sub1{sep}sub2{sep}hello.txt"; + + ProjectItemDefinitionInstance definition = new( + "Thing", + ImmutableDictionaryExtensions.EmptyMetadata.SetItem("NameMeta", $"out{sep}%(RecursiveDir)%(Filename)%(Extension)")); + + ProjectItemInstanceTaskItem item = new( + includeEscaped: $"tree{sep}sub1{sep}sub2{sep}hello.txt", + includeBeforeWildcardExpansionEscaped: $"tree{sep}**{sep}*.txt", + directMetadata: null, + itemDefinitions: [definition], + projectDirectory: Directory.GetCurrentDirectory(), + immutable: false, + definingFileEscaped: "test.proj"); + + item.GetMetadata("NameMeta").ShouldBe(expected); + RoundTrip(item).GetMetadata("NameMeta").ShouldBe(expected); + } + + private static ProjectItemInstanceTaskItem CreateItemWithDefinitionMetadata(string name, string escapedValue) + { + string itemSpec = $"folder{Path.DirectorySeparatorChar}hello.txt"; + + ProjectItemDefinitionInstance definition = new( + "Thing", + ImmutableDictionaryExtensions.EmptyMetadata.SetItem(name, escapedValue)); + + return new ProjectItemInstanceTaskItem( + includeEscaped: itemSpec, + includeBeforeWildcardExpansionEscaped: itemSpec, + directMetadata: null, + itemDefinitions: [definition], + projectDirectory: Directory.GetCurrentDirectory(), + immutable: false, + definingFileEscaped: "test.proj"); + } + + private static ITaskItem RoundTrip(ITaskItem item) + { + TaskParameter t = new(item); + ((ITranslatable)t).Translate(TranslationHelpers.GetWriteTranslator()); + + return (ITaskItem)TaskParameter.FactoryForDeserialization(TranslationHelpers.GetReadTranslator()).WrappedParameter; + } + /// /// Regression test for https://github.com/dotnet/msbuild/issues/13140 /// RecursiveDir (built-in, non-derivable metadata) must survive TaskParameter From 114ee898b1186ae9991231145d78c4862045a5cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Thu, 20 Aug 2026 16:41:37 +0200 Subject: [PATCH 2/7] Substitute when cloning, and invalidate cached path metadata A task that clones its input, through CopyMetadataTo or the TaskItem copy constructor that calls it, was handed the stored text rather than the finished value, so the inconsistency remained for that very common shape. An engine item substitutes when copying onto an item a task can reach; do the same here. Values the task wrote stay literal, as they do in-proc. Reassigning ItemSpec now clears the derived-metadata cache, as it does on an engine item. Without this a FullPath, RootDir or Directory read before the move was returned again after it. That was already the case for direct reads of those modifiers, and substitution on read would otherwise have extended it to references embedded in item definition metadata. Only remember a written value as literal when it could otherwise be substituted, so ordinary metadata writes no longer allocate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6 --- src/Shared/TaskParameter.cs | 54 ++++++++++++++++-- src/Shared/UnitTests/TaskParameter_Tests.cs | 61 +++++++++++++++++++++ 2 files changed, 109 insertions(+), 6 deletions(-) diff --git a/src/Shared/TaskParameter.cs b/src/Shared/TaskParameter.cs index c88fdae5560..067fc0c4466 100644 --- a/src/Shared/TaskParameter.cs +++ b/src/Shared/TaskParameter.cs @@ -678,6 +678,7 @@ public string ItemSpec set { _escapedItemSpec = value; + _cachedModifiers.Clear(); } } @@ -728,6 +729,7 @@ string ITaskItem2.EvaluatedIncludeEscaped set { _escapedItemSpec = value; + _cachedModifiers.Clear(); } } @@ -763,8 +765,12 @@ public void SetMetadata(string metadataName, string metadataValue) _customEscapedMetadata[metadataName] = metadataValue ?? String.Empty; - _locallySetMetadata ??= new HashSet(MSBuildNameIgnoreCaseComparer.Default); - _locallySetMetadata.Add(metadataName); + // Only a value that would otherwise be substituted on read has to be remembered as literal. + if (metadataValue?.IndexOf("%(", StringComparison.Ordinal) >= 0) + { + _locallySetMetadata ??= new HashSet(MSBuildNameIgnoreCaseComparer.Default); + _locallySetMetadata.Add(metadataName); + } } /// @@ -809,6 +815,14 @@ public void CopyMetadataTo(ITaskItem destinationItem) IEnumerable> metadataToImport = _customEscapedMetadata .Where(metadatum => string.IsNullOrEmpty(destinationItem.GetMetadata(metadatum.Key))); + // The destination has no notion of a value awaiting substitution, so hand it finished values, + // as an engine item does when copying onto an item a task can reach. + if (HasAnyExpandableExpressions()) + { + metadataToImport = metadataToImport + .Select(metadatum => new KeyValuePair(metadatum.Key, Substitute(metadatum.Key, metadatum.Value))); + } + #if FEATURE_APPDOMAIN if (RemotingServices.IsTransparentProxy(destinationItem)) { @@ -827,7 +841,7 @@ public void CopyMetadataTo(ITaskItem destinationItem) if (String.IsNullOrEmpty(value)) { - destinationItem.SetMetadata(entry.Key, entry.Value); + destinationItem.SetMetadata(entry.Key, Substitute(entry.Key, entry.Value)); } } } @@ -902,14 +916,42 @@ string ITaskItem2.GetMetadataValueEscaped(string metadataName) // flattened into a single dictionary, so the distinction is instead carried by the value itself: // anything expanded during evaluation has no reference left to substitute. Metadata a task sets is // literal, so it is excluded. - if (metadataValue.IndexOf("%(", StringComparison.Ordinal) < 0 || _locallySetMetadata?.Contains(metadataName) == true) + return Substitute(metadataName, metadataValue); + } + + /// + /// Substitutes any built-in metadata references in a stored metadata value, unless the value was + /// written by the task and is therefore literal. + /// + private string Substitute(string metadataName, string escapedValue) + { + if (escapedValue.IndexOf("%(", StringComparison.Ordinal) < 0 || _locallySetMetadata?.Contains(metadataName) == true) { - return metadataValue; + return escapedValue; } _customEscapedMetadata.TryGetValue(ItemSpecModifiers.RecursiveDir, out string escapedRecursiveDir); - return BuiltInMetadataExpander.Expand(metadataValue, _escapedItemSpec, _escapedDefiningProject, escapedRecursiveDir, ref _cachedModifiers); + return BuiltInMetadataExpander.Expand(escapedValue, _escapedItemSpec, _escapedDefiningProject, escapedRecursiveDir, ref _cachedModifiers); + } + + /// + /// Indicates whether any stored value may contain a built-in metadata reference awaiting substitution. + /// + private bool HasAnyExpandableExpressions() + { + if (_customEscapedMetadata is not null) + { + foreach (KeyValuePair metadatum in _customEscapedMetadata) + { + if (metadatum.Value.IndexOf("%(", StringComparison.Ordinal) >= 0) + { + return true; + } + } + } + + return false; } /// diff --git a/src/Shared/UnitTests/TaskParameter_Tests.cs b/src/Shared/UnitTests/TaskParameter_Tests.cs index d7905e704fc..c7274a98791 100644 --- a/src/Shared/UnitTests/TaskParameter_Tests.cs +++ b/src/Shared/UnitTests/TaskParameter_Tests.cs @@ -711,6 +711,67 @@ public void RecursiveDirReferenceIsExpandedAcrossTaskHostBoundary() RoundTrip(item).GetMetadata("NameMeta").ShouldBe(expected); } + /// + /// A task that clones its input, whether through CopyMetadataTo or the copy constructor that calls + /// it, must get the same values the engine item hands out. The destination has no notion of a value + /// awaiting substitution, so the copy has to be made with finished values. + /// + [Fact] + public void CopyingMetadataSubstitutesReferencesAcrossTaskHostBoundary() + { + ProjectItemInstanceTaskItem item = CreateItemWithDefinitionMetadata("NameMeta", "%(Filename)"); + ITaskItem marshalled = RoundTrip(item); + + new TaskItem(item).GetMetadata("NameMeta").ShouldBe("hello"); + new TaskItem(marshalled).GetMetadata("NameMeta").ShouldBe("hello"); + + TaskItem viaCopyFromEngineItem = new("dest"); + TaskItem viaCopyFromMarshalledItem = new("dest"); + + item.CopyMetadataTo(viaCopyFromEngineItem); + marshalled.CopyMetadataTo(viaCopyFromMarshalledItem); + + viaCopyFromEngineItem.GetMetadata("NameMeta").ShouldBe("hello"); + viaCopyFromMarshalledItem.GetMetadata("NameMeta").ShouldBe("hello"); + } + + /// + /// A value the task wrote is literal, so cloning has to carry it across as written. + /// + [Fact] + public void MetadataSetByTheTaskIsCopiedLiterally() + { + ProjectItemInstanceTaskItem item = CreateItemWithDefinitionMetadata("NameMeta", "%(Filename)"); + ITaskItem marshalled = RoundTrip(item); + + item.SetMetadata("Local", "%(Filename)"); + marshalled.SetMetadata("Local", "%(Filename)"); + + new TaskItem(marshalled).GetMetadata("Local").ShouldBe(new TaskItem(item).GetMetadata("Local")); + } + + /// + /// Metadata derived from the item's path is cached, so reassigning ItemSpec has to invalidate it. Otherwise + /// a value read before the move leaks into one read after it. + /// + [Fact] + public void ReassigningItemSpecInvalidatesCachedPathMetadataAcrossTaskHostBoundary() + { + ProjectItemInstanceTaskItem item = CreateItemWithDefinitionMetadata("PathMeta", "%(Directory)"); + ITaskItem marshalled = RoundTrip(item); + + // Read first so that anything derived from the original path is cached. + item.GetMetadata("FullPath").ShouldBe(marshalled.GetMetadata("FullPath")); + + string renamed = $"other{Path.DirectorySeparatorChar}renamed.txt"; + item.ItemSpec = renamed; + marshalled.ItemSpec = renamed; + + marshalled.GetMetadata("FullPath").ShouldBe(item.GetMetadata("FullPath")); + marshalled.GetMetadata("Directory").ShouldBe(item.GetMetadata("Directory")); + marshalled.GetMetadata("PathMeta").ShouldBe(item.GetMetadata("PathMeta")); + } + private static ProjectItemInstanceTaskItem CreateItemWithDefinitionMetadata(string name, string escapedValue) { string itemSpec = $"folder{Path.DirectorySeparatorChar}hello.txt"; From b3ac4d61126d96085ae9db19d75b853917c0081c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Mon, 24 Aug 2026 13:15:48 +0200 Subject: [PATCH 3/7] Name the concepts this relies on, and guard them with tests The marshalled item recovers something the boundary erased, but nothing said so. Name it: a value has an origin, either set directly on the item or inherited from an item definition, and only the latter is expanded on read. Say at the type level that this is a flattened view of an engine item, how origin is recovered, and which accessors expand. Rename to match, and use one word for one idea. Expansion is what MSBuild calls this, so drop "substitute" as a synonym: _locallySetMetadata becomes _writtenByTask, Substitute becomes ExpandIfFromItemDefinition, and the repeated inline checks for a remaining "%(" become IsUnexpanded, which is what actually distinguishes the two origins. Add tests that hold the concepts still: every name in ItemSpecModifiers.All reads the same on both sides, so a modifier added later is covered without anyone remembering to; qualified references stay a decision rather than an accident; and receiving an item does not mark its metadata as written by the task. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6 --- .../ItemDefinitionMetadataInTaskHost_Tests.cs | 6 +- src/Framework/BuiltInMetadataExpander.cs | 31 +++-- src/Shared/TaskParameter.cs | 76 +++++++------ src/Shared/UnitTests/TaskParameter_Tests.cs | 106 ++++++++++++++++-- 4 files changed, 157 insertions(+), 62 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/ItemDefinitionMetadataInTaskHost_Tests.cs b/src/Build.UnitTests/BackEnd/ItemDefinitionMetadataInTaskHost_Tests.cs index e966a7d6735..4563951e327 100644 --- a/src/Build.UnitTests/BackEnd/ItemDefinitionMetadataInTaskHost_Tests.cs +++ b/src/Build.UnitTests/BackEnd/ItemDefinitionMetadataInTaskHost_Tests.cs @@ -14,9 +14,9 @@ namespace Microsoft.Build.Engine.UnitTests.BackEnd { /// - /// Item definition metadata referencing built-in metadata, such as %(Filename), is stored unexpanded - /// and substituted when the metadata is read. These tests assert that a task observes the same value whether - /// it runs in-proc or in a task host. + /// Metadata inherited from an item definition, such as %(Filename), is stored unexpanded and expanded + /// when it is read. These tests assert that a task observes the same value whether it runs in-proc or in a + /// task host. /// Regression tests for https://github.com/dotnet/msbuild/issues/14763. /// public sealed class ItemDefinitionMetadataInTaskHost_Tests diff --git a/src/Framework/BuiltInMetadataExpander.cs b/src/Framework/BuiltInMetadataExpander.cs index daeeefef104..cbf1308b58d 100644 --- a/src/Framework/BuiltInMetadataExpander.cs +++ b/src/Framework/BuiltInMetadataExpander.cs @@ -7,26 +7,23 @@ namespace Microsoft.Build.Framework; /// -/// Substitutes built-in metadata references, such as %(Filename), with the values they denote for a -/// given item. +/// Expands built-in metadata references, such as %(Filename), against an item. /// /// -/// -/// Item definition metadata may reference built-in metadata. Such values are stored unexpanded so that they -/// follow the item they are read from, and are substituted whenever the metadata is read. -/// -/// -/// This is a deliberately minimal substitute for the evaluation expander, which also handles properties, item -/// vectors, custom metadata, transforms and truncation. Built-in metadata references are the only expression -/// form that survives evaluation unexpanded. It lives here because the evaluation expander is internal to -/// Microsoft.Build, while this is needed by MSBuild and Microsoft.Build.Tasks as well. -/// +/// A deliberately minimal stand-in for the evaluation expander, which also handles properties, item vectors, +/// custom metadata, transforms and truncation. Built-in metadata references are the only expression form that +/// survives evaluation unexpanded, so they are the only one that can reach a task host still unexpanded. This +/// lives in Framework because the evaluation expander is internal to Microsoft.Build, while this is needed +/// by MSBuild and Microsoft.Build.Tasks too. +/// +/// Keep in step with Expander.ExpandIntoStringLeaveEscaped under ExpanderOptions.ExpandBuiltInMetadata, +/// which is what ProjectItemInstance.TaskItem.GetMetadataEscaped uses for the same job in-proc. /// internal static class BuiltInMetadataExpander { /// /// Expands every built-in metadata reference in against the given item. - /// Anything else, including a reference that cannot be satisfied, is left untouched. + /// Anything else, including a reference that cannot be satisfied, is left as it is. /// /// The escaped value to expand. /// The escaped item spec that built-in metadata is derived from. @@ -35,7 +32,7 @@ internal static class BuiltInMetadataExpander /// The item's RecursiveDir, which comes from the wildcard the item was expanded from rather than the item spec. /// /// Cache of already derived modifier values for this item. - /// The value with built-in metadata references substituted. + /// The value with built-in metadata references expanded. internal static string? Expand( string? escapedValue, string escapedItemSpec, @@ -93,9 +90,9 @@ internal static class BuiltInMetadataExpander /// /// Reads the metadata name between %( and its closing parenthesis and resolves it to a built-in - /// metadata kind, tolerating surrounding whitespace as the evaluation expander does. A name qualified by an - /// item type is rejected, since the engine resolves built-in metadata against an untyped table and so never - /// satisfies one either. + /// metadata kind, allowing surrounding whitespace as the evaluation expander does. A name qualified by an item + /// type is rejected, since the engine resolves built-in metadata against a table with no item type and so + /// never satisfies one either. /// private static bool TryGetModifier(string value, int start, int end, out ItemSpecModifierKind kind) { diff --git a/src/Shared/TaskParameter.cs b/src/Shared/TaskParameter.cs index 067fc0c4466..2778877a79b 100644 --- a/src/Shared/TaskParameter.cs +++ b/src/Shared/TaskParameter.cs @@ -546,6 +546,16 @@ private void TranslateValueTypeArray(ITranslator translator) /// /// Super simple ITaskItem derivative that we can use as a container for read items. /// + /// + /// This is a flattened view of an engine item. An engine item keeps metadata set directly on the item apart + /// from metadata inherited from an item definition, and expands only the latter on read, so that a value such + /// as %(Filename) follows the item it is read from. Both kinds arrive here in one dictionary, so that + /// origin is recovered instead: a value that evaluation already expanded has no %( left in it, and a + /// value the task itself writes is recorded as it is written. + /// + /// Reads that hand a value to a task expand; reads that hand back the whole collection do not, so a value + /// returns to the engine as it left. Keep any new accessor on the side of the one it resembles. + /// private class TaskParameterTaskItem : #if FEATURE_APPDOMAIN MarshalByRefObject, @@ -576,10 +586,9 @@ private class TaskParameterTaskItem : private ItemSpecModifiers.Cache _cachedModifiers; /// - /// Names of metadata written on this item after it was received. Their values are literal, so they are - /// never expanded on read. + /// Names of metadata the task wrote on this item. Their values are read as stored. /// - private HashSet _locallySetMetadata = null; + private HashSet _writtenByTask = null; /// /// Constructor for serialization @@ -624,12 +633,11 @@ internal TaskParameterTaskItem(ITaskItem copyFrom) } } - // RecursiveDir is a built-in metadata that cannot be derived from the item spec alone - - // it requires the original wildcard pattern (_includeBeforeWildcardExpansionEscaped). - // When crossing process boundaries (e.g., to TaskHost in -mt mode), built-in metadata - // is not included in CloneCustomMetadataEscaped(). Explicitly preserve RecursiveDir - // as custom metadata so it survives serialization. - // See https://github.com/dotnet/msbuild/issues/13140 + // RecursiveDir cannot be derived from the item spec, only from the wildcard the item was expanded + // from, and CloneCustomMetadataEscaped() does not return built-in metadata. Carry it over explicitly + // so it survives the boundary. See https://github.com/dotnet/msbuild/issues/13140. + // Written straight to the dictionary rather than through SetMetadata: this is the item being built, + // not a task writing to it, and the value is already expanded. if (copyFrom is ITaskItem2 copyFromForRecursiveDir) { string recursiveDirEscaped = copyFromForRecursiveDir.GetMetadataValueEscaped(ItemSpecModifiers.RecursiveDir); @@ -765,11 +773,11 @@ public void SetMetadata(string metadataName, string metadataValue) _customEscapedMetadata[metadataName] = metadataValue ?? String.Empty; - // Only a value that would otherwise be substituted on read has to be remembered as literal. - if (metadataValue?.IndexOf("%(", StringComparison.Ordinal) >= 0) + // Only a value that would otherwise be expanded on read has to be remembered. + if (IsUnexpanded(metadataValue)) { - _locallySetMetadata ??= new HashSet(MSBuildNameIgnoreCaseComparer.Default); - _locallySetMetadata.Add(metadataName); + _writtenByTask ??= new HashSet(MSBuildNameIgnoreCaseComparer.Default); + _writtenByTask.Add(metadataName); } } @@ -788,7 +796,7 @@ public void RemoveMetadata(string metadataName) } _customEscapedMetadata.Remove(metadataName); - _locallySetMetadata?.Remove(metadataName); + _writtenByTask?.Remove(metadataName); } /// @@ -815,12 +823,12 @@ public void CopyMetadataTo(ITaskItem destinationItem) IEnumerable> metadataToImport = _customEscapedMetadata .Where(metadatum => string.IsNullOrEmpty(destinationItem.GetMetadata(metadatum.Key))); - // The destination has no notion of a value awaiting substitution, so hand it finished values, - // as an engine item does when copying onto an item a task can reach. - if (HasAnyExpandableExpressions()) + // The destination has no notion of an unexpanded value, so hand it expanded ones, as an engine + // item does when copying onto an item a task can reach. + if (HasUnexpandedMetadata()) { metadataToImport = metadataToImport - .Select(metadatum => new KeyValuePair(metadatum.Key, Substitute(metadatum.Key, metadatum.Value))); + .Select(metadatum => new KeyValuePair(metadatum.Key, ExpandIfFromItemDefinition(metadatum.Key, metadatum.Value))); } #if FEATURE_APPDOMAIN @@ -841,7 +849,7 @@ public void CopyMetadataTo(ITaskItem destinationItem) if (String.IsNullOrEmpty(value)) { - destinationItem.SetMetadata(entry.Key, Substitute(entry.Key, entry.Value)); + destinationItem.SetMetadata(entry.Key, ExpandIfFromItemDefinition(entry.Key, entry.Value)); } } } @@ -911,40 +919,44 @@ string ITaskItem2.GetMetadataValueEscaped(string metadataName) return string.Empty; } - // Item definition metadata referencing built-in metadata is stored unexpanded and substituted on read - // so that it tracks the item it is read from. Definition and directly set metadata arrive here - // flattened into a single dictionary, so the distinction is instead carried by the value itself: - // anything expanded during evaluation has no reference left to substitute. Metadata a task sets is - // literal, so it is excluded. - return Substitute(metadataName, metadataValue); + return ExpandIfFromItemDefinition(metadataName, metadataValue); } /// - /// Substitutes any built-in metadata references in a stored metadata value, unless the value was - /// written by the task and is therefore literal. + /// Expands a stored value if it came from an item definition, and returns it as stored otherwise. /// - private string Substitute(string metadataName, string escapedValue) + private string ExpandIfFromItemDefinition(string metadataName, string escapedValue) { - if (escapedValue.IndexOf("%(", StringComparison.Ordinal) < 0 || _locallySetMetadata?.Contains(metadataName) == true) + if (!IsUnexpanded(escapedValue) || _writtenByTask?.Contains(metadataName) == true) { return escapedValue; } + // RecursiveDir comes from the wildcard the item was expanded from, not from the item spec, so it is + // read from the item's own metadata rather than derived. _customEscapedMetadata.TryGetValue(ItemSpecModifiers.RecursiveDir, out string escapedRecursiveDir); return BuiltInMetadataExpander.Expand(escapedValue, _escapedItemSpec, _escapedDefiningProject, escapedRecursiveDir, ref _cachedModifiers); } /// - /// Indicates whether any stored value may contain a built-in metadata reference awaiting substitution. + /// Indicates whether a value still holds a built-in metadata reference, and so was never expanded. + /// Evaluation expands every other expression form, so this is what distinguishes a value inherited from + /// an item definition from one set directly on the item. + /// + private static bool IsUnexpanded(string escapedValue) + => escapedValue?.IndexOf("%(", StringComparison.Ordinal) >= 0; + + /// + /// Indicates whether any stored value is still unexpanded. /// - private bool HasAnyExpandableExpressions() + private bool HasUnexpandedMetadata() { if (_customEscapedMetadata is not null) { foreach (KeyValuePair metadatum in _customEscapedMetadata) { - if (metadatum.Value.IndexOf("%(", StringComparison.Ordinal) >= 0) + if (IsUnexpanded(metadatum.Value)) { return true; } diff --git a/src/Shared/UnitTests/TaskParameter_Tests.cs b/src/Shared/UnitTests/TaskParameter_Tests.cs index c7274a98791..b9353eb48fd 100644 --- a/src/Shared/UnitTests/TaskParameter_Tests.cs +++ b/src/Shared/UnitTests/TaskParameter_Tests.cs @@ -608,8 +608,8 @@ public void ITaskItemParameter_EscapableNotEscapedMetadata() /// /// Regression test for https://github.com/dotnet/msbuild/issues/14763 - /// Item definition metadata referencing built-in metadata is stored unexpanded and substituted on read, - /// so a task must observe the same value whether or not the item crossed the TaskHost boundary. + /// Metadata inherited from an item definition is stored unexpanded and expanded on read, so a task must + /// observe the same value whether or not the item crossed the TaskHost boundary. /// [Theory] [InlineData("%(Filename)", "hello")] @@ -618,7 +618,7 @@ public void ITaskItemParameter_EscapableNotEscapedMetadata() [InlineData("pre-%(Filename)-post", "pre-hello-post")] [InlineData("%(Filename)%(Extension)", "hello.txt")] [InlineData("no expression", "no expression")] - // An escaped reference is literal text rather than an expression. + // An escaped reference is text, not an expression. [InlineData("%25(Filename)", "%(Filename)")] // Malformed references are not expressions either. [InlineData("%(Filename", "%(Filename")] @@ -634,7 +634,7 @@ public void ItemDefinitionMetadataReadsTheSameAcrossTaskHostBoundary(string defi /// /// A task that reassigns ItemSpec sees item definition metadata re-derived from the new spec. That must - /// hold on both sides of the TaskHost boundary, so the value cannot simply be substituted up front. + /// hold on both sides of the TaskHost boundary, so the value cannot simply be expanded up front. /// [Fact] public void ItemDefinitionMetadataFollowsReassignedItemSpecAcrossTaskHostBoundary() @@ -652,10 +652,10 @@ public void ItemDefinitionMetadataFollowsReassignedItemSpecAcrossTaskHostBoundar } /// - /// A value a task writes on the item is literal, matching what the same task would read back in-proc. + /// A value the task writes is read back as stored, matching what the same task would see in-proc. /// [Fact] - public void MetadataSetByTheTaskIsNotExpanded() + public void MetadataWrittenByTheTaskIsNotExpanded() { ITaskItem marshalled = RoundTrip(CreateItemWithDefinitionMetadata("NameMeta", "%(Filename)")); @@ -714,10 +714,10 @@ public void RecursiveDirReferenceIsExpandedAcrossTaskHostBoundary() /// /// A task that clones its input, whether through CopyMetadataTo or the copy constructor that calls /// it, must get the same values the engine item hands out. The destination has no notion of a value - /// awaiting substitution, so the copy has to be made with finished values. + /// unexpanded value, so the copy has to be made with expanded ones. /// [Fact] - public void CopyingMetadataSubstitutesReferencesAcrossTaskHostBoundary() + public void CopyingMetadataExpandsReferencesAcrossTaskHostBoundary() { ProjectItemInstanceTaskItem item = CreateItemWithDefinitionMetadata("NameMeta", "%(Filename)"); ITaskItem marshalled = RoundTrip(item); @@ -736,10 +736,10 @@ public void CopyingMetadataSubstitutesReferencesAcrossTaskHostBoundary() } /// - /// A value the task wrote is literal, so cloning has to carry it across as written. + /// A value the task wrote is read as stored, so cloning has to carry it across as written. /// [Fact] - public void MetadataSetByTheTaskIsCopiedLiterally() + public void MetadataWrittenByTheTaskIsCopiedAsStored() { ProjectItemInstanceTaskItem item = CreateItemWithDefinitionMetadata("NameMeta", "%(Filename)"); ITaskItem marshalled = RoundTrip(item); @@ -772,6 +772,92 @@ public void ReassigningItemSpecInvalidatesCachedPathMetadataAcrossTaskHostBounda marshalled.GetMetadata("PathMeta").ShouldBe(item.GetMetadata("PathMeta")); } + /// + /// The expansion here is a deliberately minimal stand-in for the evaluation expander, so it has to agree + /// with it for every built-in metadata name, including any added later. Driving the theory from + /// rather than a fixed list is what keeps that true over time. + /// + [Theory] + [MemberData(nameof(AllItemSpecModifiers))] + public void EveryBuiltInMetadataReferenceReadsTheSameAcrossTaskHostBoundary(string modifier) + { + // A wildcard origin so that RecursiveDir is a real value rather than empty on both sides. + string sep = Path.DirectorySeparatorChar.ToString(); + + ProjectItemDefinitionInstance definition = new( + "Thing", + ImmutableDictionaryExtensions.EmptyMetadata.SetItem("NameMeta", $"[%({modifier})]")); + + ProjectItemInstanceTaskItem item = new( + includeEscaped: $"tree{sep}sub1{sep}sub2{sep}hello.txt", + includeBeforeWildcardExpansionEscaped: $"tree{sep}**{sep}*.txt", + directMetadata: null, + itemDefinitions: [definition], + projectDirectory: Directory.GetCurrentDirectory(), + immutable: false, + definingFileEscaped: "test.proj"); + + string expected = item.GetMetadata("NameMeta"); + + // The time-based modifiers read the file system and are empty for an item that does not exist. They go + // through the same shared call as the rest, so equality is still worth asserting, but only the others + // can be required to resolve to something. + if (!modifier.EndsWith("Time", StringComparison.Ordinal)) + { + expected.ShouldNotBe("[]", $"%({modifier}) should resolve to something for this test to mean anything"); + } + + RoundTrip(item).GetMetadata("NameMeta").ShouldBe(expected, $"for %({modifier})"); + } + + public static TheoryData AllItemSpecModifiers + { + get + { + TheoryData data = []; + + foreach (string modifier in ItemSpecModifiers.All) + { + data.Add(modifier); + } + + return data; + } + } + + /// + /// A reference qualified by an item type is left as written. The engine resolves built-in metadata against + /// a table with no item type, so a qualified reference never matches and evaluates to an empty string; + /// expanding to empty here instead would mean blanking any text of that shape. This pins the difference so + /// that it stays a decision rather than an accident. + /// + [Theory] + [InlineData("%(Thing.Filename)")] + [InlineData("%( Thing.Filename )")] + [InlineData("%(Other.Filename)")] + public void QualifiedMetadataReferenceIsLeftAsWritten(string definitionValue) + { + ProjectItemInstanceTaskItem item = CreateItemWithDefinitionMetadata("NameMeta", definitionValue); + + item.GetMetadata("NameMeta").ShouldBe(""); + RoundTrip(item).GetMetadata("NameMeta").ShouldBe(definitionValue); + } + + /// + /// Only values the task writes are read as stored. Nothing on the receiving path may record metadata that + /// way, or item definition values would stop being expanded. + /// + [Fact] + public void ReceivingAnItemDoesNotMarkItsMetadataAsWrittenByTheTask() + { + ITaskItem marshalled = RoundTrip(CreateItemWithDefinitionMetadata("NameMeta", "%(Filename)")); + + marshalled.GetMetadata("NameMeta").ShouldBe("hello"); + + // A second hop must not change that either. + RoundTrip(marshalled).GetMetadata("NameMeta").ShouldBe("hello"); + } + private static ProjectItemInstanceTaskItem CreateItemWithDefinitionMetadata(string name, string escapedValue) { string itemSpec = $"folder{Path.DirectorySeparatorChar}hello.txt"; From 9312f25034ffea0c1e8eeb2c9f437d09daf62d3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Mon, 24 Aug 2026 14:15:50 +0200 Subject: [PATCH 4/7] Unit test the expander, and correct its scan for a nested reference The expander had no tests of its own. It was covered only where TaskParameter happened to exercise it, which left its own edge cases unchecked. Add direct tests: whitespace and casing, text that is not a well formed reference, several references in one value, a supplied RecursiveDir, derivation from the given item spec, and no allocation when there is nothing to expand. One test failed. After a "%(" that does not start a reference, the expander resumed after the closing parenthesis, so it did not see a well formed reference that began inside the text it had spanned. "%(foo%(Filename)" stayed as it was, where evaluation gives "%(foohello". Resume just after the "%(", which is what the evaluation expander does. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6 --- .../BuiltInMetadataExpander_Tests.cs | 131 ++++++++++++++++++ src/Framework/BuiltInMetadataExpander.cs | 12 +- 2 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 src/Framework.UnitTests/BuiltInMetadataExpander_Tests.cs diff --git a/src/Framework.UnitTests/BuiltInMetadataExpander_Tests.cs b/src/Framework.UnitTests/BuiltInMetadataExpander_Tests.cs new file mode 100644 index 00000000000..c2ff610bc35 --- /dev/null +++ b/src/Framework.UnitTests/BuiltInMetadataExpander_Tests.cs @@ -0,0 +1,131 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.IO; +using Microsoft.Build.Framework; +using Shouldly; +using Xunit; + +#nullable enable + +namespace Microsoft.Build.UnitTests +{ + /// + /// Tests for , which expands references such as %(Filename) when + /// an item is read after it crossed a process boundary. + /// + public class BuiltInMetadataExpander_Tests + { + private static readonly string s_sep = Path.DirectorySeparatorChar.ToString(); + private static readonly string s_itemSpec = $"folder{Path.DirectorySeparatorChar}hello.txt"; + + private static string? Expand(string? value, string? recursiveDir = null, string? itemSpec = null) + { + ItemSpecModifiers.Cache cache = default; + + return BuiltInMetadataExpander.Expand( + value, + itemSpec ?? s_itemSpec, + escapedDefiningProject: "project.proj", + escapedRecursiveDir: recursiveDir, + ref cache); + } + + [Theory] + [InlineData("%(Filename)", "hello")] + [InlineData("%(Extension)", ".txt")] + [InlineData("%(Identity)", @"folder\hello.txt")] + [InlineData("a%(Filename)b", "ahellob")] + [InlineData("%(Filename)%(Extension)", "hello.txt")] + [InlineData("%(Filename)%(Filename)", "hellohello")] + [InlineData("%(Filename)trailing", "hellotrailing")] + [InlineData("leading%(Filename)", "leadinghello")] + public void ExpandsBuiltInMetadata(string value, string expected) + => Expand(value).ShouldBe(expected.Replace(@"\", s_sep)); + + [Theory] + [InlineData("%( Filename )", "hello")] + [InlineData("%( Filename )", "hello")] + [InlineData("%(FILENAME)", "hello")] + [InlineData("%(filename)", "hello")] + public void AcceptsWhitespaceAndAnyCasing(string value, string expected) + => Expand(value).ShouldBe(expected); + + [Theory] + // No reference at all. + [InlineData("")] + [InlineData("plain text")] + [InlineData("100% done")] + // Not a well formed reference. + [InlineData("%(Filename")] + [InlineData("a%(")] + [InlineData("%()")] + [InlineData("%( )")] + [InlineData("%((Filename)")] + [InlineData("%(Fi lename)")] + // A name that is not built-in metadata. Evaluation expands custom metadata, so a value that reaches + // this point with one left in it is text. + [InlineData("%(NotAModifier)")] + public void LeavesEverythingElseAsItIs(string value) + => Expand(value).ShouldBe(value); + + [Fact] + public void ReturnsNullForNull() + => Expand(null).ShouldBeNull(); + + /// + /// A value with nothing to expand must come back as the same instance, not a copy. Metadata is read on + /// hot paths, and most values have no reference in them. + /// + [Fact] + public void DoesNotAllocateWhenThereIsNothingToExpand() + { + string value = "no reference here"; + + Expand(value).ShouldBeSameAs(value); + Expand("%(NotAModifier)").ShouldBeSameAs("%(NotAModifier)"); + } + + /// + /// RecursiveDir comes from the wildcard the item was expanded from, so it is supplied rather than derived. + /// + [Theory] + [InlineData(@"sub1\sub2\", @"out\sub1\sub2\hello.txt")] + [InlineData("", @"out\hello.txt")] + [InlineData(null, @"out\hello.txt")] + public void UsesTheSuppliedRecursiveDir(string? recursiveDir, string expected) + => Expand(@"out\%(RecursiveDir)%(Filename)%(Extension)".Replace(@"\", s_sep), recursiveDir?.Replace(@"\", s_sep)) + .ShouldBe(expected.Replace(@"\", s_sep)); + + /// + /// The expander derives from the item spec it is given, so the same value gives a different result for a + /// different item. This is why metadata is expanded on each read instead of one time. + /// + [Fact] + public void DerivesFromTheGivenItemSpec() + { + Expand("%(Filename)", itemSpec: $"other{s_sep}renamed.md").ShouldBe("renamed"); + Expand("%(Extension)", itemSpec: $"other{s_sep}renamed.md").ShouldBe(".md"); + } + + /// + /// A reference that is not the first thing in the value must still be found after an earlier reference + /// failed to parse. + /// + [Theory] + [InlineData("%(NotAModifier)%(Filename)", "%(NotAModifier)hello")] + [InlineData("%(Filename)%(NotAModifier)", "hello%(NotAModifier)")] + [InlineData("%(Fi lename)%(Filename)", "%(Fi lename)hello")] + public void FindsLaterReferencesAfterOneThatDoesNotResolve(string value, string expected) + => Expand(value).ShouldBe(expected); + + /// + /// An unterminated reference must not hide a well formed one that follows it inside the same text. + /// + [Theory] + [InlineData("%(foo%(Filename)", "%(foohello")] + [InlineData("%(%(Filename)", "%(hello")] + public void FindsAReferenceNestedInsideOneThatDoesNotResolve(string value, string expected) + => Expand(value).ShouldBe(expected); + } +} diff --git a/src/Framework/BuiltInMetadataExpander.cs b/src/Framework/BuiltInMetadataExpander.cs index cbf1308b58d..77bf5a9373d 100644 --- a/src/Framework/BuiltInMetadataExpander.cs +++ b/src/Framework/BuiltInMetadataExpander.cs @@ -54,6 +54,7 @@ internal static class BuiltInMetadataExpander { while (index >= 0) { + // No closing parenthesis anywhere after this point means no reference can close, so stop. int closingParenthesis = escapedValue!.IndexOf(')', index + 2); if (closingParenthesis < 0) @@ -69,9 +70,16 @@ internal static class BuiltInMetadataExpander ? escapedRecursiveDir ?? string.Empty : ItemSpecModifiers.GetItemSpecModifier(escapedItemSpec, kind, currentDirectory: null, escapedDefiningProject, ref cache)); copiedUpTo = closingParenthesis + 1; - } - index = escapedValue.IndexOf("%(", closingParenthesis + 1, StringComparison.Ordinal); + index = escapedValue.IndexOf("%(", copiedUpTo, StringComparison.Ordinal); + } + else + { + // This "%(" does not start a reference. Resume just after it rather than after the + // parenthesis, because a well formed reference can begin inside the text it spanned, + // as in "%(foo%(Filename)". The evaluation expander advances the same way. + index = escapedValue.IndexOf("%(", index + 2, StringComparison.Ordinal); + } } if (builder is null) From fe75e5514bb9063c9a16134c079a8cb4ed7cae31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Tue, 25 Aug 2026 13:06:54 +0200 Subject: [PATCH 5/7] Address review feedback Collapse the scan that guarded the copy. HasUnexpandedMetadata walked every value to decide whether to run a transform whose first act was the same check on the same string, so the work was done twice whenever anything needed expanding. The engine item can afford that guard because it bails out at once when the item has no item definitions, and it scans only the small shared definition metadata. The marshalled item has no such field, since that is the distinction the boundary erased, so the guard could only ever be a full scan. Chain the transform unconditionally instead: ExpandIfFromItemDefinition returns the value it was given when there is nothing to expand. Look for the metadata marker by searching for a single character. IndexOf(char) vectorizes and beats an ordinal two-character search, above all when the marker is absent, which is the usual case for a metadata value. This mirrors ExpressionShredder.IndexOfMarker, which is not available in every branch this has to reach. Also correct a test comment that described what an engine item does rather than what the test asserts, and a repeated word in another. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6 --- .../BuiltInMetadataExpander_Tests.cs | 26 +++++++++++++ src/Framework/BuiltInMetadataExpander.cs | 36 ++++++++++++++++-- src/Shared/TaskParameter.cs | 37 ++++--------------- src/Shared/UnitTests/TaskParameter_Tests.cs | 7 ++-- 4 files changed, 69 insertions(+), 37 deletions(-) diff --git a/src/Framework.UnitTests/BuiltInMetadataExpander_Tests.cs b/src/Framework.UnitTests/BuiltInMetadataExpander_Tests.cs index c2ff610bc35..9f639e7c601 100644 --- a/src/Framework.UnitTests/BuiltInMetadataExpander_Tests.cs +++ b/src/Framework.UnitTests/BuiltInMetadataExpander_Tests.cs @@ -127,5 +127,31 @@ public void FindsLaterReferencesAfterOneThatDoesNotResolve(string value, string [InlineData("%(%(Filename)", "%(hello")] public void FindsAReferenceNestedInsideOneThatDoesNotResolve(string value, string expected) => Expand(value).ShouldBe(expected); + + /// + /// The scan looks for '%' alone and tests the next character, because a single character search + /// vectorizes. These cases pin that it still agrees with searching for "%(" directly. + /// + [Theory] + [InlineData("", -1)] + [InlineData("%", -1)] + [InlineData("no marker here", -1)] + [InlineData("100% done", -1)] + [InlineData("50%", -1)] + [InlineData("%(", 0)] + [InlineData("a%(", 1)] + [InlineData("%x%(", 2)] + [InlineData("%%%(", 2)] + [InlineData("%a%b%(c", 4)] + [InlineData("trailing%", -1)] + public void FindsTheMetadataMarker(string value, int expected) + => BuiltInMetadataExpander.IndexOfMetadataMarker(value, 0).ShouldBe(expected); + + [Theory] + [InlineData("%(Filename)%(Extension)", 11, 11)] + [InlineData("%(Filename)plain", 11, -1)] + [InlineData("abc", 3, -1)] + public void FindsTheMetadataMarkerFromAStartIndex(string value, int startIndex, int expected) + => BuiltInMetadataExpander.IndexOfMetadataMarker(value, startIndex).ShouldBe(expected); } } diff --git a/src/Framework/BuiltInMetadataExpander.cs b/src/Framework/BuiltInMetadataExpander.cs index 77bf5a9373d..4d0eb720896 100644 --- a/src/Framework/BuiltInMetadataExpander.cs +++ b/src/Framework/BuiltInMetadataExpander.cs @@ -1,7 +1,6 @@ // 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 Microsoft.NET.StringTools; namespace Microsoft.Build.Framework; @@ -40,7 +39,7 @@ internal static class BuiltInMetadataExpander string? escapedRecursiveDir, ref ItemSpecModifiers.Cache cache) { - int index = escapedValue is null ? -1 : escapedValue.IndexOf("%(", StringComparison.Ordinal); + int index = escapedValue is null ? -1 : IndexOfMetadataMarker(escapedValue, 0); if (index < 0) { @@ -71,14 +70,14 @@ internal static class BuiltInMetadataExpander : ItemSpecModifiers.GetItemSpecModifier(escapedItemSpec, kind, currentDirectory: null, escapedDefiningProject, ref cache)); copiedUpTo = closingParenthesis + 1; - index = escapedValue.IndexOf("%(", copiedUpTo, StringComparison.Ordinal); + index = IndexOfMetadataMarker(escapedValue, copiedUpTo); } else { // This "%(" does not start a reference. Resume just after it rather than after the // parenthesis, because a well formed reference can begin inside the text it spanned, // as in "%(foo%(Filename)". The evaluation expander advances the same way. - index = escapedValue.IndexOf("%(", index + 2, StringComparison.Ordinal); + index = IndexOfMetadataMarker(escapedValue, index + 2); } } @@ -96,6 +95,35 @@ internal static class BuiltInMetadataExpander } } + /// + /// Finds the first %( at or after , or -1 if there is none. + /// Does not check that a well formed reference follows it. + /// + /// + /// IndexOf(char) vectorizes, and is significantly faster than an ordinal two-character search when + /// the marker is absent, which is the usual case for a metadata value. So look for % alone and test + /// the next character separately. + /// + internal static int IndexOfMetadataMarker(string value, int startIndex) + { + int markerIndex = value.IndexOf('%', startIndex); + + // A marker in the last position has no room for the parenthesis. + while (markerIndex >= 0 && markerIndex < value.Length - 1) + { + int nextIndex = markerIndex + 1; + + if (value[nextIndex] == '(') + { + return markerIndex; + } + + markerIndex = value.IndexOf('%', nextIndex); + } + + return -1; + } + /// /// Reads the metadata name between %( and its closing parenthesis and resolves it to a built-in /// metadata kind, allowing surrounding whitespace as the evaluation expander does. A name qualified by an item diff --git a/src/Shared/TaskParameter.cs b/src/Shared/TaskParameter.cs index 2778877a79b..079163c0f9a 100644 --- a/src/Shared/TaskParameter.cs +++ b/src/Shared/TaskParameter.cs @@ -586,7 +586,7 @@ private class TaskParameterTaskItem : private ItemSpecModifiers.Cache _cachedModifiers; /// - /// Names of metadata the task wrote on this item. Their values are read as stored. + /// Names of metadata the task wrote on this item. The values of these metadata are returned without expansion. /// private HashSet _writtenByTask = null; @@ -820,16 +820,12 @@ public void CopyMetadataTo(ITaskItem destinationItem) if (_customEscapedMetadata != null && destinationItem is IMetadataContainer destinationItemAsMetadataContainer) { // The destination implements IMetadataContainer so we can use the ImportMetadata bulk-set operation. - IEnumerable> metadataToImport = _customEscapedMetadata - .Where(metadatum => string.IsNullOrEmpty(destinationItem.GetMetadata(metadatum.Key))); - // The destination has no notion of an unexpanded value, so hand it expanded ones, as an engine - // item does when copying onto an item a task can reach. - if (HasUnexpandedMetadata()) - { - metadataToImport = metadataToImport - .Select(metadatum => new KeyValuePair(metadatum.Key, ExpandIfFromItemDefinition(metadatum.Key, metadatum.Value))); - } + // item does when copying onto an item a task can reach. ExpandIfFromItemDefinition returns the + // value it was given when there is nothing to expand, which is the usual case. + IEnumerable> metadataToImport = _customEscapedMetadata + .Where(metadatum => string.IsNullOrEmpty(destinationItem.GetMetadata(metadatum.Key))) + .Select(metadatum => new KeyValuePair(metadatum.Key, ExpandIfFromItemDefinition(metadatum.Key, metadatum.Value))); #if FEATURE_APPDOMAIN if (RemotingServices.IsTransparentProxy(destinationItem)) @@ -945,26 +941,7 @@ private string ExpandIfFromItemDefinition(string metadataName, string escapedVal /// an item definition from one set directly on the item. /// private static bool IsUnexpanded(string escapedValue) - => escapedValue?.IndexOf("%(", StringComparison.Ordinal) >= 0; - - /// - /// Indicates whether any stored value is still unexpanded. - /// - private bool HasUnexpandedMetadata() - { - if (_customEscapedMetadata is not null) - { - foreach (KeyValuePair metadatum in _customEscapedMetadata) - { - if (IsUnexpanded(metadatum.Value)) - { - return true; - } - } - } - - return false; - } + => escapedValue is not null && BuiltInMetadataExpander.IndexOfMetadataMarker(escapedValue, 0) >= 0; /// /// Sets the exact metadata value given to the metadata name requested. diff --git a/src/Shared/UnitTests/TaskParameter_Tests.cs b/src/Shared/UnitTests/TaskParameter_Tests.cs index b9353eb48fd..17567d2b853 100644 --- a/src/Shared/UnitTests/TaskParameter_Tests.cs +++ b/src/Shared/UnitTests/TaskParameter_Tests.cs @@ -665,7 +665,8 @@ public void MetadataWrittenByTheTaskIsNotExpanded() marshalled.GetMetadata("NameMeta").ShouldBe("%(Filename)"); marshalled.GetMetadata("Other").ShouldBe("%(Filename)"); - // Removing the task's value uncovers the item definition value again. + // The two origins were flattened into one dictionary when the item crossed the boundary, so removal + // leaves nothing behind. An engine item would uncover the item definition value here. marshalled.RemoveMetadata("NameMeta"); marshalled.GetMetadata("NameMeta").ShouldBe(""); } @@ -713,8 +714,8 @@ public void RecursiveDirReferenceIsExpandedAcrossTaskHostBoundary() /// /// A task that clones its input, whether through CopyMetadataTo or the copy constructor that calls - /// it, must get the same values the engine item hands out. The destination has no notion of a value - /// unexpanded value, so the copy has to be made with expanded ones. + /// it, must get the same values the engine item hands out. The destination has no notion of an unexpanded + /// value, so the copy has to be made with expanded ones. /// [Fact] public void CopyingMetadataExpandsReferencesAcrossTaskHostBoundary() From 3c551b98b940fb989ffb026282b499a953b0b97c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Tue, 25 Aug 2026 13:56:56 +0200 Subject: [PATCH 6/7] Update VersionPrefix to 18.10.1 --- eng/Versions.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eng/Versions.props b/eng/Versions.props index 53c9b9efe0f..cfab19f574f 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -3,7 +3,7 @@ - 18.10.0 + 18.10.1 1 $(PreReleaseVersionLabel)-test 18.9.0-preview-26330-01 From 2aa0df051adb295522f2258699c4affddc902dcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Provazn=C3=ADk?= Date: Wed, 26 Aug 2026 11:39:46 +0200 Subject: [PATCH 7/7] Insert into VS rel/stable by default 18.10 has snapped to stable, so the automatic insertion target is retargeted from rel/insiders to rel/stable. This is step 4.6 of the release checklist, and matches vs18.9. A servicing commit on this branch now reaches the branch it is servicing without anyone selecting the target by hand. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6 --- azure-pipelines/vs-insertion.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azure-pipelines/vs-insertion.yml b/azure-pipelines/vs-insertion.yml index c5978f4d00d..f2c78e9d3e3 100644 --- a/azure-pipelines/vs-insertion.yml +++ b/azure-pipelines/vs-insertion.yml @@ -61,7 +61,7 @@ variables: ${{ if not(eq(parameters.TargetBranch, 'auto')) }}: value: ${{ parameters.TargetBranch }} ${{ else }}: - value: 'rel/insiders' + value: 'rel/stable' - name: TeamName value: msbuild - name: TeamEmail