Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 98 additions & 0 deletions src/Compilers/Core/MSBuildTask/ArgsTaskItem.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections;
using System.Collections.Generic;
using Microsoft.Build.Framework;

namespace Microsoft.CodeAnalysis.BuildTasks
{
internal sealed class ArgsTaskItem : ITaskItem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment summarizing that we're using this to avoid unnecessary path escaping?

{
// This list is taken from https://github.com/dotnet/msbuild/blob/291a8108761ed347562228f2f8f25477996a5a93/src/Shared/Modifiers.cs#L36-L70
private static readonly string[] WellKnownItemSpecMetadataNames =
[
"FullPath",
"RootDir",
"Filename",
"Extension",
"RelativeDir",
"Directory",
"RecursiveDir",
"Identity",
"ModifiedTime",
"CreatedTime",
"AccessedTime",
"DefiningProjectFullPath",
"DefiningProjectDirectory",
"DefiningProjectName",
"DefiningProjectExtension",
];

private readonly Dictionary<string, string> _metadata = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);

public ArgsTaskItem(string itemSpec)
{
ItemSpec = itemSpec;
}

public string ItemSpec { get; set; }

// Implementation notes that we should include the built-in metadata names as well as our custom ones.
public ICollection MetadataNames
{
get
{
var clone = new List<string>(_metadata.Keys);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var clone = new List<string>(_metadata.Keys);
var clone = new List<string>(capacity: MetadataCount);
clone.AddRange(_metadata.Keys);

Avoid unnecessary allocations.

clone.AddRange(WellKnownItemSpecMetadataNames);
return clone;
}
}

// Implementation notes that we should include the built-in metadata names as well as our custom ones.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which implementation notes this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add links.

public int MetadataCount => _metadata.Count + WellKnownItemSpecMetadataNames.Length;

public IDictionary CloneCustomMetadata()
{
return new Dictionary<string, string>(_metadata, StringComparer.OrdinalIgnoreCase);
}

public void CopyMetadataTo(ITaskItem destinationItem)
{
// Implementation notes that we should not overwrite existing metadata on the destination.
var destinationMetadataNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
foreach (var name in destinationItem.MetadataNames)
{
destinationMetadataNames.Add((string)name);
}

foreach (var metadataName in _metadata.Keys)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we only using _metadata.Keys here and not MetadataNames? Seems like it's only copying our custom metadata not the full set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my reading of the interface comments. There seems to be a distinction made between "built-in" and "custom" metadata. The "built-in" metadata seems to be related to the ItemSpec which this method is not supposed to update.

{
if (destinationMetadataNames.Contains(metadataName))
{
continue;
}

var metadataValue = _metadata[metadataName];
destinationItem.SetMetadata(metadataName, metadataValue);
}
}

public string GetMetadata(string metadataName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rainersigwald, @baronfel can you help me understand the contract of ITaskItem a bit better? One part of this impl, and others I've seen, is that they have this pattern of exporting a set of names via MetadataNames that will throw when you pass them to GetMetadata. My mind sees that as a dictionary like type exporting a key but then throwing when you ask for the value. Why does ITaskItem behave this way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a specific instance we could look at? From what we can see TaskItem MetadataNames + GetMetadata should be safe for all of the returned names, which include explicit Item Metadata as well as the historically-significant set of metadata that all Items have (because all items are files, don'tchanknow).

Copy link
Member Author

@JoeRobich JoeRobich Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I misinterpreted the comments on the interface and the implementation here. MetadataNames includes the "built-in" metadata which I wasn't sure would actually be in my custom metadata dictionary.

{
return _metadata[metadataName];
}

public void RemoveMetadata(string metadataName)
{
_metadata.Remove(metadataName);
}

public void SetMetadata(string metadataName, string metadataValue)
{
_metadata[metadataName] = metadataValue;
}
}
}
2 changes: 1 addition & 1 deletion src/Compilers/Core/MSBuildTask/ManagedToolTask.cs
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ protected static ITaskItem[] GenerateCommandLineArgsTaskItems(List<string> comma
var items = new ITaskItem[commandLineArgs.Count];
for (var i = 0; i < commandLineArgs.Count; i++)
{
items[i] = new TaskItem(commandLineArgs[i]);
items[i] = new ArgsTaskItem(commandLineArgs[i]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change from us unconditionally escaping to unconditionally not escaping. Won't this regress scenarios where escaping is making builds work today? Consider for example that /keyfile: is passed via commandLineArgs. If that was using \ today it would be / on unix but after this change it woudl still be \ correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof. Luckily many args such as Analyzers, References, Source all come from TaskItems of their own which apply the path correction. So I need to look at args which come from properties and may contain file paths.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is the expectation that MSBuild project files might do things like Foo\Bar and this conversion is what makes things still work on non-Windows? That seems terrifying, but I guess project files using \ to make paths is very common...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had thought we were going to try some trick about cloning a custom implementation. Does something like this not work?

  • We create an ArgsTaskItem that implements ITaskItem2 just to return EvaluatedIncludeEscaped
  • We wrap then call new TaskItem() passing that task specifically to hit this line that looks like it'll bypass further escaping.

That way we're returning an existing implementation rather than returning our own which risks bugs in case we don't get the semantics of the interface right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, let's do that instead.

}

return items;
Expand Down
13 changes: 13 additions & 0 deletions src/Compilers/Core/MSBuildTaskTests/VbcTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,19 @@ public void DefineConstantsSimple()
test("D1 D2");
}

[Fact]
[WorkItem(72014, "https://github.com/dotnet/roslyn/issues/72014")]
public void DefineConstantsWithEscaping()
{
var vbc = new Vbc();
vbc.DefineConstants = "CONFIG=\"DEBUG\"";
vbc.Sources = MSBuildUtil.CreateTaskItems("test.vb");
var responseFileContents = vbc.GenerateResponseFileContents();
var argTaskItems = vbc.GenerateCommandLineArgsTaskItems(responseFileContents);
var defineTaskItem = Assert.Single(argTaskItems, item => item.ItemSpec.StartsWith("/define:"));
Assert.Equal("/define:\"CONFIG=\\\"DEBUG\\\"\"", defineTaskItem.ItemSpec);
}

[Fact]
public void Features()
{
Expand Down