Unify the built-in metadata expander with the evaluation expander
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 141
Description
Follow-up to #14770, which fixed built-in metadata reaching a task host unexpanded. Two pieces of that fix were kept deliberately narrow so it could be serviced, and both should be revisited on `main`.
Raised in review by @rainersigwald ([unify the expanders](https://github.com/dotnet/msbuild/pull/14770#discussion_r3846725413), [marker search](https://github.com/dotnet/msbuild/pull/14770#discussion_r3845962709)) with input from @DustinCampbell.
## 1. Two expanders parse metadata references
#14770 added `src/Framework/BuiltInMetadataExpander.cs`. It expands `%(Filename)` and friends when an item is read after it crossed a process boundary. It is a second implementation of a grammar that `Expander.MetadataExpander` already implements.
It lives in Framework because `TaskParameter.cs` is compiled into `Microsoft.Build`, `MSBuild` and `Microsoft.Build.Tasks`, while `Expander` and `ExpressionShredder` are internal to `Microsoft.Build`.
The two already disagree in one known place, and the difference is deliberate and covered by a test:
| Value | `Expander` | `BuiltInMetadataExpander` |
| --- | --- | --- |
| `%(Thing.Filename)` | empty, because built-in metadata resolves against a table with no item type | left as written |
Expanding to empty would blank any text of that shape, which seemed worse than leaving it. That is a judgement call worth revisiting once there is one implementation.
Suggested direction: move the reference grammar into a single place both can use, most likely `src/Shared/`, and have `BuiltInMetadataExpander` parse with it. `ExpressionShredder.TryParseMetadataExpression` is already the grammar; its helpers (`SinkValidName`, `SinkWhitespace`, `Sink`) depend only on `XmlUtilities.IsValidInitialElementNameCharacter` and `IsValidSubsequentElementNameCharacter`, which are two character-range checks with no further dependencies.
This was prototyped during review and abandoned, because moving `ExpressionShredder` and `XmlUtilities` touches three project files and would have blocked a clean cherry-pick to `vs18.9` and `vs18.10`. On `main` that constraint does not apply.
## 2. Adopt `ExpressionShredder.ContainsMetadataMarker`
#14697 added a vectorized marker search:
```csharp
// IndexOf(char) is significantly faster than an ordinal two-character search,
// especially when the marker is absent, so check the opening parenthesis separately.
```
#14770 needed the same thing on a hot path, since every metadata read in a task host asks whether a value still contains `%(`. It could not call the new helper, because #14697 had not reached the servicing branches, so it carries its own copy as `BuiltInMetadataExpander.IndexOfMetadataMarker`. The two have the same shape on purpose.
Replacing the copy needs one thing resolved first. `ContainsMetadataMarker` is on `ExpressionShredder` in `src/Build/Evaluation/`, so it is only reachable from `Microsoft.Build`, while the callers are in `src/Shared/TaskParameter.cs` and `src/Framework/`. So this depends on the move in item 1, or on `ExpressionShredder` moving on its own.
## Scope
- One implementation of the metadata reference grammar, used by both expanders.
- `BuiltInMetadataExpander.IndexOfMetadataMarker` replaced by `ExpressionShredder.IndexOfMarker`.
- Decide whether `%(ItemType.Name)` should expand to empty in a task host, matching evaluation, and update the test that currently pins the opposite.
`main` only. #14770 has to stay serviceable for `vs18.9` and `vs18.10`.
## Tests that already cover this
`src/Framework.UnitTests/BuiltInMetadataExpander_Tests.cs` covers the grammar and the marker search, including whitespace, casing, malformed references such as `%(Fi lename)` and `%((Filename)`, a reference nested in one that does not parse (`%(foo%(Filename)`), and the item type case above. It should keep passing throughout, apart from the one test that pins the qualified-reference decision.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with src/Framework/BuiltInMetadataExpander.cs, src/Build/Evaluation/ExpressionShredder.cs, and the project references affected by moving shared parsing helpers. Read ExpressionShredder.TryParseMetadataExpression and ContainsMetadataMarker, then run src/Framework.UnitTests/BuiltInMetadataExpander_Tests.cs. Done means both expanders share the grammar and marker search, with the qualified-reference test updated after the behavior decision.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- build-system
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100