-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix DateOnly/TimeOnly TryParse to throw on invalid DateTimeStyles and format specifiers #123805
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
…imeStyles Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
…tency Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/DateOnlyTests.cs
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/TimeOnlyTests.cs
Show resolved
Hide resolved
|
@copilot please address all opened feedback |
…thods Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
… for bad format specifiers Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
Fixed in 492c226. Now checking
Fixed in 492c226. Applied the same fix for TimeOnly single format overload.
Fixed in 492c226. Applied the same fix for TimeOnly multiple formats overload.
Fixed in 492c226. Now tracking and preserving
All feedback addressed in commit 492c226. Summary of changes:
|
Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
|
/asp run runtime |
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 consistency issue where DateOnly.TryParse/TryParseExact and TimeOnly.TryParse/TryParseExact methods were returning false for invalid DateTimeStyles parameters instead of throwing an ArgumentException, aligning their behavior with other TryParse APIs in .NET. Additionally, it ensures that invalid format specifiers (null/empty formats in arrays) properly throw FormatException instead of being masked as general format errors.
Changes:
- Modified all
TryParseandTryParseExactoverloads in bothDateOnlyandTimeOnlyto throw exceptions for invalidDateTimeStylesvalues - Enhanced
TryParseExactInternalmethods to detect and immediately returnArgument_BadFormatSpecifiererrors from underlyingDateTimeParse.TryParseExact - Updated existing tests to expect exceptions instead of
falsereturns, and added comprehensive new test cases for all variants
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/DateOnly.cs | Modified TryParse and all TryParseExact overloads to throw on invalid DateTimeStyles; enhanced TryParseExactInternal to detect bad format specifiers |
| src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs | Modified TryParse and all TryParseExact overloads to throw on invalid DateTimeStyles; enhanced TryParseExactInternal to detect bad format specifiers |
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/DateOnlyTests.cs | Updated existing tests to expect exceptions; added comprehensive tests for invalid styles and format specifiers |
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/TimeOnlyTests.cs | Updated existing tests to expect exceptions; added comprehensive tests for invalid styles and format specifiers |
|
CC @stephentoub |
Description
This PR fixes a consistency issue where
DateOnly.TryParse/TryParseExactandTimeOnly.TryParse/TryParseExactmethods were returningfalsefor invalidDateTimeStylesparameters instead of throwing anArgumentException, which is inconsistent with otherTryParseAPIs in .NET.Additionally, this PR ensures that
Argument_BadFormatSpecifiererrors from the underlyingDateTimeParse.TryParseExactare properly detected and returned immediately, rather than being masked as format errors or deferred until after all formats are tried.Changes Made
DateOnly.TryParseand allDateOnly.TryParseExactoverloads to throwArgumentExceptionfor invalidDateTimeStylesTimeOnly.TryParseand allTimeOnly.TryParseExactoverloads to throwArgumentExceptionfor invalidDateTimeStylesfalsereturnsTryParseExactvariants with invalid stylesAssertExtensions.Throwswith the "style" parameter name for consistencyArgument_BadFormatSpecifiercheck fromTryParsemethods (only needed inTryParseExact)TryParseExactInternalmethods to checkdtResult.failureand return immediately whenArgument_BadFormatSpecifieris encountered fromDateTimeParse.TryParseExactBreaking Change
Yes. This is a breaking change as code that previously received
falsewill now throw an exception. However, this aligns with the behavior of otherTryParseAPIs and improves consistency. The scenarios affected are those that could never succeed (invalid argument combinations), so the impact should be minimal.Examples:
Files Modified
src/libraries/System.Private.CoreLib/src/System/DateOnly.cs- Implementation changessrc/libraries/System.Private.CoreLib/src/System/TimeOnly.cs- Implementation changessrc/libraries/System.Runtime/tests/System.Runtime.Tests/System/DateOnlyTests.cs- Test updatessrc/libraries/System.Runtime/tests/System.Runtime.Tests/System/TimeOnlyTests.cs- Test updatesOriginal prompt
This section details on the original issue you should resolve
<issue_title>DateOnly TryParseExact fails to parse basic ISO-8601 input</issue_title>
<issue_description>### Description
Ive had to hunt down a bug, where I swapped a
DateTime.TryParseExactwith aDateOnly.TryParseExactand had to find out,DateOnlyrefuses to work withDateTimeStyles.AssumeUniversal.DateOnly.TryParseExactbehaves inconsistently compared toDateTimeandDateTimeOffsetwhen parsing the same ISO-8601 formatted date string andDateTimeStyles.AssumeUniversalis supplied.This is either a bug, or a highly misleading API, since (as far as I could find) nothing in the documentation indicates that
DateTimeStyles.AssumeUniversalis invalid or unsupported forDateOnly.Reproduction Steps
Expected behavior
Return true, parsing the input correctly without failure.
Or alternatively, as others suggested, throw a clear exception.
Actual behavior
Return false, whereas similar (if not under the hood identical) APIs for
DateTimeandDateTimeOffsetreturn true.Regression?
No response
Known Workarounds
Use
DateTimeStyles.NoneinsteadConfiguration
Target Framework: NET 10
SDK: 10.0.102
Arch.: x64
Other information
No response</issue_description>
<agent_instructions>We need to fix all DateOnly and TimeOnly TryParse and TryParseExact methods to throw exception when passing invalid DateTimeStyle parameter instead of just return false. Ensure adding a new tests cover the fix and if possible, file a new breading change issue in https://github.com/dotnet/docs/issues using the breaking change template.</agent_instructions>
Comments on the Issue (you are @copilot in this section)
@tarekgh This is not a bug. `DateOnly` and `TimeOnly` are intentionally designed to be independent of time zones. They represent abstract date and time values that are not associated with any region or time zone.The official documentation clearly explains this behavior:
https://learn.microsoft.com/en-us/dotnet/standard/datetime/how-to-use-dateonly-timeonly#the-dateonly-structure
If you have any suggestions to improve any docs, please feel free to submit issue or a PR in https://github.com/dotnet/dotnet-api-docs. Thanks for your report.
@tannergooding @tarekgh I believe the supposed bug here is that unlike other `TryParse` APIs, `DateOnly` doesn't throw for an invalid style and simply returns `false` making it inconsistent with the general rule that we should only have one mode of failure.That is, there is no string that would make
DateOnly.TryParseExact("2064-07-01", "yyyy-MM-dd", CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal, out _)succeed and thereforeDateTimeStyles.AssumeUniversalis "invalid" forDateOnlyand should be throwing accordingly (as all of our otherTryParseAPIs behave with invalid styles)</comment_new><comment_new>@tarekgh
@tannergooding you are correct regarding that but honestly, I am not sure if this deserve to fix for the sake of consistency while it introduces a breaking change we can avoid. Let me know if you still think we should fix it and we can consider it. Thanks!</comment_new>
<comment_new>@tannergooding
We've got at least one customer report on it already and it is something that creates other potential problems for users, i.e. its a static bug and so any typical "get user input, try to ...
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.