-
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?
Fix STJ source generator emitting culture-dependent minus signs for negative JsonPropertyOrder #121278
Changes from all commits
9e875a4
7319038
43fc3e8
a574a56
480fc7f
5e4bd24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| using System.Collections.Generic; | ||
| using System.Collections.Immutable; | ||
| using System.Globalization; | ||
| using System.Threading; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.CSharp; | ||
|
|
@@ -78,10 +79,24 @@ public void Execute(GeneratorExecutionContext executionContext) | |
|
|
||
| // Stage 3. Emit source code from the spec models. | ||
| OnSourceEmitting?.Invoke(contextGenerationSpecs.ToImmutableArray()); | ||
| Emitter emitter = new(executionContext); | ||
| foreach (ContextGenerationSpec contextGenerationSpec in contextGenerationSpecs) | ||
|
|
||
| // Ensure the source generator emits number literals using invariant culture. | ||
| // 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 commentThe 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 commentThe 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. |
||
| CultureInfo.CurrentCulture = CultureInfo.InvariantCulture; | ||
| try | ||
| { | ||
| Emitter emitter = new(executionContext); | ||
| foreach (ContextGenerationSpec contextGenerationSpec in contextGenerationSpecs) | ||
| { | ||
| emitter.Emit(contextGenerationSpec); | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| emitter.Emit(contextGenerationSpec); | ||
| CultureInfo.CurrentCulture = originalCulture; | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.