[JSC] Implement Temporal.Duration

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ross Kirsling]

...

[Ross Kirsling]

Created attachment 434401 [details]
WIP

WIP patch: constructor and getters complete

[Yusuke Suzuki]

Comment on attachment 434401 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=434401&action=review

Quick comments :)

> Source/JavaScriptCore/runtime/TemporalDuration.h:41
> +    static constexpr bool needsDestruction = true;
> +
> +    static void destroy(JSCell* cell)
> +    {
> +        static_cast<TemporalDuration*>(cell)->TemporalDuration::~TemporalDuration();
> +    }

Since std::array<double, numberOfSubdurations> is double array, we do not need to make it destructible.

> Source/JavaScriptCore/runtime/TemporalDuration.h:68
> +    double years() const { return m_subdurations[0]; }
> +    double months() const { return m_subdurations[1]; }
> +    double weeks() const { return m_subdurations[2]; }
> +    double days() const { return m_subdurations[3]; }
> +    double hours() const { return m_subdurations[4]; }
> +    double minutes() const { return m_subdurations[5]; }
> +    double seconds() const { return m_subdurations[6]; }
> +    double milliseconds() const { return m_subdurations[7]; }
> +    double microseconds() const { return m_subdurations[8]; }
> +    double nanoseconds() const { return m_subdurations[9]; }

Let's introduce enum for each field  and use it to access these fields. e.g. TemporalDurationField::Years = 0.
My suggestion is creating a macro like that,

#define JSC_TEMPORAL_DURATION_FIELDS(macro) \
    macro(years, Years) \
    macro(months, Months) \
    ....


And generate the above accessors via this macro.
And generate enum & numberOfSubdurations from this macro.

enum class TemporalDurationField : uint8_t {
#define JSC_DEFINE_TEMPORAL_DURATION_ENUM(name, capitalizedName) name,
    JSC_TEMPORAL_DURATION_FIELDS(JSC_DEFINE_TEMPORAL_DURATION_ENUM)
#undef JSC_DEFINE_TEMPORAL_DURATION_ENUM
};

And we can define numberOfSubdurations with the above macro (see numberOfLinkTimeConstants definition for example).

> Source/JavaScriptCore/runtime/TemporalDuration.h:73
> +    DECLARE_VISIT_CHILDREN;

Since this does not have a GC-managed object field, we do not need to define visitChildren for this object specially. JSObject::visitChildren will handle it.

> Source/JavaScriptCore/runtime/TemporalDurationConstructor.cpp:86
> +    TemporalDuration* duration = TemporalDuration::create(vm, structure);
> +    ASSERT(duration);
> +
> +    std::array<double, TemporalDuration::numberOfSubdurations> subdurations { };
> +    auto count = std::min<size_t>(callFrame->argumentCount(), TemporalDuration::numberOfSubdurations);
> +    for (size_t i = 0; i < count; i++) {
> +        subdurations[i] = callFrame->argument(i).toIntegerOrInfinity(globalObject);
> +        RETURN_IF_EXCEPTION(scope, { });
> +    }
> +
> +    scope.release();
> +    duration->initializeDuration(globalObject, WTFMove(subdurations));

Let's make it simpler by,

std::array<double, TemporalDuration::numberOfSubdurations> subdurations { };
auto count = std::min<size_t>(callFrame->argumentCount(), TemporalDuration::numberOfSubdurations);
for (size_t i = 0; i < count; i++) {
    subdurations[i] = callFrame->argument(i).toIntegerOrInfinity(globalObject);
    RETURN_IF_EXCEPTION(scope, { });
}

if (!isValidDuration(subdurations)) {
    throwRangeError(...)
    return { };
}

