Separate the data-tracking parts of Terminal Logger into a reusable base

[baronfel]

This makes it easier to implement other Loggers - you just need to pick out what kinds of data you need from evaluation/projects, and then render warnings, errors, etc however you want during certain lifecycle events. The thought is that all of the data required to track/associate/trigger the lifecycle warnings is kept by the 'tracker', so your use-case-specific logger can mostly ignore that.

This is low-priority ATM, but something that may be useful for us as we think about how Loggers might need to change to more easily react to LLMs and other tools. For example, @richlander has a prototype logger that emits markdown - I'm pretty sure that logger doesn't understand the relationship between 'outer' builds and 'inner' like TL does. If it built on this base and then it could more easily get that understanding for free.

I kind of think of this interface as a step towards a more user-friendly Logger programming model - for loggers that care about rendering the 'intent' of the build and reshaping/papering over MSBuild specifics in favor of a more user-centric model this base can hide a lot of the guts.

This is opened as a new PR because rebasing the previous iteration closed the PR and I was unable to open it.

[Copilot]

Pull request overview

Refactors TerminalLogger by extracting build/project/evaluation tracking into a reusable generic base class, with the goal of enabling additional logger implementations to reuse the same lifecycle/state association logic.

Changes:

• Introduces ProjectTrackingLoggerBase<...> to centralize event subscriptions and tracking of eval/project/build state.
• Updates TerminalLogger to derive from the new base and moves build-scoped counters/state into TerminalBuildData.
• Promotes several TerminalLogger-related helper types (and TerminalColor) from internal to public to support reuse.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/Framework/Logging/TerminalColor.cs	Makes TerminalColor public (now part of Framework API surface).
src/Build/Microsoft.Build.csproj	Adds ProjectTrackingLoggerBase.cs to the Build project compilation items.
src/Build/Logging/ProjectTrackingLoggerBase.cs	New reusable base class to track eval/project/build state and route events to virtual hooks.
src/Build/Logging/TerminalLogger/TerminalLogger.cs	Switches TerminalLogger to use the new base and build-scoped TerminalBuildData.
src/Build/Logging/TerminalLogger/TerminalBuildData.cs	New build-scoped state container (start time, restore state, totals).
src/Build/Logging/TerminalLogger/TerminalProjectInfo.cs	Promotes and simplifies project/eval info types for reuse.
src/Build/Logging/TerminalLogger/TerminalNodeStatus.cs	Promotes node status type to public for reuse by other renderers.
src/Build/Logging/TerminalLogger/TerminalMessageSeverity.cs	Makes severity enum public.
src/Build/Logging/TerminalLogger/TerminalBuildMessage.cs	Makes build message struct public.
src/Build/Logging/TerminalLogger/StopwatchAbstraction.cs	Makes stopwatch abstraction public.

Comments suppressed due to low confidence (2)

src/Build/Logging/TerminalLogger/StopwatchAbstraction.cs:12

• StopwatchAbstraction was made public but has no XML documentation. If it’s intended as part of a reusable logger programming model, it should be documented (purpose, threading expectations, how ElapsedSeconds is defined). Otherwise, consider keeping it internal to avoid committing to this abstraction as a supported API.

public abstract class StopwatchAbstraction
{
    public abstract void Start();
    public abstract void Stop();

    public abstract double ElapsedSeconds { get; }
}

src/Build/Logging/TerminalLogger/TerminalNodeStatus.cs:19

• TerminalNodeStatus is now public, but it overrides GetHashCode() by throwing NotImplementedException. This is a footgun for any consumer that uses the type in hashed collections or calls GetHashCode implicitly. Implement a stable hash code consistent with the equality logic (or remove the override if it’s not intended to be used).

