dotnet / dotnet/msbuild

Refactoring: LazyItemOperation should mutate items for all the operations that do mutations

Open
#2,328 0 comments 0 reactions 0 assignees View on GitHub
Refactoring triaged
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.