Enlighten CreateItem task for multithreaded mode
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 141
Description
# Enlighten CreateItem task for multithreaded mode
Parent: #11834
## Context
`CreateItem` is one of the remaining tasks listed in the migration epic (#11834) under "Other (either simple or with unknown issues)". It forwards items from `Include` to its `[Output]` `Include`, optionally honoring `Exclude` and applying `AdditionalMetadata`. Two of its three logical paths are already cwd-independent — only one is not.
The cwd-dependence is concentrated in a single line inside `TryExpandWildcards`:
```csharp
// src/Tasks/CreateItem.cs:181
(files, _, _, string? globFailure) =
FileMatcher.Default.GetFiles(null /* use current directory */, i.ItemSpec);
```
`FileMatcher.GetFiles` accepts `string? projectDirectoryUnescaped` as its first parameter (`src/Framework/Utilities/FileMatcher.cs:1923`). Passing `null` causes wildcard expansion to be rooted at `Environment.CurrentDirectory`. Under multithreaded execution that is wrong — concurrent `CreateItem` instances may belong to different projects, and there is only one cwd per process. A relative pattern like `**\*.cs` could resolve against an arbitrary peer project's directory.
The other `FileMatcher` calls in this method are pure string analysis on the filespec and a candidate file path:
- `FileMatcher.Default.GetFileSpecInfo(i.ItemSpec, ...)` — parses the filespec only.
- `FileMatcher.IsDriveEnumeratingWildcardPattern(directoryPart, wildcardPart)` — pattern shape inspection.
- `FileMatcher.Default.FileMatch(i.ItemSpec, file)` — regex match of the filespec against the already-returned `file` string; does not touch the filesystem and does not consume cwd.
The non-wildcard branch (`expanded.Add(i)`), `CreateOutputItems`, `GetUniqueItems`, and `PropertyParser.GetTable` are all path-string-pure — no migration needed.
## Approach
Apply the **PR #13267 (MSBuild/CallTarget) pattern**: pass the project directory through at the boundary, leaving the helper untouched. The helper signature already supports it — there is nothing to refactor downstream.
1. Mark `CreateItem` with `[MSBuildMultiThreadableTask]` and implement `IMultiThreadableTask` with `public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback;` (built-in style).
2. In `TryExpandWildcards`, replace the `null` first argument to `FileMatcher.Default.GetFiles` with `(string)TaskEnvironment.ProjectDirectory`. `TaskEnvironment.ProjectDirectory` is already typed as `AbsolutePath` and is guaranteed canonical by the engine (PR #13267) — no extra `GetAbsolutePath`/`GetCanonicalForm` call is required.
3. No other changes to `CreateItem.cs`. The non-wildcard path, output construction, exclude handling, and metadata application remain byte-identical.
## ChangeWave consideration
For absolute and rooted wildcard patterns, `GetFiles` ignores the project directory entirely — output is byte-identical to today. For *relative* wildcard patterns the base directory changes from `Environment.CurrentDirectory` to `TaskEnvironment.ProjectDirectory`. In legacy (non-MT) execution the two are equal during normal builds, so observable behavior is unchanged. The semantic change only manifests under MT or in custom hosts that have intentionally diverged cwd from the project directory — which is the entire point of the migration.
There is one edge that *could* differ today: the strings in the returned `string[] files` are formatted relative to the `projectDirectoryUnescaped` argument when supplied, vs. relative to cwd when `null`. In the cwd == project-dir case the returned strings are character-for-character identical. If unforeseen behavioral diffs are found during review (e.g., a divergent shape on Linux with mixed separators), gate the diff behind a new ChangeWave following the precedent set in PR #13069 (which gated `FindUnderPath`/`AssignTargetPath` semantic diffs behind `Wave18_5`). **Recommendation: ship without a wave unless a test demonstrates a real diff** — `CreateItem`'s wildcard expansion is conceptually identical to evaluation-time globbing, which already passes the project directory through.
## Test coverage assessment
### Existing direct unit tests (`src/Tasks.UnitTests/CreateItem_Tests.cs`)
`OneFromOneIsZero`, `OneFromOneMismatchIsOne`, `UnspecifiedFromOneIsOne`, `OneFromUnspecifiedIsEmpty`, `UnspecifiedFromUnspecifiedIsEmpty`, `CaseDoesntMatter`, `WildcardsWithRecursiveDir`, `RecursiveDirOutOfProc`, `AdditionalMetaData`, `AdditionalMetaDataPreserveExisting`, `AdditionalMetaDataOverwriteExisting`, `WildcardDriveEnumerationTaskItemLogsError`, `LogWindowsWarningUponCreateItemExecution`, `LogUnixWarningUponCreateItemExecution`, `ThrowExceptionUponItemCreationWithDriveEnumeration`, `LogWindowsWarningUponItemCreationWithDriveEnumeration`, `LogUnixWarningUponItemCreationWithDriveEnumeration`.
Coverage of pre-expanded items, exclude semantics, metadata application, and drive-enumeration detection is good. Coverage of MT-specific concerns (project-relative wildcard expansion, concurrency across projects) is zero. `WildcardsWithRecursiveDir` and `RecursiveDirOutOfProc` exercise wildcard expansion but rely on the implicit `Environment.CurrentDirectory == project directory` assumption.
### Integration tests in this repo
`CreateItem` is invoked across `Microsoft.Common.CurrentVersion.targets` and friends for dynamic item generation. End-to-end coverage in this repo is incidental via `Build.UnitTests` scenarios; we will **not** add new E2E targets-flow tests as part of this issue.
### Gaps to fill in this PR
- **G1 — Concurrency test.** Two `CreateItem` instances with **different** `ProjectDirectory` values, both given the same relative wildcard (e.g. `**\*.cs`), assert each returns disjoint file sets rooted at the corresponding project dir. Use `TestEnvironment` to scaffold two separate transient project trees with distinct contents. (SKILL red-team Phase 4.)
- **G2 — Project-relative wildcard expansion.** `ProjectDirectory != Environment.CurrentDirectory`: relative wildcard input resolves against `ProjectDirectory`, not cwd. Documents the intentional semantic change. Use `TaskEnvironment.CreateWithProjectDirectoryAndEnvironment("")`.
- **G3 — Inject `TaskEnvironment` into existing tests.** Set `t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest()` in every existing test that constructs `CreateItem` directly so the wildcard tests (`WildcardsWithRecursiveDir`, `RecursiveDirOutOfProc`, drive-enumeration tests) keep their cwd-rooted behavior under the Fallback environment.
- **G4 — Baseline parity.** Pre-expanded literal items (no wildcards), exclude lists, and `AdditionalMetadata` produce byte-identical `[Output] Include` items, including `RecursiveDir` metadata and item ordering. Captures that the non-wildcard branch and `CreateOutputItems`/`GetUniqueItems` remain unchanged.
## Acceptance criteria
- [ ] `CreateItem` decorated `[MSBuildMultiThreadableTask]` and implements `IMultiThreadableTask` with `TaskEnvironment` defaulted to `TaskEnvironment.Fallback` (built-in style).
- [ ] `TryExpandWildcards` passes `(string)TaskEnvironment.ProjectDirectory` to `FileMatcher.Default.GetFiles` instead of `null`.
- [ ] No signature changes to `FileMatcher.GetFiles`, `GetFileSpecInfo`, `FileMatch`, or `IsDriveEnumeratingWildcardPattern` — they already accept the project directory or are pure string analysis.
- [ ] All existing `CreateItem_Tests` pass on `net472` and `net10.0` with no behavior changes for the non-wildcard, exclude, and metadata paths.
- [ ] `Include` `[Output]` items have byte-identical `ItemSpec` and `RecursiveDir` metadata for relative wildcard inputs when `Environment.CurrentDirectory == ProjectDirectory` (the legacy invariant).
- [ ] No `AbsolutePath` leak into `[Output] Include` `ItemSpec` strings or warning/error messages (SKILL Sin 1, Sin 2). Drive-enumeration warnings continue to embed `EscapingUtilities.UnescapeAll(i.ItemSpec)` — the user's original spec.
- [ ] Drive-enumerating wildcard detection (`WildcardResultsInDriveEnumeration`) produces identical warning/error codes and message arguments.
- [ ] Tests **G1**, **G2**, **G3**, **G4** added and pass.
- [ ] `.\build.cmd -v quiet` succeeds; `Tasks.UnitTests` and `Build.UnitTests` pass.
- [ ] No new compiler warnings (warnings-as-errors in official build).
- [ ] No `ChangeWave` required; if any test required modification beyond G3, document why in the PR and gate the diff behind the next wave per PR #13069's pattern.
- [ ] `multithreaded-task-migration` SKILL sign-off checklist walked and passes.
## Implementation order
1. Apply `[MSBuildMultiThreadableTask]` + `IMultiThreadableTask` to `CreateItem`; route `TryExpandWildcards` through `TaskEnvironment.ProjectDirectory`.
2. Update existing tests to inject `TaskEnvironment` (**G3**).
3. Add **G1** (concurrency), **G2** (project-relative wildcard), **G4** (baseline parity) tests.
4. Run full `Tasks.UnitTests` and `Build.UnitTests`; verify clean.
5. Run repo build with `-v quiet` to ensure no new warnings.
## Risks / open questions
- `GetUniqueItems` keys excludes by `item.ItemSpec` using `StringComparer.OrdinalIgnoreCase`. This is a path comparison performed against un-absolutized item-spec strings supplied by the user — it has the same Windows-vs-Linux case-sensitivity caveat that PR #13069 review flagged for `FindUnderPath`/`AssignTargetPath` (`FileUtilities.PathComparison` would be more correct on Linux). **Out of scope for this issue** — `Exclude` is matched by raw string today and changing it would alter observable behavior independently of the MT migration. Tracked as a follow-up.
- The `match.wildcardDirectoryPart` string returned by `FileMatcher.FileMatch` is computed by re-parsing the filespec, not by inspecting the project directory. Verify no implicit cwd dependence in `FileMatcher.FileMatch` during PR review (initial reading: pure regex match, no filesystem access).
- Custom hosts that intentionally point `Environment.CurrentDirectory` away from the project directory and rely on `CreateItem` resolving against cwd will see different output under MT mode. This is the intended semantic change.
## References
- Parent epic: #11834
- Pattern PRs:
- #13267 — Enlighten public versions of intrinsic tasks (absolutize-at-boundary pattern)
- #13069 — Enlighten FindUnderPath and AssignTargetPath (ChangeWave usage; conceptually similar wildcard/path-rooting concerns)
- #13045 — Enlighten more tasks that require no change (attribute-only baseline)
- #13282 — Enlighten GetFrameworkPath and GetFrameworkSdkPath (locking shared static state)
- #13319 — Enlighten RAR (heavyweight thread-TaskEnvironment-through-helpers pattern; for reference only)
- #13567 — Enlighten FormatUrl (sibling migration; same boundary-pattern shape)
- SKILL: `.github/skills/multithreaded-task-migration/SKILL.md`
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.