Skip to content

Round a fractional offset given as a string - #190

Open
youdie006 wants to merge 1 commit into
ruby:masterfrom
youdie006:fix-string-fractional-offset
Open

Round a fractional offset given as a string#190
youdie006 wants to merge 1 commit into
ruby:masterfrom
youdie006:fix-string-fractional-offset

Conversation

@youdie006

Copy link
Copy Markdown

The inconsistency

date_zone_to_diff returns a Rational for a fractional-hour zone with more
than two digits (ext/date/date_parse.c:529-535), and the comment just above it
says that is intended — "no over precision for offset; 10**-7 hour = 0.36
milliseconds should be enough"
(:506-508).

offset_to_sec's string branch threw that away: if (!FIXNUM_P(vs)) return 0;
at ext/date/date_core.c:2639.

Its two sibling branches in the same function do the opposite — they round and
warn. T_FLOAT at :2590-2592 and T_RATIONAL at :2621-2623 both emit
rb_warning("fraction of offset is ignored").

So one quantity, three ways, two answers:

DateTime.new(2001,2,3, 0,0,0, '+00.123')                #=> offset 0     "invalid offset is ignored"
DateTime.new(2001,2,3, 0,0,0, 442.8/86400)              #=> offset 443   "fraction of offset is ignored"
DateTime.new(2001,2,3, 0,0,0, Rational(2214, 5*86400))  #=> offset 443   "fraction of offset is ignored"

442.8 seconds either way. The string form — the only one a user actually types —
is the one that silently becomes +00:00.

Not currently tested

git ls-files -z test | xargs -0 grep -nE "['\"][-+][0-9]{2}\.[0-9]+['\"]" returns
nothing: no test passes a dotted zone as an offset argument. The .123 hits in
the suite are sub-second fractions of the time ('19990523235521.123456+0900'),
not zones, and grep -n 'fraction of offset' test/ finds nothing.

The one fractional zone that is tested, '[-9.50]' at test_date_parse.rb:137-138,
has two digits, so it takes date_zone_to_diff's n <= 2 Integer path
(date_parse.c:524-528) — already consistent, and untouched by this. The Rational
path is what was untested.

The change

Round it, exactly as the sibling branches do. k_rational_p, f_round,
f_eqeq_p and rb_warning are all already used in this file.

The k_rational_p guard matters: date_zone_to_diff returns Qnil for a zone it
rejects, and without the guard that nil reaches f_round.

Not a widening

'+24:00' and '+99:00' still give 0 — date_zone_to_diff rejects them before
this code runs. '+00:00' → 0, '+01.5' → 5400, '+9.50' → 34200, '+00.1'
360, '+00.12' → 432, '+23:59:59' → 86399 are all unchanged. The
n < -DAY_IN_SECONDS || n > DAY_IN_SECONDS check at :2642 still runs on the
rounded value, and '+23.9999999' rounds to exactly 86400, which the inclusive
guard accepts — consistent with Rational(1,1) being legal per the existing
test_civil__offset at :196-197.

Direction

I want to be straight about the tension here. Read shallowly, the recent commits
on these files are a tightening trend (#183's range check, #188's commercial-week
validation, c98d85d verifying argument classes), which would argue for making the
string branch keep rejecting.

I do not think that is the right read. #183's own commit message frames its defect
as "the Integer 2 is rejected, but Rational(2,1) is the same quantity" — the
principle is consistency between representations of one quantity, which is exactly
this. And rounding is the long-standing behaviour here: rb_warning("fraction of offset is ignored") already exists at :2592, :2623 and :3349. The string
branch is the deviation, not the rule.

If you would rather go the other way and make Float and Rational reject too, that
is a coherent position and I am happy to write that instead.

Verification

  • Full suite on CRuby 3.2.11: 148 tests, 162616 assertions, 0 failures, 0
    errors
    . Pristine control run: same 148 tests with exactly one failure, mine —
    so nothing else changes behaviour.
  • TruffleRuby row verified, not inferred. That row was my one open question,
    since TruffleRuby ships its own date. It does compile this C extension via
    Sulong, and I ran it: with master's code '+00.123' gives 0 and warns "invalid
    offset is ignored"; with this patch it gives 443 and warns "fraction of offset is
    ignored", while '+24:00'/'+99:00' stay 0 and '+01.5'/'+23:59:59' are
    unchanged. So the truffleruby job is fixed by this too rather than broken by it.
  • Mutation checks: truncating instead of rounding gives 442 and fails; dropping the
    warning fails; dropping the k_rational_p guard raises
    NoMethodError: undefined method 'round' for nil on the '+24:00' row.

One thing I deliberately left out

DateTime.parse('2001-02-03T00:00:00+00.123') goes through dt_new_by_frags,
which does of = NUM2INT(t) at date_core.c:8519 and truncates to 442. So
after this change DateTime.parse and DateTime.new differ by one second on the
same zone string.

That is a real but separate inconsistency in a different function, and bundling it
would make this patch harder to judge. Happy to follow up. (Same spot also narrows
before its range check at :8520, the inverse of #183's principle — currently
unreachable, since date_zone_to_diff caps output near 3.6e8.)

Disclosure

I used an AI assistant to help find and prepare this change. I reviewed and tested
it myself, and the outputs above are from runs I performed.

date_zone_to_diff returns a Rational for a fractional-hour zone with
more than two digits, and offset_to_sec's string branch discarded it:

  DateTime.new(2001,2,3, 0,0,0, '+00.123')   #=> offset 0
  DateTime.new(2001,2,3, 0,0,0, 442.8/86400) #=> offset 443
  DateTime.new(2001,2,3, 0,0,0, Rational(2214, 5*86400))
                                             #=> offset 443

The same quantity, three ways, two answers. The Float branch rounds and
warns "fraction of offset is ignored", and the Rational branch does the
same; only the string branch returned 0 with "invalid offset is
ignored".

Round it the way the two sibling branches already do. Zones that
date_zone_to_diff rejects are still rejected, and the DAY_IN_SECONDS
range check still runs on the rounded value.
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.

1 participant