[Debugger] Treat Nullable<T> as safe for snapshot ToString#8568
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b26e8866d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenchmarksBenchmark execution time: 2026-05-11 16:24:52 Comparing candidate commit f4a10bf in PR branch Some scenarios are present only in baseline or only in candidate runs. If you didn't create or remove some scenarios in your branch, this maybe a sign of crashed benchmarks 💥💥💥 Scenarios present only in baseline:
Found 5 performance improvements and 3 performance regressions! Performance is the same for 46 metrics, 18 unstable metrics, 89 known flaky benchmarks, 37 flaky benchmarks without significant changes.
|
3b26e88 to
5211ced
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8568) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8568) - mean (73ms) : 71, 76
master - mean (73ms) : 71, 76
section Bailout
This PR (8568) - mean (80ms) : 76, 84
master - mean (80ms) : 75, 85
section CallTarget+Inlining+NGEN
This PR (8568) - mean (1,085ms) : 1037, 1134
master - mean (1,089ms) : 1014, 1164
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8568) - mean (119ms) : 112, 127
master - mean (115ms) : 110, 120
section Bailout
This PR (8568) - mean (119ms) : 111, 127
master - mean (119ms) : 113, 124
section CallTarget+Inlining+NGEN
This PR (8568) - mean (781ms) : 753, 809
master - mean (784ms) : 762, 805
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8568) - mean (105ms) : 98, 111
master - mean (103ms) : 96, 110
section Bailout
This PR (8568) - mean (102ms) : 98, 105
master - mean (101ms) : 98, 104
section CallTarget+Inlining+NGEN
This PR (8568) - mean (946ms) : 907, 985
master - mean (944ms) : 912, 977
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8568) - mean (102ms) : 96, 108
master - mean (103ms) : 97, 108
section Bailout
This PR (8568) - mean (106ms) : 100, 111
master - mean (104ms) : 99, 109
section CallTarget+Inlining+NGEN
This PR (8568) - mean (822ms) : 790, 854
master - mean (824ms) : 787, 861
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8568) - mean (212ms) : 195, 230
master - mean (213ms) : 193, 233
section Bailout
This PR (8568) - mean (218ms) : 200, 235
master - mean (218ms) : 199, 238
section CallTarget+Inlining+NGEN
This PR (8568) - mean (1,277ms) : 1213, 1340
master - mean (1,289ms) : 1221, 1356
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8568) - mean (314ms) : 279, 348
master - mean (311ms) : 278, 344
section Bailout
This PR (8568) - mean (310ms) : 283, 336
master - mean (317ms) : 275, 359
section CallTarget+Inlining+NGEN
This PR (8568) - mean (1,024ms) : 983, 1065
master - mean (1,033ms) : 983, 1084
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8568) - mean (305ms) : 273, 337
master - mean (307ms) : 255, 358
section Bailout
This PR (8568) - mean (300ms) : 275, 324
master - mean (305ms) : 264, 346
section CallTarget+Inlining+NGEN
This PR (8568) - mean (1,193ms) : 1135, 1251
master - mean (1,217ms) : 1128, 1305
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8568) - mean (299ms) : 268, 331
master - mean (306ms) : 262, 351
section Bailout
This PR (8568) - mean (298ms) : 268, 329
master - mean (312ms) : 264, 360
section CallTarget+Inlining+NGEN
This PR (8568) - mean (1,101ms) : 977, 1224
master - mean (1,135ms) : 986, 1284
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
7a64cb7 to
d7fa866
Compare
When serializing a snapshot, IsSafeToCallToString unwraps Nullable<T> before
checking IsSimple/the allow-list. Without this, fields typed as e.g. DateTime?,
TimeSpan?, DateTimeOffset?, int? etc. are treated as complex objects and the
serializer recurses into their internal fields ('hasValue', 'value') instead of
rendering the underlying value via ToString.
Update SupportedTypesServiceTests to test types directly (so the test reflects
what the runtime sees in IsSafeToCallToString) and add cases for the common
Nullable<T> shapes. Add a snapshot-level test in DebuggerSnapshotCreatorTests
that asserts a holder with DateTime? and TimeSpan? fields renders as a flat
value with no recursion into fields.
Co-authored-by: Cursor <[email protected]>
d7fa866 to
f4a10bf
Compare
Summary of changes
Redaction.IsSafeToCallToStringnow unwrapsNullable<T>before checkingIsSimple/the allow-list of types that are safe toToString.IsSafeToCallToStringso expression dumping fallback paths can still pass a nullType.SupportedTypesServiceTeststo testTypevalues directly and coverint?,DateTime?,TimeSpan?,DateTimeOffset?.ToStringvalues and non-generic dictionary null values.Reason for change
DateTime?/TimeSpan?/etc. were treated as complex objects, so snapshot serialization recursed intoNullable<T>internals instead of rendering the underlying value viaToString.GetType()returns the underlying type rather thanNullable<T>.IsSafeToCallToStringis also used by expression dumping, where existing callers can pass a nullTypefor null dictionary values; that fallback behavior must be preserved.Implementation details
Nullable.GetUnderlyingTypeis only called after a null check.IsSupportedCollection(type)is still passed the original type so collection detection is unchanged.Test coverage
SupportedTypesServiceTests,DebuggerSnapshotCreatorTests, andDebuggerExpressionLanguageTests.