return JSValue::encode(TemporalDuration::create(vm, structure, WTFMove(subdurations));

> Source/JavaScriptCore/runtime/VM.cpp:332
> +    , temporalDurationHeapCellType(IsoHeapCellType::create<TemporalDuration>())

Since this is not destructible, we do not need this heap cell type.

> Source/JavaScriptCore/runtime/VM.cpp:1622
> +DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER_SLOW(temporalDurationSpace, temporalDurationHeapCellType.get(), TemporalDuration)

Since this is not destructible, we can use cellHeapCellType

[Radar WebKit Bug Importer]

<rdar://problem/81497521>

[Ross Kirsling]

Thanks for the awesome tips! :D

[Ross Kirsling]

Created attachment 434956 [details]
WIP

WIP patch: addressed feedback, implemented five and a half more methods, added tests for current functionality

[Ross Kirsling]

Created attachment 435100 [details]
WIP

WIP patch: finished toString and from(obj)

[Ross Kirsling]

Created attachment 436031 [details]
WIP

implemented from(str), adjusted some behavior based on pending spec PRs

[Ross Kirsling]

Created attachment 436044 [details]
WIP

refactored from(str) to use StringParsingBuffer

[Ross Kirsling]

Created attachment 436251 [details]
WIP

implemented add/subtract methods

[Ross Kirsling]

Created attachment 436568 [details]
WIP

implemented compare() and total(), turned Subdurations into a struct to avoid calling TemporalDuration::create during internal calculations

[Ross Kirsling]

Created attachment 436604 [details]
WIP

fix const_iterator usage

[Ross Kirsling]

Created attachment 436688 [details]
Patch

[Ross Kirsling]

Created attachment 436690 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 436690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436690&action=review

Quick review comments. Still reviewing.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:2102
> +    if (Options::useTemporal())

We do not need to have this. If it is not initLater-ed, then visit becomes nop.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:73
> +// IsValidDuration ( years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds )

Let's paste a link to the spec proposal :) (e.g. https://tc39.es/proposal-temporal/#sec-temporal-isvalidduration)

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:88
> +//  CreateTemporalDuration ( years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds [ , newTarget ] )

Ditto.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:89
> +TemporalDuration* TemporalDuration::createIfValid(JSGlobalObject* globalObject, Subdurations&& subdurations, Structure* structure)

I like the name like `tryCreateIfValid(...)` :) Then the caller knows that we need to handle exception since it is saying "try".

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:102
> +// DurationHandleFractions ( fHours, minutes, fMinutes, seconds, fSeconds, milliseconds, fMilliseconds, microseconds, fMicroseconds, nanoseconds, fNanoseconds )

Ditto.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:112
> +        subdurations.setMilliseconds(factor * parseInt(padded.substring(0, 3), 10));
> +        subdurations.setMicroseconds(factor * parseInt(padded.substring(3, 3), 10));
> +        subdurations.setNanoseconds(factor * parseInt(padded.substring(6, 3), 10));

We should avoid using WTF::String::substring since it creates a new string. I think creating a StringView over this string is enough.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:149
> +// ParseTemporalDurationString ( isoString )

Ditto.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:264
> +// ToTemporalDurationRecord ( temporalDurationLike )

Ditto.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:300
> +// ToTemporalDuration ( item )

Ditto.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:431
> +// DefaultTemporalLargestUnit ( years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds )

Ditto.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:437
> +    uint8_t index = 0;
> +    while (index < numberOfTemporalUnits - 1 && !m_subdurations[index])
> +        index++;
> +    return static_cast<TemporalUnit>(index);

Is this correct? It seems that it is returning the largest index which does not have zero.
But if index = 0 is year, then we should return as early as we found non-zero.

How about,

static_assert(static_cast<uint8_t>(TemporalUnit::Year) == 0);
static_assert(static_cast<uint8_t>(TemporalUnit::Nanoseconds) == numberOfTemporalUnits - 1);
for (uint8_t index = 0; index < numberOfTemporalUnits; ++index) {
    if (!m_subdurations[index])
        return static_cast<TemporalUnit>(index);
}
return TemporalUnit::Nanoseconds;

> Source/JavaScriptCore/runtime/TemporalDurationConstructor.cpp:94
> +        subdurations[i] = callFrame->argument(i).toIntegerOrInfinity(globalObject);

We can use uncheckedArgument since count is `std::min<size_t>(callFrame->argumentCount(), numberOfTemporalUnits);`

> Source/JavaScriptCore/runtime/TemporalDurationPrototype.cpp:193
> +    auto options = callFrame->argument(0);
> +    if (options.isUndefined())
> +        return throwVMTypeError(globalObject, scope, "Temporal.Duration.prototype.round requires an options argument"_s);

Since round is using https://tc39.es/proposal-temporal/#sec-getoptionsobject, we should create an object if it is undefined.
Can you also add a test for that?

> Source/JavaScriptCore/runtime/TemporalDurationPrototype.cpp:209
> +    auto options = callFrame->argument(0);
> +    if (options.isUndefined())
> +        return throwVMTypeError(globalObject, scope, "Temporal.Duration.prototype.total requires an options argument"_s);

Since total is using https://tc39.es/proposal-temporal/#sec-getoptionsobject, we should create an object if it is undefined.
Can you also add a test for that?

> Source/JavaScriptCore/runtime/TemporalObject.cpp:123
> +static StringView singularUnit(StringView unit)
> +{
> +    // Plurals are allowed, but thankfully they're all just a simple -s.
> +    return unit.endsWith("s") ? unit.left(unit.length() - 1) : unit;
> +}
> +
> +std::optional<TemporalUnit> temporalUnitType(StringView unit)
> +{
> +    StringView singular = singularUnit(unit);
> +
> +    if (singular == "year")
> +        return TemporalUnit::Year;
> +    if (singular == "month")
> +        return TemporalUnit::Month;
> +    if (singular == "week")
> +        return TemporalUnit::Week;
> +    if (singular == "day")
> +        return TemporalUnit::Day;
> +    if (singular == "hour")
> +        return TemporalUnit::Hour;
> +    if (singular == "minute")
> +        return TemporalUnit::Minute;
> +    if (singular == "second")
> +        return TemporalUnit::Second;
> +    if (singular == "millisecond")
> +        return TemporalUnit::Millisecond;
> +    if (singular == "microsecond")
> +        return TemporalUnit::Microsecond;
> +    if (singular == "nanosecond")
> +        return TemporalUnit::Nanosecond;
> +    
> +    return std::nullopt;
> +}

So then, probably, this is not necessary.

> Source/JavaScriptCore/runtime/TemporalObject.cpp:144
> +    String largestUnit = intlStringOption(globalObject, options, vm.propertyNames->largestUnit, { }, nullptr, nullptr);
> +    RETURN_IF_EXCEPTION(scope, std::nullopt);
> +
> +    if (!largestUnit)
> +        return std::nullopt;
> +    
> +    if (largestUnit == "auto")
> +        return autoValue;
> +
> +    auto unitType = temporalUnitType(largestUnit);
> +    if (!unitType) {
> +        throwRangeError(globalObject, scope, "largestUnit is an invalid Temporal unit"_s);
> +        return std::nullopt;
> +    }

You can write it via intlOption<>

std::optional<TemporalUnit> largestUnit = intlOption<std::optional<TemporalUnit>>(globalObject, options, vm.propertyNames->largestUnit,
    {
        { "year"_s, TemporalUnit::Year },
        { "years"_s, TemporalUnit::Year },
        ...
    }, "largestUnit is an invalid Temporal unit"_s, fallback);

fallback can be std::nullopt. Let's take `std::optional<TemporalUnit> fallback` parameter!

> Source/JavaScriptCore/runtime/TemporalObject.cpp:170
> +    String smallestUnit = intlStringOption(globalObject, options, vm.propertyNames->smallestUnit, { }, nullptr, nullptr);
> +    RETURN_IF_EXCEPTION(scope, std::nullopt);
> +
> +    if (!smallestUnit)
> +        return std::nullopt;
> +
> +    auto unitType = temporalUnitType(smallestUnit);
> +    if (!unitType) {
> +        throwRangeError(globalObject, scope, "smallestUnit is an invalid Temporal unit"_s);
> +        return std::nullopt;
> +    }

You can write it via intlOption<>

std::optional<TemporalUnit> smallestUnit = intlOption<std::optional<TemporalUnit>>(globalObject, options, vm.propertyNames->smallestUnit,
    {
        { "year"_s, TemporalUnit::Year },
        { "years"_s, TemporalUnit::Year },
        ...
    }, "smallestUnit is an invalid Temporal unit"_s, fallback);

fallback can be std::nullopt. Let's take `std::optional<TemporalUnit> fallback` parameter!

> Source/JavaScriptCore/runtime/TemporalObject.cpp:259
> +// MaximumTemporalDurationRoundingIncrement ( unit )

Ditto.

> Source/JavaScriptCore/runtime/TemporalObject.cpp:260
> +double maximumRoundingIncrement(TemporalUnit unit)

Should we align this code to more like a spec one?
like, we can return std::optional<double>.

> Source/JavaScriptCore/runtime/TemporalObject.cpp:263
> +        return std::numeric_limits<double>::infinity();

Let's return std::nullopt;

> Source/JavaScriptCore/runtime/TemporalObject.cpp:271
> +// ToTemporalRoundingIncrement ( normalizedOptions, dividend, inclusive )

Ditto.

> Source/JavaScriptCore/runtime/TemporalObject.cpp:289
> +double temporalRoundingIncrement(JSGlobalObject* globalObject, JSObject* options, double dividend, bool inclusive)
> +{
> +    VM& vm = globalObject->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);
> +
> +    auto hasFiniteDividend = dividend < std::numeric_limits<double>::infinity();
> +    auto maximum = (hasFiniteDividend && dividend > 1 && !inclusive) ? dividend - 1 : dividend;
> +    double increment = intlNumberOption(globalObject, options, vm.propertyNames->roundingIncrement, 1, maximum, 1);
> +    RETURN_IF_EXCEPTION(scope, 0);
> +
> +    increment = std::floor(increment);
> +    if (hasFiniteDividend && std::fmod(dividend, increment)) {
> +        throwRangeError(globalObject, scope, "roundingIncrement value does not divide evenly"_s);
> +        return 0;
> +    }
> +
> +    return increment;
> +}

Should we align this code to more like a spec one?
Like, we can take `std::optional<double>` for dividend, and we can write code looks like a spec, like,

double maximum = 0;
if (!dividend)
    maximum = std::numeric_limits<double>::infinity();
else if (inclusive)
    maximum = dividend.value();
else if (dividend.value() > 1)
    maximum = dividend.value() - 1;
else
    maximum = 1;

> Source/JavaScriptCore/runtime/TemporalObject.cpp:291
> +// RoundNumberToIncrement ( x, increment, roundingMode )

Ditto.

[Yusuke Suzuki]

Comment on attachment 436690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436690&action=review

> Source/JavaScriptCore/runtime/TemporalDurationPrototype.cpp:56
> +static JSC_DECLARE_HOST_FUNCTION(TemporalDurationPrototypeFuncWith);
> +static JSC_DECLARE_HOST_FUNCTION(TemporalDurationPrototypeFuncNegated);
> +static JSC_DECLARE_HOST_FUNCTION(TemporalDurationPrototypeFuncAbs);
> +static JSC_DECLARE_HOST_FUNCTION(TemporalDurationPrototypeFuncAdd);
> +static JSC_DECLARE_HOST_FUNCTION(TemporalDurationPrototypeFuncSubtract);
> +static JSC_DECLARE_HOST_FUNCTION(TemporalDurationPrototypeFuncRound);
> +static JSC_DECLARE_HOST_FUNCTION(TemporalDurationPrototypeFuncTotal);
> +static JSC_DECLARE_HOST_FUNCTION(TemporalDurationPrototypeFuncToString);
> +static JSC_DECLARE_HOST_FUNCTION(TemporalDurationPrototypeFuncToJSON);
> +static JSC_DECLARE_HOST_FUNCTION(TemporalDurationPrototypeFuncToLocaleString);
> +static JSC_DECLARE_HOST_FUNCTION(TemporalDurationPrototypeFuncValueOf);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterYears);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterMonths);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterWeeks);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterDays);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterHours);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterMinutes);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterSeconds);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterMilliseconds);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterMicroseconds);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterNanoseconds);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterSign);
> +static JSC_DECLARE_CUSTOM_GETTER(TemporalDurationPrototypeGetterBlank);

And I like naming it `temporalDurationXXX` instead of `TemporalDurationXXX`.

[Yusuke Suzuki]

Comment on attachment 436690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436690&action=review

>> Source/JavaScriptCore/runtime/TemporalDuration.cpp:149
>> +// ParseTemporalDurationString ( isoString )
> 
> Ditto.

Since many of Temporal feature requires some part of ISO8601 parsing, I suggest,

1. Creating ISO8601 parser interface (ISO8601)
2. Define ISO8601::Duration (Subduration)
2. Expose interfaces like, static std::optional<ISO8601::Duration> ISO8601::parseDuration(StringView)

[Yusuke Suzuki]

Comment on attachment 436690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436690&action=review

>>> Source/JavaScriptCore/runtime/TemporalDuration.cpp:149
>>> +// ParseTemporalDurationString ( isoString )
>> 
>> Ditto.
> 
> Since many of Temporal feature requires some part of ISO8601 parsing, I suggest,
> 
> 1. Creating ISO8601 parser interface (ISO8601)
> 2. Define ISO8601::Duration (Subduration)
> 2. Expose interfaces like, static std::optional<ISO8601::Duration> ISO8601::parseDuration(StringView)

