Data-driven exceptions in calendar method lookups

[gibson042]

The add and subtract methods of Plain{Date,DateTime,YearMonth} and ZonedDateTime and the until and since methods of PlainYearMonth perform conditional lookup of calendar methods, such that e.g.

const BrokenCalendar = class extends Temporal.Calendar { dateAdd = null; };
const yearMonth = new Temporal.PlainYearMonth(0, 1, new BrokenCalendar("iso8601"));
yearMonth.add({ weeks: 0 }); // => Temporal.PlainYearMonth <0000-01>
yearMonth.add({ weeks: 1 }); // => TypeError: dateAdd should be present on Calendar

Since we have agreed elsewhere that it is the responsibility of a calendar author to implement all methods (and indeed the constructors verify that the properties expected to be methods are at least present on a calendar argument), I think I would instead expect unconditional lookup of all methods that might be called... for example, in AddDurationToOrSubtractDurationFromPlainYearMonth:

--- spec/plainyearmonth.html
+++ spec/plainyearmonth.html
@@ -681,13 +681,7 @@
           1. Set _duration_ to ! CreateNegatedTemporalDuration(_duration_).
         1. Let _balanceResult_ be ? BalanceTimeDuration(_duration_.[[Days]], _duration_.[[Hours]], _duration_.[[Minutes]], _duration_.[[Seconds]], _duration_.[[Milliseconds]], _duration_.[[Microseconds]], _duration_.[[Nanoseconds]], *"day"*).
         1. Let _sign_ be ! DurationSign(_duration_.[[Years]], _duration_.[[Months]], _duration_.[[Weeks]], _balanceResult_.[[Days]], 0, 0, 0, 0, 0, 0).
-        1. Let _calendarRec_ be ! CreateCalendarMethodsRecord(_yearMonth_.[[Calendar]], « »).
-        1. If _sign_ < 0, or _duration_.[[Years]] ≠ 0, or _duration_.[[Months]] ≠ 0, or _duration_.[[Weeks]] ≠ 0, then
-          1. Perform ? CalendarMethodsRecordLookup(_calendarRec_, ~dateAdd~).
-        1. Perform ? CalendarMethodsRecordLookup(_calendarRec_, ~dateFromFields~).
-        1. If _sign_ < 0, perform ? CalendarMethodsRecordLookup(_calendarRec_, ~day~).
-        1. Perform ? CalendarMethodsRecordLookup(_calendarRec_, ~fields~).
-        1. Perform ? CalendarMethodsRecordLookup(_calendarRec_, ~yearMonthFromFields~).
+        1. Let _calendarRec_ be ! CreateCalendarMethodsRecord(_yearMonth_.[[Calendar]], « ~dateAdd~, ~dateFromFields~, ~day~, ~fields~, ~yearMonthFromFields~ »).
         1. Let _fieldNames_ be ? CalendarFields(_calendarRec_, « *"monthCode"*, *"year"* »).
         1. Let _fields_ be ? PrepareTemporalFields(_yearMonth_, _fieldNames_, «»).
         1. Let _fieldsCopy_ be ! SnapshotOwnProperties(_fields_, *null*).

A case can be made for keeping the lookups conditional in e.g. Temporal.Duration.{add,subtract,compare,round,total}, but even there I'm not sure it provides sufficient benefit to justify the complexity and potential for user issues.

[ptomato]

This seems in conflict with the goal of #2289 of reducing the number of observable Gets, which is the reason why we introduced these method records.

Personally I'd be happy if we made this "post-merge" adjustment to #2519 — I also don't like this complexity. Let's talk about the conflict with the original goal in the next champions meeting.

(But I wouldn't call these data-driven exceptions. They won't occur in any reasonable data; only with malicious calendar objects such as the BrokenCalendar from your example.)

[gibson042]

This seems in conflict with the goal of #2289 of reducing the number of observable Gets, which is the reason why we introduced these method records.

Ah, I think that's a slight mischaracterization... #2289 wasn't about minimizing the raw count of observable Gets for any given operation, but rather about eliminating redundant observable Gets and (more broadly) minimizing leakage of spec algorithm internal details. The Calendar Methods Record approach is great and absolutely addresses that concern, I just think it would be better to populate them unconditionally.

Personally I'd be happy if we made this "post-merge" adjustment to #2519 — I also don't like this complexity. Let's talk about the conflict with the original goal in the next champions meeting.

👍

[ptomato]

Temporal champions meeting, 2023-11-09: Richard's point has broad agreement. We'll make this adjustment, as it is essentially a code review comment from Richard on #2519 that didn't quite make it in before the PR was merged.