Skip to content

Infer the zone datetime in FFI when possible #6517

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.

Sign up for GitHub

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

Merged
sffc merged 1 commit into from
May 2, 2025
Merged

Infer the zone datetime in FFI when possible #6517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions components/datetime/src/format/input.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,26 +67,26 @@ impl DateTimeInputUnchecked {
/// responsible for making sure the calendar matches the formatter.
pub fn set_date_fields_unchecked<C: Calendar, A: AsCalendar<Calendar = C>>(
&mut self,
input: Date<A>,
date_in_calendar: Date<A>,
) {
self.year = Some(input.year());
self.month = Some(input.month());
self.day_of_month = Some(input.day_of_month());
self.iso_weekday = Some(input.day_of_week());
self.day_of_year = Some(input.day_of_year());
self.year = Some(date_in_calendar.year());
self.month = Some(date_in_calendar.month());
self.day_of_month = Some(date_in_calendar.day_of_month());
self.iso_weekday = Some(date_in_calendar.day_of_week());
self.day_of_year = Some(date_in_calendar.day_of_year());
}

/// Sets all fields from a [`Time`] input.
pub fn set_time_fields(&mut self, input: Time) {
self.hour = Some(input.hour);
self.minute = Some(input.minute);
self.second = Some(input.second);
self.subsecond = Some(input.subsecond);
pub fn set_time_fields(&mut self, time: Time) {
self.hour = Some(time.hour);
self.minute = Some(time.minute);
self.second = Some(time.second);
self.subsecond = Some(time.subsecond);
}

/// Sets the time zone UTC offset.
pub fn set_time_zone_utc_offset(&mut self, offset: UtcOffset) {
self.zone_offset = Some(offset);
pub fn set_time_zone_utc_offset(&mut self, utc_offset: UtcOffset) {
self.zone_offset = Some(utc_offset);
}

/// Sets the time zone ID.
Expand Down
8 changes: 4 additions & 4 deletions ffi/capi/src/zoned_date_formatter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,8 +517,8 @@ pub mod ffi {
write: &mut diplomat_runtime::DiplomatWrite,
) -> Result<(), DateTimeWriteError> {
let mut input = icu_datetime::DateTimeInputUnchecked::default();
let date = date.0.to_calendar(self.0.calendar());
input.set_date_fields_unchecked(date); // calendar conversion on previous line
let date_in_calendar = date.0.to_calendar(self.0.calendar());
input.set_date_fields_unchecked(date_in_calendar); // calendar conversion on previous line
input.set_time_zone_id(zone.id);
if let Some(offset) = zone.offset {
input.set_time_zone_utc_offset(offset);
Expand Down Expand Up @@ -1009,8 +1009,8 @@ pub mod ffi {
write: &mut diplomat_runtime::DiplomatWrite,
) -> Result<(), DateTimeWriteError> {
let mut input = icu_datetime::DateTimeInputUnchecked::default();
let date = date.0.to_calendar(Gregorian);
input.set_date_fields_unchecked(date); // calendar conversion on previous line
let date_in_calendar = date.0.to_calendar(Gregorian);
input.set_date_fields_unchecked(date_in_calendar); // calendar conversion on previous line
input.set_time_zone_id(zone.id);
if let Some(offset) = zone.offset {
input.set_time_zone_utc_offset(offset);
Expand Down
20 changes: 16 additions & 4 deletions ffi/capi/src/zoned_date_time_formatter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,8 +518,8 @@ pub mod ffi {
write: &mut diplomat_runtime::DiplomatWrite,
) -> Result<(), DateTimeWriteError> {
let mut input = icu_datetime::DateTimeInputUnchecked::default();
let date = date.0.to_calendar(self.0.calendar());
input.set_date_fields_unchecked(date); // calendar conversion on previous line
let date_in_calendar = date.0.to_calendar(self.0.calendar());
input.set_date_fields_unchecked(date_in_calendar); // calendar conversion on previous line
input.set_time_fields(time.0);
input.set_time_zone_id(zone.id);
if let Some(offset) = zone.offset {
Expand All @@ -528,6 +528,12 @@ pub mod ffi {
if let Some(zone_name_timestamp) = zone.zone_name_timestamp {
input.set_time_zone_name_timestamp(zone_name_timestamp);
}
else {
input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime {
date: date.0,
time: time.0
}))
}
Comment on lines +531 to +536
Copy link

@gemini-code-assist gemini-code-assist bot May 1, 2025

Choose a reason for hiding this comment

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

high

This else block is intended to infer the time zone name timestamp when it's not explicitly provided. However, it's unconditionally setting the timestamp based on the date and time. This might not always be the desired behavior. It would be better to check if the timestamp can be inferred from the zone ID before setting it here. Otherwise, the code might be overwriting a valid timestamp with an inferred one.

Suggested change
else {
input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime {
date: date.0,
time: time.0
}))
}
if let Some(zone_name_timestamp) = zone.zone_name_timestamp {
input.set_time_zone_name_timestamp(zone_name_timestamp);
}
else if zone.id.is_some() {
input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime {
date: date.0,
time: time.0
}))
}

if let Some(variant) = zone.variant {
input.set_time_zone_variant(variant);
}
Expand Down Expand Up @@ -1012,8 +1018,8 @@ pub mod ffi {
write: &mut diplomat_runtime::DiplomatWrite,
) -> Result<(), DateTimeWriteError> {
let mut input = icu_datetime::DateTimeInputUnchecked::default();
let date = date.0.to_calendar(Gregorian);
input.set_date_fields_unchecked(date); // calendar conversion on previous line
let date_in_calendar = date.0.to_calendar(Gregorian);
input.set_date_fields_unchecked(date_in_calendar); // calendar conversion on previous line
input.set_time_fields(time.0);
input.set_time_zone_id(zone.id);
if let Some(offset) = zone.offset {
Expand All @@ -1022,6 +1028,12 @@ pub mod ffi {
if let Some(zone_name_timestamp) = zone.zone_name_timestamp {
input.set_time_zone_name_timestamp(zone_name_timestamp);
}
else {
input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime {
date: date.0,
time: time.0
}))
}
Comment on lines +1031 to +1036
Copy link

@gemini-code-assist gemini-code-assist bot May 1, 2025

Choose a reason for hiding this comment

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

high

This else block is intended to infer the time zone name timestamp when it's not explicitly provided. However, it's unconditionally setting the timestamp based on the date and time. This might not always be the desired behavior. It would be better to check if the timestamp can be inferred from the zone ID before setting it here. Otherwise, the code might be overwriting a valid timestamp with an inferred one.

Suggested change
else {
input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime {
date: date.0,
time: time.0
}))
}
if let Some(zone_name_timestamp) = zone.zone_name_timestamp {
input.set_time_zone_name_timestamp(zone_name_timestamp);
}
else if zone.id.is_some() {
input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime {
date: date.0,
time: time.0
}))
}