In a separate file.

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #14)
> > Source/JavaScriptCore/runtime/TemporalDuration.cpp:437
> > +    uint8_t index = 0;
> > +    while (index < numberOfTemporalUnits - 1 && !m_subdurations[index])
> > +        index++;
> > +    return static_cast<TemporalUnit>(index);
> 
> Is this correct? It seems that it is returning the largest index which does
> not have zero.
> But if index = 0 is year, then we should return as early as we found
> non-zero.
> 
> How about,
> 
> static_assert(static_cast<uint8_t>(TemporalUnit::Year) == 0);
> static_assert(static_cast<uint8_t>(TemporalUnit::Nanoseconds) ==
> numberOfTemporalUnits - 1);
> for (uint8_t index = 0; index < numberOfTemporalUnits; ++index) {
>     if (!m_subdurations[index])
>         return static_cast<TemporalUnit>(index);
> }
> return TemporalUnit::Nanoseconds;

Hm? Mine is saying "advance while the subdurations are zero and we haven't reached the final subduration (Nanosecond) yet". Yours is currently flipped, but should be equivalent if the ! is removed.

> > Source/JavaScriptCore/runtime/TemporalDurationPrototype.cpp:193
> > +    auto options = callFrame->argument(0);
> > +    if (options.isUndefined())
> > +        return throwVMTypeError(globalObject, scope, "Temporal.Duration.prototype.round requires an options argument"_s);
> 
> Since round is using
> https://tc39.es/proposal-temporal/#sec-getoptionsobject, we should create an
> object if it is undefined.
> Can you also add a test for that?
> 
> > Source/JavaScriptCore/runtime/TemporalDurationPrototype.cpp:209
> > +    auto options = callFrame->argument(0);
> > +    if (options.isUndefined())
> > +        return throwVMTypeError(globalObject, scope, "Temporal.Duration.prototype.total requires an options argument"_s);
> 
> Since total is using
> https://tc39.es/proposal-temporal/#sec-getoptionsobject, we should create an
> object if it is undefined.
> Can you also add a test for that?

The case where an object would be created in GetOptionsObject is pre-empted by step 3 in round(); this line is also being added for total() in PR #1720.

https://tc39.es/proposal-temporal/#sec-temporal.duration.prototype.round

https://github.com/tc39/proposal-temporal/pull/1720/files#diff-ceec2609082b71fe76df3f43d148b97d667dde310f51a514141886246bf3fc74

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #14)
> > Source/JavaScriptCore/runtime/TemporalObject.cpp:144
> > +    String largestUnit = intlStringOption(globalObject, options, vm.propertyNames->largestUnit, { }, nullptr, nullptr);
> > +    RETURN_IF_EXCEPTION(scope, std::nullopt);
> > +
> > +    if (!largestUnit)
> > +        return std::nullopt;
> > +    
> > +    if (largestUnit == "auto")
> > +        return autoValue;
> > +
> > +    auto unitType = temporalUnitType(largestUnit);
> > +    if (!unitType) {
> > +        throwRangeError(globalObject, scope, "largestUnit is an invalid Temporal unit"_s);
> > +        return std::nullopt;
> > +    }
> 
> You can write it via intlOption<>
> 
> std::optional<TemporalUnit> largestUnit =
> intlOption<std::optional<TemporalUnit>>(globalObject, options,
> vm.propertyNames->largestUnit,
>     {
>         { "year"_s, TemporalUnit::Year },
>         { "years"_s, TemporalUnit::Year },
>         ...
>     }, "largestUnit is an invalid Temporal unit"_s, fallback);
> 
> fallback can be std::nullopt. Let's take `std::optional<TemporalUnit>
> fallback` parameter!

This was a cool idea but I'll leave it as-is given our Slack discussion.

[Philip Chimento]

Comment on attachment 436690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436690&action=review

Cool! I think the main comment I have on the patch in general, is that we should try to make error messages as educational as possible for the developer. I think that will be good for adoption of Temporal in the JS ecosystem. Specific comments follow.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:95
> +        throwRangeError(globalObject, scope, "cannot create Duration with inconsistent numeric values"_s);

For developer-experience purposes we should probably have a more informative error message here. It would be great if the message could say either that infinities or mixed-sign values are not allowed depending on what the problem was.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:150
> +template<typename CharacterType>

would code like `*buffer == 0x2212` be correct for all CharacterTypes? or should this function deal explicitly with WTFString? (Ignore me if there is an existing convention in the codebase to do it this way)

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:648
> +        throwRangeError(globalObject, scope, "unit is an invalid Temporal unit"_s);

Would be good to list which units are valid in the error message.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:708
> +    auto formatInteger = [](double value) -> double {

Could (for the time being, pending resolution of that issue) do something like

    JSBigInt* bigint = JSBigInt::createFrom(globalObject, value);
    ASSERT_NO_EXCEPTION();
    String resut = bigint->toString(globalObject, 10);
    ASSERT_NO_EXCEPTION();
    return result;

> Source/JavaScriptCore/runtime/TemporalDurationPrototype.cpp:125
> +        return throwVMTypeError(globalObject, scope, "Temporal.Duration.prototype.with called on value that's not an object initialized as a Duration"_s);

I feel the brand check messages would be somewhat clearer to callers without "an object initialized as" since that's an implementation detail.

>> Source/JavaScriptCore/runtime/TemporalDurationPrototype.cpp:193
>> +        return throwVMTypeError(globalObject, scope, "Temporal.Duration.prototype.round requires an options argument"_s);
> 
> Since round is using https://tc39.es/proposal-temporal/#sec-getoptionsobject, we should create an object if it is undefined.
> Can you also add a test for that?

Currently we don't get to GetOptionsObject if undefined is passed. https://tc39.es/proposal-temporal/#sec-temporal.duration.prototype.round step 3

See https://github.com/tc39/proposal-temporal/issues/1756 for discussion, it's possible TC39 will force this to change, though I hope that won't be necessary.

>> Source/JavaScriptCore/runtime/TemporalDurationPrototype.cpp:209
>> +        return throwVMTypeError(globalObject, scope, "Temporal.Duration.prototype.total requires an options argument"_s);
> 
> Since total is using https://tc39.es/proposal-temporal/#sec-getoptionsobject, we should create an object if it is undefined.
> Can you also add a test for that?

Same here.

> Source/JavaScriptCore/runtime/TemporalDurationPrototype.cpp:256
> +    return throwVMTypeError(globalObject, scope, "Temporal.Duration.prototype.valueOf must not be called"_s);

I would suggest making the error message more educational and saying what you should do instead (e.g. "use Temporal.Duration.compare to compare Duration instances" or something like that)

> Source/JavaScriptCore/runtime/TemporalObject.cpp:93
> +    // Plurals are allowed, but thankfully they're all just a simple -s.

...and none of the singular names end with 's' :-)

> JSTests/stress/temporal-duration.js:1
> +//@ requireOptions("--useTemporal=1")

What's the plan for the tests in this file? Are they going to be removed later when test262 coverage is improved?

[Ross Kirsling]

(In reply to Philip Chimento from comment #20)
> > Source/JavaScriptCore/runtime/TemporalDuration.cpp:150
> > +template<typename CharacterType>
> 
> would code like `*buffer == 0x2212` be correct for all CharacterTypes? or
> should this function deal explicitly with WTFString? (Ignore me if there is
> an existing convention in the codebase to do it this way)

I think this should be okay -- if the left side is LChar, i.e., unsigned char, it's guaranteed to return false (says a clang warning message when I tried to do this directly).

> > Source/JavaScriptCore/runtime/TemporalDuration.cpp:708
> > +    auto formatInteger = [](double value) -> double {
> 
> Could (for the time being, pending resolution of that issue) do something
> like
> 
>     JSBigInt* bigint = JSBigInt::createFrom(globalObject, value);
>     ASSERT_NO_EXCEPTION();
>     String resut = bigint->toString(globalObject, 10);
>     ASSERT_NO_EXCEPTION();
>     return result;

Hmm, I went with the truncation for now so that it's more obviously a placeholder; JSBigInt would require us to do real error handling for this function, so I don't think it's right to just pretend nothing bad happened...

> > JSTests/stress/temporal-duration.js:1
> > +//@ requireOptions("--useTemporal=1")
> 
> What's the plan for the tests in this file? Are they going to be removed
> later when test262 coverage is improved?

We tend to have our own tests even when they're redundant with test262 -- certainly not for the purpose of *being* redundant with test262, but generally speaking it helps us check against the actual codepaths we've written, and at Stage 3, I think it helps to guide development. Perhaps most importantly, JSC stress tests are run by EWS and will thus block a patch from landing, while test262 tests are just run after a patch is landed. It may well be justifiable not to write a stress test for something that's known to be covered by test262, but we probably wouldn't bother to prune existing stress tests on that basis.

[Ross Kirsling]

Created attachment 436829 [details]
Patch

Addressed current feedback and enabled relevant test262 tests.

[Ross Kirsling]

Created attachment 436911 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 436911 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436911&action=review

r=me

> Source/JavaScriptCore/runtime/ISO8601.cpp:43
> +        auto padded = makeString(fractionString, "00000000");

Maybe we should use Vector<LChar, 9 + 8> buffer to avoid heap allocation here (We know that all fractionString's characters are ASCIIDigit, so it should be fit in LChar. And we know the maximum length is 9. And padding length is 8. So 9 + 8 should be enough.
But it is OK for the subsequent patch for optimization.

> Source/JavaScriptCore/runtime/ISO8601.cpp:57
> +        duration.setMinutes(std::trunc(fraction));

OK, this is a spec bug.

> Source/JavaScriptCore/runtime/ISO8601.cpp:97
> +    if (buffer.lengthRemaining() < 3)
> +        return std::nullopt;

This looks nice. Let's add a test cases like,

""
"+"
"+P"
"-P"
"+-P"

> Source/JavaScriptCore/runtime/ISO8601.cpp:136
> +        if (toASCIIUpper(*buffer) == 'Y' && !datePartIndex) {
> +            result.setYears(integer);
> +            datePartIndex = 1;
> +        } else if (toASCIIUpper(*buffer) == 'M' && datePartIndex < 2) {
> +            result.setMonths(integer);
> +            datePartIndex = 2;
> +        } else if (toASCIIUpper(*buffer) == 'W' && datePartIndex < 3) {
> +            result.setWeeks(integer);
> +            datePartIndex = 3;
> +        } else if (toASCIIUpper(*buffer) == 'D') {
> +            result.setDays(integer);
> +            datePartIndex = 4;
> +        } else
> +            return std::nullopt;

Using switch would be better for readability?

switch (toASCIIUpper(*buffer)) {
case 'Y': {
    if (datePartIndex)
        return std::nullopt;
    result.setYears(integer);
    datePartIndex = 1;
    break;
}
...
default:
    return std::nullopt;
}

> Source/JavaScriptCore/runtime/ISO8601.cpp:163
> +            if (!digits || digits > 9)
> +                return std::nullopt;

Looks nice. Let's add a test case like,

"+P20.S"
"+P.20S"

> Source/JavaScriptCore/runtime/ISO8601.cpp:171
> +        if (toASCIIUpper(*buffer) == 'H' && !timePartIndex) {

Ditto about switch.

> Source/JavaScriptCore/runtime/ISO8601.cpp:175
> +                timePartIndex = 3;

This trick is nice.

> Source/JavaScriptCore/runtime/ISO8601.h:48
> +    std::array<double, numberOfTemporalUnits> data;

Let's have a default initialization here since std::array can be uninitialized.

std::array<double, numberOfTemporalUnits> data { };

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:202
> +    if (one->years() || two->years() || one->months() || two->months() || one->weeks() || two->weeks()) {
> +        throwRangeError(globalObject, scope, "Cannot compare a duration of years, months, or weeks without a relativeTo option"_s);
> +        return { };
> +    }

Let's have a FIXME for handling this case when Temporal DateTime is supported.

> Source/JavaScriptCore/runtime/TemporalDuration.cpp:387
> +        static const double nanosecondsPerDay = 24.0 * 60 * 60 * 1000 * 1000 * 1000;

It appears twice, so let's define it as a global static constexpr value.

> Source/JavaScriptCore/runtime/TemporalObject.cpp:149
> +    if (largestUnit == "auto")

Use "auto"_s.

[Ross Kirsling]

Created attachment 436988 [details]
Patch

[Ross Kirsling]

Created attachment 436993 [details]
Patch for landing

[EWS]

Committed r281838 (241171@main): <https://commits.webkit.org/241171@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436993 [details].