Stop escaped compiler args from being mangled by MSBuild #76888
Conversation
The TaskItem implementation from MSBuild treats the ItemSpec as file path and tries to normalize the path separators. When running on non-windows machines this means changing '\' to '/'. However this breaks how double quotes are escaped in the compiler args. By using our own implemetation of TaskItem we can use the compiler args as the ItemSpec without any modification. Resolves #72014
ghost
added
Area-Compilers
untriaged
| for (var i = 0; i < commandLineArgs.Count; i++) | ||
| { | ||
| items[i] = new TaskItem(commandLineArgs[i]); | ||
| items[i] = new ArgsTaskItem(commandLineArgs[i]); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
| { | ||
| get | ||
| { | ||
| var clone = new List<string>(_metadata.Keys); |
There was a problem hiding this comment.
| var clone = new List<string>(_metadata.Keys); | |
| var clone = new List<string>(capacity: MetadataCount); | |
| clone.AddRange(_metadata.Keys); |
Avoid unnecessary allocations.
| } | ||
| } | ||
|
|
||
| // Implementation notes that we should include the built-in metadata names as well as our custom ones. |
There was a problem hiding this comment.
Which implementation notes this?
|
|
||
| namespace Microsoft.CodeAnalysis.BuildTasks | ||
| { | ||
| internal sealed class ArgsTaskItem : ITaskItem |
There was a problem hiding this comment.
Can we add a comment summarizing that we're using this to avoid unnecessary path escaping?
| destinationMetadataNames.Add((string)name); | ||
| } | ||
|
|
||
| foreach (var metadataName in _metadata.Keys) |
There was a problem hiding this comment.
Why are we only using _metadata.Keys here and not MetadataNames? Seems like it's only copying our custom metadata not the full set.
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| public string GetMetadata(string metadataName) |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| for (var i = 0; i < commandLineArgs.Count; i++) | ||
| { | ||
| items[i] = new TaskItem(commandLineArgs[i]); | ||
| items[i] = new ArgsTaskItem(commandLineArgs[i]); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yep, let's do that instead.
jaredpar
removed
the
untriaged
The TaskItem implementation from MSBuild treats the ItemSpec as file path and tries to normalize the path separators. When running on non-windows machines this means changing '\' to '/'. However this breaks how double quotes are escaped in the compiler args. By using our own implementation of TaskItem we can use the compiler args as the ItemSpec without any modification.
Resolves #72014