-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Copilot: Fix culture-sensitive StringBuilder.Append and ToString calls across codebase and add banned symbols #80800
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
Conversation
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
| // This test verifies that negative alignment values are formatted using invariant culture. | ||
| // The issue occurs when compiling under non-US cultures (e.g., Swedish sv-SE) where the | ||
| // minus sign is formatted as '−' (U+2212) instead of '-' (U+002D), causing runtime errors. | ||
| var source = @" |
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.
@copilot use raw string literals for tests.
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.
Converted to raw string literals in commit b0a5c12.
| } | ||
| } | ||
| "; | ||
| var comp = CompileAndVerify(source, expectedOutput: @" 1 |
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.
@copilot is tehre some way to set teh culture for the compiler for a particular test?
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.
Added CultureContext to explicitly test compilation under Swedish culture (sv-SE) in commit b0a5c12. The test now wraps the compilation in using (new CultureContext(new CultureInfo("sv-SE", useUserOverride: false))) to verify the fix works correctly even when the compiler runs under a culture where minus signs are formatted differently.
…ng alignment test Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Co-authored-by: AlekseyTs <10522107+AlekseyTs@users.noreply.github.com>
|
Done with review pass (commit 3) |
| } | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/runtime/issues/44678")] | ||
| public void AlignmentWithNegativeValue() |
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.
@copilot Please add similar test for VB compiler
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.
Added VB test in commit 4f8e7a9. The test verifies that negative alignment values compile correctly under Swedish culture (sv-SE) and validates the emitted IL contains the proper format string {0,-100}.
…lues Co-authored-by: AlekseyTs <10522107+AlekseyTs@users.noreply.github.com>
|
Done with review pass (commit 9) |
AlekseyTs
left a comment
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.
LGTM (commit 9)
|
@CyrusNajmabadi Please review |
|
@dotnet/roslyn-compiler For a second review |
333fred
left a comment
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.
Can we add the bad ToString apis to src/Compilers/Core/Portable/BannedSymbols.CompilerLibraries.txt, so we don't accidentally reintroduce this problem?
jasonmalinowski
left a comment
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.
Signing off on the non-Compiler changes.
|
@copilot Can you add the bad ToString/Append apis to src/Compilers/Core/Portable/BannedSymbols.CompilerLibraries.txt |
…nned symbols Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Added in commit a7f9c2e. Banned all culture-sensitive numeric ToString() overloads (parameterless versions for SByte, Byte, Int16, UInt16, Int32, UInt32, Int64, UInt64, Single, Double, Decimal) and their corresponding StringBuilder.Append() overloads. These methods will now trigger build errors if used in compiler code, directing developers to use ToString(CultureInfo.InvariantCulture) instead. |
|
@copilot the bans caught a few more locations that are using the banned apis; run a build locally using |
… symbols Co-authored-by: 333fred <2371880+333fred@users.noreply.github.com>
Fixed in commit e4a8f9b. Applied InvariantCulture to all additional ToString() violations caught by the banned symbols across 22 more files (10 C# compiler files, 10 VB compiler files, and 2 dependency files). Build with |
Fix culture-sensitive numeric formatting in compiler-generated strings
Issue: When compiling under non-US cultures (e.g., Swedish sv-SE), the compiler uses culture-specific number formatting when appending numeric values to strings. This causes issues like:
$"{st,-100}") generate invalid format strings with culture-specific minus signs (−U+2212 instead of-U+002D), leading to runtime FormatExceptionRoot Cause: Throughout the codebase,
stringBuilder.Appendwith numeric values and parameterlessToString()methods were used, which use the current culture's formatting rules.Solution: Modified the code to use
ToString(System.Globalization.CultureInfo.InvariantCulture)for all numeric StringBuilder.Append and ToString operations to ensure values are always formatted with standard ASCII characters, regardless of the compilation culture.Changes:
Core Compiler (
src/Compilers/Core/Portable/) - 12 files:DocumentationCommentId.cs: Fixed TypeParameters.Length and Ordinal appends (5 instances)AssemblyIdentity.DisplayName.cs: Fixed version number appends (Major, Minor, Build, Revision)Syntax/LineMapping.cs: Fixed CharacterOffset appendBannedSymbols.CompilerLibraries.txt: Added 23 banned symbol entries to prevent future regressionsInternalUtilities/StringExtensions.cs: Fixed numeric ToStringCodeGen/PrivateImplementationDetails.cs: Fixed submissionSlotIndex and analysisIndex ToString (2 instances)CodeGen/LocalOrParameter.cs: Fixed ParameterIndex ToStringCodeGen/DebugId.cs: Fixed Ordinal ToStringPEWriter/MetadataWriter.PortablePdb.cs: Fixed CompilationOptionsVersion, SourceFileCount, and PortabilityPolicy ToString (3 instances)ExtendedSpecialType.cs: Fixed _value ToStringDiagnosticAnalyzer/ShadowCopyAnalyzerPathResolver.cs: Fixed _directoryCount ToStringMetadataReader/MetadataTypeName.cs: Fixed _forcedArity ToStringC# Compiler (
src/Compilers/CSharp/Portable/) - 5 files:Binder/Binder_InterpolatedString.cs: Fixed nextFormatPosition and alignment value appendsDocumentationComments/DocumentationCommentIDVisitor.PartVisitor.cs: Fixed Arity and Ordinal appends (3 instances)Symbols/SymbolCompletionState.cs: Fixed completion part index appendSymbols/Synthesized/GeneratedNames.cs: Fixed methodOrdinal, entityOrdinal, generation, slotIndex, ownerUniqueId, and id appends (7 instances)Symbols/Synthesized/SynthesizedLocal.cs: Fixed sequence and createdAtLineNumber appends (3 instances)VB Compiler (
src/Compilers/VisualBasic/Portable/) - 11 files:Binding/Binder_InterpolatedString.vb: Already had InvariantCulture in place (verified)DocumentationComments/DocumentationCommentIDVisitor.PartVisitor.vb: Fixed Arity and Ordinal appends (3 instances)Symbols/SynthesizedSymbols/GeneratedNames.vb: Fixed methodOrdinal, entityOrdinal, and generation appends (4 instances)Binding/Binder_AnonymousTypes.vb: Fixed fieldIndex ToStringBinding/Binder_Query.vb: Fixed i ToString in anonymous field nameCodeGen/EmitStatement.vb: Fixed cur ToString in case block labelCodeGen/Optimizer/StackScheduler.LocalDefUseSpan.vb: Fixed Start and End ToString (2 instances)Errors/DiagnosticFormatter.vb: Fixed line number ToStringSymbolDisplay/ObjectDisplay.vb: Fixed codepoint ToString (2 instances)SymbolDisplay/SymbolDisplayVisitor.Types.vb: Fixed Arity ToStringSymbols/Attributes/AttributeData.vb: Fixed value ToStringSymbols/Source/RangeVariableSymbol.vb: Fixed _syntax.Position ToStringDependencies (
src/Dependencies/) - 2 files:Contracts/Index.cs: Fixed Value ToString (2 instances)Contracts/Range.cs: Fixed Value ToStringWorkspace Files (
src/Workspaces/) - 4 files:Remote/ServiceHub/Services/DiagnosticAnalyzer/PerformanceTrackerService.cs: Fixed unitCount, BuiltIn, and TotalMilliseconds appendsCore/Portable/Log/HistogramLogAggregator.cs: Fixed bucket value appendsCore/Portable/Log/RoslynEventSource.cs: Fixed enum value appendRemote/Core/ExportProviderBuilder.cs: Fixed Version.Major appendTest Infrastructure (
src/Compilers/Test/) - 9 files:CSharp/Test/Emit3/Diagnostics/OperationAnalyzerTests.cs: Fixed test code generationCSharp/Test/Emit3/Semantics/CollectionExpressionTests.cs: Fixed test code generationCSharp/Test/Emit/CodeGen/CodeGenOperators.cs: Fixed test code generation (9 instances)CSharp/Test/EndToEnd/EndToEndTests.cs: Fixed test code generationCSharp/Test/Semantic/Semantics/SemanticErrorTests.cs: Fixed test code generationTest/Core/Diagnostics/DiagnosticDescription.cs: Fixed line and character position appendsTest/Core/Platform/Desktop/CLRHelpers.cs: Fixed format string index appendTest/Utilities/VisualBasic/ParserTestUtilities.vb: Fixed error ID appendVisualBasic/Test/Emit/CodeGen/CodeGenTests.vb: Fixed test code generation (6 instances)Editor Features (
src/EditorFeatures/) - 1 file:Test2/Classification/SyntacticChangeRangeComputerTests.vb: Fixed test code generationVisual Studio Integration (
src/VisualStudio/) - 1 file:Core/Impl/CodeModel/MetadataNameHelpers.cs: Fixed TypeArguments.Length appendTesting:
C# Test (
CodeGenInterpolatedString.cs):CultureContextto explicitly test compilation under Swedish culture (sv-SE){0,-100}) even when compiled under sv-SE cultureVB Test (
InterpolatedStringTests.vb):All existing tests continue to pass for both C# and VB compilers
Build with analyzers (
./build.sh --runAnalyzers) passes with no RS0030 violationsPrevention of Future Regressions:
Added 23 entries to
BannedSymbols.CompilerLibraries.txtto prevent reintroduction of culture-sensitive formatting issues:StringBuilder.Append()overloads (SByte, Byte, Int16, UInt16, Int32, UInt32, Int64, UInt64, Single, Double, Decimal)ToString()methods for all numeric typesToString(CultureInfo.InvariantCulture)insteadFiles Modified: 44 files total across:
This comprehensive fix ensures all parts of the compiler, test infrastructure, workspace services, and related tools generate consistent, culture-independent output regardless of the system culture during compilation or testing. The banned symbols list prevents future regressions by failing the build if culture-sensitive APIs are used.
Fixes #49826
Original prompt
This section details on the original issue you should resolve
<issue_title>FormatException on right padding a interpolated string</issue_title>
<issue_description>
Description
Right padding a string with interpolated strings throw a System.FormatException
Code to reproduce:
Configuration
Bug reproduced with net5.0 and netcore3.0 build target.
dotnet --info
.NET SDK (reflecting any global.json):
Version: 5.0.100
Commit: 5044b93829
Runtime Environment:
OS Name: ubuntu
OS Version: 18.04
OS Platform: Linux
RID: ubuntu.18.04-x64
Base Path: /usr/share/dotnet/sdk/5.0.100/
Host (useful for support):
Version: 5.0.0
Commit: cf258a14b7
.NET SDKs installed:
3.1.404 [/usr/share/dotnet/sdk]
5.0.100 [/usr/share/dotnet/sdk]
.NET runtimes installed:
Microsoft.AspNetCore.App 3.1.10 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 5.0.0 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.NETCore.App 3.1.10 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.0 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
Operating System: Feren OS 2020.07
KDE Plasma Version: 5.19.4
KDE Frameworks Version: 5.72.0
Qt Version: 5.14.2
Kernel Version: 5.4.0-56-generic
OS Type: 64-bit
Processors: 16 × AMD Ryzen 7 3700X 8-Core Processor
Memory: 31,4 GiB of RAM
Graphics Processor: AMD Radeon RX 5700
Regression?
Found this when porting a project from .net framework 4.6.
Other information
I have found no workarounds.</issue_description>
<agent_instructions>Copilot, it looks like we're appending a value in a culture sensitive fashion, instead of in an invariant fashion. Can you fix the offending line
stringBuilder.Append(',').Append(fillin.Alignment.ConstantValueOpt.Int64Value);We'll likely need a test that come somehow verify the emitted IL under a non en-US culture. Not sure if the compiler test harness makes that possible or not.</agent_instructions>
Comments on the Issue (you are @copilot in this section)
@Dotnet-GitSync-Bot I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one [area label](https://github.com/dotnet/runtime/blob/master/docs/area-owners.md). @stephentoub I tried this on both .NET Core 3.1 and .NET 5, on both Windows and Ubuntu, and I could not repro a FormatException. @stephentoub What is your CultureInfo.CurrentCulture? @stephentoub I'm guessing this is due to the CLDR defining the minus sign as `−` rather than `-` for sv-SE: https://github.com/dotnet/runtime/issues/44678#issuecomment-727261177 @Dotnet-GitSync-Bot I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one [area label](https://github.com/dotnet/runtime/blob/master/docs/area-owners.md). @ Tagging subscribers to this area: @tarekgh, @safern, @krwq See info in area-owners.md if you want to be subscribed.Issue Details
Description
Right padding a string with interpolated strings throw a System.FormatException
Code to reproduce:
Configuration
Bug reproduced with net5.0 and netcore3.0 build target.
dotnet --info
.NET SDK (reflecting any global.json):
Version: 5.0.100
Commit: 5044b93829
Runtime Environment:
OS Name: ubuntu
OS Version: 18.04
OS Platform: Linux
RID: ubuntu.18.04-x64
Base Path: /usr/share/dotnet/sdk/5.0.100/
Host (useful for support):
Version: 5.0.0
Commit: cf258a14b7
.NET SDKs installed:
3.1.404 [/usr/share/dotnet/sdk]
5.0.100 [/usr/share/dotnet/sdk]
.NET runtimes installed:
Microsoft.AspNetCore.App 3.1.10 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 5.0.0 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.NETCore.App 3.1.10 [/usr/share/dotnet/shared/Microsoft....
Fixes #49826
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.