Skip to content
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

[Foundation] Fix NSDate's explicit conversion operators with DateTime. #16872

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

rolfbjarne
Copy link
Member

We recently tried to fix NSDate's conversion operators with DateTime
(3c65ab1), but unfortunately a corner case
was missed.

The new approach in the above-mentioned commit would get the individual
date/time components for a given date and use the appropriate constructor for
the other type to re-construct the date/time in question.

However, one case was missed: when converting from NSDate to DateTime, we'd
get a fractional number of milliseconds. This fractional number could be
something like 999.99 milliseconds, and when converting that to the int the
DateTime constructor expected for the number of milliseconds, then DateTime
would throw an exception, because the number of milliseconds could only be
between 0 and 999.

I've solved this by not using floating-point math in the computations. We're
now getting the number of nanoseconds from the NSDate (which is a natural
number, and represents the total number of nanoseconds less than a whole
second), and then converting that to the number of milliseconds, microseconds
and ticks that can be used with DateTime using integral math. Unfortunately
DateTime doesn't have a constructor that takes the remaining number of ticks
after all the other fields have been provided, but that can be added
afterwards.

I've also made a few other improvements:

  • Improve the validation for the NSDate -> DateTime conversion to detect BC
    dates by using the NSDate's Era component (to throw because DateTime only
    supports AC dates). Also don't allow a tick later than year 10.000 (DateTime
    only supports up to a tick before year 10.000) - but explicitly support
    exactly year 10.000, and convert it to DateTime.MaxValue (this is because
    due to precision errors NSDate can't actually express 'a tick before year
    10.000', it ends up being rounded up to year 10.000 exactly). This means
    there are no more magical values in the range validation checks.
  • Increase precision in the DateTime -> NSDate conversion by starting with the
    sub-second amount of ticks from the DateTime instance (instead of the number
    of milliseconds). This allows us to compute the nanoseconds NSDate expects
    with much higher precision.
  • More tests!

Fixes this test:

MonoTouchFixtures.Foundation.DateTest.DateTimeToNSDate : 2 ms
    [FAIL] Precision32022 : System.ArgumentOutOfRangeException : Valid values are between 0 and 999, inclusive.
        Parameter name: millisecond
        at System.DateTime..ctor (System.Int32 year, System.Int32 month, System.Int32 day, System.Int32 hour, System.Int32 minute, System.Int32 second, System.Int32 millisecond, System.DateTimeKind kind) [0x0002d] in <4d40c65adfc14d7fb19bad9310f3eb2a>:0
        at Foundation.NSDate.op_Explicit (Foundation.NSDate d) [0x000b8] in <9cb1e1018c034b75ba5f4ed7b83ba2f2>:0
        at MonoTouchFixtures.Foundation.DateTest.Precision32022 () [0x0000c] in <c44b5df5f7b84b69b737e9fd61bddaed>:0
        at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&)
        at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x0006a] in <4d40c65adfc14d7fb19bad9310f3eb2a>:0

Fixes https://github.com/xamarin/maccore/issues/2632.

Date and time is difficult.

Ref: https://gist.github.com/timvisee/fcda9bbdff88d45cc9061606b4b923ca
Ref: the rest of internet...

We recently tried to fix NSDate's conversion operators with DateTime
(3c65ab1), but unfortunately a corner case
was missed.

The new approach in the above-mentioned commit would get the individual
date/time components for a given date and use the appropriate constructor for
the other type to re-construct the date/time in question.

However, one case was missed: when converting from NSDate to DateTime, we'd
get a fractional number of milliseconds. This fractional number could be
something like 999.99 milliseconds, and when converting that to the int the
DateTime constructor expected for the number of milliseconds, then DateTime
would throw an exception, because the number of milliseconds could only be
between 0 and 999.

I've solved this by not using floating-point math in the computations. We're
now getting the number of nanoseconds from the NSDate (which is a natural
number, and represents the total number of nanoseconds less than a whole
second), and then converting that to the number of milliseconds, microseconds
and ticks that can be used with DateTime using integral math. Unfortunately
DateTime doesn't have a constructor that takes the remaining number of ticks
after all the other fields have been provided, but that can be added
afterwards.

I've also made a few other improvements:

