Fix PointConverter's leading white-space handling#130692
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
@dotnet-policy-service agree |
|
Tagging subscribers to this area: @dotnet/area-system-componentmodel |
bd48b6f to
a44a455
Compare
jeffhandley
left a comment
There was a problem hiding this comment.
Note
This review was generated by the holistic code review workflow being iterated on as part of #130339. Please treat its findings as assistive input for human review.
Holistic Review
Motivation: The compatibility problem is real: PointConverter trims the input before splitting, so leading whitespace makes the split ranges relative to a different span than the original string that was being sliced. Matching .NET Framework behavior for WinForms/resx data with leading whitespace is justified.
Approach: For PointConverter, slicing the same trimmed span that produced the Range values is the right targeted fix and preserves the existing culture-specific separator and number conversion behavior. However, the same parsing pattern exists in sibling drawing converters, so the root cause appears only partially addressed.
Summary: PointConverter code change itself looks correct, and no new public API surface is introduced. Before merging, I think the same trimmed-span/range mismatch should be fixed and tested in the sibling geometric converters (RectangleConverter, SizeConverter, and SizeFConverter) or explicitly acknowledged as a deliberately deferred follow-up by the area owners.
Detailed Findings
✅ Correctness — PointConverter now slices the same span it split
In src/libraries/System.ComponentModel.TypeConverter/src/System/Drawing/PointConverter.cs lines 29-48, the ranges returned by text.Split(...) are now applied back to text rather than to strValue. That fixes the leading-whitespace failure because the range coordinates are relative to the trimmed span, while keeping null/empty handling, culture-specific list separators, trailing whitespace tolerance, and int conversion semantics unchanged.
❌ Cross-cutting completeness — sibling converters still have the same bug
The same pattern remains in the other geometric converters:
src/libraries/System.ComponentModel.TypeConverter/src/System/Drawing/RectangleConverter.cslines 29-50 trims intotext, splitstext, then slicesstrValue[ranges[i]].src/libraries/System.ComponentModel.TypeConverter/src/System/Drawing/SizeConverter.cslines 29-48 does the same forSize.src/libraries/System.ComponentModel.TypeConverter/src/System/Drawing/SizeFConverter.cslines 29-48 does the same forSizeF.
Those ranges are also relative to the trimmed span, so inputs like " 1, 2", " 1, 2, 3, 4", or culture-equivalent values will still parse the wrong substring when the original string has leading whitespace. Recent file history shows these converters were refactored together in 350b9cb5438 Refactor geometric type converters for improved parsing (#111349), which is consistent with this being the same regression. Given the PR is fixing .NET Framework-compatible leading-whitespace handling, I would update the sibling converters in the same PR (or have maintainers explicitly decide to defer them with a tracking issue).
⚠️ Test Quality — coverage is narrow to PointConverter
src/libraries/System.ComponentModel.TypeConverter/tests/Drawing/PointConverterTests.cs lines 233-241 now covers a leading-whitespace ConvertFromString input for PointConverter, which is useful and should fail without the production fix. If the sibling converters are fixed here, please add matching regression coverage in RectangleConverterTests, SizeConverterTests, and SizeFConverterTests as well so the shared parsing bug cannot recur there.
Allow PointConverter to convert string values that have leading white-space. Aligns behavior with .NET Framework 4.x
Test have been updated to include an extra space.
Fixes #130690