Skip to content

Commit 839c101

Browse files
committed
fix(time-zone): conversions clamp, adjustment rules validate (#2186)
#2186 asked five System::TimeZone parity questions, each "requiring the .NET reference or a managed runtime that this container does not have". /rv answers all five: three repairs, two already correct. (1) THE CONVERSIONS CLAMP. .NET builds its result through private static DateTime SafeCreateDateTimeFromTicks(long ticks, DateTimeKind kind = ...) => (ulong)ticks <= DateTime.MaxTicks ? new DateTime(ticks, kind) : (ticks < 0 ? DateTime.MinValue : DateTime.MaxValue); -- TimeZoneInfo.Cache.cs:340-342 so ConvertTimeFromUtc(DateTime::MaxValue, +14) is MaxValue, not an exception. The cast to ulong is the whole trick and is reproduced: a negative tick count wraps to something enormous, so one unsigned comparison rejects both ends at once. ConvertTime clamps ONCE, at the end. .NET computes the result "from raw ticks to avoid precision loss from double-clamping" (TimeZoneInfo.cs:683-685), so an intermediate UTC value that leaves the range must not drag a representable final answer to a bound. A mutation that clamps twice is caught. .NET does NOT clamp everywhere, and that is recorded rather than smoothed over: its invalid-time compatibility path builds a raw `new DateTime(...)` and lets it throw, with a comment saying so (:661-667). That path needs TimeZoneInfoOptions and adjustment rules this port's TimeZoneInfo does not model. (2)+(3) THE THREE VALIDATIONS, AND THE REFERENCE CORRECTS THE TICKET ON TWO OF THEM. #2179 measured all three as accepted and declined to repair them because "inventing three more rejections on a recollection of the .NET source is exactly what this review declines to do". That was right: * the daylightDelta range is NOT +/-14 hours. It is -23.0 .. 14.0, and .NET explains why in a comment of its own -- Samoa moved across the International Date Line, so describing its delta needs -23. THE MESSAGE STILL SAYS "plus or minus 14.0 hours", because it is shared with UtcOffsetOutOfRange. That inconsistency is .NET's and is transcribed rather than tidied; * the seconds check is not "sub-minute". It is "not a whole number of minutes", so 1h30m30s fails as surely as 30s does; * the time-of-day check EXEMPTS MinValue for dateStart and MaxValue for dateEnd. One conjunct of .NET's time-of-day check is `Kind == Unspecified`. This port has no DateTimeKind (a permanent deviation), so it is absent -- which makes this port stricter for a UTC-kinded argument and identical for every argument it can express. (4) TWO QUESTIONS, TWO DIFFERENT ANSWERS. TryFindSystemTimeZoneById returns false for every failure and already matched: .NET's discards the exception outright (TimeZoneInfo.cs:526-527). The throwing form was the divergence -- #2183 folded three failures into one boolean and kept TimeZoneNotFoundException "rather than guessing InvalidTimeZoneException". .NET raises InvalidTimeZoneException when the file EXISTS but is not zone data (TimeZoneInfo.Unix.cs:697) and reserves TimeZoneNotFoundException for an id that names nothing. Those are different answers to different questions, and a caller catching only the first used to swallow the second. (5) ALREADY CORRECT, AND THE REFERENCE CONTAINS A TRAP. TimeZone.CalculateUtcOffset first decides isDst from the DST window, which puts an ambiguous 01:30 INSIDE daylight -- and the next line overrides it: if (isDst && time >= ambiguousStart && time < ambiguousEnd) isDst = time.IsAmbiguousDaylightSavingTime(); -- TimeZone.cs:237-240 That flag is set only by a prior UTC-to-local conversion, so a DateTime built from its fields carries false and the answer is the STANDARD offset. Reading only the window test gives the opposite answer. #2182 chose standard and chose right; the pin's parenthetical is now a measurement rather than a recollection. Two gated pins inverted, three cases replacing two. Five mutations, all caught. Downstream: neither cna nor mobile-eggbert references TimeZoneInfo or TimeZone -- zero sites in both. Gate: 17,329 run, 17,329 passed, 0 failed, 0 skipped across 38 executables, GREEN. docs/Migration-TimeZoneClampAndValidation.md
1 parent 6404029 commit 839c101

7 files changed

Lines changed: 410 additions & 34 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — time-zone conversions clamp, and adjustment rules validate (ticket #2186)
5+
6+
*2026-08-18.* #2186 asked five `System::TimeZone` parity questions, each *"requiring the .NET
7+
reference or a managed runtime that this container does not have"*. `/rv` answers all five.
8+
**Three were repairs, two were already correct — and the reference corrects the ticket's own
9+
statement of two of the repairs.**
10+
11+
Landed under `docs/StandingApprovals.md` SA-5. No signature, layout or `noexcept` change.
12+
13+
---
14+
15+
## 1. The five answers
16+
17+
| # | Question | .NET | Outcome |
18+
|---|---|---|---|
19+
| 1 | Do the conversions clamp at `DateTime::Min/MaxValue`? | **yes** (`TimeZoneInfo.Cache.cs:340-342`) | **repaired** |
20+
| 2 | Does `CreateAdjustmentRule` reject three further shapes? | **yes** (`AdjustmentRule.cs:206-223`) | **repaired**, with two corrections |
21+
| 3 | What are the exact resource strings? | four of them | **landed** |
22+
| 4 | Does `TryFind…` return false for every failure? Which exception for non-zone data? | false always; **`InvalidTimeZoneException`** | half already correct, half **repaired** |
23+
| 5 | For an ambiguous local time, is the standard reading preferred? | **yes** | **already correct** |
24+
25+
## 2. Question 1 — the conversions clamp
26+
27+
```csharp
28+
private static DateTime SafeCreateDateTimeFromTicks(long ticks, DateTimeKind kind = …)
29+
=> (ulong)ticks <= DateTime.MaxTicks ? new DateTime(ticks, kind)
30+
: (ticks < 0 ? DateTime.MinValue : DateTime.MaxValue);
31+
```
32+
33+
| Call | Was | Is |
34+
|---|---|---|
35+
| `ConvertTimeFromUtc(DateTime::MaxValue, +14)` | `ArgumentOutOfRangeException` | `DateTime::MaxValue` |
36+
| `ConvertTimeToUtc(DateTime::MinValue, +14)` | `ArgumentOutOfRangeException` | `DateTime::MinValue` |
37+
| any in-range conversion || **unchanged** |
38+
39+
The cast to `ulong` is the whole trick and is reproduced deliberately: a negative tick count wraps
40+
to something enormous, so **one** unsigned comparison rejects both ends at once.
41+
42+
`ConvertTime(dt, source, destination)` clamps **once, at the end**. .NET computes the result *"from
43+
raw ticks to avoid precision loss from double-clamping"* (`TimeZoneInfo.cs:683-685`), so an
44+
intermediate UTC value that leaves the range must not drag a representable final answer to a bound.
45+
A test pins that, and a mutation that clamps twice is caught.
46+
47+
**.NET does not clamp everywhere**, and the exception is documented rather than smoothed over: its
48+
invalid-time compatibility path builds a raw `new DateTime(...)` and lets it throw, with a comment
49+
saying so (`TimeZoneInfo.cs:661-667`). That path needs `TimeZoneInfoOptions` and adjustment rules
50+
this port's `TimeZoneInfo` does not model, so it is unreachable here.
51+
52+
## 3. Question 2 — and two of the ticket's three statements were wrong
53+
54+
#2179 measured all three as accepted and declined to repair them, because *"inventing three more
55+
rejections on a recollection of the .NET source is exactly what this review declines to do."* That
56+
was the right call, and the reference shows why:
57+
58+
* **the `daylightDelta` range is not ±14 hours.** It is `-23.0 .. 14.0`, and .NET explains why in
59+
a comment of its own: *Samoa moved across the International Date Line*, so describing its delta
60+
needs −23. **The message still says "plus or minus 14.0 hours"**, because it is shared with
61+
`UtcOffsetOutOfRange`. That inconsistency is .NET's and is transcribed rather than tidied;
62+
* **the seconds check is not "sub-minute".** It is *not a whole number of minutes*, so
63+
`1h30m30s` fails as surely as `30s` does;
64+
* the time-of-day check **exempts** `MinValue` for `dateStart` and `MaxValue` for `dateEnd`.
65+
66+
One conjunct of .NET's time-of-day check is `Kind == Unspecified`. This port has no `DateTimeKind`
67+
(a permanent deviation), so it is absent — which makes this port **stricter** for a UTC-kinded
68+
argument and identical for every argument this port can express.
69+
70+
## 4. Question 4 — two questions, two different answers
71+
72+
`TryFindSystemTimeZoneById` returns **false for every failure**: .NET's discards the exception
73+
outright (`TimeZoneInfo.cs:526-527`), and this port's `catch (...)` already did the same.
74+
75+
The throwing form is where the divergence was. #2183 folded three failures into one boolean and
76+
kept `TimeZoneNotFoundException` for all of them *"rather than guessing InvalidTimeZoneException"*.
77+
.NET raises `InvalidTimeZoneException` when the file **exists but is not zone data**
78+
(`TimeZoneInfo.Unix.cs:697`) and reserves `TimeZoneNotFoundException` for an id that names nothing.
79+
Those are different answers to different questions — *"there is no such zone"* versus *"that is not
80+
a zone"* — and a caller catching only the first used to swallow the second.
81+
82+
## 5. Question 5 — already correct, and the reference contains a trap
83+
84+
.NET's legacy `TimeZone.CalculateUtcOffset` first decides `isDst` from the DST window, which puts
85+
an ambiguous 01:30 **inside daylight** — and then overrides it:
86+
87+
```csharp
88+
if (isDst && time >= ambiguousStart && time < ambiguousEnd)
89+
isDst = time.IsAmbiguousDaylightSavingTime(); // TimeZone.cs:237-240
90+
```
91+
92+
That flag is set only by a prior UTC→local conversion, so a `DateTime` built from its fields
93+
carries `false` and the answer is the **standard** offset. **Reading only the window test gives the
94+
opposite answer**, and that is the trap: the override decides it, not the window. #2182 chose
95+
standard, and it chose right.
96+
97+
## 6. To migrate
98+
99+
Code that caught `ArgumentOutOfRangeException` around a conversion at the extremes will no longer
100+
see it; the result is clamped instead. Code that builds adjustment rules with a time-of-day, a
101+
sub-minute delta or a delta beyond `-23..14` hours will now be rejected.
102+
103+
## 7. Evidence
104+
105+
| Mutation | Caught |
106+
|---|---|
107+
| Clamp both ends to `MaxValue` (the sign is ignored) ||
108+
| `ConvertTime` clamps the intermediate UTC value too ||
109+
| The `daylightDelta` range becomes a symmetric ±14 ||
110+
| The whole-minutes check becomes a sub-minute check ||
111+
| Non-zone-data goes back to `TimeZoneNotFoundException` | ✅ (3 tests) |
112+
113+
## 8. Downstream
114+
115+
Neither `cna` nor `mobile-eggbert` references `TimeZoneInfo` or `TimeZone` — zero sites in both.

modules/time-zone/include/System/TimeZoneInfo.hpp

Lines changed: 106 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,68 @@ namespace System {
293293
"dateStart");
294294
}
295295

296+
/**
297+
* @brief The three further validations .NET's ValidateAdjustmentRule performs.
298+
*
299+
* Ticket #2186 (2026-08-18). #2179 measured all three as accepted here and
300+
* deliberately did not repair them, because "the audit's managed probe covers only
301+
* the reversed date range, and inventing three more rejections on a recollection of
302+
* the .NET source is exactly what this review declines to do". The reference is
303+
* available now (`TimeZoneInfo.AdjustmentRule.cs:174-223`), and it **corrects the
304+
* ticket's own statement of two of the three**:
305+
*
306+
* - the `daylightDelta` range is NOT +/-14 hours. It is `-23.0 .. 14.0`, and .NET
307+
* explains why in a comment of its own: Samoa moved across the International Date
308+
* Line, so describing its daylight delta needs -23. The MESSAGE still says "plus
309+
* or minus 14.0 hours", because it is shared with `UtcOffsetOutOfRange`. That
310+
* inconsistency is .NET's and is transcribed rather than tidied;
311+
* - the seconds check is not "sub-minute". It is "not a whole number of minutes",
312+
* so 1h30m30s fails as surely as 30s does;
313+
* - the time-of-day check EXEMPTS `DateTime::MinValue` for `dateStart` and
314+
* `MaxValue` for `dateEnd`, which is how a rule that spans all time is spelled.
315+
*/
316+
static void validateAdjustmentRule(const DateTime& dateStart, const DateTime& dateEnd,
317+
const TimeSpan& daylightDelta) {
318+
validateDateRange(dateStart, dateEnd);
319+
320+
// TimeZoneInfo.AdjustmentRule.cs:206-209.
321+
constexpr double kMinDaylightDeltaHours = -23.0;
322+
constexpr double kMaxDaylightDeltaHours = 14.0;
323+
if (daylightDelta.getTotalHoursProperty() < kMinDaylightDeltaHours ||
324+
daylightDelta.getTotalHoursProperty() > kMaxDaylightDeltaHours) {
325+
throw System::ArgumentOutOfRangeException(
326+
"daylightDelta", daylightDelta.ToString(),
327+
"The TimeSpan parameter must be within plus or minus 14.0 hours.");
328+
}
329+
330+
// :211-214.
331+
if (daylightDelta.getTicksProperty() % TimeSpan::TicksPerMinute != 0) {
332+
throw System::ArgumentException(
333+
"The TimeSpan parameter cannot be specified more precisely than whole "
334+
"minutes.",
335+
"daylightDelta");
336+
}
337+
338+
// :216-223. This port has no DateTimeKind (a permanent deviation), so the
339+
// `Kind == Unspecified` conjunct is not reproducible and is simply absent -- which
340+
// makes this port's check STRICTER than .NET's for a UTC-kinded argument, and
341+
// identical for every argument this port can express.
342+
if (dateStart != DateTime::MinValue &&
343+
dateStart.getTimeOfDayProperty() != TimeSpan::Zero) {
344+
throw System::ArgumentException(
345+
"The supplied DateTime includes a TimeOfDay setting. This is not "
346+
"supported.",
347+
"dateStart");
348+
}
349+
if (dateEnd != DateTime::MaxValue &&
350+
dateEnd.getTimeOfDayProperty() != TimeSpan::Zero) {
351+
throw System::ArgumentException(
352+
"The supplied DateTime includes a TimeOfDay setting. This is not "
353+
"supported.",
354+
"dateEnd");
355+
}
356+
}
357+
296358
/**
297359
* @brief Creates an adjustment rule with zero BaseUtcOffsetDelta.
298360
*
@@ -304,7 +366,7 @@ namespace System {
304366
DateTime dateStart, DateTime dateEnd, TimeSpan daylightDelta,
305367
TransitionTime daylightTransitionStart, TransitionTime daylightTransitionEnd)
306368
{
307-
validateDateRange(dateStart, dateEnd);
369+
validateAdjustmentRule(dateStart, dateEnd, daylightDelta);
308370
auto r = std::shared_ptr<AdjustmentRule>(new AdjustmentRule());
309371
r->dateStart_ = dateStart;
310372
r->dateEnd_ = dateEnd;
@@ -330,7 +392,7 @@ namespace System {
330392
TransitionTime daylightTransitionStart, TransitionTime daylightTransitionEnd,
331393
TimeSpan baseUtcOffsetDelta)
332394
{
333-
validateDateRange(dateStart, dateEnd);
395+
validateAdjustmentRule(dateStart, dateEnd, daylightDelta);
334396
auto r = std::shared_ptr<AdjustmentRule>(new AdjustmentRule());
335397
r->dateStart_ = dateStart;
336398
r->dateEnd_ = dateEnd;
@@ -782,11 +844,47 @@ namespace System {
782844
*
783845
* C++ counterpart of .NET TimeZoneInfo.ConvertTime(DateTime, TimeZoneInfo, TimeZoneInfo).
784846
*/
847+
/**
848+
* @brief `DateTime` from a tick count, clamped to the representable range.
849+
*
850+
* Ticket #2186 (2026-08-18) answered question 1. The three conversion doors used
851+
* `DateTime::Add`, whose overflow is an `ArgumentOutOfRangeException`; .NET **clamps**:
852+
*
853+
* @code
854+
* private static DateTime SafeCreateDateTimeFromTicks(long ticks, DateTimeKind kind = …)
855+
* => (ulong)ticks <= DateTime.MaxTicks ? new DateTime(ticks, kind)
856+
* : (ticks < 0 ? DateTime.MinValue : DateTime.MaxValue);
857+
* @endcode
858+
* (`TimeZoneInfo.Cache.cs:340-342`), and `ConvertTime` builds its result through it
859+
* (`TimeZoneInfo.cs:685`).
860+
*
861+
* **The cast to `ulong` is the whole trick and is reproduced deliberately**: a negative
862+
* tick count wraps to something enormous, so one unsigned comparison rejects both ends of
863+
* the range at once. Spelling it as two signed comparisons would be equivalent, and this
864+
* spelling is kept because it is the reference's.
865+
*
866+
* .NET does NOT clamp everywhere. Its invalid-time compatibility path builds a raw
867+
* `new DateTime(...)` and lets it throw, with a comment saying so explicitly
868+
* (`TimeZoneInfo.cs:661-667`) — that path needs `TimeZoneInfoOptions` and adjustment
869+
* rules this port's `TimeZoneInfo` does not model, so it is not reachable here.
870+
*/
871+
[[nodiscard]] static DateTime safeFromTicks(SharpRuntime::longcs ticks) {
872+
const auto unsignedTicks = static_cast<unsigned long long>(ticks);
873+
if (unsignedTicks <= static_cast<unsigned long long>(DateTime::MaxTicks))
874+
return DateTime(ticks);
875+
return ticks < 0 ? DateTime::MinValue : DateTime::MaxValue;
876+
}
877+
785878
static DateTime ConvertTime(const DateTime& dt,
786879
const TimeZoneInfo& sourceTimeZone,
787880
const TimeZoneInfo& destinationTimeZone) {
788-
DateTime utc = dt.Add(-sourceTimeZone.baseUtcOffset_);
789-
return utc.Add(destinationTimeZone.baseUtcOffset_);
881+
// #2186: the intermediate UTC ticks may leave the range while the final local ticks
882+
// land back inside it, which is why .NET computes the result "from raw ticks to avoid
883+
// precision loss from double-clamping" (TimeZoneInfo.cs:683-685) and clamps only once,
884+
// at the end.
885+
const SharpRuntime::longcs utcTicks =
886+
dt.getTicksProperty() - sourceTimeZone.baseUtcOffset_.getTicksProperty();
887+
return safeFromTicks(utcTicks + destinationTimeZone.baseUtcOffset_.getTicksProperty());
790888
}
791889

792890
/**
@@ -796,7 +894,8 @@ namespace System {
796894
*/
797895
static DateTime ConvertTimeFromUtc(const DateTime& dt,
798896
const TimeZoneInfo& destinationTimeZone) {
799-
return dt.Add(destinationTimeZone.baseUtcOffset_);
897+
return safeFromTicks(dt.getTicksProperty() +
898+
destinationTimeZone.baseUtcOffset_.getTicksProperty()); // #2186
800899
}
801900

802901
/**
@@ -805,7 +904,8 @@ namespace System {
805904
* C++ counterpart of .NET TimeZoneInfo.ConvertTimeToUtc(DateTime, TimeZoneInfo).
806905
*/
807906
static DateTime ConvertTimeToUtc(const DateTime& dt, const TimeZoneInfo& sourceTimeZone) {
808-
return dt.Add(-sourceTimeZone.baseUtcOffset_);
907+
return safeFromTicks(dt.getTicksProperty() -
908+
sourceTimeZone.baseUtcOffset_.getTicksProperty()); // #2186
809909
}
810910

811911
/**

modules/time-zone/src/System/TimeZoneInfo.cpp

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include "System/PlatformNotSupportedException.hpp"
66
#include "System/TimeZoneNotFoundException.hpp"
77
#include "TimeZonePosixSupport.hpp"
8+
#include "System/InvalidTimeZoneException.hpp"
89
#include <cstdlib>
910
#include <ctime>
1011

@@ -102,12 +103,28 @@ bool hasTzifMagic(const std::string& path) {
102103

103104
} // namespace
104105

106+
// Ticket #2186, question 4. #2183 folded three distinct failures into one boolean and kept
107+
// TimeZoneNotFoundException for all of them "rather than guessing InvalidTimeZoneException".
108+
// The guess is unnecessary now: .NET raises TimeZoneNotFoundException when the id names nothing,
109+
// and InvalidTimeZoneException when the file EXISTS but is not usable zone data
110+
// (`TimeZoneInfo.Unix.cs:697`, SR.InvalidTimeZone_NoTTInfoStructures). The two are different
111+
// answers to different questions -- "there is no such zone" versus "that is not a zone" -- and a
112+
// caller that catches only the first would previously have swallowed the second.
113+
//
114+
// TryFindSystemTimeZoneById is unaffected: .NET's discards the exception entirely
115+
// (`TimeZoneInfo.cs:526-527`), so it returns false for every failure, and this port's `catch (...)`
116+
// already did the same.
105117
static bool zoneFileExists(const std::string& id) {
106118
if (!isWellFormedZoneId(id)) return false;
107119
std::string path = "/usr/share/zoneinfo/" + id;
108120
struct stat st {};
109-
if (stat(path.c_str(), &st) != 0 || !S_ISREG(st.st_mode)) return false;
110-
return hasTzifMagic(path);
121+
return stat(path.c_str(), &st) == 0 && S_ISREG(st.st_mode);
122+
}
123+
124+
// True when a file that EXISTS does not carry zone data. Separated from zoneFileExists so the
125+
// caller can tell the two failures apart; see the comment above.
126+
static bool zoneFileIsNotZoneData(const std::string& id) {
127+
return !hasTzifMagic("/usr/share/zoneinfo/" + id);
111128
}
112129
#endif
113130

@@ -262,6 +279,10 @@ std::shared_ptr<TimeZoneInfo> TimeZoneInfo::FindSystemTimeZoneById(const std::st
262279
if (!zoneFileExists(id))
263280
throw System::TimeZoneNotFoundException(
264281
"The time zone ID '" + id + "' was not found on the local computer.");
282+
if (zoneFileIsNotZoneData(id))
283+
throw System::InvalidTimeZoneException(
284+
"There are no ttinfo structures in the tzfile. At least one ttinfo structure is "
285+
"required in order to construct a TimeZoneInfo object.");
265286

266287
detail::ZoneMetadata meta;
267288
{

0 commit comments

Comments
 (0)