Refactoring: LazyItemOperation should mutate items for all the operations that do mutations
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 133
Description
Benefit: reduce the chances of introducing bugs by reducing clones
When is it worth doing: when new item operations are added which mutate existing items
Risk: perf
Whenever an item operation needs to mutate existing items (for now only Update), it first has to clone them. Otherwise the mutations leak into the item cache and weird behaviour starts happening, like future operations mutating the state of past operations.
Update does this in a [custom manner](https://github.com/Microsoft/msbuild/blob/master/src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs#L38-L41).
To enable all operations that mutate existing items to automatically benefit from cloning the following refactorings are needed:
- [LazyItemOperation.SelectItems](https://github.com/Microsoft/msbuild/blob/master/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs#L57) needs to disambiguate between newly added items and existing items (providing their index in the list builder)
- [LazyItemOperation.MutateItems](https://github.com/Microsoft/msbuild/blob/master/src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs#L62) (or maybe a new method that runs before MutateItems) needs to become a template that takes the input of SelectItems and clones the already existing items.
- Update should switch to using Select / Mutate / Save instead of reimplementing Apply
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with src/Build/Evaluation/LazyItemEvaluator.LazyItemOperation.cs, especially SelectItems and MutateItems, then compare the custom Apply implementation in src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs. Trace how newly added and existing items are represented, and ensure Update uses the shared Select/Mutate/Save flow while existing items are cloned before mutation.
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
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100