-
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
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 |
Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
tannergooding
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. It'd probably be nice to have the style checking in a single centralized helper, like it is for the other types, but this is much more consistent now
📋 Breaking Change Documentation RequiredCreate a breaking change issue with AI-generated content Generated by Breaking Change Documentation Tool - 2026-02-03 17:33:42 |
… format specifiers (dotnet#123805) ## Fix DateOnly/TimeOnly TryParse to throw on invalid DateTimeStyles and format specifiers **Summary:** 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`, which is inconsistent with other `TryParse` APIs in .NET. **Changes Made:** - [x] Moved `DateTimeStyles` validation to the public Parse/TryParse methods (upfront validation before calling internal methods) - [x] Moved format validation (null/empty checks) to the public ParseExact/TryParseExact methods (upfront validation) - [x] Simplified internal methods - they no longer need to check for argument validation errors - [x] Modified all `TryParse` and `TryParseExact` overloads in both `DateOnly` and `TimeOnly` to throw exceptions for invalid `DateTimeStyles` values - [x] Enhanced `TryParseExactInternal` methods to detect and immediately return `Argument_BadFormatSpecifier` errors from underlying `DateTimeParse.TryParseExact` - [x] Updated existing tests to expect exceptions instead of `false` returns - [x] Added comprehensive tests for all `TryParseExact` variants with invalid styles and format specifiers - [x] All tests use `AssertExtensions.Throws` with the "style" parameter name for consistency - [x] All 69,639 tests passing (0 failures, 0 errors) **Pattern Consistency:** The implementation now follows the same pattern as `DateTime`, `DateTimeOffset`, and numeric types by validating arguments upfront in the public methods before calling internal parsing logic. This: - Reduces code duplication - Avoids unnecessary work when arguments are invalid - Makes the code more maintainable - Aligns with framework conventions **Breaking Change:** **Yes.** This is a breaking change as code that previously received `false` will now throw an exception. However, this aligns with the behavior of other `TryParse` APIs and improves consistency. The scenarios affected are those that could never succeed (invalid argument combinations), so the impact should be minimal. **Examples:** ```csharp // Before: returned false DateOnly.TryParseExact("2064-07-01", "yyyy-MM-dd", null, DateTimeStyles.AssumeUniversal, out var result); // After: throws ArgumentException with ParamName = "style" // Before: returned false DateOnly.TryParseExact("2064-07-01", new[] { "", "yyyy-MM-dd" }, null, DateTimeStyles.None, out var result); // After: throws FormatException ``` **Files Modified:** - `src/libraries/System.Private.CoreLib/src/System/DateOnly.cs` - Implementation changes - `src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs` - Implementation changes - `src/libraries/System.Runtime/tests/System.Runtime.Tests/System/DateOnlyTests.cs` - Test updates - `src/libraries/System.Runtime/tests/System.Runtime.Tests/System/TimeOnlyTests.cs` - Test updates Fixes dotnet#111969 <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> ---- *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.TryParseExact` with a `DateOnly.TryParseExact` and had to find out, `DateOnly` refuses to work with `DateTimeStyles.AssumeUniversal`. `DateOnly.TryParseExact` behaves inconsistently compared to `DateTime` and `DateTimeOffset` when parsing the same ISO-8601 formatted date string and `DateTimeStyles.AssumeUniversal` is 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.AssumeUniversal` is invalid or unsupported for `DateOnly`. ### Reproduction Steps ```C# using System; using System.Globalization; Console.WriteLine("DateTime:"); Console.WriteLine(DateTime.TryParseExact("2064-07-01", "yyyy-MM-dd", CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal, out _)); Console.WriteLine(DateTime.TryParseExact("2064-07-01", "yyyy-MM-dd", CultureInfo.InvariantCulture, DateTimeStyles.None, out _)); Console.WriteLine("\nDateTimeOffset:"); Console.WriteLine(DateTimeOffset.TryParseExact("2064-07-01", "yyyy-MM-dd", CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal, out _)); Console.WriteLine(DateTimeOffset.TryParseExact("2064-07-01", "yyyy-MM-dd", CultureInfo.InvariantCulture, DateTimeStyles.None, out _)); Console.WriteLine("\nDateOnly:"); Console.WriteLine(DateOnly.TryParseExact("2064-07-01", "yyyy-MM-dd", CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal, out _)); Console.WriteLine(DateOnly.TryParseExact("2064-07-01", "yyyy-MM-dd", CultureInfo.InvariantCulture, DateTimeStyles.None, out _)); // Output: // DateTime: // True // True // DateTimeOffset: // True // True // DateOnly: // False // True ``` ### 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 `DateTime` and `DateTimeOffset` return true. ### Regression? _No response_ ### Known Workarounds Use `DateTimeStyles.None` instead ### Configuration 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) <comments> <comment_new><author>@tarekgh</author><body> 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](https://learn.microsoft.com/en-us/dotnet/standard/datetime/how-to-use-dateonly-timeonly#the-dateonly-structure) > The `DateTime` structure may roll into the previous or next day when adjusted for a time zone. `DateOnly` cannot be offset by a time zone and always represents the exact date that was set. 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. </body></comment_new> <comment_new><author>@tannergooding</author><body> @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 therefore `DateTimeStyles.AssumeUniversal` is "invalid" for `DateOnly` and *should* be throwing accordingly (as all of our other `TryParse` APIs behave with invalid styles)</body></comment_new> <comment_new><author>@tarekgh</author><body> @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!</body></comment_new> <comment_new><author>@tannergooding</author><body> 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 ... </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes dotnet#123802 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
Fix DateOnly/TimeOnly TryParse to throw on invalid DateTimeStyles and format specifiers
Summary:
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.Changes Made:
DateTimeStylesvalidation to the public Parse/TryParse methods (upfront validation before calling internal methods)TryParseandTryParseExactoverloads in bothDateOnlyandTimeOnlyto throw exceptions for invalidDateTimeStylesvaluesTryParseExactInternalmethods to detect and immediately returnArgument_BadFormatSpecifiererrors from underlyingDateTimeParse.TryParseExactfalsereturnsTryParseExactvariants with invalid styles and format specifiersAssertExtensions.Throwswith the "style" parameter name for consistencyPattern Consistency:
The implementation now follows the same pattern as
DateTime,DateTimeOffset, and numeric types by validating arguments upfront in the public methods before calling internal parsing logic. This:Breaking 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 updatesFixes #111969
Original 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.