public class TerminalNodeStatus
{
    public string Project { get; }
    public string? TargetFramework { get; }
    public string? RuntimeIdentifier { get; }
    public TerminalColor TargetPrefixColor { get; } = TerminalColor.Default;

[github-actions]

Public API Surface Review

This PR makes 9 previously-internal types public across Microsoft.Build and Microsoft.Build.Framework. Since MSBuild ships as part of .NET SDK and VS, these become permanent API commitments. A few concerns:

Types being made public

Type	Assembly	Concern
ProjectTrackingLoggerBase<,,, >	Microsoft.Build	New public base class — reasonable
TerminalBuildData	Microsoft.Build	All settable properties — no encapsulation
StopwatchAbstraction	Microsoft.Build	Abstracts Stopwatch — is this the right public API shape?
TerminalBuildMessage	Microsoft.Build	record struct — ok
EvalProjectInfo	Microsoft.Build	record struct — lost Id property and context parameter
TerminalProjectInfo	Microsoft.Build	Mutable class with AddBuildMessage — significant surface
TerminalNodeStatus	Microsoft.Build	Has TerminalColor in constructor — couples to rendering
TerminalMessageSeverity	Microsoft.Build	Simple enum — ok
TerminalColor	Microsoft.Build.Framework	VT100-specific enum in Framework — belongs here?

Key questions

1. Naming: These types are prefixed with "Terminal" but the goal is a reusable base for any logger. Should the base class types have more generic names (e.g., BuildProjectInfo instead of TerminalProjectInfo)?

2. TerminalColor in Framework assembly: This is a rendering concern being placed in the Framework assembly which is referenced by every task and logger. It should probably stay in Microsoft.Build or move to a rendering-specific namespace.

3. TerminalBuildData mutability: All properties are { get; set; } with no validation. The RestoreContext, RestoreFailed, RestoreFinished properties expose TerminalLogger-specific restore tracking state as public API. A subclass implementing a different logger probably doesn't need these. Consider splitting TL-specific state from truly generic build data.

4. No test coverage: This is a significant refactoring of a complex, stateful logger with multiple behavioral paths (quiet mode, restore mode, test projects, cache plugins, binary log replay). There are no new or updated tests in this PR to verify behavioral equivalence.

Generated by Expert Code Review (on open) for issue #13591 · ● 12.3M · ◷

[github-actions]

Review Summary

This is a well-motivated refactoring — extracting build state tracking into a reusable base class is a great direction for making logger authoring easier. However, the current implementation has several behavioral regressions that need to be fixed before merging.

🔴 Bugs (3)

1. Quiet-mode regression in OnProjectFinished (line 712-716): Early return at Verbosity <= Quiet skips error/warning count accumulation, Succeeded assignment, and Stopwatch.Stop(). The old code always accumulated counts even in quiet mode.

2. Quiet-mode regression in OnWarningRaised/OnErrorRaised (lines 1194-1196, 1276): New Verbosity > Quiet guard prevents per-project warning/error accumulation in quiet mode. Old code always accumulated, contradicting the existing comment "In quiet mode, we still accumulate."

3. Missing RestoreContext guard in OnTargetStarted (line 943): Old code was gated by _restoreContext is null — new code processes ALL targets during restore, causing incorrect node status updates.

🟡 Design Issues (3)

4. Restore-phase project tracking change: The base class unconditionally creates project data, but the old TL only created entries when _restoreContext is null. This cascading change affects every downstream callback during restore.

5. TNodeData unused: The type parameter exists in the public signature but the base class never references it. Once shipped, it can't be removed.

6. YieldNode is accidentally public on a sealed class — should be private.

🟡 API Surface (see separate comment)

9 internal types made public with Terminal-specific names for a "generic" base class. No test coverage for behavioral equivalence.

Recommendation

Fix the three bugs first (ideally with tests that cover quiet mode and restore scenarios). Then consider whether the API shape is ready for public consumption or whether the types should remain internal until the design stabilizes.

Generated by Expert Code Review (on open) for issue #13591 · ● 12.3M

[baronfel]

/review

[github-actions]

✅ Expert Code Review (command) completed successfully!

Expert reviewer sub-agent launched for PR #13591. Running 24-dimension code review in parallel batches. Will post inline review comments, design-level concerns, and final verdict via safe-output tools once complete.

[github-actions]

24-Dimension Code Review — PR #13591

Summary: The refactoring direction is sound — extracting build-state tracking into a reusable base is a good idea for the logger ecosystem. The behavioral refactoring of TerminalLogger onto the new base correctly preserves the original restore-tracking, node-yielding, and project-lifecycle semantics. However, there are API surface concerns that should be resolved before shipping since this introduces permanent public API.

Findings

#	Dimension	Severity	Finding
1	API Surface	🔴 High	TNodeData type parameter is unused — phantom public API that's locked once shipped
2	API Surface	🟡 Medium	TerminalBuildData has all-public mutable setters; should restrict to internal set
3	Design	🟡 Medium	IncludeTaskInputs()/IncludeEvaluationPropertiesAndItems() unconditionally called — taxes lightweight subclasses
4	Concurrency	🟡 Medium	Threading contract not documented — critical for public API since Dictionary is used unsynchronized
5	Performance	🟢 Low	.ToArray() on BuildFinished allocates unnecessarily
6	Formatting	🟢 Low	Indentation inconsistency in TryGetEvalDataForProject

Clean Dimensions (18/24)

Backwards Compat ✅ | ChangeWave ✅ | Allocation (except #5) ✅ | Test Coverage ✅ (existing TL tests cover refactored behavior) | Error Messages ✅ | Logging ✅ | Target Authoring ✅ | Cross-Platform ✅ | Code Simplification ✅ | Naming ✅ | SDK Integration ✅ | Evaluation Model ✅ | Correctness ✅ | Dependency Mgmt ✅ | Security ✅ | Build Infra ✅ | Binary Log Compat ✅ | Nullable ✅

Notes

• The TerminalLogger refactoring faithfully preserves restore lifecycle semantics (verified by tracing the ProjectStarted → CreateProjectData → OnProjectStarted → OnProjectFinished flow against the original handler).
• ProjectContext and EvalContext correctly remain internal — good API discipline.
• No PublicAPI.Unshipped.txt files exist in this repo, so that's not a concern here (the project uses CompatibilitySuppressions.xml for API compat instead).

Generated by Expert Code Review (command) for issue #13591 · ● 7.4M

[baronfel]

/review

[github-actions]

❌ Expert Code Review (command) failed. Please review the logs for details.

Performance & Allocation Awareness: LGTM

Reviewed ProjectTrackingLoggerBase.cs for allocation and hot-path concerns. All flagged areas are clean:

1. record struct dictionary keys — value-type equality avoids boxing; Dictionary<TKey,TValue> uses EqualityComparer<T>.Default which resolves to the struct's IEquatable<T> implementation without boxing. ✅

2. int? from GetNodeArrayIndexForEvent — Nullable<int> is a value type. Callers pattern-match with is int nodeId, which is a value-type operation. No boxing occurs in any call site. ✅

3. No AggressiveInlining on NodeIdForContext — The method is private, has a single arithmetic operation (context.NodeId - 1), and tiny IL. The JIT will auto-inline this without any hint; adding AggressiveInlining would be cargo-cult. ✅

4. EnsurePerNodeDataEnabled() with NoInlining — In Release, Debug.Assert compiles away leaving an empty body. The NoInlining prevents the JIT from inlining the Debug.Assert string into callers in Debug builds, keeping the hot path smaller. The ~1-5ns empty-call overhead in Release is negligible vs. the dictionary work in each handler. ✅

5. Dictionary<>.Values — Returns a ValueCollection view, no allocation. ✅

No performance issues found.

re-reviewing

[github-actions]

Correctness & Edge Cases Review — LGTM (with one minor defensive note)

I evaluated the five potential issues in ProjectTrackingLoggerBase.cs:

✅ Issue 1 & 2: Generic pattern matching — Correct

is TEvalData evalData correctly unwraps Nullable<TEvalData> for value types (like EvalProjectInfo record struct) and is a standard null check for reference types. Similarly, projectData != null on TProjectData? correctly checks HasValue for value types and performs a null check for reference types. No issue.

✅ Issue 3: _buildData! null-forgiving — Pre-existing, acceptable

This pattern is inherited directly from the old TerminalLogger code which also accessed _buildData without null guards. BuildStarted is contractually the first event in both normal builds and binary log replay. While a malformed custom IEventSource could fire events out of order, defending against that is not a reasonable expectation for this class.

✅ Issue 4: Negative NodeId — Pre-existing, acceptable

BuildEventContext.InvalidNodeId (-2) would produce index -3, which would throw IndexOutOfRangeException. However, this is identical to the old TerminalLogger behavior (same context.NodeId - 1 arithmetic, no bounds check). In practice, node data methods are only called by subclass code that already has valid contexts. Not a regression.

⚠️ Issue 5: Initialize ordering — Theoretical, but a defensive regression

Old code (defensively correct):

_nodes = new TerminalNodeStatus[nodeCount + 1]; // allocate FIRST
Initialize(eventSource);                         // subscribe SECOND

New base class:

Initialize(eventSource);                         // subscribe FIRST (virtual dispatch → events subscribed)
_nodeDataByNodeId = UsesPerNodeData ? ... : [];  // allocate SECOND

Between lines 113 and 115, events are subscribed but _nodeDataByNodeId hasn't been resized yet (still [] from the field initializer). In MSBuild's execution model this is safe because the engine doesn't fire events during Initialize. However, the old code was defensively ordered correctly and this is a minor regression in that regard. Consider swapping the order:

public virtual void Initialize(IEventSource eventSource, int nodeCount)
{
    NodeCount = nodeCount + 1;
    _nodeDataByNodeId = UsesPerNodeData ? new TNodeData?[NodeCount] : [];
    Initialize(eventSource);
}

This restores the old defensive ordering at no cost.

Verdict: No major correctness issues. The generic type handling is sound, null patterns are correct for both value and reference type parameters, and the remaining concerns are either pre-existing or purely theoretical.

Generated by Expert Code Review (command) for issue #13591 · ● 59.8M · ◷

[github-actions]

📋 Design Discussion: Consider simplifying the generic API before merging

The 4-parameter generic signature ProjectTrackingLoggerBase<TEvalData, TNodeData, TProjectData, TBuildData> is a high complexity bar for a permanent public API. Some observations:

1. TNodeData is runtime-optional — controlled by UsesPerNodeData virtual bool. Subclasses that don't track per-node data must still provide a type parameter they never use. Consider making node tracking a separate concern (e.g., a mixin interface or a ProjectTrackingLoggerBase<TEvalData, TProjectData, TBuildData> base with a NodeTrackingLoggerBase extending it).

2. TEvalData has limited callback visibility — it only surfaces in OnProjectStarted. After that, subclasses must go through TProjectData. If eval data is always folded into project data by the time callbacks fire, the separate TEvalData parameter adds complexity without proportional value.

3. No tests for the new base class — This is a 604-line public abstract class with complex state management. While existing TerminalLogger tests provide indirect coverage of one parameterization, the contract should be tested independently. Key untested scenarios:

   • UsesPerNodeData = false (node methods should be no-ops or assert)
   • CreateProjectData returning null (project should be skipped)
   • Binary log replay mode with node ID resizing
   • State cleanup between builds (BuildFinished → BuildStarted sequence)

This isn't blocking since the PR is described as low-priority exploration, but these design decisions will be permanent once shipped. Worth iterating on the API shape before committing to it.

Generated by Expert Code Review (command) for issue #13591 · ● 59.8M · ◷

[github-actions]

Test Coverage & Completeness: ISSUE (MAJOR)

A 604-line public abstract base class (ProjectTrackingLoggerBase<,,, >) with complex state management, concurrent data access patterns, and 4 abstract + 11 virtual methods ships with zero dedicated tests.

The existing TerminalLogger_Tests provide indirect coverage of one specific parameterization but do not exercise the base class contract independently. Since this type is explicitly designed as a public extensibility point for third-party loggers, its contract needs direct test coverage — particularly for null-context guards, the UsesPerNodeData opt-in toggle, replay-mode array resizing, and state cleanup between builds.

See inline comment for specific untested paths and recommended test approach.

Generated by Expert Code Review (command) for issue #13591 · ● 59.8M

Cause I said so.