Skip to content

Commit 50003f0

Browse files
authored
Reimplement Unit Group with different approach (#183)
This PR reimplements the UnitGroup enum with a different approach to validation. Previously this was implemented, but it broke quite a few tests in Boa. I think that was partially due to a flawed implementation and a host of bugs that were previously present. Those appear to have all been fixed with recent bug changes, and this reimplementation is primarily to address the broken tests in boa-dev/boa#4142 that require unit groups to be implemented. The tests that the unit groups specifically address specifically are tests like: [largestunit-invalid-string.js](https://github.com/tc39/test262/blob/main/test/built-ins/Temporal/Instant/prototype/since/largestunit-invalid-string.js).
1 parent e9448cc commit 50003f0

File tree

6 files changed

+39
-5
lines changed

6 files changed

+39
-5
lines changed

src/builtins/core/date.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use crate::{
77
iso::{IsoDate, IsoDateTime, IsoTime},
88
options::{
99
ArithmeticOverflow, DifferenceOperation, DifferenceSettings, DisplayCalendar,
10-
ResolvedRoundingOptions, TemporalUnit,
10+
ResolvedRoundingOptions, TemporalUnit, UnitGroup,
1111
},
1212
parsers::{parse_date_time, IxdtfStringBuilder},
1313
primitive::FiniteF64,
@@ -253,6 +253,7 @@ impl PlainDate {
253253
let resolved = ResolvedRoundingOptions::from_diff_settings(
254254
settings,
255255
op,
256+
UnitGroup::Date,
256257
TemporalUnit::Day,
257258
TemporalUnit::Day,
258259
)?;

src/builtins/core/datetime.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use crate::{
99
iso::{IsoDate, IsoDateTime, IsoTime},
1010
options::{
1111
ArithmeticOverflow, DifferenceOperation, DifferenceSettings, DisplayCalendar,
12-
ResolvedRoundingOptions, RoundingOptions, TemporalUnit, ToStringRoundingOptions,
12+
ResolvedRoundingOptions, RoundingOptions, TemporalUnit, ToStringRoundingOptions, UnitGroup,
1313
},
1414
parsers::{parse_date_time, IxdtfStringBuilder},
1515
provider::NeverProvider,
@@ -121,6 +121,7 @@ impl PlainDateTime {
121121
let options = ResolvedRoundingOptions::from_diff_settings(
122122
settings,
123123
op,
124+
UnitGroup::DateTime,
124125
TemporalUnit::Day,
125126
TemporalUnit::Nanosecond,
126127
)?;

src/builtins/core/instant.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use crate::{
1010
iso::IsoDateTime,
1111
options::{
1212
DifferenceOperation, DifferenceSettings, DisplayOffset, ResolvedRoundingOptions,
13-
RoundingOptions, TemporalUnit, ToStringRoundingOptions,
13+
RoundingOptions, TemporalUnit, ToStringRoundingOptions, UnitGroup,
1414
},
1515
parsers::{parse_instant, IxdtfStringBuilder},
1616
primitive::FiniteF64,
@@ -88,6 +88,7 @@ impl Instant {
8888
let resolved_options = ResolvedRoundingOptions::from_diff_settings(
8989
options,
9090
op,
91+
UnitGroup::Time,
9192
TemporalUnit::Second,
9293
TemporalUnit::Nanosecond,
9394
)?;

src/builtins/core/time.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use crate::{
55
iso::IsoTime,
66
options::{
77
ArithmeticOverflow, DifferenceOperation, DifferenceSettings, ResolvedRoundingOptions,
8-
RoundingIncrement, TemporalRoundingMode, TemporalUnit, ToStringRoundingOptions,
8+
RoundingIncrement, TemporalRoundingMode, TemporalUnit, ToStringRoundingOptions, UnitGroup,
99
},
1010
parsers::{parse_time, IxdtfStringBuilder},
1111
primitive::FiniteF64,
@@ -127,6 +127,7 @@ impl PlainTime {
127127
let resolved = ResolvedRoundingOptions::from_diff_settings(
128128
settings,
129129
op,
130+
UnitGroup::Time,
130131
TemporalUnit::Hour,
131132
TemporalUnit::Nanosecond,
132133
)?;

src/builtins/core/zoneddatetime.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use crate::{
1717
ArithmeticOverflow, DifferenceOperation, DifferenceSettings, Disambiguation,
1818
DisplayCalendar, DisplayOffset, DisplayTimeZone, OffsetDisambiguation,
1919
ResolvedRoundingOptions, RoundingIncrement, TemporalRoundingMode, TemporalUnit,
20-
ToStringRoundingOptions,
20+
ToStringRoundingOptions, UnitGroup,
2121
},
2222
parsers::{self, FormattableOffset, FormattableTime, IxdtfStringBuilder, Precision},
2323
partial::{PartialDate, PartialTime},
@@ -259,6 +259,7 @@ impl ZonedDateTime {
259259
let resolved_options = ResolvedRoundingOptions::from_diff_settings(
260260
options,
261261
op,
262+
UnitGroup::DateTime,
262263
TemporalUnit::Hour,
263264
TemporalUnit::Nanosecond,
264265
)?;

src/options.rs

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,11 +177,13 @@ impl ResolvedRoundingOptions {
177177
pub(crate) fn from_diff_settings(
178178
options: DifferenceSettings,
179179
operation: DifferenceOperation,
180+
unit_group: UnitGroup,
180181
fallback_largest: TemporalUnit,
181182
fallback_smallest: TemporalUnit,
182183
) -> TemporalResult<Self> {
183184
// 4. Let resolvedOptions be ? SnapshotOwnProperties(? GetOptionsObject(options), null).
184185
// 5. Let settings be ? GetDifferenceSettings(operation, resolvedOptions, DATE, « », "day", "day").
186+
unit_group.validate_unit(options.largest_unit)?;
185187
let increment = options.increment.unwrap_or_default();
186188
let rounding_mode = match operation {
187189
DifferenceOperation::Since => options
@@ -341,6 +343,33 @@ impl ResolvedRoundingOptions {
341343

342344
// ==== Options enums and methods ====
343345

346+
pub enum UnitGroup {
347+
Date,
348+
Time,
349+
DateTime,
350+
}
351+
352+
impl UnitGroup {
353+
pub fn validate_unit(self, unit: Option<TemporalUnit>) -> TemporalResult<()> {
354+
// TODO: Determine proper handling of Auto.
355+
match self {
356+
UnitGroup::Date => match unit {
357+
Some(unit) if !unit.is_time_unit() => Ok(()),
358+
None => Ok(()),
359+
_ => Err(TemporalError::range()
360+
.with_message("Unit was not part of the date unit group.")),
361+
},
362+
UnitGroup::Time => match unit {
363+
Some(unit) if unit.is_time_unit() => Ok(()),
364+
None => Ok(()),
365+
_ => Err(TemporalError::range()
366+
.with_message("Unit was not part of the time unit group.")),
367+
},
368+
UnitGroup::DateTime => Ok(()),
369+
}
370+
}
371+
}
372+
344373
/// The relevant unit that should be used for the operation that
345374
/// this option is provided as a value.
346375
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]

0 commit comments

Comments
 (0)