Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with Temporal.Calendar.monthDayFromFields #1636

Closed
FrankYFTang opened this issue Jul 13, 2021 · 21 comments
Closed

Problem with Temporal.Calendar.monthDayFromFields #1636

FrankYFTang opened this issue Jul 13, 2021 · 21 comments

Comments

@FrankYFTang
Copy link
Contributor

FrankYFTang commented Jul 13, 2021

ok, I filed
#1614 before and there are several different point in that, and one of the piont got move to a more general case for discussion. But I have a specific issue of Temporal.Calendar.monthDayFromFields regarding the reference year.

Let's say we have the following code

let iso8601 = new Temporal.Calendar("iso8601");
let monthDay1 =  iso8601.monthDayFromFields(
    {year: 2021, month: 2, day: 29}, 
    {overflow: "constrain"})
let out1 = monthDay1.getISOFields( )
let monthDay2 =  iso8601.monthDayFromFields(
    {year: 2021, month: 2, day: 29, monthCode: "M02"}, 
    {overflow: "constrain"})
let out2 = monthDay2.getISOFields( )

What should be out1? (I am just use "iso8601" below for an object of iso8601 calendar, and that part is not my issue.

A. out1 = {calendar: "iso8601", isoDay:29 , isoMonth:2 , isoYear: 1972}
B. out1 = {calendar: "iso8601", isoDay:28 , isoMonth:2 , isoYear: 1972}
C. out1 = {calendar: "iso8601", isoDay:28 , isoMonth:2 , isoYear: 2021}

What should be out2?

D. out2 = {calendar: "iso8601", isoDay:29 , isoMonth:2 , isoYear: 1972}
E. out2 = {calendar: "iso8601", isoDay:28 , isoMonth:2 , isoYear: 1972}
F. out2 = {calendar: "iso8601", isoDay:28 , isoMonth:2 , isoYear: 2021}

Make up your mind BEFORE you read MY answer below first.

From my understanding of the current spec, the answer is

B. out1 = {calendar: "iso8601", isoDay:28 , isoMonth:2 , isoYear: 1972}
D. out2 = {calendar: "iso8601", isoDay:29 , isoMonth:2 , isoYear: 1972}

and I think that is very very strange. Notice both output isoYear as 1972 even the input is with year: 2021. But more strangely is one will output isoDay as 28 but the other as 29, while both of the input are having day:29.

Please let me know if you think my calcualted answer B and D is wrong after you carefully walk through the current spec with tese input values. Maybe I misssed some step that I didn't realize.

@sffc @justingrant @ptomato @ryzokuken @ljharb @Ms2ger

I have hard time to comprehance the design of such differences. Is this intentional and have discussed by champions already?

@sffc
Copy link
Collaborator

sffc commented Jul 13, 2021

My expectation would have been B and E. The fields should be constrained to 2021-02-28 before converted to MonthDay, at which point the year is always set to 1972.

@ljharb
Copy link
Member

ljharb commented Jul 13, 2021

Why 1972?

@FrankYFTang
Copy link
Contributor Author

My expectation would have been B and E. The fields should be constrained to 2021-02-28 before converted to MonthDay, at which point the year is always set to 1972.

A. I believe the current spec make it B and D (not B and E)
B. Why 02-28 but not 02-29 in 1972? (1972 is a leap year and there are 02-29 in that year)

@ptomato
Copy link
Collaborator

ptomato commented Jul 13, 2021

My expectation would have been B and E. The fields should be constrained to 2021-02-28 before converted to MonthDay, at which point the year is always set to 1972.

I would expect B and D but that's maybe obvious since I reviewed this part of the spec and am quite familiar with it (I think Cam wrote it.) Before going into details I'm curious if my intuition matches @justingrant's, who was the designer of this logic.

Why 1972?

1972 is an arbitrarily chosen reference year for PlainMonthDays in the ISO calendar - it is the first leap year after the Unix epoch. (See step 11 of ISOMonthDayFromFields and #391 for even more background.)

@FrankYFTang
Copy link
Contributor Author

Could someone write a simple explaination of why it is B and D but not B and E to "a user who will not look at the spec text" and communicate of the rational via "plain English" why out2 is D but not E? but out1 is B?

I understand the spec make it D but not E but I have hard time to understand the logic to design the spec in such a way. I believe normal user will also very confused. What do we want to achieve by such differences? (one return 28 w/o monthCode: "M02" and the other return 29 because it has monthCode: "M02")

@justingrant
Copy link
Collaborator

This is indeed complicated. Here's my general understanding:

  • The goal of the algorithm is to end up with a {monthCode, day} pair.
  • If monthCode isn't provided by the caller, then both month and year (or era and eraYear) are required in order to project that month and day into a real year, from which a monthCode can be derived. (There's an exception in PlainMonthDay.from if the calendar is omitted then only month and day are required, although I'm not sure if this exception applies to calling the Calendar method directly, per @ptomato 's comment in another issue-- maybe Problem in ISOMonthDayFromFields #1614?)

Frank brings up interesting cases:

  1. How to handle {year, month, day} when the month and/or day doesn't exist in that year? (assuming "constrain" mode)

I think the answer is to constrain the month and day to that year, and then use the resulting ISO date to calculate the monthCode. That's (B) above.

The wildcard here is whether the ISO calendar should behave differently from non-ISO calendars which should all use behavior (B). I know in PlainMonthDay.from we allow {month, day} when the calendar is unspecified, but I'm assuming that when calling the calendar directly then the ISO calendar should act like all the others. (If I remember correctly, this behavior in from is really there to support ISO strings like 02-29, and the object-initializer form of from adopted the same behavior)

  1. How to handle redundant data e.g. {year, month, monthCode, day} where both year and month are not needed?

I think a reasonable way to handle this case would be to ignore month and year because we already have the info we need from monthCode and day. That's (D) above.

There are more cases to consider:

  1. How to handle redundant data e.g. {year, month, monthCode, day} where month and monthCode refer to a different month? Something like {year: 2021, month: 3, monthCode: 'M02', day: 29}. A variant of this would be a lunisolar calendar where the month and monthCode are sometimes the same month, but not in the provided year. I assume we'd also go with the (D) behavior here too: we ignore year and month if the caller gave us monthCode.

  2. A similar problem applies if we get year, era, and eraYear and they don't resolve to the same year.

I think the easiest rule to handle all these cases is that monthCode always supersedes month/year, and year always supersedes era/eraYear. Is this how the spec is currently written?

Another option would be to require that redundant data always matches, and we should throw in case of conflicts.

How is the spec currently written?

Note that none of these solutions are perfect. I'm open to being convinced of a different position.

@FrankYFTang
Copy link
Contributor Author

FrankYFTang commented Jul 14, 2021

If monthCode isn't provided by the caller, then both month and year (or era and eraYear) are required in order to project that month and day into a real year, from which a monthCode can be derived.

ok, If I follow this logic, then

{year: 2021, month: 2, day: 29},
sould be projected to {year: 2021, month: 2, day: 28}
and "M02" would be derived for monthCode and we can put back to the input of
{year: 2021, month: 2, day: 29} as
{year: 2021, month: 2, day: 29, monthCode: "M02"} right? Because you only need to project to derived monthCode but there are no need to keep the projected day 28 around once you got monthCode, right?

The later operation of using 1972 as reference could then be used with {month: 2, day: 29, monthCode: "M02"}
and stay in {year: 1972, month: 2, day: 29, monthCode: "M02"} since ISO calendar will always use 1972 as reference year for MonthYear, right? so you use the year 2021 to project the monthCode and monthCode only, and not it to constrain the day but use 1972 to constrain the day in the case of MonthDay because we have decided always use 1972 as referene for ISO calendar on MonthDay. Otherwise, it does not make sense that isoYear is set to 1972 as in
B, but should use

C. out1 = {calendar: "iso8601", isoDay:28 , isoMonth:2 , isoYear: 2021}

right?

In other words, we need to define what the isoYear here mean. Is it the year we used to constrain the MonthDay? if it is, then we should either use 2021 to constrain the day as 28 and put 2021 into isoYear (out1=C), or use 1972 to constain the day as 29 and report isoYear as 1972 (out1=A) but I see no reason out1=B.

@ptomato
Copy link
Collaborator

ptomato commented Jul 14, 2021

I think the easiest rule to handle all these cases is that monthCode always supersedes month/year, and year always supersedes era/eraYear. Is this how the spec is currently written?

@justingrant If month and monthCode don't agree, then ResolveISOMonth throws an exception. Otherwise, what you said is correct. ISOMonthDayFromFields is currently written so that monthCode supersedes month/year: in ResolveISOMonth, monthCode beats month, and in step 12 of ISOMonthDayFromFields year is used only if monthCode is undefined, otherwise 1972 is used.

What happens with year vs eraYear is not specified, because non-ISO calendars are in the implementation-defined part.

ok, If I follow this logic, then {year: 2021, month: 2, day: 29}, sould be projected to {year: 2021, month: 2, day: 28} and "M02" would be derived for monthCode and we can put back to the input of {year: 2021, month: 2, day: 29} as {year: 2021, month: 2, day: 29, monthCode: "M02"} right? Because you only need to project to derived monthCode but there are no need to keep the projected day 28 around once you got monthCode, right?

The later operation of using 1972 as reference could then be used with {month: 2, day: 29, monthCode: "M02"} and stay in {year: 1972, month: 2, day: 29, monthCode: "M02"} since ISO calendar will always use 1972 as reference year for MonthYear, right? so you use the year 2021 to project the monthCode and monthCode only, and not it to constrain the day but use 1972 to constrain the day in the case of MonthDay because we have decided always use 1972 as referene for ISO calendar on MonthDay.

@FrankYFTang I think your suggestion (output equals A) is a self-consistent alternative, and I would be fine with adopting it. But I wouldn't say there is no reason for the output to equal B. The mental model of B is:

  • Validate the input fields, including year, according to the overflow parameter so that together they form at least a correct month-day pair
  • Put that month-day pair in the calendar's correct reference year

(Maybe your mental model of B is missing the second step? The issue explaining it is #1239 and the consensus from the champions is described in #1239 (comment) plus a clarification from Justin in #1239 (comment)))

In other words B ensures these operations are the same: (note that I personally don't care that much about this consistency, but maybe other champions do!)

input = {year: 2021, month: 2, day: 29}
out1 = Temporal.PlainMonthDay.from(input).getISOFields()
// out1 is { calendar, isoDay: 28, isoMonth: 2, isoYear: 1972 }
out2 = Temporal.PlainMonthDay.from(Temporal.PlainDate.from(input)).getISOFields()
// out2 is the same as out1

Also note that Temporal.PlainMonthDay.from({ year: 2021, month: 2, day: 29 }, { overflow: 'reject' }) would currently throw. I guess in the A situation, this would no longer throw.

@FrankYFTang
Copy link
Contributor Author

I think the interesting part is the 1972. If we decide to use 1972 eventually, regardless what the user give us, then why not use 1972 FIRST while deciding the monthCode by ignoring 2021 in that step too since eventually we are going to ignore 2021 and use 1972 anyway, right? I am NOT feeling strongly with this, but it is odd to me that if we want to use 1972 eventually and ignore 2021 as year, then why not igore 2021 in the very beginning but only igore it in the end. Also, since we are going to use 1972 anyway, shouldn't we just use 1972 as year while it is not present instead of throw ?

@FrankYFTang
Copy link
Contributor Author

ok, at this point I ask too many questions about this and I think it is detail which any answer have some merit and any answer will always have some strange thing. So... not to waste your time, let me just close it. It is not a big deal as long as you folks confirm this is by design but not a mistake. (I cannot tell until I ask since there are mistakes I found in some other places.)

@justingrant
Copy link
Collaborator

Yep, this behavior is by design. If the user gives us a YMD input, we expect it to be a valid date and will constrain or reject it just like we would for any other date construction. In theory we could use YMD to calculate the monthCode, and then combine that with the original day to get the MD to use to find the correct reference year. At that point you still have a {monthCode, day} pair that is not guaranteed to be valid, so you need to do another constrain operation on it. But you don't know the year to use for constraining. (1972 may not contain every {monthCode, day} in some calendars.) This additional complexity didn't seem worth it, especially for a case which is almost certainly a programmer error because they're giving us a date that doesn't exist. So I opted for a design that doesn't require any additional constrain/reject checks after the initial YMD is resolved into a {monthCode, day}.

Note that the reference year algorithm is calendar-specific and is not defined in 262 spec except for ISO. The only requirement on the calendar implementations is that for any {monthCode, day} pair in the result of PlainMonthDay.from, the same reference year should be used. This requirement enables PlainMonthDay equality comparisons to use ISO fields only without calling into the calendar to calculate monthCode and day. In the current polyfill, the algorithm for all calendars (including ISO) is to start with 1972 and if that {monthCode, day} doesn't exist, subtract one year at a time until we find a year where {monthCode, day} exists. 60 years should be sufficient for all built-in calendars. I think the polyfill uses 100 years before throwing.

@ljharb
Copy link
Member

ljharb commented Jul 14, 2021

@justingrant any reason not to omit the year entirely?

@justingrant
Copy link
Collaborator

@justingrant any reason not to omit the year entirely?

@ljharb do you mean why does PlainMonthDay care about the reference year at all? Or how can the user avoid providing the year when calling Temporal.Calendar.monthDayFromFields? Or something else?

@ljharb
Copy link
Member

ljharb commented Jul 14, 2021

I mean like, why make "1972" an observable value anywhere

@ptomato
Copy link
Collaborator

ptomato commented Jul 14, 2021

Without it being observable, PlainMonthDay in a non-ISO calendar wouldn't be serializable as an ISO string.

@ljharb
Copy link
Member

ljharb commented Jul 14, 2021

if there's no ISO string format for just "month and day", why would one need an ISO string for a MonthDay without also explicitly providing a year to go along with it?

@justingrant
Copy link
Collaborator

There needs to be some string serialization format for all Temporal types, e.g. so you can store them in a single database column and later deserialize back into an ECMAScript instance. The convention across Temporal is that all Temporal types are string-serializable using ISO grammar, including the timezone and calendar extensions that @ryzokuken is working with IETF on standardizing. We could invent a new string serialization format that used calendar units (not ISO) which I think would remove the need for an observable ISO year, but that would be different behavior from all other Temporal types. Hard to see how that divergence would be better.

Or am I misunderstanding what you're asking?

@ljharb
Copy link
Member

ljharb commented Jul 14, 2021

I'm wondering if perhaps that MonthDay serialization require an explicit year, like monthDay.toString({ year: 1234 }), and that absent a year, it throw - that way, the user won't get an arbitrary, and thus surprising, year value.

@justingrant
Copy link
Collaborator

justingrant commented Jul 14, 2021

I'm wondering if perhaps that MonthDay serialization require an explicit year, like monthDay.toString({ year: 1234 }), and that absent a year, it throw - that way, the user won't get an arbitrary, and thus surprising, year value.

One challenge with that approach (in addition to the worse serialization ergonomics in all calendars, including ISO where the year is irrelevant) is that PlainMonthDay instances that are serialized by different developers could have different string serializations. If you're communicating with an external store or service, then it's really helpful to have a deterministic serialization format, where "deterministic" means that if md1.equals(md2) then md1.toString() === md2.toString() even if different developers who don't know each other are calling toString().

Keep in mind that the year only shows up when serializing PlainMonthDay for non-ISO calendars. For the ISO calendar, the format is MM-DD. If it's not the ISO calendar, all fields (not just year!) are already surprising because the date shown is the ISO date, not the calendar year/month/day. For example:

Temporal.PlainMonthDay.from({ month: 1, day: 1 }).toString();
// => "01-01"
Temporal.PlainMonthDay.from({ monthCode: 'M01', day: 1 }).toString();
// => "01-01"
Temporal.PlainMonthDay.from({ monthCode: 'M01', day: 1, calendar: 'iso8601' }).toString();
// => "01-01"
Temporal.PlainMonthDay.from({ monthCode: 'M01', day: 1, calendar: 'chinese' }).toString();
// => "1971-01-27[u-ca=chinese]"
Temporal.PlainYearMonth.from({ year: 2020, month: 1, calendar: 'chinese' }).toString();
// => "2020-01-25[u-ca=chinese]"
Temporal.PlainDate.from({ year: 2020, month: 1, day: 15, calendar: 'chinese' }).toString();
// => "2020-02-08[u-ca=chinese]"

@ljharb
Copy link
Member

ljharb commented Jul 14, 2021

ah ok, that makes a lot of sense then - so it's only when 1) using a MonthDay, 2) a non-ISO calendar, and 3) serializing to string that you'd even see the arbitrary year?

It does kind of seem like preserving the year provided to monthDayFromFields would be a less surprising approach, but if the criteria above are applied then it seems like enough of an edge case to not worry about too much.

@justingrant
Copy link
Collaborator

ah ok, that makes a lot of sense then - so it's only when 1) using a MonthDay, 2) a non-ISO calendar, and 3) serializing to string that you'd even see the arbitrary year?

You'll also see it when calling getISOFields() to serialize to an object for both ISO and non-ISO calendars. This method is really for advanced usage only-- mainly for implementers of custom calendars. So we're not too worried about weirdness there.

Otherwise you're correct.

It does kind of seem like preserving the year provided to monthDayFromFields would be a less surprising approach

Yep, it's a tradeoff between deterministic serialization and somewhat-less-surprising (but still surprising!) serialization for instances using non-ISO calendars. We felt that the former was more important given that the primary string-serialization use case is for machine-readability, not human-readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants