-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix STJ source generator emitting culture-dependent minus signs for negative JsonPropertyOrder #121278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…alues Co-authored-by: stephentoub <[email protected]>
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
|
@copilot, are there any other cases of this in the STJ emitter? Places where something like a number or DateTime or something else that's culture-sensitive may end up rendering with the current culture inappropriately? I'm wondering whether a more encompassing fix might be to instead surround the emitter with setting CultureInfo.CurrentCulture to InvariantCulture (and then setting it back in a finally). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a culture-specific bug in the JSON source generator where negative JsonPropertyOrder values could generate invalid C# code in locales that use non-ASCII minus signs (e.g., Finnish locale uses U+2212 instead of U+002D).
Key changes:
- Modified the source generator to use culture-invariant formatting when emitting property order values
- Added a comprehensive unit test that validates the fix by testing code generation under the Finnish locale
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs | Fixed numeric formatting to use InvariantCulture when generating code for property.Order values |
| src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs | Added test to verify negative JsonPropertyOrder values generate valid code across different locales |
...tem.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs
Outdated
Show resolved
Hide resolved
I searched through the emitter and found:
However, wrapping the entire
I'll implement that approach - setting |
Co-authored-by: stephentoub <[email protected]>
@copilot, you didn't make the changes. Please wrap the invocation of the emitter with such a try/finally. |
Co-authored-by: stephentoub <[email protected]>
I've implemented the try/finally wrapper approach. The Emit invocation is now wrapped with code that sets |
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn3.11.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn4.0.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn4.0.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn4.0.cs
Outdated
Show resolved
Hide resolved
...tem.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorTests.cs
Outdated
Show resolved
Hide resolved
…atting, use ThreadCultureChange helper Co-authored-by: stephentoub <[email protected]>
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
| // This prevents issues such as locale-specific negative signs (e.g., U+2212 in fi-FI) | ||
| // from being written to generated source files. | ||
| // Note: RS1035 is already disabled at the file level for this Roslyn version. | ||
| CultureInfo originalCulture = CultureInfo.CurrentCulture; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub this seems a bit heavy handed to me and I could see this regressing accidentally in the future. Shouldn't we instead fix the individual callsites hitting culture sensitive APIs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it seem heavy handed? It's making it so that everything is done culture-invariant, which is what we want. Is there ever a situation where we'd want something the generator to be culture-aware to the build machine that ran the generator? Assuming not, the only thing we'd get from doing it per call-site is greater risk of a bug tail and more expensive formatting.
Fix STJ source generator producing non-compilable code on some locales
This PR addresses an issue where the System.Text.Json source generator produces non-compilable code when using
JsonPropertyOrderwith negative values on certain locales (e.g., fi_FI.UTF-8).Changes Made
Technical Details
The issue was in
JsonSourceGenerator.Emitter.cswhere numeric values were being directly interpolated into generated C# code without specifying culture. On locales like fi_FI, negative numbers use the Unicode minus sign (U+2212) instead of the ASCII hyphen-minus, making the generated code invalid.Fix:
Wrapped the invocation of
emitter.Emit()with code that setsCultureInfo.CurrentCulturetoInvariantCultureand restores it in a finally block. This comprehensive approach:Files Modified:
using System.Globalization;since culture is now managed in calling codeTests:
NegativeJsonPropertyOrderGeneratesValidCodethat tests negative JsonPropertyOrder with fi_FI culture using ThreadCultureChange helperFixes #121277
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.