* Improve the validation for the NSDate -> DateTime conversion to detect BC
  dates by using the NSDate's Era component (to throw because DateTime only
  supports AC dates). Also don't allow a tick later than year 10.000 (DateTime
  only supports up to a tick before year 10.000) - but explicitly support
  exactly year 10.000, and convert it to DateTime.MaxValue (this is because
  due to precision errors NSDate can't actually express 'a tick before year
  10.000', it ends up being rounded up to year 10.000 exactly). This means
  there are no more magical values in the range validation checks.
* Increase precision in the DateTime -> NSDate conversion by starting with the
  sub-second amount of ticks from the DateTime instance (instead of the number
  of milliseconds). This allows us to compute the nanoseconds NSDate expects
  with much higher precision.
* More tests!

Fixes this test:

    MonoTouchFixtures.Foundation.DateTest.DateTimeToNSDate : 2 ms
        [FAIL] Precision32022 : System.ArgumentOutOfRangeException : Valid values are between 0 and 999, inclusive.
            Parameter name: millisecond
            at System.DateTime..ctor (System.Int32 year, System.Int32 month, System.Int32 day, System.Int32 hour, System.Int32 minute, System.Int32 second, System.Int32 millisecond, System.DateTimeKind kind) [0x0002d] in <4d40c65adfc14d7fb19bad9310f3eb2a>:0
            at Foundation.NSDate.op_Explicit (Foundation.NSDate d) [0x000b8] in <9cb1e1018c034b75ba5f4ed7b83ba2f2>:0
            at MonoTouchFixtures.Foundation.DateTest.Precision32022 () [0x0000c] in <c44b5df5f7b84b69b737e9fd61bddaed>:0
            at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&)
            at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x0006a] in <4d40c65adfc14d7fb19bad9310f3eb2a>:0

Fixes xamarin/maccore#2632.

Date and time is difficult.

Ref: https://gist.github.com/timvisee/fcda9bbdff88d45cc9061606b4b923ca
Ref: the rest of internet...
@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)
  • iOS (no change detected)
  • tvOS (no change detected)
  • watchOS (no change detected)
  • macOS (no change detected)
NET (empty diffs)
  • iOS: (empty diff detected)
  • tvOS: (empty diff detected)
  • MacCatalyst: (empty diff detected)
  • macOS: (empty diff detected)

❗ API diff vs stable (Breaking changes)

Legacy Xamarin (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
.NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
Legacy Xamarin (stable) vs .NET

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: 285eb190ff6d5397701eba3ba4a1319167494ccd [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: 285eb190ff6d5397701eba3ba4a1319167494ccd [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1180.Monterey'
Hash: 285eb190ff6d5397701eba3ba4a1319167494ccd [PR build]

Copy link
Contributor

@tj-devel709 tj-devel709 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, now I have some understanding why Steve always warns us about DateTime!

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🔥 [CI Build] Test results 🔥

Test results

❌ Tests failed on VSTS: simulator tests

0 tests crashed, 5 tests failed, 218 tests passed.

Failures

❌ bcl tests

5 tests failed, 64 tests passed.
  • [NUnit] Mono Mac OS X BCL tests group 3/Mac Full/Debug: Failed (Test run failed.
    Tests run: 11328 Passed: 10552 Inconclusive: 0 Failed: 1 Ignored: 98)
  • [NUnit] Mono Mac OS X BCL tests group 3/Mac Modern/Debug: Failed (Test run failed.
    Tests run: 8755 Passed: 8174 Inconclusive: 0 Failed: 1 Ignored: 46)
  • [NUnit] Mono BCL tests group 2/iOS Unified 64-bits - simulator/Debug: Failed
  • [NUnit] Mono BCL tests group 2/tvOS - simulator/Debug: Failed
  • [NUnit] Mono CorlibTests/watchOS 32-bits - simulator/Debug: Failed

Html Report (VSDrops) Download

Successes

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 23 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 285eb190ff6d5397701eba3ba4a1319167494ccd [PR build]

@rolfbjarne
Copy link
Member Author

Test failures are unrelated (https://github.com/xamarin/maccore/issues/2629).

@rolfbjarne rolfbjarne merged commit e4c940c into xamarin:main Nov 23, 2022
@rolfbjarne rolfbjarne deleted the fix-nsdate branch November 23, 2022 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants