Normative: Get the month from the internal result directly

[FrankYFTang]

Since the step 4 stement in CalendarDateUntil is
4. Perform ? RequireInternalSlot(duration, [[InitializedTemporalDuration]]).
untilResult is guarantee to be a TemporalDuration object and there are no need to call Get - which could return any kind of values, but
only need to acecss [[Months]] which is guarantee for certain type and range.

[FrankYFTang]

I think this Editoral change @ptomato @jugglinmike @ljharb @justingrant Is it correct?

[codecov]

Codecov Report

Merging #1673 (dc13d68) into main (3337074) will decrease coverage by 3.93%.
The diff coverage is n/a.

❗ Current head dc13d68 differs from pull request most recent head 04c110d. Consider uploading reports for the commit 04c110d to get more accurate results

@@            Coverage Diff             @@
##             main    #1673      +/-   ##
==========================================
- Coverage   95.02%   91.09%   -3.94%     
==========================================
  Files          19       17       -2     
  Lines       10778    10768      -10     
  Branches     1725     1608     -117     
==========================================
- Hits        10242     9809     -433     
- Misses        523      945     +422     
- Partials       13       14       +1     

Flag	Coverage Δ
test262	?
tests	90.14% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
polyfill/lib/plaindate.mjs	72.06% <0.00%> (-18.31%)	⬇️
polyfill/lib/plaintime.mjs	79.72% <0.00%> (-11.03%)	⬇️
polyfill/lib/plainmonthday.mjs	83.56% <0.00%> (-9.59%)	⬇️
polyfill/lib/zoneddatetime.mjs	90.62% <0.00%> (-7.77%)	⬇️
polyfill/lib/instant.mjs	87.15% <0.00%> (-7.30%)	⬇️
polyfill/lib/timezone.mjs	86.27% <0.00%> (-7.19%)	⬇️
polyfill/lib/plaindatetime.mjs	90.34% <0.00%> (-6.97%)	⬇️
polyfill/lib/plainyearmonth.mjs	89.77% <0.00%> (-6.63%)	⬇️
polyfill/lib/duration.mjs	93.65% <0.00%> (-4.43%)	⬇️
polyfill/lib/calendar.mjs	91.40% <0.00%> (-2.97%)	⬇️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3337074...04c110d. Read the comment docs.

[ljharb]

CalendarDateUntil ensures the result has the [[InitializedTemporalDuration]] slot, so theoretically that’s always something with a [[Months]] slot.

However, since this calls user code, it could return an object with a [[Months]] slot but that has a .months property/accessor that provides a different value - so i think this is actually both normative, and hostile to subclassing?

[FrankYFTang]

you are right! It is a normative change. I think what I think is this access should NOT need to be observable but it is now observable via Get. But if we look at other places in the Temporal after calling CalendarDateUntil, we always use direct access in the following places in stead of Get() . so unlike there is a very good reason we like to let the Calendar to observe such access, we may want to change this place like other places with direct access to internal slot.

3.3.23 Temporal.PlainDate.prototype.until ( other [ , options ] )
13. Let result be ? CalendarDateUntil(temporalDate.[[Calendar]], temporalDate, other, untilOptions).
14. If smallestUnit is not "day" or roundingIncrement ≠ 1, then
...
b. Set result to ? RoundDuration(result.[[Years]], result.[[Months]], result.[[Weeks]], result.[[Days]], 0, 0, 0, 0, 0, 0, roundingIncrement, smallestUnit, roundingMode, relativeTo).
15. Return ? CreateTemporalDuration(result.[[Years]], result.[[Months]], result.[[Weeks]], result.[[Days]], 0, 0, 0, 0, 0, 0).

3.3.24 Temporal.PlainDate.prototype.since ( other [ , options ] )
14. Let result be ? CalendarDateUntil(temporalDate.[[Calendar]], other, temporalDate, untilOptions).
15. If smallestUnit is "day" and roundingIncrement = 1, then
a. Return ? CreateTemporalDuration(result.[[Years]], result.[[Months]], result.[[Weeks]], result.[[Days]], 0, 0, 0, 0, 0, 0).
...
17. Set result to ? RoundDuration(−result.[[Years]], −result.[[Months]], −result.[[Weeks]], −result.[[Days]], 0, 0, 0, 0, 0, 0, roundingIncrement, smallestUnit, roundingMode, relativeTo).

5.5.11 DifferenceISODateTime ( y1, mon1, d1, h1, min1, s1, ms1, mus1, ns1, y2, mon2, d2, h2, min2, s2, ms2, mus2, ns2, calendar, largestUnit [ , options ] )

Let dateDifference be ? CalendarDateUntil(calendar, date1, date2, untilOptions).
13. Return ? BalanceDuration(dateDifference.[[Years]], dateDifference.[[Months]], dateDifference.[[Weeks]], dateDifference.[[Days]], timeDifference.[[Hours]], timeDifference.[[Minutes]], timeDifference.[[Seconds]], timeDifference.[[Milliseconds]], timeDifference.[[Microseconds]], timeDifference.[[Nanoseconds]], largestUnit).

7.5.13 AddDuration ( y1, mon1, w1, d1, h1, min1, s1, ms1, mus1, ns1, y2, mon2, w2, d2, h2, min2, s2, ms2, mus2, ns2, relativeTo )
step 6
m. Let dateDifference be ? CalendarDateUntil(calendar, datePart, end, differenceOptions).
n. Let result be ! BalanceDuration(dateDifference.[[Days]], h1 + h2, min1 + min2, s1 + s2, ms1 + ms2, mus1 + mus2, ns1 + ns2, largestUnit).
o. Let years be dateDifference.[[Years]].
p. Let months be dateDifference.[[Months]].
q. Let weeks be dateDifference.[[Weeks]].

7.5.17 RoundDuration ( years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds, increment, unit, roundingMode [ , relativeTo ] )
step 9
p. Let timePassed be ? CalendarDateUntil(calendar, relativeTo, daysLater, untilOptions).
q. Let yearsPassed be timePassed.[[Years]].

9.3.14 Temporal.PlainYearMonth.prototype.until ( other [ , options ] )

21. Let result be ? CalendarDateUntil(calendar, thisDate, otherDate, untilOptions).
22. If smallestUnit is "month" and roundingIncrement = 1, then
a. Return ? CreateTemporalDuration(result.[[Years]], result.[[Months]], 0, 0, 0, 0, 0, 0, 0, 0).
...
24. Let result be ? RoundDuration(result.[[Years]], result.[[Months]], 0, 0, 0, 0, 0, 0, 0, 0, roundingIncrement, smallestUnit, roundingMode, relativeTo).

[ljharb]

I mean, the good reason is so subclasses can’t override the months value.

Implementations can of course optimize the current spec to an internal slot access when they know that the object has the native months getter, so I’m not sure what the value is of removing the subclassing hook

In other words, if there’s a chance that the other places using the slot should be doing a Get, perhaps those should be changed to a Get instead?

[FrankYFTang]

I mean, the good reason is so subclasses can’t override the months value.

Implementations can of course optimize the current spec to an internal slot access when they know that the object has the native months getter, so I’m not sure what the value is of removing the subclassing hook

In other words, if there’s a chance that the other places using the slot should be doing a Get, perhaps those should be changed to a Get instead?

Actually the origional reason I disconvered this is not becaue I like to have direct access but I realize after the Get, there are no type/range checking before treating it as numeric value. I was intending to file a fix for that but then I realize maybe the right way is to directly access the internal slot since the CalendarDateUntil already gunarantee it has those values in the internal slot in the correct type/range. We do need range/type check and throw we call Get. Direct access won't need those. But if we want to keep using the Get(), I believe we should

1. Have a consistent behavior on all other unless there is a particular reason the "Months" should be treated differently from other fields (and notice this is not a Month but a Months of the duration, which have nothing to do with monthCode/month issue. ) - so either we A) change the access "months" as this PR, or B) change other access to Get.- unless there are good reason not to.
2. We have to do type/range checking after the Get. IF not, the value could be anything (String, Symbol, Bollean, undefined, BigInt, NaN, Infinity, non integer, etc) and the following statements won't make sense until we type/range check + throw + conversion-

7.5.11 UnbalanceDurationRelative ( years, months, weeks, days, largestUnit, relativeTo )
https://tc39.es/proposal-temporal/#sec-temporal-unbalancedurationrelative

Inside Step 10-d
vi.Let oneYearMonths be ? Get(untilResult, "months").
...
ix. Set months to months + oneYearMonths.

what is the result of months after ix. if Get return non numeric value? (undefind, String, Symbol, etc)

7.5.12 BalanceDurationRelative ( years, months, weeks, days, largestUnit, relativeTo )
https://tc39.es/proposal-temporal/#sec-temporal-balancedurationrelative

Inside step 9
p. Let oneYearMonths be ? Get(untilResult, "months").
q. Repeat, while abs(months) ≥ abs(oneYearMonths),
i. Set months to months − oneYearMonths.
...
ix. Set oneYearMonths to ? Get(untilResult, "months").

What is abs of non numeric value? (undefind, String, Symbol, etc) and the resul tof months...

The subclass could return value other than what is stored in [[Months]] right? therefore it could any kind of values. So if we really want to let subclass of Duration to return differnt value, we need to range/type check and convert after the Get, right?

[ljharb]

I would expect typechecking after the Get, certainly.

I agree it makes sense to have consistent treatment here - either always get the fields and typecheck/coerce, or always read the slots. I assume the temporal champions have an overarching ethos here and the inconsistency is an oversight, but i don’t know which matches their intention.

[ptomato]

It looks like this partly overlaps with #1616 which was already on the queue for normative changes. Would it be possible to add a test for these other instances?

I agree it makes sense to have consistent treatment here - either always get the fields and typecheck/coerce, or always read the slots. I assume the temporal champions have an overarching ethos here and the inconsistency is an oversight, but i don’t know which matches their intention.

Indeed, this was an oversight. The intention was to always read the slots. (See #231). Subclasses of Temporal.Duration could still override the .months getter, but CalendarDateUntil requires that the user code returns a Temporal.Duration instance precisely so that we can get at the underlying data model rather than go through subclass code to get it.

[ptomato]

(Converted to draft so the normative change isn't accidentally merged without consensus)

[ptomato]

I've copied this one over to #1616 since they are very similar, and added tests there, so closing. (Less work for delegates to review)

[FrankYFTang]

thanks