Operation steps perform too many observable Gets #2289
Comments
|
FWIW, the correct output should be: The other proposal-temporal/polyfill/lib/ecmascript.mjs Line 1521 in bfeb523 |
|
#2267 addresses the repeated Gets of |
|
Another example: DifferenceTemporalPlainDate and DifferenceTemporalPlainDateTime and DifferenceTemporalZonedDateTime clone options after sending it to GetDifferenceSettings, resulting in multiple reads of some properties. All of the following produce identical log output: Temporal.Now.plainDateISO().since(Temporal.Now.plainDateISO(), {
get ignored() { console.log("get ignored"); },
get smallestUnit() { console.log("get smallestUnit"); },
get largestUnit() { console.log("get largestUnit"); },
});
Temporal.Now.plainDateTimeISO().since(Temporal.Now.plainDateTimeISO(), {
get ignored() { console.log("get ignored"); },
get smallestUnit() { console.log("get smallestUnit"); },
get largestUnit() { console.log("get largestUnit"); },
});
Temporal.Now.zonedDateTimeISO().since(Temporal.Now.zonedDateTimeISO(), {
get ignored() { console.log("get ignored"); },
get smallestUnit() { console.log("get smallestUnit"); },
get largestUnit() { console.log("get largestUnit"); return "day"; },
});
Temporal.Now.plainDateISO().toPlainYearMonth().since(Temporal.Now.plainDateISO().toPlainYearMonth(), {
get ignored() { console.log("get ignored"); },
get smallestUnit() { console.log("get smallestUnit"); },
get largestUnit() { console.log("get largestUnit"); },
}); |
|
Another example: widespread use of iterators in Temporal means that e.g. custom Calendar const originalGetIterator = Array.prototype[Symbol.iterator];
Array.prototype[Symbol.iterator] = function (...args) {
const iterator = Reflect.apply(originalGetIterator, this, args);
const iteratorProxy = new Proxy(iterator, {
get(target, key) {
const propValue = target[key];
// Wrap the "next" method to capriciously subtract 12 years from any Instant result.
if (key !== "next" || typeof propValue !== "function") return propValue;
return new Proxy(propValue, {
apply(target, thisArg, args) {
const result = Reflect.apply(target, thisArg === iteratorProxy ? iterator : thisArg, args);
const value = result?.value;
if (value instanceof Temporal.Instant) {
result.value = value.subtract({ hours: 12 * 365.25 * 24 });
}
return result;
},
});
},
});
return iteratorProxy;
};
const zdt = Temporal.ZonedDateTime.from("2023-01-31T10:00[America/New_York]");
zdt.with({ hour: 0 });
// => 1999-01-31T00:00:00-05:00[America/New_York]Given that expected results are small, I think results should be consumed as arraylikes rather than iterables. |
Iterables were requested during the editors' review for Stage 3 (#1427). Kevin clarified later in the long thread in #1610 that there was no problem with reverting back to arraylikes if there was a good reason, but accepting iterables should be the default for new facilities in the language. I'd guess that "userland could patch |
|
Are we sure that's not good enough reason? It even allows userland code to override built-ins at a distance, which in addition to hindering comprehensibility, seems like a bug factory—potentially with security implications. I'm not aware of any existing interface between built-ins in which output from one is consumed as an iterator without a short-circuit that inherently prevents observation and/or interference by user code such as the implicit constructor in ClassDefinitionEvaluation (which is also true of Reflect.{apply,construct} argument consumption, and even of the handoff from an "ownKeys" proxy trap to [[OwnPropertyKeys]]), but perhaps @bakkot has a specific pattern in mind? |
|
My comment about accepting iterables as inputs was meant for user-exposed APIs, not for stuff which is internal to an algorithm. Why does the snippet above end up triggering the iteration protocol at all? If you're using a built-in calendar and timezone, don't you know their fields already? I don't think "you can change the behavior of this algorithm by patching |
|
I'm not OK with either, and I think both are in scope for this issue. But there's a logical difference between patching a built-in API vs. interfering with the mechanism by which built-in APIs communicate. |
|
If the built-in API is part of the mechanism by which other APIs communicate, I don't see what difference you're pointing at. |
|
With the change that we approved at last week's plenary, I'm pretty sure it's still possible to write a code sample that would have the problem Richard was trying to illustrate, but it'd need to use a custom calendar or time zone. |
|
Ah, nice. (I assume that's #2482.) If the path which is affected by patching |
It would also be possible to get this behavior if a built-in timezone or calendar is constructed as an object (e.g. |
|
I was half right in my previous comment. |
…ds() NOTE: This commit doesn't actually do anything in this current branch, because CalendarFields is never called with a String _calendar_. This just illustrates the change, which will only take effect when #2482 is merged. When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() NOTE: This commit doesn't actually do anything in this current branch, because CalendarFields is never called with a String _calendar_. This just illustrates the change, which will only take effect when #2482 is merged. When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() NOTE: This commit doesn't actually do anything in this current branch, because CalendarFields is never called with a String _calendar_. This just illustrates the change, which will only take effect when #2482 is merged. When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
…ds() When calling CalendarFields with a built-in calendar, previously we would create a JS Array from the passed List, and pass it to %Temporal.Calendar. prototype.fields%, which would iterate it observably. So, instead we call CalendarDateFields when the calendar is a built-in calendar, which doesn't do anything observable at all. See: #2289
|
Completed by #2519. |
Too many specifics of the internal spec algorithms are exposed in redundant observable interactions with input objects as opposed to up-front method/property extraction and subsequent invocation as necessary.
For example, in
Temporal.Duration.compare, steps 7a and 7b are sequential calls to UnbalanceDurationRelative with largestUnit "day" and the same relativeTo argument extracted fromcompareoptions, each of which independently calls ToTemporalDate(relativeTo) (which observably Gets "calendar") and PrepareTemporalFields (which observably Gets the other Temporal fields) and also one more instances of MoveRelativeDate(calendar, relativeTo, one{Year,Month,Week}) (each of which itself observably gets the calendar's "dateAdd" via CalendarDateAdd).Temporal.Duration.comparealso exhibits similar behavior with a pair of calls to CalculateOffsetShift(relativeTo, …), each of which observably Get the time zone's "getOffsetNanosecondsFor" (twice!) via GetOffsetNanosecondsFor.Temporal.Duration.prototype.roundandTemporal.Duration.prototype.totalsimilarly send relativeTo to a variety of operations that redundantly extract information from it and/or its calendar and/or time zone, and I suspect the problem exists elsewhere as well (such as from AddDuration with plain object relativeTo observably calling AddZonedDateTime twice, cf. #2290 (comment) ).Every entry point should instead extract the methods and properties once and then pass them down to deeper levels, resulting in more reasonable and opaque externally-observable behavior rather than something like this:
The text was updated successfully, but these errors were encountered: