From f44bbe4035b37a50e8d55136e31d7c5929267c53 Mon Sep 17 00:00:00 2001 From: Philip Chimento Date: Thu, 2 Mar 2023 18:13:06 -0800 Subject: [PATCH] Temporal: Avoid calling user code in no-op round operations This shortcut path now exists in all round(), since(), and until() operations. In Instant, PlainDate, PlainDateTime, and PlainTime, the change isn't observable, so no tests could be added. This adds test coverage for - Duration.p.round() - PlainYearMonth.p.since() - PlainYearMonth.p.until() - ZonedDateTime.p.round() - ZonedDateTime.p.since() - ZonedDateTime.p.until() As well as a few cases where we are testing that certain calendar methods get called during a round operation, but previously were doing so with options that now become a no-op and no longer call those calendar methods. In those cases, round to 2 ns, rather than 1 ns. --- harness/temporalHelpers.js | 24 +++++++ .../prototype/round/rounding-is-noop.js | 69 +++++++++++++++++++ .../prototype/since/order-of-operations.js | 5 ++ .../prototype/until/order-of-operations.js | 5 ++ .../ZonedDateTime/prototype/round/div-zero.js | 4 +- .../prototype/round/order-of-operations.js | 2 +- .../prototype/round/rounding-is-noop.js | 35 ++++++++++ .../prototype/since/order-of-operations.js | 14 +++- .../prototype/until/order-of-operations.js | 14 +++- 9 files changed, 163 insertions(+), 9 deletions(-) create mode 100644 test/built-ins/Temporal/Duration/prototype/round/rounding-is-noop.js create mode 100644 test/built-ins/Temporal/ZonedDateTime/prototype/round/rounding-is-noop.js diff --git a/harness/temporalHelpers.js b/harness/temporalHelpers.js index 68f32d33ba..ea662ed3b0 100644 --- a/harness/temporalHelpers.js +++ b/harness/temporalHelpers.js @@ -1947,6 +1947,30 @@ var TemporalHelpers = { }); }, + /* + * A custom time zone that does not allow any of its methods to be called, for + * the purpose of asserting that a particular operation does not call into + * user code. + */ + timeZoneThrowEverything() { + class TimeZoneThrowEverything extends Temporal.TimeZone { + constructor() { + super("UTC"); + } + getOffsetNanosecondsFor() { + TemporalHelpers.assertUnreachable("getOffsetNanosecondsFor should not be called"); + } + getPossibleInstantsFor() { + TemporalHelpers.assertUnreachable("getPossibleInstantsFor should not be called"); + } + toString() { + TemporalHelpers.assertUnreachable("toString should not be called"); + } + } + + return new TimeZoneThrowEverything(); + }, + /* * Returns an object that will append logs of any Gets or Calls of its valueOf * or toString properties to the array calls. Both valueOf and toString will diff --git a/test/built-ins/Temporal/Duration/prototype/round/rounding-is-noop.js b/test/built-ins/Temporal/Duration/prototype/round/rounding-is-noop.js new file mode 100644 index 0000000000..f5fefd3ca0 --- /dev/null +++ b/test/built-ins/Temporal/Duration/prototype/round/rounding-is-noop.js @@ -0,0 +1,69 @@ +// Copyright (C) 2023 Igalia, S.L. All rights reserved. +// This code is governed by the BSD license found in the LICENSE file. + +/*--- +esid: sec-temporal.duration.prototype.round +description: > + No calendar or time zone methods are called under circumstances where rounding + is a no-op +includes: [temporalHelpers.js] +features: [Temporal] +---*/ + +const calendar = TemporalHelpers.calendarThrowEverything(); +const timeZone = TemporalHelpers.timeZoneThrowEverything(); +const plainRelativeTo = new Temporal.PlainDate(2000, 1, 1, calendar); +const zonedRelativeTo = new Temporal.ZonedDateTime(0n, timeZone, calendar); + +const d = new Temporal.Duration(0, 0, 0, 0, 23, 59, 59, 999, 999, 997); + +const noopRoundingOperations = [ + [d, { smallestUnit: "nanoseconds" }, "smallestUnit ns"], + [d, { smallestUnit: "nanoseconds", relativeTo: plainRelativeTo }, "smallestUnit ns and plain relativeTo"], + [d, { smallestUnit: "nanoseconds", relativeTo: zonedRelativeTo }, "smallestUnit ns and zoned relativeTo"], + [d, { smallestUnit: "nanoseconds", roundingIncrement: 1 }, "round to 1 ns"], + // No balancing because largestUnit is already the largest unit and no time units overflow: + [d, { largestUnit: "hours" }, "largestUnit hours"], + // Unless relativeTo is ZonedDateTime, no-op is still possible with days>0: + [new Temporal.Duration(0, 0, 0, 1), { smallestUnit: "nanoseconds" }, "days>0 and smallestUnit ns"], + [new Temporal.Duration(0, 0, 0, 1), { smallestUnit: "nanoseconds", relativeTo: plainRelativeTo }, "days>0, smallestUnit ns, and plain relativeTo"], +]; +for (const [duration, options, descr] of noopRoundingOperations) { + const result = duration.round(options); + assert.notSameValue(result, duration, "rounding result should be a new object"); + TemporalHelpers.assertDurationsEqual(result, duration, `rounding should be a no-op with ${descr}`); +} + +// These operations are not no-op rounding operations, but still should not call +// any calendar methods: +const roundingOperationsNotCallingCalendarMethods = [ + [d, { smallestUnit: "microseconds" }, "round to 1 µs"], + [d, { smallestUnit: "nanoseconds", roundingIncrement: 2 }, "round to 2 ns"], + [new Temporal.Duration(0, 0, 0, 0, 24), { largestUnit: "days" }, "upwards balancing requested"], + [d, { largestUnit: "minutes" }, "downwards balancing requested"], + [new Temporal.Duration(0, 0, 0, 0, 1, 120), { smallestUnit: "nanoseconds" }, "time units could overflow"], + [new Temporal.Duration(0, 0, 0, 1, 24), { smallestUnit: "nanoseconds" }, "hours-to-days conversion could occur"], +]; +for (const [duration, options, descr] of roundingOperationsNotCallingCalendarMethods) { + const result = duration.round(options); + let equal = true; + for (const prop of ['years', 'months', 'weeks', 'days', 'hours', 'minutes', 'seconds', 'milliseconds', 'microseconds', 'nanoseconds']) { + if (result[prop] !== duration[prop]) { + equal = false; + break; + } + } + assert(!equal, `round result ${result} should be different from ${duration} with ${descr}`); +} + +// These operations should not be short-circuited because they have to call +// calendar methods: +const roundingOperationsCallingCalendarMethods = [ + [new Temporal.Duration(0, 0, 1), { smallestUnit: "nanoseconds", relativeTo: plainRelativeTo }, "calendar units present"], + [d, { largestUnit: "days", relativeTo: zonedRelativeTo }, "largestUnit days with zoned relativeTo"], + [new Temporal.Duration(0, 0, 0, 1), { smallestUnit: "nanoseconds", relativeTo: zonedRelativeTo }, "hours-to-days conversion could occur with zoned relativeTo"], +]; + +for (const [duration, options, descr] of roundingOperationsCallingCalendarMethods) { + assert.throws(Test262Error, () => duration.round(options), `rounding should not be a no-op with ${descr}`); +} diff --git a/test/built-ins/Temporal/PlainYearMonth/prototype/since/order-of-operations.js b/test/built-ins/Temporal/PlainYearMonth/prototype/since/order-of-operations.js index 5cc60b85e0..735225b469 100644 --- a/test/built-ins/Temporal/PlainYearMonth/prototype/since/order-of-operations.js +++ b/test/built-ins/Temporal/PlainYearMonth/prototype/since/order-of-operations.js @@ -117,6 +117,11 @@ function createOptionsObserver({ smallestUnit = "months", largestUnit = "auto", // clear any observable things that happened while constructing the objects actual.splice(0); +// code path that skips RoundDuration: +instance.since(otherYearMonthPropertyBag, createOptionsObserver({ smallestUnit: "months", roundingIncrement: 1 })); +assert.compareArray(actual, expected, "order of operations with no rounding"); +actual.splice(0); // clear + // code path through RoundDuration that rounds to the nearest year: const expectedOpsForYearRounding = expected.concat([ "get this.calendar.dateAdd", // 9.b diff --git a/test/built-ins/Temporal/PlainYearMonth/prototype/until/order-of-operations.js b/test/built-ins/Temporal/PlainYearMonth/prototype/until/order-of-operations.js index 3eb4a0f3bf..3e6864fc4b 100644 --- a/test/built-ins/Temporal/PlainYearMonth/prototype/until/order-of-operations.js +++ b/test/built-ins/Temporal/PlainYearMonth/prototype/until/order-of-operations.js @@ -117,6 +117,11 @@ function createOptionsObserver({ smallestUnit = "months", largestUnit = "auto", // clear any observable things that happened while constructing the objects actual.splice(0); +// code path that skips RoundDuration: +instance.since(otherYearMonthPropertyBag, createOptionsObserver({ smallestUnit: "months", roundingIncrement: 1 })); +assert.compareArray(actual, expected, "order of operations with no rounding"); +actual.splice(0); // clear + // code path through RoundDuration that rounds to the nearest year: const expectedOpsForYearRounding = expected.concat([ "get this.calendar.dateAdd", // 9.b diff --git a/test/built-ins/Temporal/ZonedDateTime/prototype/round/div-zero.js b/test/built-ins/Temporal/ZonedDateTime/prototype/round/div-zero.js index ca2e5cdb18..a88825161a 100644 --- a/test/built-ins/Temporal/ZonedDateTime/prototype/round/div-zero.js +++ b/test/built-ins/Temporal/ZonedDateTime/prototype/round/div-zero.js @@ -18,7 +18,7 @@ class Calendar extends Temporal.Calendar { const zdt = new Temporal.ZonedDateTime(0n, "UTC", new Calendar()); -const units = ["day", "hour", "minute", "second", "millisecond", "microsecond", "nanosecond"]; +const units = ["hour", "minute", "second", "millisecond", "microsecond", "nanosecond"]; for (const smallestUnit of units) { - assert.throws(RangeError, () => zdt.round({ smallestUnit })); + assert.throws(RangeError, () => zdt.round({ smallestUnit, roundingIncrement: 2 })); } diff --git a/test/built-ins/Temporal/ZonedDateTime/prototype/round/order-of-operations.js b/test/built-ins/Temporal/ZonedDateTime/prototype/round/order-of-operations.js index 2724d14fc7..121f8fe514 100644 --- a/test/built-ins/Temporal/ZonedDateTime/prototype/round/order-of-operations.js +++ b/test/built-ins/Temporal/ZonedDateTime/prototype/round/order-of-operations.js @@ -42,7 +42,7 @@ const actual = []; const options = TemporalHelpers.propertyBagObserver(actual, { smallestUnit: "nanoseconds", roundingMode: "halfExpand", - roundingIncrement: 1, + roundingIncrement: 2, }, "options"); const instance = new Temporal.ZonedDateTime( diff --git a/test/built-ins/Temporal/ZonedDateTime/prototype/round/rounding-is-noop.js b/test/built-ins/Temporal/ZonedDateTime/prototype/round/rounding-is-noop.js new file mode 100644 index 0000000000..f0e7787490 --- /dev/null +++ b/test/built-ins/Temporal/ZonedDateTime/prototype/round/rounding-is-noop.js @@ -0,0 +1,35 @@ +// Copyright (C) 2023 Igalia, S.L. All rights reserved. +// This code is governed by the BSD license found in the LICENSE file. + +/*--- +esid: sec-temporal.zoneddatetime.prototype.round +description: > + No calendar or time zone methods are called under circumstances where rounding + is a no-op +includes: [temporalHelpers.js] +features: [Temporal] +---*/ + +const calendar = TemporalHelpers.calendarThrowEverything(); +const timeZone = TemporalHelpers.timeZoneThrowEverything(); +const instance = new Temporal.ZonedDateTime(0n, timeZone, calendar); + +const noopRoundingOperations = [ + [{ smallestUnit: "nanoseconds" }, "smallestUnit ns"], + [{ smallestUnit: "nanoseconds", roundingIncrement: 1 }, "round to 1 ns"], +]; +for (const [options, descr] of noopRoundingOperations) { + const result = instance.round(options); + assert.notSameValue(result, instance, "rounding result should be a new object"); + assert.sameValue(result.epochNanoseconds, instance.epochNanoseconds, "instant should be unchanged"); + assert.sameValue(result.getCalendar(), instance.getCalendar(), "calendar should be preserved"); + assert.sameValue(result.getTimeZone(), instance.getTimeZone(), "time zone should be preserved"); +} + +const notNoopRoundingOperations = [ + [{ smallestUnit: "microseconds" }, "round to 1 µs"], + [{ smallestUnit: "nanoseconds", roundingIncrement: 2 }, "round to 2 ns"], +]; +for (const [options, descr] of notNoopRoundingOperations) { + assert.throws(Test262Error, () => instance.round(options), `rounding should not be a no-op with ${descr}`); +} diff --git a/test/built-ins/Temporal/ZonedDateTime/prototype/since/order-of-operations.js b/test/built-ins/Temporal/ZonedDateTime/prototype/since/order-of-operations.js index 5c50c76e4f..55352a7426 100644 --- a/test/built-ins/Temporal/ZonedDateTime/prototype/since/order-of-operations.js +++ b/test/built-ins/Temporal/ZonedDateTime/prototype/since/order-of-operations.js @@ -186,6 +186,9 @@ const expectedOpsForCalendarDifference = [ "call this.calendar.dateAdd", "get this.timeZone.getPossibleInstantsFor", "call this.timeZone.getPossibleInstantsFor", +]; + +const expectedOpsForCalendarRounding = [ // RoundDuration → ToTemporalDate "get this.timeZone.getOffsetNanosecondsFor", "call this.timeZone.getOffsetNanosecondsFor", @@ -213,8 +216,13 @@ const expectedOpsForCalendarDifference = [ "call this.timeZone.getPossibleInstantsFor", ]; +// code path that skips RoundDuration: +instance.since(otherDateTimePropertyBag, createOptionsObserver({ largestUnit: "years", smallestUnit: "nanoseconds", roundingIncrement: 1 })); +assert.compareArray(actual, expected.concat(expectedOpsForCalendarDifference), "order of operations with largestUnit years and no rounding"); +actual.splice(0); // clear + // code path through RoundDuration that rounds to the nearest year: -const expectedOpsForYearRounding = expected.concat(expectedOpsForCalendarDifference, [ +const expectedOpsForYearRounding = expected.concat(expectedOpsForCalendarDifference, expectedOpsForCalendarRounding, [ "get this.calendar.dateAdd", // 9.b "call this.calendar.dateAdd", // 9.c "call this.calendar.dateAdd", // 9.e @@ -229,7 +237,7 @@ assert.compareArray(actual, expectedOpsForYearRounding, "order of operations wit actual.splice(0); // clear // code path through RoundDuration that rounds to the nearest month: -const expectedOpsForMonthRounding = expected.concat(expectedOpsForCalendarDifference, [ +const expectedOpsForMonthRounding = expected.concat(expectedOpsForCalendarDifference, expectedOpsForCalendarRounding, [ "get this.calendar.dateAdd", // 10.b "call this.calendar.dateAdd", // 10.c "call this.calendar.dateAdd", // 10.e @@ -240,7 +248,7 @@ assert.compareArray(actual, expectedOpsForMonthRounding, "order of operations wi actual.splice(0); // clear // code path through RoundDuration that rounds to the nearest week: -const expectedOpsForWeekRounding = expected.concat(expectedOpsForCalendarDifference, [ +const expectedOpsForWeekRounding = expected.concat(expectedOpsForCalendarDifference, expectedOpsForCalendarRounding, [ "get this.calendar.dateAdd", // 11.c "call this.calendar.dateAdd", // 11.d MoveRelativeDate ]); // (11.g.iii MoveRelativeDate not called because days already balanced) diff --git a/test/built-ins/Temporal/ZonedDateTime/prototype/until/order-of-operations.js b/test/built-ins/Temporal/ZonedDateTime/prototype/until/order-of-operations.js index 9a7d72c48e..04d5514e55 100644 --- a/test/built-ins/Temporal/ZonedDateTime/prototype/until/order-of-operations.js +++ b/test/built-ins/Temporal/ZonedDateTime/prototype/until/order-of-operations.js @@ -186,6 +186,9 @@ const expectedOpsForCalendarDifference = [ "call this.calendar.dateAdd", "get this.timeZone.getPossibleInstantsFor", "call this.timeZone.getPossibleInstantsFor", +]; + +const expectedOpsForCalendarRounding = [ // RoundDuration → ToTemporalDate "get this.timeZone.getOffsetNanosecondsFor", "call this.timeZone.getOffsetNanosecondsFor", @@ -213,8 +216,13 @@ const expectedOpsForCalendarDifference = [ "call this.timeZone.getPossibleInstantsFor", ]; +// code path that skips RoundDuration: +instance.until(otherDateTimePropertyBag, createOptionsObserver({ largestUnit: "years", smallestUnit: "nanoseconds", roundingIncrement: 1 })); +assert.compareArray(actual, expected.concat(expectedOpsForCalendarDifference), "order of operations with largestUnit years and no rounding"); +actual.splice(0); // clear + // code path through RoundDuration that rounds to the nearest year: -const expectedOpsForYearRounding = expected.concat(expectedOpsForCalendarDifference, [ +const expectedOpsForYearRounding = expected.concat(expectedOpsForCalendarDifference, expectedOpsForCalendarRounding, [ "get this.calendar.dateAdd", // 9.b "call this.calendar.dateAdd", // 9.c "call this.calendar.dateAdd", // 9.e @@ -229,7 +237,7 @@ assert.compareArray(actual, expectedOpsForYearRounding, "order of operations wit actual.splice(0); // clear // code path through RoundDuration that rounds to the nearest month: -const expectedOpsForMonthRounding = expected.concat(expectedOpsForCalendarDifference, [ +const expectedOpsForMonthRounding = expected.concat(expectedOpsForCalendarDifference, expectedOpsForCalendarRounding, [ "get this.calendar.dateAdd", // 10.b "call this.calendar.dateAdd", // 10.c "call this.calendar.dateAdd", // 10.e @@ -240,7 +248,7 @@ assert.compareArray(actual, expectedOpsForMonthRounding, "order of operations wi actual.splice(0); // clear // code path through RoundDuration that rounds to the nearest week: -const expectedOpsForWeekRounding = expected.concat(expectedOpsForCalendarDifference, [ +const expectedOpsForWeekRounding = expected.concat(expectedOpsForCalendarDifference, expectedOpsForCalendarRounding, [ "get this.calendar.dateAdd", // 11.c "call this.calendar.dateAdd", // 11.d MoveRelativeDate ]); // (11.g.iii MoveRelativeDate not called because days already balanced)