Modernize System.Text.Json product code#130976
Conversation
Use C# 14 field-backed properties for private state used only by accessors, and expression-bodied members for single-expression methods. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69ae6b80-f573-41bd-8cf9-e0f858510bbe
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-text-json |
There was a problem hiding this comment.
Pull request overview
This PR updates System.Text.Json product sources to use newer C# syntax in a behavior-preserving way, primarily by replacing single-use private backing fields with field-backed properties and by converting a handful of trivial methods to expression-bodied members.
Changes:
- Converted several properties from explicit private backing fields to C# field-backed properties (using
field) while keeping existing validation/side effects in setters. - Converted multiple single-expression helper methods/wrappers to expression-bodied members.
- Kept the changes scoped to implementation details (no new members introduced in the reviewed diffs).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs | Converts simple type-kind predicates to expression-bodied methods. |
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs | Migrates several callback/configuration properties to field-backed properties while preserving validation in setters. |
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs | Migrates several configuration properties to field-backed properties while preserving VerifyMutable() and validation logic. |
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPolymorphismOptions.cs | Converts configuration properties (including defaulting behavior) to field-backed properties. |
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs | Converts ConverterStrategy to a field-backed init-only property while retaining side-effect initialization in init. |
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ConfigurationList.cs | Converts thin wrappers over _list to expression-bodied members. |
| src/libraries/System.Text.Json/src/System/Text/Json/Schema/JsonSchema.cs | Converts many schema keyword properties to field-backed properties, keeping VerifyMutable() enforcement and initial values. |
| src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs | Converts CaptureState to an expression-bodied method without changing the captured values. |
Replace built-in null comparisons and stable compound comparisons with equivalent C# patterns. Retain reflection comparisons that bind user-defined equality operators. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69ae6b80-f573-41bd-8cf9-e0f858510bbe
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "6c94dc327cecedf449638d0b4c5595e9d1167cc6",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "034f7f5c843988fd82ddfb8369b27c67ab986c86",
"last_reviewed_commit": "6c94dc327cecedf449638d0b4c5595e9d1167cc6",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "034f7f5c843988fd82ddfb8369b27c67ab986c86",
"last_recorded_worker_run_id": "29687161478",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "6c94dc327cecedf449638d0b4c5595e9d1167cc6",
"review_id": 4730768943
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Adopting modern C# syntax (C# 14 field-backed properties, is null/is not null, relational/or/and patterns, expression-bodied members) in System.Text.Json product code is a reasonable, low-risk readability/consistency improvement. The area builds with LangVersion=preview, so the field keyword is supported.
Approach: The change is purely syntactic. Field-to-field conversions preserve all validation (VerifyMutable()), side effects (_isConfigured = true, CanUseDirectReadOrWrite/RequiresReadAhead in the ConverterStrategy init), and default initializers (e.g. JsonSchemaType.Any). Backing fields that are still referenced from other members or derived types (_effectiveConverter) were correctly left untouched, and fully-removed backing fields (_converterStrategy, _customConverter, _typeDiscriminatorPropertyName, etc.) have no remaining references. Pattern-matching rewrites are logically equivalent to the original boolean expressions. The author states the deliberate retention of null comparisons that bind to reflection equality operators, avoiding a real behavioral pitfall.
Summary: ✅ LGTM. Spot-checked the field-keyword conversions in JsonSchema, JsonPolymorphismOptions, JsonPropertyInfo, and JsonConverter (side effects and defaults preserved), confirmed removed backing fields are unreferenced, and reviewed a sample of the is/or/and pattern rewrites for semantic equivalence. No behavioral or public-API changes; project config and tests are unchanged. Verification rests on the author's reported full clr+libs build and passing test run (129,867 passed / 0 failed), which a reviewer worker cannot reproduce here — CI should confirm the multi-target build (net462/netstandard2.0/net10.0/net11.0) since field is a newer language feature.
Detailed Findings
✅ Field-backed property conversions — semantics preserved
Each converted property retains its setter validation and side effects. Notable non-trivial cases verified: JsonSchema.Type keeps its = JsonSchemaType.Any initializer; JsonPolymorphismOptions setters still set _isConfigured = true; TypeDiscriminatorPropertyName keeps get => field ?? JsonSerializer.TypePropertyName; JsonConverter.ConverterStrategy init still computes CanUseDirectReadOrWrite/RequiresReadAhead before assigning field.
✅ Backing-field removal is compile-safe
Fields converted to the field keyword are no longer referenced elsewhere; fields still referenced from other members or subclasses (_effectiveConverter) were intentionally retained.
✅ Pattern-matching rewrites — logically equivalent
Rewrites such as initialSize is > OneMegabyte and <= 4 * OneMegabyte, tokenType is JsonTokenType.StartArray or JsonTokenType.StartObject, and nextByte is not ((byte)'.' or (byte)'E' or (byte)'e') preserve the original truth tables and precedence.
💡 CI multi-target confirmation
Since this relies on the field contextual keyword and preview language features, the value of this review hinges on the full CI matrix (including the down-level target frameworks) building successfully. No action needed beyond letting CI complete.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 119.2 AIC · ⌖ 10.7 AIC · ⊞ 10K
Adopts modern C# syntax in
System.Text.Jsonproduct sources without changing behavior or public API.is nulloris not null.or,not, and relational patterns.The pattern audit deliberately retains 56 null comparisons that bind to user-defined reflection equality operators and 19 compound checks whose conversion would reduce property or mutable-field reads. Project configuration and tests are unchanged. I also audited escaped literals and formatting calls, but retained the existing forms where raw strings or interpolation would not improve readability or would bypass localized resource formatting.
Validation
build.cmd clr+libs -rc releaseSystem.Text.Json.csprojacrossnet462,netstandard2.0,net10.0, andnet11.0dotnet build /t:testfor the mainSystem.Text.Jsontests and all four Roslyn 3.11/4.4 source-generation test projectsNote
This pull request was prepared by GitHub Copilot.