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

appearing Immutability is not guaranteed by the calling to Calendar during getter calls #2842

Open
FrankYFTang opened this issue May 17, 2024 · 4 comments
Labels

Comments

@FrankYFTang
Copy link
Contributor

I hate to bring up this in this stage of a TC39 proposal, but it seems I have to

In https://github.com/tc39/proposal-temporal
Principles:
All Temporal objects are immutable.

But I do not believe this is current guaranteed by the spec text

I believe when we advertise Temporal objects are immutable, it not only mean the internal state of the Temporal objects are immutable but the appearing behavior of these objects (including the value returned by getters) should be also.

https://www.oxfordlearnersdictionaries.com/us/definition/english/immutable

Oxford Learner's Dictionary define immutable as
"that cannot be changed; that will never change"

therefore, if I have the following call

let t = new Temporal.PlainDate( /* whatever call */ )

I assume t.year will always need to return the same value no matter how many times I call it and what I put into the /* whatever call */ . and

t.year == t.year

need to always be always true no matter what.

But the current Temporal spec text does not guarantee this.

This is because internally during the "get Temporal.PlainDate.prototype.year" (and many other getters), it calls the calendar to calculation the result , rather than calling the calendar during the time of the constructor then remember the result resolved by the Calendar. Because of that, since the calendar .year() is just a function, it could return different results each time it got called.

here is a sample call to show the problem from the current v8 prototype

$ out/x64.release/d8  --harmony-temporal
V8 is running with experimental features enabled. Stability and security will suffer.
V8 version 12.7.0 (candidate)
d8>     class RandomYearCalendar extends Temporal.Calendar { \
     constructor() {  super("iso8601");  } \
      get id() { return "random-year"; } \
      toString() { return this.id; } \
      year(t) { print("call year"); return super.year(t) +  Math.floor(Math.random() * 10); } \
    }
undefined
d8> 
undefined
d8> let t = new Temporal.PlainDate(2024, 3, 15, new RandomYearCalendar())
undefined
d8> t.year == t.year
call year
call year
false
d8> 

notice because the spec text mandate the

4. [CalendarYear](https://tc39.es/proposal-temporal/#sec-temporal-calendaryear)(calendar, temporalDate).

is called during getter, not only it not guarantee immutability as what Temporal design for, it also prevent the calling to calendar to be optimize , it is an observed behavior therefore it
A. required the Temporal.Calendar.prototype.year() MUST be called during the call to get Temporal.PlainDate.prototype.year and

B. required the Temporal.Calendar.prototype.year() MUST be called for EACH call to Temporal.PlainDate.prototype.year

Both A and B are very strong and unnecessary requirement from my point of view, it break the very first principle Temporal advertised to solve and prevent the implementaor to optimze the performance for repeating getter calls. Also, because this is called during the getter, it make the Calendar object have to surface many calls for each one of this getter call, rather than a batch in / batch out operations to be called during the constructor calls of Temporal.PlainDate and make the API surface unncessary large.

@sffc @anba

@sffc
Copy link
Collaborator

sffc commented May 17, 2024

Right, the inner data model is immutable (the ISO fields) but the calendar fields currently invoke the Calendar protocol.

@FrankYFTang
Copy link
Contributor Author

Another example:

$ out/x64.release/d8  --harmony-temporal
V8 is running with experimental features enabled. Stability and security will suffer.
V8 version 12.7.0 (candidate)
d8> c = new Temporal.Calendar("iso8601")
iso8601
d8> c.oldyear = c.year
function year() { [native code] }
d8> let kk = new Temporal.PlainDate(2024, 3, 15, c)
undefined
d8> c.year = function(pd) {print("called"); return c.oldyear(pd) + +  Math.floor(Math.random() * 10);}
function(pd) {print("called"); return c.oldyear(pd) + +  Math.floor(Math.random() * 10);}
d8> kk.year
called
2030
d8> kk.year
called
2031
d8> kk.year
called
2029

@ptomato
Copy link
Collaborator

ptomato commented May 17, 2024

This was by design. The only thing that's actually immutable as @sffc pointed out is the internal slots, which you can get directly through getISOFields():

t.getISOFields().year === t.getISOFields().year  // always guaranteed

However,

t.year === t.year  // guaranteed if calendar is a string

is also guaranteed if t is constructed with a string calendar, not a custom calendar.

The reasoning is that custom calendars are a niche use case, and the calendar implementor is responsible for writing calendar code that makes sense internally and doesn't give random results. If your code needs the immutability guarantee, you can reject Temporal objects that don't satisfy typeof t.getISOFields().calendar === 'string'.

To have t.year === t.year guaranteed while still allowing custom calendars, you'd have to eagerly compute all of the properties by calling all the calendar methods (year, month, monthCode, day, dayOfWeek, weekOfYear, monthsInYear, etc., etc.) in the constructor of t. Even though most of the results will never be used. See #2834 for more discussion of this.

However please note #2826. I think removing custom calendars and time zones will remove this issue entirely.

@ljharb
Copy link
Member

ljharb commented May 17, 2024

I don't think "immutable" means it can never change - a function can be immutable and still return a random value. That property might be called "idempotent"? (but i'm not sure)

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

No branches or pull requests

4 participants