Fix Task<TResult> debug proxy Result behavior and rename from Future#123752
Fix Task<TResult> debug proxy Result behavior and rename from Future#123752stephentoub merged 4 commits intomainfrom
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-threading-tasks |
stephentoub
left a comment
There was a problem hiding this comment.
Code Review: PR #123752
🎯 Holistic Assessment
Motivation: Address a debugger UX issue where \Task\ debug proxy showed \default(TResult)\ for faulted/cancelled tasks, misleading developers into thinking \Result\ would return that value rather than throw.
Approach:
- Rename \SystemThreadingTasks_FutureDebugView\ → \SystemThreadingTasks_TaskOfTResultDebugView\ to modernize naming
- Change \Result\ property to delegate directly to \m_task.Result\ instead of returning \default\ for non-success states
Net Positive: ✅ Yes - Simple, targeted fix that improves debugger accuracy.
📋 Detailed Findings
✅ Correctness
- The rename from "Future" to "TaskOfTResult" is appropriate - "Future" was an internal code name for \Task\ from early development
- Delegating \Result\ directly to \m_task.Result\ correctly shows the exception that would be thrown for faulted/cancelled tasks in the debugger
- The change is consistent with the non-generic \SystemThreadingTasks_TaskDebugView\ naming pattern in \Task.cs\
✅ Consistency
- Only one file contains references to this type, so the rename is complete
- The new naming follows the established pattern from the base \Task\ class's debug view (\SystemThreadingTasks_TaskDebugView)
💡 Minor Observation
- The removal of ?\ from \TResult?\ to \TResult\ is correct since the property now directly returns the task's result (which throws for non-success) rather than potentially returning \default\ which could be null
🧪 Test Quality
This is a debugger metadata change. The behavior is verified by debugging experience rather than unit tests, which is appropriate for this type of change.
✅ Summary
Verdict: Approve
This is a clean, minimal fix that:
- Modernizes legacy naming ("Future" → "TaskOfTResult")
- Correctly exposes the throwing behavior in the debugger for faulted tasks
- Follows established patterns in the codebase
The change is surgical (4 lines) and addresses exactly what was requested in the issue.
🤖 Copilot Code Review — PR #123752Holistic AssessmentMotivation: The PR correctly identifies a real usability issue: the debug proxy was displaying Approach: The approach is incomplete. While delegating directly to Summary: Detailed Findings❌ Blocking Risk for Incomplete Tasks — Must fixThe new implementation // From Future.cs lines 449-452
internal TResult GetResultCore(bool waitCompletionNotification)
{
// If the result has not been calculated yet, wait for it.
if (!IsCompleted) InternalWait(Timeout.Infinite, default);
...
}When a debugger evaluates properties on the debug proxy, this will cause the debugger to hang indefinitely for any task that is still Suggested fix: Guard the access to only invoke public TResult Result => m_task.IsCompleted ? m_task.Result : default!;This achieves the PR's goal (showing exceptions for faulted/canceled tasks) while avoiding blocking for incomplete tasks. ✅ Naming — CorrectThe rename from ✅ DebuggerTypeProxy Attribute — CorrectThe 💡 Nullability of Result Property — SuggestionThe return type changed from
The null-forgiving operator ( 💡
|
|
@copilot please address the "Must fix" issue called out in the comment |
|
@copilot can you add new tests covering this area and validating the change? |
Added 6 tests in bbfc1dc covering the debug proxy behavior:
|
tarekgh
left a comment
There was a problem hiding this comment.
@stephentoub the changes LGTM. Some tests have been added to cover this area, maybe worth taking a quick look before merging. Thanks!
…askOfTResultDebugView and delegate Result to task.Result Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…complete tasks Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
bbfc1dc to
40b2026
Compare
Description
Task<TResult>'s debug proxy was showingdefault(TResult)for theResultproperty of faulted/cancelled tasks, misleading developers about actual behavior. The debugger should show that accessingResultwill throw.Changes
SystemThreadingTasks_FutureDebugView<TResult>→SystemThreadingTasks_TaskOfTResultDebugView<TResult>("Future" was an old name for "Task")Resultproperty to delegate tom_task.Resultwhen the task is completed, guarded byIsCompletedto avoid blocking the debugger on incomplete tasksBefore:
public TResult? Result => m_task.Status == TaskStatus.RanToCompletion ? m_task.Result : default;After:
public TResult Result => m_task.IsCompleted ? m_task.Result : default!;This correctly handles all three task states:
m_task.Result(the original issue's goal)defaultwithout blocking the debuggerTests Added
Added 6 tests in
TaskDebugViewTests.csvalidating debug proxy behavior:ResultthrowsAggregateExceptionwrapping the original exceptionResultthrowsAggregateExceptionResultreturnsdefaultwithout blocking (reference and value types)Original prompt
Task<T>debug proxy is misleading aboutResultof a non-success task #123751✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.