Make IActivity.SetTag/SetTags return non-nullable IActivity

[DustinCampbell]

While adding telemetry reporting to the MSBuild coordinator, I noticed that the SetTag and SetTags methods on IActivity always return this, so their return type should be non-nullable. This removes unnecessary null-conditional chaining (?.) after the first call in fluent chains, since the activity reference is already known to be non-null after StartActivity succeeds. Also removes the redundant ?. on TelemetryManager.Instance which is never null.

[Copilot]

Pull request overview

Updates MSBuild’s internal telemetry activity API to reflect that fluent SetTag/SetTags calls always return this, enabling cleaner chaining and removing redundant null-conditional usage around TelemetryManager.Instance.

Changes:

• Make IActivity.SetTag / IActivity.SetTags return non-nullable IActivity and update implementations accordingly.
• Simplify fluent telemetry chains by removing redundant ?. after the first conditional access.
• Remove redundant null-conditional access on TelemetryManager.Instance call sites.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/MSBuild/XMake.cs	Removes redundant null-conditional calls when initializing/disposing telemetry.
src/MSBuild.Coordinator/CoordinatorTelemetry.cs	Simplifies fluent SetTag chaining after the initial conditional access.
src/Framework/Telemetry/VSTelemetryActivity.cs	Updates IActivity implementation signatures to non-nullable fluent returns.
src/Framework/Telemetry/IActivity.cs	Updates interface return nullability for fluent methods.
src/Framework/Telemetry/DiagnosticActivity.cs	Updates IActivity implementation signatures to non-nullable fluent returns.
src/Framework/Telemetry/CrashTelemetryRecorder.cs	Removes redundant null-conditional access on TelemetryManager.Instance.
src/Build/BackEnd/BuildManager/BuildManager.cs	Simplifies end-build telemetry chaining and removes redundant TelemetryManager.Instance null checks.

[github-actions]

Code Review — PR #14091

Reviewed all 24 dimensions. Key claims verified against the actual source:

Verification

TelemetryManager.Instance is never null:

public static TelemetryManager Instance { get; } = new TelemetryManager();

Static property initializer — guaranteed non-null at .NET class initialization. Removing ?. everywhere is correct.

SetTag/SetTags always return this: Confirmed in both DiagnosticActivity and VsTelemetryActivity. Changing the return type from IActivity? to IActivity accurately reflects the implementations.

Null-conditional chain safety: The pattern ?.SetTag(A).SetTag(B).SetTag(C) is safe by C# null-conditional semantics — the entire trailing . member-access chain is absorbed into the preceding ?. conditional. If StartActivity() returns null, no SetTag calls execute. Verified for both CoordinatorTelemetry.cs and BuildManager.cs.

IActivity is internal — no public API surface affected; no ChangeWave needed.

Summary

#	Dimension	Verdict
All 24	—	✅

✅ 24/24 dimensions clean — no findings.

---

Cosmetic note (not a blocking issue): The PR introduces _ = SetTag(...) and _ = _activity.SetTag(...) discards inside DiagnosticActivity and VsTelemetryActivity. This is unnecessary in C# (unused return values don't produce warnings for non-[MustUseReturnValue] methods) and is inconsistent with the existing pattern elsewhere (e.g., activity?.SetTags(crashTelemetry) in CrashTelemetryRecorder.cs). Dropping the _ = would be marginally cleaner, but this is purely cosmetic and doesn't affect correctness.

Generated by Expert Code Review (on open) for issue #14091 · 566.8 AIC · ⊞ 30K ambient context