Should PrepareTemporalFields vend frozen objects?

[bakkot]

PrepareTemporalFields eagerly sets up an object with certain fields guaranteed to exist and be of the expected type.

But then the object passed to user code on multiple occasions: for example with passes it to CalendarMergeFields which passes it to calendar. mergeFields, and then to DateFromFields which passes it to calendar.dateFromFields.

Nothing prevents the earlier code from messing with the object before it gets to the later object, which seems like it defeats much of the purpose of PrepareTemporalFields. Should PrepareTemporalFields perhaps vend a frozen object? If not, what function does it serve?

[littledan]

It looks like that case is working with a fresh object, isn't it? The fields variable is overwritten with a new value in that algorithm. If I'm understanding correctly, maybe it's fine if we freeze these objects, but I also don't understand what it would achieve.

[bakkot]

Oh, yeah, I completely missed that. A better example is perhaps in InterpretTemporalDateTimeFields, which is given a one of these object by ToTemporalZonedDateTime (via ToTemporalZonedDateTimeFields).

Step 1 of InterpretTemporalDateTimeFields passes the fields object to calendar.dateFromFields; step 2 then reads from it again. I guess that's not quite the same as passing it to user code? But it still ends up leading to a possible assertion failure, since it can create an object with Infinity in its [[Hour]] slot (among), and then passes it to RegulateTime, which passes it to ConstrainTime or ValidateTime, both of which assert the hour argument is an integer.

Though, I guess that's just as a much a problem without the re-use, since PrepareTemporalFields doesn't bounds-check either. So maybe the mutability of these objects isn't a problem per se. Still, it does seem unfortunate (which is to say, the sort of thing which seems like it might lead to later correctness or security bugs) that we set up this object which can have certain properties accessed infallibly for some of its lifetime, but then accessing those properties becomes fallible again.

[ptomato]

It sounds like maybe the best course of action is to ensure that the same object is never passed twice to user code if we can avoid it? I'm not sure how many instances there are where it's really necessary, or even if there are any.

As for this case, we could pass the fields object to ToTemporalTimeRecord before passing it to calendar.dateFromFields. I think it's probably a good idea to do that and to audit any additional instances of the same pattern.

I'm not sure if there will be any of these concerning cases remaining after that, but if there are, then it seems like we should either freeze the object or copy it before it's touched by user code. I don't have an opinion on which one is better.

[littledan]

Well, from the cases I've been able to understand, it seems like swapping steps 1 and 2 of InterpretTemporalDateTimeFields will be enough. I guess we only need to get into freezing/copying if we find something that's not fixed in a simple way like that. I think copying would probably fit with the rest of the style of this spec, as we're allocating mutable objects all over the place.

[ptomato]

Here's what I've found when auditing the places where objects are passed in to user code:

• Temporal.{PlainDate,PlainDateTime,ZonedDateTime}.with() pass the return value of calendar.mergeFields() to calendar.dateFromFields().
• Ditto for Temporal.{PlainMonthDay,PlainYearMonth}.toPlainDate().
• Temporal.PlainYearMonth.with() passes the return value of calendar.mergeFields() to calendar.yearMonthFromFields().
• Temporal.PlainMonthDay.with() passes the return value of calendar.mergeFields() to calendar.monthDayFromFields().
• Everywhere else, either no properties are read from an object obtained from user code, or else the object obtained from user code is a Temporal object and it's read via its internal slots, which the user code cannot modify.

For the with() and toPlainDate() cases mentioned above, I think copying the object using a second invocation of PrepareTemporalFields is probably the best solution, though I think doing nothing is also acceptable. Any opinions on this?

Aside from that, I've come to believe that the bounds-checking issue @bakkot mentioned also needs to be addressed. Here's an example of code that would run afoul of it:

class MyCal extends Temporal.Calendar {
  constructor() {
    super('iso8601');
  }

  mergeFields(fields, additionalFields) {
    const retval = super.mergeFields(fields, additionalFields);
    return { ...retval, year: Infinity };
  }

  toString() {
    return 'borked-merge-fields';
  }
}

const calendar = new MyCal();
const date = Temporal.PlainDate.from('2021-03-31').withCalendar(calendar);
const newDate = date.with({ year: 2022 });

This should throw in the last line, but according to the current spec text hits the assertion in step 1 of ConstrainISODate regardless of whether we copy the object or not. One solution seems to me to be to change that and similar assertions to "Assert: year, month, and day are integers or infinite." (This was probably a leftover confusion from before ToInteger got renamed to ToIntegerOrInfinity.) Any opinions on this?

[bakkot]

For the with() and toPlainDate() cases mentioned above, I think copying the object using a second invocation of PrepareTemporalFields is probably the best solution, though I think doing nothing is also acceptable. Any opinions on this?

Doing nothing seems OK, particularly since it's the same object which observes the object both times. Mostly I'm just not entirely clear on why it exists. I'd originally thought the idea was to guarantee that spec algorithms and user code would be provided a well-formed object, but it doesn't actually provide that guarantee, which means I don't know what function it's serving. (Not to say there isn't one; I just can't intuit what it is.)

One solution seems to me to be to change that and similar assertions to "Assert: year, month, and day are integers or infinite."

There's a number of places with a similar confusion - for example, as mentioned in #1424, PlainDate does ToIntegerOrInfinity and then passes its arguments to CreateTemporalDate, which asserts its arguments are integers (and not infinity). It's probably a good idea to audit these more comprehensively and figure out the general strategy for fixing them, rather than trying to fix them just for this issue.

With that said, for the particular case of Constrain, it seems fine to handle infinities as well as integers.

[ptomato]

Doing nothing seems OK, particularly since it's the same object which observes the object both times.

OK, then maybe fixing InterpretTemporalDateTimeFields is sufficient here.

Mostly I'm just not entirely clear on why it exists. I'd originally thought the idea was to guarantee that spec algorithms and user code would be provided a well-formed object, but it doesn't actually provide that guarantee, which means I don't know what function it's serving. (Not to say there isn't one; I just can't intuit what it is.)

It's in order to provide spec algorithms and user code with an object with only data properties, so that if getters are called they are only called once at the beginning, in a defined order. (See #1388, though this was achieved through other means before that change as well)

With that said, for the particular case of Constrain, it seems fine to handle infinities as well as integers.

It seems fine to me to handle infinities in Reject as well. I've opened #1467 for this.

[bakkot]

It's in order to provide spec algorithms and user code with an object with only data properties, so that if getters are called they are only called once at the beginning, in a defined order.

It doesn't accomplish that goal, because the object vended by PrepareTemporalFields is still used after it has been passed to user code, which could have installed getters on it. That's what I was pointing at with the original issue.

[ptomato]

Indeed, that's why I suggested copying the object again with a second invocation of PrepareTemporalFields in these cases.

[ptomato]

In #1470 I've gone with the approach of copying the objects.