Skip to content

Use System.Text.Json source generation to remove some trim/AOT warnings - #4683

Open
charlesroddie wants to merge 1 commit into
dotnet:mainfrom
charlesroddie:trim-json-source-gen
Open

charlesroddie wants to merge 1 commit into
dotnet:mainfrom
charlesroddie:trim-json-source-gen

Conversation

@charlesroddie

@charlesroddie charlesroddie commented Sep 13, 2026

Copy link
Copy Markdown

Description

Replaces reflection-based JsonSerializer calls with an internal JsonSerializerContext (SqlClientJsonSerializerContext), removing the IL2026/IL3050 warnings they raise under trimming and Native AOT:

  • SqlVector<T>.GetString() – serializes ReadOnlyMemory<float> (the constructor already restricts T to float).
  • SqlParameter string-to-vector coercion – deserializes float[].
  • HostGuardianServiceEnclaveProvider.MakeRequest – deserializes List<byte>.

Trim/AOT warnings with -p:IsAotCompatible=true (unique by location and code):

TFM Before After
net8.0 50 44
net9.0 52 46

No public API or behavioral changes; JSON output is unchanged.

Issues

Part of #1947.

Testing

  • Built net462, net8.0 and net9.0 with -p:IsAotCompatible=true; none of the touched files emit trim/AOT warnings.
  • Existing SqlVector/SqlParameter unit tests (including exact GetString() output) pass on net462, net8.0 and net10.0.
  • The string-to-vector and HGS attestation paths are covered only by tests requiring SQL Server / HGS and were not run locally.

🤖 Generated with Claude Code

Replaces reflection-based JsonSerializer calls in SqlVector, SqlParameter
and HostGuardianServiceEnclaveProvider with a JsonSerializerContext,
removing IL2026/IL3050 warnings.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 13, 2026 09:10
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Sep 13, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

This PR removes trim/NativeAOT warnings by switching JSON (de)serialization from reflection-based JsonSerializer APIs to System.Text.Json source-generated metadata via a new internal SqlClientJsonSerializerContext.

Changes:

  • Added SqlClientJsonSerializerContext with [JsonSerializable] entries for the JSON types used.
  • Updated SqlVector<T>.GetString() to serialize using source-generated JsonTypeInfo.
  • Updated SqlParameter vector coercion and VirtualSecureModeEnclaveProvider.MakeRequest to deserialize using source-generated JsonTypeInfo.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlTypes/SqlVector.cs Uses source-generated type info for vector JSON serialization
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/VirtualSecureModeEnclaveProvider.cs Uses source-generated type info for HGS response deserialization
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlParameter.cs Uses source-generated type info for string-to-vector JSON coercion
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlClientJsonSerializerContext.cs Introduces the source-generation context and registered serializable types

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@charlesroddie
charlesroddie marked this pull request as ready for review September 13, 2026 15:53
@charlesroddie
charlesroddie requested a review from a team as a code owner September 13, 2026 15:53
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@charlesroddie charlesroddie changed the title Use System.Text.Json source generation to remove trim/AOT warnings Use System.Text.Json source generation to remove some trim/AOT warnings Sep 13, 2026
@cheenamalhotra

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 2 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.

@cheenamalhotra cheenamalhotra added this to the 8.0.0-preview1 milestone Sep 14, 2026
@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.60%. Comparing base (f2310e9) to head (7a2392c).

Files with missing lines Patch % Lines
...Data/SqlClient/VirtualSecureModeEnclaveProvider.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4683      +/-   ##
==========================================
- Coverage   66.27%   64.60%   -1.67%     
==========================================
  Files         290      284       -6     
  Lines       45062    68015   +22953     
==========================================
+ Hits        29865    43943   +14078     
- Misses      15197    24072    +8875     
Flag Coverage Δ
CI-SqlClient ?
PR-SqlClient-Project 64.60% <66.66%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To triage

Development

Successfully merging this pull request may close these issues.

3 participants