Skip to content

Change DirectoryNotFoundException binary serialization constructor to use GetValueNoThrow#126648

Merged
danmoseley merged 2 commits intomainfrom
copilot/change-directorynotfoundexception-constructor
Apr 9, 2026
Merged

Change DirectoryNotFoundException binary serialization constructor to use GetValueNoThrow#126648
danmoseley merged 2 commits intomainfrom
copilot/change-directorynotfoundexception-constructor

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 8, 2026

Replace the linear foreach scan over SerializationInfo entries in DirectoryNotFoundException's deserialization constructor with a direct GetValueNoThrow lookup. Consistent other similar constructors in CoreLib..

… use GetValueNoThrow

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/722a255e-0a89-4407-bb17-9b9605d62655

Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Copilot AI self-assigned this Apr 8, 2026
Copilot AI review requested due to automatic review settings April 8, 2026 14:46
Copilot AI review requested due to automatic review settings April 8, 2026 14:46
Copilot AI review requested due to automatic review settings April 8, 2026 14:47
@jkotas jkotas marked this pull request as ready for review April 8, 2026 14:47
@jkotas jkotas requested a review from ViveliDuCh April 8, 2026 14:47
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates DirectoryNotFoundException binary deserialization to use SerializationInfo.GetValueNoThrow for the optional DirectoryPath field, aligning with patterns used by other exceptions and avoiding a linear scan over SerializationInfo entries.

Changes:

  • Replace foreach (SerializationEntry ...) scan in the serialization constructor with a direct GetValueNoThrow lookup.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

Note

This review was generated by Copilot.

🤖 Copilot Code Review — PR #126648

Holistic Assessment

Motivation: The PR simplifies the DirectoryNotFoundException binary serialization constructor by replacing a manual foreach (SerializationEntry) loop with the existing SerializationInfo.GetValueNoThrow helper — a well-established internal API used by Exception, SecurityException, TimeZoneInfo.AdjustmentRule, and others for exactly this purpose. The cleanup is clearly justified.

Approach: This is the right approach. GetValueNoThrow does exactly what the manual loop did (find by name, return null if missing), and the codebase has no remaining foreach SerializationEntry patterns — this was the last one. The second commit correctly removes a superfluous comment at the maintainer's suggestion.

Summary: ✅ LGTM. A clean, one-line simplification that aligns with existing codebase patterns. No behavioral change, no new public API, no risk.


Detailed Findings

✅ Correctness — Behavioral equivalence verified

The old code iterated entries looking for "DirectoryNotFound_DirectoryPath" and returned null if not found. GetValueNoThrow does the same via GetElementNoThrowFindElement, returning null when the key is absent. Both paths cast the result to (string?). The only difference is GetValueNoThrow adds type-compatibility checking and converter fallback, which is strictly more robust (though irrelevant in practice since the value is serialized as typeof(string)).

✅ Consistency — Matches established pattern

All other optional-field deserialization sites use GetValueNoThrow:

  • Exception.cs line 50 (Data)
  • SecurityException.cs lines 60–65 (multiple fields)
  • TimeZoneInfo.AdjustmentRule.cs lines 286, 292
  • StringComparer.cs line 243

This change eliminates the last divergent pattern.

✅ No public API changes — No ref assembly impact

No changes to ref/ files. No new public members. The modified constructor is protected and [Obsolete] (legacy binary serialization). No API approval needed.

Generated by Code Review for issue #126648 ·

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Apr 9, 2026

@danmoseley Minor cleanup

@danmoseley danmoseley enabled auto-merge (squash) April 9, 2026 01:02
@jkotas
Copy link
Copy Markdown
Member

jkotas commented Apr 9, 2026

/ba-g trimming test failure is known issue #126675

@danmoseley danmoseley merged commit eba6c8b into main Apr 9, 2026
151 of 164 checks passed
@danmoseley danmoseley deleted the copilot/change-directorynotfoundexception-constructor branch April 9, 2026 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants