Calendars can't accept fields in from() or with() that are not emitted by getFields()

[justingrant]

The Calendar.prototype.fields method is currently used for two purposes:
a) to determine the list of fields emitted by getFields()
b) to constrain the list of fields accepted by with() and the property-bag input of from().

This means that calendars cannot accept any fields in from() or with() input that are not also present in getFields() output. This is problematic because calendars may want to offer flexible ways to specify dates. For example:

• Specifying a year either via a combination of year and era (e.g. eraYear: 100, era: 'bc', or via a single signed epoch-relative year value e.g. year: -99
• Specifying a month either as a month index in the current year (e.g. 6 for Adar I, the Hebrew leap month), or as a string monthCode like '5L'.

The simplest way to solve this problem would be to simply not use Calendar.prototype.fields in the implementation of from() or with. Instead, we'd just pass the bag input straight through to the calendar. This works OK for from in PlainDate, MonthDay, and YearMonth where the calendar is the only code that needs to access those date fields (per #1229). I think (although not 100% sure) that the same approach will work for with too on these types.

But if ZonedDateTime.from and PlainDateTime.from used that pass-through approach, then there'd be no way for those methods to guarantee that field access happens in alphabetical order per #736 (comment), because the calendar will access the date fields only, and the time fields will be accessed separately (and out of order from the date fields) inside ecmascript.mjs.

If we do want to enable calendars to accept arbitrary fields as input, here's a few possible options:

1. Relax the requirement that each from() call should always access fields of input objects in alphabetical order.
2. Add another parameter to Calendar.prototype.fields to indicate whether the field list will be used for input (from or with) or for output (getFields).
3. Accept that all fields accepted as input must be output by getFields, even if they overlap with other output fields, e.g. month and monthCode in the same output bag. I'm skeptical about this option because it'd introduce the possibility of field conflicts if the getFields() output is modified and then sent back into Temporal, e.g. if a developer changes month but forgets to also change monthCode. Also, this would prevent calendars from offering an opinionated canonical set of fields while still being flexible about alternate/aliased forms, e.g. Chinese named years.

(2) seems like the easiest and most flexible solution. Most folks are probably ignoring GitHub issues for the holidays, so unless anyone has a strong opinion about this I'll plan to adopt (2) to avoid blocking my almost-complete non-ISO calendar PR. We can always revise later as needed.

[justingrant]

A related problem I found today: with is also problematic if input fields can overlap or be aliased, because the merging of user-inputted fields and existing fields happens inside ecmascript.mjs, not inside the calendar. This means that date.with({monthCode: '5L'}) will fail because by the time the calendar gets the list of fields, there's no way for it to know if month or monthCode is the correct input field to use when constructing the new date.

One way to handle this (probably the easiest and perhaps the best) would be to move the merging of new vs. existing date fields into the calendar implementation, which I assume would mean extending the CalendarProtocol API, e.g. a new Calendar.prototype.dateWith({bag}) method. I'll plan to add this in the non-ISO-calendar PR unless someone has a better idea!

EDIT: another possible solution might be to add a third "role" in the fields() method per (2) above. The roles would be:

• 'input' - fields accepted by .from() or other APIs that create instances from bags.
• 'output' - fields to be emitted by .getFields()
• 'merge' - fields to exclude from merging in .with(). The input to fields() would be the list of fields that the user provided, and the output would be the list of fields that should be extracted from the current instance before merging it with the user-provided fields. For example, fields(['monthCode'], { role: 'merge' }) would return ['month'].

This seems to be cleaner and easier than my first proposal above because doesn't grow the CalendarProtocol API surface and concentrates all non-ISO-calendar-related changes to the fields() method.

[Louis-Aime]

I feel OK with the track opened per (2). I think example usage will be easy to explain.

[Ms2ger]

Most of the decisions about processing option bags date back to before we supported dynamically changing the supported fields, so I don't see a problem with reconsidering. The impact is pretty limited anyway.

Right now, the calendar-independent code does a bunch of preprocessing and passes fairly normalized values to the (built-in or author-implemented) Calendar methods. This is mostly for convenience when implementing a custom calendar (as the implementation doesn't need to be as robust to weird input), and for predictability when using a custom calendar (since weird inputs are handled uniformly); both of those reasons apply only when calling through the main objects - if you call the Calendar methods directly, you're on your own.

If it turns out this is blocking useful functionality, I don't mind dropping the preprocessing entirely.

That would solve from and getFields, but leaves with. I don't have a clear idea of how to handle that at this point.

[justingrant]

That would solve from and getFields, but leaves with. I don't have a clear idea of how to handle that at this point.

in my upcoming PR for basic implementations of ICU calendars, I'm currently using this proposal from above:

EDIT: another possible solution might be to add a third "role" in the fields() method per (2) above. The roles would be:

• 'input' - fields accepted by .from() or other APIs that create instances from bags.
• 'output' - fields to be emitted by .getFields()
• 'merge' - fields to exclude from merging in .with(). The input to fields() would be the list of fields that the user provided, and the output would be the list of fields that should be extracted from the current instance before merging it with the user-provided fields. For example, fields(['monthCode'], { role: 'merge' }) would return ['month'].

So far it's been working OK. Here's the current implementation:

  fields(fields, role) {
    // Some non-ISO calendars accept multiple ways to specify a year and/or a
    // month. Here's where we add this support for all calendars (including
    // ISO), which makes it easier to write cross-calendar code. Note that this
    // only affects only how input is interpreted. `getFields()` output is not
    // affected.
    if (role === 'input') {
      if (fields.includes('year')) {
        fields.push('era');
        fields.push('eraYear');
      }
      if (fields.includes('month')) {
        fields.push('monthCode');
      }
    } else if (role === 'merge') {
      // If the user includes one of `fields` as input to `with()`, then the
      // returned fields array will be not be pulled from this before `with()`
      // does the merge. For example, if the user calls `date.with({monthCode})`
      // then `month` will be excluded from the merge so the input to the
      // calendar's `dateFromFields()` method will be `{day, monthCode, year}`
      // not `{day, month, monthCode, year}` which would be problematic because
      // the month values could conflict and the calendar wouldn't know which
      // one to use.
      const removeFields = [];
      if (fields.includes('year')) {
        removeFields.push('era');
        removeFields.push('eraYear');
        removeFields.push('year');
      }
      if (fields.includes('eraYear')) {
        removeFields.push('eraYear');
        removeFields.push('year');
      }
      if (fields.includes('month') || fields.includes('monthCode')) {
        removeFields.push('month');
        removeFields.push('monthCode');
      }
      return removeFields;
    }
    return fields;
  },

To get the new role option to work, I needed to modify a handful of places in the polyfill.

  CalendarFields: (calendar, fieldNames, role = 'input') => {
    let fields = calendar.fields;
    if (fields === undefined) fields = GetIntrinsic('%Temporal.Calendar.prototype.fields%');
    const array = ES.Call(fields, calendar, [fieldNames, { role }]);
    return ES.CreateListFromArrayLike(array, ['String']);
  },

  MergeFields: (calendar, fields, props) => {
    const doNotMergeNames = ES.CalendarFields(calendar, ES.GetOwnPropertyKeys(props, 'String'), 'merge');
    let merged = ObjectAssign({}, fields);
    for (const prop of doNotMergeNames) {
      delete merged[prop];
    }
    ObjectAssign(merged, props);
    return merged;
  },

And here's how it's used in PlainDate.prototype.with:

    const calendar = GetSlot(this, CALENDAR);
    const fieldNames = ES.CalendarFields(calendar, ['day', 'month', 'year']);
    const props = ES.ToPartialRecord(temporalDateLike, fieldNames);
    if (!props) {
      throw new TypeError('invalid date-like');
    }
    let fields = ES.ToTemporalDateFields(this, fieldNames);
    fields = ES.MergeFields(calendar, fields, props);

And also here:

  getFields() {
    if (!ES.IsTemporalDate(this)) throw new TypeError('invalid receiver');
    const calendar = GetSlot(this, CALENDAR);
    const fieldNames = ES.CalendarFields(calendar, ['day', 'month', 'year'], 'output');
    const fields = ES.ToTemporalDateFields(this, fieldNames);
    fields.calendar = calendar;
    return fields;
  }

Finally, I modified the calendar-calling ToTemporalXxxFields methods to not require any of the fields, instead deferring that responsibility to the calendar.

  // field access in the following operations is intentionally alphabetical
  ToTemporalDateFields: (bag, fieldNames) => {
    const entries = [
      ['day', undefined],
      ['month', undefined],
      ['year', undefined]
    ];
    // Add extra fields from the calendar at the end
    fieldNames.forEach((fieldName) => {
      if (!entries.some(([name]) => name === fieldName)) {
        entries.push([fieldName, undefined]);
      }
    });
    return ES.ToRecord(bag, entries, false);
  },

One change I'm planning to make: instead of after-the-fact delete in MergeFields, it's probably better to avoid pulling those values from the original bag.

Assuming that change is made, @Ms2ger what do you think of this approach above?

[ptomato]

I do think this is a finding from our reviews that we need to address, but I'm not convinced that the role parameter is the right way to do it. @cjtenny and/or I will investigate alternatives over the coming days. Personally I'm fine with (3) because

• developers shouldn't be changing one property from the return value of getFields() and feeding it back into from(); they should be using with();
• it wouldn't affect developers using the ISO calendar or calendars based on it, because those wouldn't have both the month index and month code, so the only developers likely to run into the problem would be aware of the distinction already;
• it's the least churn.

[justingrant]

• developers shouldn't be changing one property from the return value of getFields() and feeding it back into from(); they should be using with();

Unless I misunderstood the logic flow in the polyfill, I don't think we can make with work ing the current Calendar API for calendars that accept custom fields. Here's my understanding of the flow of PlainDate.prototype.with:

1. Fetch the list of PlainDate fields from the calendar.
2. Call ES.ToRecord to get those fields' values. ToRecord calls a bunch of calendar getters.
3. Merge those fields with the user's input bag, with user fields overwriting this fields.
4. Pass the merged fields to the calendar's dateFromFields method.

The problem is that in step (4), how does the calendar know which fields were provided by the user and which ones come from this? For example:

Temporal.PlainDate.from({ year: 5760, month: 1, day: 1, calendar: 'hebrew'}).with({monthCode: '5' });

The bag passed to dateFromFields is this: {year: 5760, month: 1, monthCode: '5', day: 1, calendar: 'hebrew'}. Who wins: month or monthCode?

I guess one solution is to say that calendars can't have conflicting fields. All fields must be mutually-exclusive and non-overlapping. But based on what I learned from #1245, many calendars have overlapping ways to refer to years and months. month vs. monthCode is one example, but I saw others that I'd expect in userland, e.g. chinese zodiac applied to years.

I agree that the other role ('output') is less critical. But it does limit the calendar API significantly, because it means that accepting a new field in from means that it must be emitted from getFields. This can have perf and other issues, e.g. {calendar, year, eraYear, era, month, monthCode, day, animal, zodiacSign, yearCycle, monthFlavor, someOtherField, ...} when really only {calendar, year, month, day} is needed. If we must change the API to support with, then this seems worth considering.

[ptomato]

Conclusions from 2021-01-19 meeting:

• Calendars being able to add convenience fields that allow specifying fields in other ways (e.g., monthCode vs month+year) means that spreading Temporal objects has become much less useful.
• At the same time, spreading Temporal objects is less necessary due to the changes we've made in accepting either Temporal objects or property bags, and the improvements we've made to the with() method.
• Spreading Temporal objects was the only(?) reason we added the getFields() method.
• Therefore, unless we find use cases other than spreading which we are currently not aware of, remove the getFields() method from all types, possibly deferring it to a follow up proposal.
• The problem of the calendar not being able to distinguish in dateFromFields(), yearMonthFromFields(), and monthDayFromFields(), which fields were passed in as arguments to with(), still remains and must be solved internally, but we don't have a strong preference on which solution. @ptomato or @cjtenny to run a solution by the Temporal champions meeting on Thursday.

I will close this issue and open two new ones for those actions (remove getFields(), and solve with().)