<i18n dev> RFR: 8225641: Calendar.roll(int field) does not work correctly for WEEK_OF_YEAR

[Naoto Sato]

On Tue, 14 Mar 2023 22:16:45 GMT, Justin Lu <jlu at openjdk.org> wrote:

> This PR fixes the bug which occurred when `Calendar.roll(WEEK_OF_YEAR)` rolled into a minimal first week with an invalid `WEEK_OF_YEAR` and `DAY_OF_WEEK` combo.
> 
> For example, Rolling _Monday, 30 December 2019_ by 1 week produced _Monday, 31 December 2018_, which is incorrect. This is because `WEEK_OF_YEAR` is rolled from 52 to 1, and the original `DAY_OF_WEEK` is 1. However, there is no Monday in week 1 of 2019. This is exposed when a future method calls `Calendar.complete()`, which eventually calculates a `fixedDate` with the invalid `WEEK_OF_YEAR` and `DAY_OF_WEEK` combo.
> 
> To prevent this, a check is added for rolls into week 1, which determines if the first week is minimal. If it is indeed minimal, then it is checked if  `DAY_OF_WEEK` exists in that week, if not, `WEEK_OF_YEAR` must be incremented by one.
> 
> After the fix, Rolling _Monday, 30 December 2019_ by 1 week produces _Monday, 7 January 2019_

Hi Justin,
Thanks for the fix. Still reviewing the changes, but here are some comments I have noticed:

src/java.base/share/classes/java/util/GregorianCalendar.java line 1314:

> 1312:                     // current DAY_OF_WEEK
> 1313:                     if (newWeekOfYear == 1 && isInvalidWeek1()) {
> 1314:                         newWeekOfYear+=1;

is `+1` always correct? Does it work when the amount is negative?

src/java.base/share/classes/java/util/GregorianCalendar.java line 3030:

> 3028:         }
> 3029:     }
> 3030: 

Are these `GregorianCalendar` specific? What about other calendars?

test/jdk/java/util/Calendar/RollToMinWeek.java line 30:

> 28:  * is rolled into a minimal week 1
> 29:  * @run junit RollToMinWeek
> 30:  */

Have you considered adding test cases into `test/jdk/java/util/Calendar/CalendarTestScripts`, instead of creating a single-purpose test case?

test/jdk/java/util/Calendar/RollToMinWeek.java line 79:

> 77:         return Stream.of(
> 78:                 // Test a variety of rolls that previously produced incorrect results
> 79:                 Arguments.of(buildCalendar(27, 11, 2020, Locale.ENGLISH),

The first day of week depends on the region, not the language. In fact, I would prefer explicitly specifying it via a locale like `en-US-u-fw-mon`, and testing all the weekdays.

test/jdk/java/util/Calendar/RollToMinWeek.java line 95:

> 93: 
> 94:     private static Calendar buildCalendar(int day, int month, int year, Locale locale) {
> 95:         Calendar calendar = Calendar.getInstance(locale);

If the fix is `GregorianCalendar` specific, should it check the type?

-------------

PR: https://git.openjdk.org/jdk/pull/13031