Hide experimental properties from the JSON source generator to avoid MCPEXP001 diagnostics#1260
Open
MackinnonBuck wants to merge 4 commits intomainfrom
Open
Hide experimental properties from the JSON source generator to avoid MCPEXP001 diagnostics#1260MackinnonBuck wants to merge 4 commits intomainfrom
MackinnonBuck wants to merge 4 commits intomainfrom
Conversation
…MCPEXP001 diagnostics
src/ModelContextProtocol.Core/Protocol/CallToolRequestParams.cs
Outdated
Show resolved
Hide resolved
Collaborator
Author
|
Pushed an update addressing review feedback:
|
jeffhandley
reviewed
Feb 16, 2026
Comment on lines
+46
to
+51
| /// <summary>Backing field for <see cref="Task"/>. This field is not intended to be used directly.</summary> | ||
| [JsonInclude] | ||
| [JsonPropertyName("task")] | ||
| public McpTaskMetadata? Task { get; set; } | ||
| [JsonConverter(typeof(ExperimentalJsonConverter<McpTaskMetadata>))] | ||
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| public object? _task; |
Collaborator
There was a problem hiding this comment.
I recommend a much stronger statement about not being intended for consumption. We can take inspiration from efcore. Example: https://github.com/dotnet/efcore/blob/a471ebfd9564bc14fb188407a84a031b69d29f77/src/EFCore/Query/QueryContext.cs#L109-L116
We should also give the backing field a more egregious name to emphasize the internal aspect too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prevent experimental API types (Tasks, ToolExecution) from leaking into user code via the System.Text.Json source generator, eliminating spurious MCPEXP001 diagnostics for users who don't use experimental features.
Description
When users define their own
JsonSerializerContextthat includes MCP protocol types (e.g.,Tool,ServerCapabilities), the System.Text.Json source generator follows property types transitively and generates code that references experimental types. This causes MCPEXP001 errors in user projects even when they aren't using experimental features like Tasks.Approach: Object Backing Field Pattern
For each stable type that exposes an experimental property, the public typed property is now marked with
[JsonIgnore]and backed by aninternal object?field annotated with[JsonInclude],[JsonPropertyName], and a new[JsonConverter(typeof(ExperimentalJsonConverter<T>))]. Because the source generator seesobject?rather than the experimental type, it no longer walks the experimental type graph, so no MCPEXP001 diagnostic is emitted in consuming projects.The
ExperimentalJsonConverter<T>delegates serialization toMcpJsonUtilities.DefaultOptions, which already contains source-generated contracts for all experimental types. This preserves NativeAOT compatibility and ensures the experimental properties continue to round-trip correctly over the wire.A
DiagnosticSuppressorwas considered but is not necessary since the backing field pattern fully hides experimental types from the source generator (and it wouldn't help anyway becauseDiagnosticSuppressors cannot suppress error-severity diagnostics).Other changes
[Experimental(MCPEXP001)]to the 6 experimental properties that were missing it (onlyTool.Executionhad it previously). Happy to undo this change if the omission of the[Experimental]attribute was deliberate.WriteIndentedpropagation.Fixes #1255