I was using `%w` to format the day of the week in examples referencing
ISO 8601 week days, but the correct directive is `%u`. The difference
is that the former uses `0` for Sunday and the latter uses `7` for
Sunday. (So in the examples given, this doesn't actually change the
output.)
Specifically, that you can't get any units bigger than hours *by
default*.
We also fix what was probably a copy-and-paste error in the `Sub` trait
implementation docs for `Zoned`. (I probably copied it from
`jiff::civil::DateTime`, where it can return days by default.)
And we apply similar clarifications to the `jiff::civil::DateTime` docs
as well.
Ref https://github.com/microsoft/openvmm/pull/1028#discussion_r1994483092
Unfortunately, these impls can cause inference regressions when
non-robust code is written that assumes there is only one
Partial{Eq,Ord} impl for a particular integer type.
It would be one thing if these trait impls were external or somehow
fundamental to Jiff's design. But they only existed as a convenience. So
we remove the trait impls and take our medicine. We already had a
`Constant` wrapper type (also used for trait impls), so we just switch
all equality and inequality comparisons over to that.
I tested this with the following program:
```rust
use env_logger;
fn main() {
let x: u64 = 1;
let y: i128 = 0;
assert!(y < x.into());
let x: u32 = 1;
let y: i64 = 0;
assert!(y < x.into());
let x: u16 = 1;
let y: i32 = 0;
assert!(y < x.into());
let x: u8 = 1;
let y: i16 = 0;
assert!(y < x.into());
}
```
And this `Cargo.toml`:
```toml
[package]
publish = false
name = "jiff-inference-regression"
version = "0.1.0"
edition = "2024"
[patch.crates-io]
jiff = { path = "/home/andrew/rust/jiff/fixit" }
[dependencies]
env_logger = { version = "0.11.7", features = ["humantime"] }
[[bin]]
name = "jiff-inference-regression"
path = "main.rs"
[profile.release]
debug = true
```
I took this path because it's either this or the reporter fixes their
code. Arguably, the reporter should fix their code since it's likely
their code will break when or if some other crate adds similar trait
impls. But as I said, these trait impls are just for convenience, so
the pragmatic trade-off is to remove them and thus not be the source of
whatever problems folks hit.
[I asked the lang team about this problem][lang-zulip-question], and
they seem to agree that this is the right course of action. (And there
are ideas swirling around on how to mitigate this problem, but that's
for the future.)
Fixes#293
[lang-zulip-question]: 504689811
When I originally wrote the comment on `Repr`, I got the alignment
wrong. But I caught that mistake and fixed it before merging anything
to `master`. I just hadn't updated the comment.
The `repr(align(..))` was leftovers from experimenting with a
`TzifDateTime` that *wasn't* packed. I was trying to get `rustc` to
optimize comparisons automatically to a single integer, but couldn't get
ti to work. So I resorted to bit-packing. Since the representation is
now just an `i64`, an explicit alignment is not needed. (And it didn't
help anyway.)
Basically, it was wrong when the date was the first day of the month.
This only impacts `jiff 0.2.2`. The bug wasn't present in previous
releases.
This was a transcription error during one of my refactors and it looks
like there unfortunately wasn't test coverage for it. Test coverage has
been added in this PR.
Fixes#290
I wasn't sure why `cargo package` wasn't picking up
`crates/jiff-static/shared`. So I switched to a traditional `src`
layout, which shouldn't be necessary. Indeed, that didn't fix things.
Turns out, I had a bunk `include` rule.
But I wanted to switch to the `src` scheme anyway, so leave that.
For whatever reason, these seem to take a hideously long time to run in
CI. They even take a long time to run locally, *relatively* speaking. In
core-only, `insta` doesn't support snapshotting at all, which is a huge
bummer. So we just tell insta to force the tests to pass and don't do
any updating. So these tests weren't really being run anyway.
I'm not sure what insta is doing here to be honest, and I don't really
understand why insta can't handle the core-only tests. I mean, I am
still importing the standard library when tests are run, even in
core-only mode. Maybe the insta macros assume the standard library
prelude is present or something? IDK.
... so that we can run each piece in its own job in CI.
This creates an obscene number of jobs, but I'm really hoping this cuts
down on the total wall clock time.
We are going to try and break `test` apart in order to speed up CI
builds. I don't want to pollute the root project directory with more
random test scripts, so let's tuck them away for now.
It is tempting to think of this method as just being a shortcut for
`zdt.timestamp().subsec_nanosecond()`, but it actually isn't! It's
returning the fractional seconds on the *civil* datetime, not the
timestamp. These are usually the same for times after the Unix epoch,
but can differ for times before it.
While the original request in #283 asks for `subsec_millisecond()` and
`subsec_microsecond()` on `Zoned` in order to be consistent with
`Timestamp`, I'm going to pass on those for now. In particular, since
these would return the fractional second value from the *civil*
datetime, `subsec_millisecond()` would always be equivalent to
`millisecond()`. `subsec_microsecond()` wouldn't be the same as
`microsecond()` (just like `subsec_nanosecond()` isn't the same as
`nanosecond()`), but I find that this just overall adds to the confusion
of the methods here. And if you do need `subsec_microsecond()`, you can
just do `subsec_nanosecond() / 1_000`.
The reason that these additional methods make sense for `Timestamp` is
that `Timestamp` doesn't have a civil datetime. So there are no
individual `millisecond()` or `microsecond()` units. A `Timestamp` is
closer to a `SignedDuration` than a `civil::DateTime`.
Closes#283
This should help ensure that generated code doesn't get stale.
This is especially pertinent with the new `src/shared` module, which has
to be copied over to `crates/jiff-static/shared` any time a change is
made. Not all changes result in breakage (theoretical or otherwise), so
it's easy to forget to do.
This makes binary search for TZ lookups substantially faster.
This is yet another brutal refactor. Changing anything in POSIX time
zones or TZif handling is now a monster pain in the ass because all
of that code is shared in a very awkward way with `jiff-static`.
Ref #271
This is an easy win that uses 64-bit integers to represent a timestamp
instead of 96-bit integers. This is okay because this reflects what the
actual source IANA time zone database uses.
This makes the binary search lookup a fair a bit faster.
Next I'd like to split `Transition` into three sequences: timestamps,
civil datetimes and the local type index. This should make them as
small as possible and further improve binary search lookups (I hope).
When enabled, this feature will "fatten" TZif data by adding more time
zone transitions. This corresponds to what tzdb's `zic` program does
when `-b fat` is given, except Jiff does it at runtime. If the TZif data
has already been fattened, then this has no effect.
The reason for this is that it smooths out performance differences in
time zone runtime lookups between pre-fattened TZif data and "slim"
TZif data. It is unpredictable whether `/usr/share/zoneinfo` is
actually fat or not, so this helps makes performance more predictable
regardless of what the source TZif data looks like.
This uses about 25% more heap memory in my experiments. For a single
time zone, this is, in an absolute sense, likely insignificant. But if
you have thousands of time zones loaded into memory, it can add up. But
that's a somewhat niche use case. However, this can make binary sizes
bigger when the `jiff-static` proc macro is used.
So while unlikely to matter too much, the `tz-fat` feature can be
disabled if you want to prioritize memory usage and binary size.
Fixes#271
This was yet another absolutely brutal refactor. But in order to
"fatten" up TZif data after parsing, we need to be able to actually use
POSIX time zones in order to compute missing transitions. And in order
to do that, basically the entire POSIX time zone implementation needs to
be in `shared`. And that means no ranged integers. Which in turn means
implementing several datetime algorithms on just primitives.
This was just overall brutal, and I am getting very close to ripping
out ranged integers.
It looks like I never circled back around to fix the error message here
when I added the `SpanRelativeTo::days_are_24_hours()` functionality.
So fix that here.
It's hard to keep `SpanRelativeToKind`'s `Display` impl, because the
error message kinda needs to be rejiggered at a higher level.
Thankfully, this was only used in one place.
Instead of just free functions operating on tuples, we actually give
them names.
Ranged integers keep pissing me off. The fact that I have "primitive"
datetime types and "ranged" datetime types is just absolutely
infuriating and creating a lot of dissonance.
But at least the new composite stuff makes moving back-and-forth a
little easier now. Of course, the composite stuff is also write-once
and read-never. *heavy sigh*
In the next commit, we're going to start moving some more of our
datetime algorithms to `shared`.
The ultimate goal here is to have enough in `shared` that we can handle
POSIX time zones.
This takes some brutalness out of writing routines for converting
to and from composite types over ranged integers (like `civil::Date`).
This whole mess is a consequence of using ranged integers and
simultaneously implementing our low level datetime algorithms on
primitive integers instead of ranged integers. It's a fucking mess.
I think I am steadily marching toward ripping out ranged integers.
Sigh. Very unfortunate.
This was just really bugging me. And if we're going to move more of the
POSIX time zone implementation into `shared`, we might as well just bite
the bullet and do this too.
Now I believe the only parts of POSIX time zones that require `alloc`
are parsing the `:blah` implementation defined strings for the `TZ`
environment variable and error messages. Not that it really matters
much I think.
Now that we eagerly reject unreasonable POSIX time zones, we can
simplify our type definitions. There's no more split between
`PosixTimeZone` and `ReasonablePosixTimeZone`. Everything is
just reasonable.
This makes the POSIX time zone parser reject strings like `EST5EDT`.
That is, a time zone with daylight saving time, but without an explicit
rule stating when daylight saving time becomes active/inactive.
We were already doing this, but more explicitly by calling
`PosixTimeZone::reasonable`, so there is no public API breakage here.
The only difference is that `EST5EDT` will be treated as invalid and
will instead be attempted to be used as an IANA time zone identifier.
(Which, incidentally enough, actually exists. Odd, but I suppose
technically more correct than the current behavior of just rejecting it
outright.)
I did this because it makes the type definitions simpler. There was a
lot of cognitive energy on my part being devoted to parsing unreasonable
POSIX time zones successfully and only later asserting that they are
reasonable through a fallible API. But I don't think this was really
buying us anything, and we should just reject them outright.
Interestingly, PostgreSQL does support these "unreasonable" POSIX time
zones[1]:
> If a daylight-savings abbreviation is given but the transition
> rule field is omitted, the fallback behavior is to use the rule
> M3.2.0,M11.1.0, which corresponds to USA practice as of 2020 (that
> is, spring forward on the second Sunday of March, fall back on
> the first Sunday of November, both transitions occurring at 2AM
> prevailing time). Note that this rule does not give correct USA
> transition dates for years before 2007.
But POSIX has literally nothing to say about it[2], despite providing a
grammar that clearly makes the DST transition rule optional even when a
DST abbreviation is provided. Like it doesn't even mention that it's
unspecified, despite bloviating about how certain abbreviation lengths
lead to unspecified behavior. Why does POSIX suck so bad?
Anyway, it seems like there are really only two choices here. We could
either reject unreasonable time zones as invalid POSIX time zone
strings, or we could just "helpfully" assume a particular DST transition
rule. Jiff isn't legacy software (yet), so maybe don't try to be so
helpful that we assume one country's DST transition rules silently for
everyone in the world.
This commit does the bare minimum to reject these time zones.
The next commit will be the payoff.
[1]: https://www.postgresql.org/docs/current/datetime-posix-timezone-specs.html
[2]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html