if let Some(variant) = zone.variant {
input.set_time_zone_variant(variant);
}
Expand Down
20 changes: 19 additions & 1 deletion ffi/dart/test/icu_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,16 @@ void main() {
VariantOffsetsCalculator(),
);

final utcOffset = UtcOffset.fromSeconds(-420);
final customZDT = ZonedIsoDateTime.fromEpochMillisecondsAndUtcOffset(
1746140981731,
utcOffset,
);
final customZone = TimeZoneInfo(
TimeZone.fromBcp47('uslax'),
offset: utcOffset,
);

var locale = Locale.fromString('de-u-ca-islamic-umalqura');

///// DateFormatter /////
Expand Down Expand Up @@ -233,7 +243,7 @@ void main() {
);

expect(
() => ZonedDateTimeFormatter.genericLong(
() => ZonedDateTimeFormatter.specificLong(
locale,
DateTimeFormatter.ymdet(locale),
).formatIso(
Expand Down Expand Up @@ -268,6 +278,14 @@ void main() {
'15.07.46 AH, 14:32:12 MEZ',
);

expect(
ZonedDateTimeFormatter.genericLong(
locale,
DateTimeFormatter.mdt(locale),
).formatIso(customZDT.date, customZDT.time, customZone),
'03.11., 23:02:41 Nordamerikanische Westküstenzeit',
);

///// ZonedDateTimeFormatterGregorian /////

expect(
Expand Down
14 changes: 11 additions & 3 deletions tools/make/codegen/templates/zoned_formatter.rs.jinja
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,11 @@ pub mod ffi {
let mut input = icu_datetime::DateTimeInputUnchecked::default();
{%- if flavor.has_date() %}
{%- if formatter_kind.is_fixed_calendar %}
let date = date.0.to_calendar(Gregorian);
let date_in_calendar = date.0.to_calendar(Gregorian);
{%- else %}
let date = date.0.to_calendar(self.0.calendar());
let date_in_calendar = date.0.to_calendar(self.0.calendar());
{%- endif %}
input.set_date_fields_unchecked(date); // calendar conversion on previous line
input.set_date_fields_unchecked(date_in_calendar); // calendar conversion on previous line
{%- endif %}
{%- if flavor.has_time() %}
input.set_time_fields(time.0);
Expand All @@ -206,6 +206,14 @@ pub mod ffi {
if let Some(zone_name_timestamp) = zone.zone_name_timestamp {
input.set_time_zone_name_timestamp(zone_name_timestamp);
}
{%- if flavor.has_date() && flavor.has_time() %}
else {
input.set_time_zone_name_timestamp(icu_time::zone::ZoneNameTimestamp::from_date_time_iso(&icu_time::DateTime {
date: date.0,
time: time.0
}))
}
{%- endif %}
if let Some(variant) = zone.variant {
input.set_time_zone_variant(variant);
}
Expand Down