dotnet / dotnet/msbuild

Unify the built-in metadata expander with the evaluation expander

Open
#14,833 0 comments 0 reactions 0 assignees View on GitHub
Area: Engine backlog Priority:3 triaged
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.