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

8262108: SimpleDateFormat formatting broken for sq_MK Locale #3463

Closed
wants to merge 10 commits into from

Conversation

@jaikiran
Copy link
Member

@jaikiran jaikiran commented Apr 13, 2021

Can I please get a review for this proposed fix for https://bugs.openjdk.java.net/browse/JDK-8262108?

As noted in a comment in that issue, the bug relates to the return value of Calendar.getDisplayNames for the Calendar.AM_PM field. The implementation has started returning invalid values for the AM_PM field after the "day period" support was added recently in the JDK as part of https://bugs.openjdk.java.net/browse/JDK-8262108.

The commit here adds a check in the internal implementation of the display name handling logic, to special case the AM_PM field and properly convert the display name array indexes (which is an internal detail) to valid values that represent the AM_PM calendar field.

The commit also has a new jtreg test case CalendarDisplayNamesTest reproduces this issue and verifies the fix.

After this fix was introduced, I ran the test in test/jdk/java/util/Calendar/ and that showed up a failure in an existing test case NarrowNamesTest. Looking at that test case, IMO, the current testing in that NarrowNamesTest is incorrect and is probably what hid this issue in the first place? To fix this, I have added an additional commit which updates this test case to properly test the AM_PM field values.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8262108: SimpleDateFormat formatting broken for sq_MK Locale

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3463/head:pull/3463
$ git checkout pull/3463

Update a local copy of the PR:
$ git checkout pull/3463
$ git pull https://git.openjdk.java.net/jdk pull/3463/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3463

View PR using the GUI difftool:
$ git pr show -t 3463

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3463.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 13, 2021

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Apr 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 13, 2021

@jaikiran The following labels will be automatically applied to this pull request:

  • core-libs
  • i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Apr 13, 2021

While we are at this, the Calendar.getDisplayNames(...) states this:

....
@throws IllegalArgumentException
* if {@code field} or {@code style} is invalid,
* or if this {@code Calendar} is non-lenient and any
* of the calendar fields have invalid values

If I understand this correctly, if the Calendar instance was non-lenient and this method returned invalid values for the AM_PM field, it should have thrown an IllegalArgumentException (which is a strange exception to throw for an invalid result, but that's a different matter). However, with the current code in upstream master, I saw no such exception being thrown (nor do I see any such validating code in the Calendar implementation) when the calendar instance was non-lenient. Is that expected?

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 13, 2021

// evening2 = 9, night1 = 10, night2 = 11
switch (i) {
case 0, 2, 3, 4, 5 -> map.put(name, AM);
case 1, 6, 7, 8, 9, 10, 11 -> map.put(name, PM);

This comment has been minimized.

@jaikiran

jaikiran Apr 13, 2021
Author Member

One part where I need input here, is the "midnight" and the "noon" types. I haven't found anything in the JDK code which I can use as a reference to classify "midnight" and "noon" as AM (which is what this code is doing). The doc here https://unicode.org/reports/tr35/tr35-dates.html#Day_Period_Rules speaks about these 2 types but I don't think it's something that I can use to translate those types into a field value for Calendar, to represent AM or PM. As such, right now, I've just guessed that AM is probably the right value for these types, but there's no technical reason or reference backing it.

@naotoj
Copy link
Member

@naotoj naotoj commented Apr 13, 2021

Have you run regression tests in java.time? If I am not mistaken, your changes simply seem to nullify the day period support.

@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Apr 14, 2021

Have you run regression tests in java.time? If I am not mistaken, your changes simply seem to nullify the day period support.

Hello @naotoj, my tier1 test run passed without issues locally with this change:

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
>> jtreg:test/hotspot/jtreg:tier1                     1750  1746     4     0 <<
   jtreg:test/jdk:tier1                               2022  2022     0     0   
   jtreg:test/langtools:tier1                         4158  4158     0     0   
   jtreg:test/jaxp:tier1                                 0     0     0     0   
   jtreg:test/lib-test:tier1                             0     0     0     0   
==============================

(the 4 hotspot failures are unrelated environmental issues).

After seeing your message, I now ran the java/time regression tests:

jtreg -jdk:build/macosx-x86_64-server-release/images/jdk/ test/jdk/java/time/
Test results: passed: 184

and even those passed.

I don't have much knowledge in this area of the code, but as far as I can see, this doesn't touch the changes introduced in the java.time to support the new day period construct.

@naotoj
Copy link
Member

@naotoj naotoj commented Apr 14, 2021

Thanks for checking.
CalendarNameProviderImpl is an implementation detail, which is shared between j.u.Calendar and j.t.f.DateTimeFormatter(Builder) class, where it provides localized names for Calendar fields. For the field Calendar.AM_PM, it includes not only am/pm translations, but also day periods translations as you see. So blindly mapping day period translations to Calendar.AM/PM index is incorrect. I'd rather modify the code like:

                            if (field == AM_PM && !javatime && i > PM) {
                                continue;
                            } else {
                                map.put(name, base + i);
                            }
@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Apr 14, 2021

Hello Naoto,

CalendarNameProviderImpl is an implementation detail, which is shared between j.u.Calendar and j.t.f.DateTimeFormatter(Builder) class, where it provides localized names for Calendar fields. For the field Calendar.AM_PM, it includes not only am/pm translations, but also day periods translations as you see.

Thank you for those details. That's very helpful.

I'd rather modify the code like:

                        if (field == AM_PM && !javatime && i > PM) {
                            continue;
                        } else {
                            map.put(name, base + i);
                        }

I've taken your suggestion and updated the code to match this. In the update, I've included a code comment to explain what this block does. Please do let me know if that code comment isn't accurate and needs any updates.

The new CalendarDisplayNamesTest continues to pass with this change. I updated the existing NarrowNamesTest to match the expectations of the Calendar.getDisplayName() API.

All jtreg tests in test/jdk/java/util/Calendar and test/jdk/java/time/ continue to pass too. I have triggered a local run of tier1 tests and will check back the results early morning tomorrow.

Thank you for your reviews so far.

@@ -189,7 +189,14 @@ public String getDisplayNameImpl(String calendarType, int field, int value, int
if (name.isEmpty()) {
continue;
}
map.put(name, base + i);
if (field == AM_PM && !javatime && i > PM) {
// when dealing with calendar fields, don't set AM_PM field value

This comment has been minimized.

@naotoj

naotoj Apr 14, 2021
Member

Make it explicit that this block only serves for java.util.Calendar, not java.time.format.DateTimeFormatter(Builder).

This comment has been minimized.

@jaikiran

jaikiran Apr 15, 2021
Author Member

Hello Naoto, I've now updated the PR to be more explicit in this code comment.

for (final Locale locale : Locale.getAvailableLocales()) {
for (final int style : styles) {
final Calendar cal = Calendar.getInstance();
cal.setLenient(false);

This comment has been minimized.

@naotoj

naotoj Apr 14, 2021
Member

Don't think leniency is relevant here.

This comment has been minimized.

@jaikiran

jaikiran Apr 15, 2021
Author Member

Removed in the latest update of this PR. This was a leftover from my experimental testing.

Map<String, Integer> expectedFieldValues = new HashMap<>();
expectedFieldValues.put("AM", Calendar.AM);
expectedFieldValues.put("PM", Calendar.PM);
expectedFieldValues.put("midnight", null);
expectedFieldValues.put("noon", null);
expectedFieldValues.put("in the morning", null);
expectedFieldValues.put("in the afternoon", null);
expectedFieldValues.put("in the evening", null);
expectedFieldValues.put("at night", null);
expectedFieldValues.put("a", Calendar.AM);
expectedFieldValues.put("p", Calendar.PM);
expectedFieldValues.put("mi", null);
expectedFieldValues.put("n", null);
testAM_PM(US, ALL_STYLES, expectedFieldValues);
} else {
testMap(US, AM_PM, ALL_STYLES,
"AM", "PM",
RESET_INDEX,
"a", "p");
Map<String, Integer> expectedFieldValues = new HashMap<>();
expectedFieldValues.put("AM", Calendar.AM);
expectedFieldValues.put("PM", Calendar.PM);
expectedFieldValues.put("a", Calendar.AM);
expectedFieldValues.put("p", Calendar.PM);
testAM_PM(US, ALL_STYLES, expectedFieldValues);
Comment on lines 99 to 119

This comment has been minimized.

@naotoj

naotoj Apr 14, 2021
Member

I believe this can be reverted to the original, as the original code compares the exact map (including the length of the map). , i.e.,

           testMap(US, AM_PM, ALL_STYLES,
                    "AM", "PM",
                    RESET_INDEX,
                    "a", "p");

This comment has been minimized.

@jaikiran

jaikiran Apr 15, 2021
Author Member

Done. Reverted this specific part to what it was originally. Test continues to pass with this change and the latest PR update.

if (providers.startsWith("CLDR")) {
testMap(US, AM_PM, ALL_STYLES,
"AM",
"PM",
"midnight",
"noon",
"in the morning",
"",
"in the afternoon",
"",
"in the evening",
"",
"at night",
"",
RESET_INDEX,
"a", "p", "mi", "n", "", "", "", "", "", "", "", "");
Map<String, Integer> expectedFieldValues = new HashMap<>();
expectedFieldValues.put("AM", Calendar.AM);
expectedFieldValues.put("PM", Calendar.PM);
expectedFieldValues.put("midnight", null);
expectedFieldValues.put("noon", null);
expectedFieldValues.put("in the morning", null);
expectedFieldValues.put("in the afternoon", null);
expectedFieldValues.put("in the evening", null);
expectedFieldValues.put("at night", null);
expectedFieldValues.put("a", Calendar.AM);
expectedFieldValues.put("p", Calendar.PM);
expectedFieldValues.put("mi", null);
expectedFieldValues.put("n", null);
testAM_PM(US, ALL_STYLES, expectedFieldValues);
} else {
testMap(US, AM_PM, ALL_STYLES,
"AM", "PM",
Comment on lines 98 to 115

This comment has been minimized.

@naotoj

naotoj Apr 15, 2021
Member

What I meant was there is no need to check the providers and introduce the am/pm specific test method. Both CLDR and COMPAT (i.e., no "if" statement) should be tested with testMap() method as other tests do.

This comment has been minimized.

@jaikiran

jaikiran Apr 16, 2021
Author Member

Hello Naoto,

As far as I can see, the testMap cannot be used to test the day period strings. More specifically, consider this change (git diff) that I did as you suggested (unless I misunderstood what you meant):

+        testMap(US, AM_PM, ALL_STYLES,
+                "AM",
+                "PM",
+                "midnight",
+                "noon",
+                "in the morning",
+                "",
+                "in the afternoon",
+                "",
+                "in the evening",
+                "",
+                "at night",
+                "",
+                RESET_INDEX,
+                "a", "p", "mi", "n", "", "", "", "", "", "", "", "");
+        testMap(US, AM_PM, ALL_STYLES,
+                "AM", "PM",
+                RESET_INDEX,
+                "a", "p");

This will fail with the following error.

testMap: locale=en_US, field=9, style=0, expected={in the afternoon=6, in the evening=8, in the morning=4, at night=10, midnight=2, noon=3, AM=0, PM=1, mi=2, a=0, n=3, p=1}, got={AM=0, PM=1, a=0, p=1}
java.lang.RuntimeException: test failed

That failure is due to the testMap method expecting these day period string to be part of the returned Map from Calendar.getDisplayNames() which won't be the case because that API (as per the javadoc) will only return a Map whose display names have valid field values, so in this case only those display names which will have AM or PM as a value.

If it's easier to review, I will go ahead and push the suggested change in this testcase and let if fail so that you can get a chance to look at the actual test code and error. Let me know.

This comment has been minimized.

@naotoj

naotoj Apr 16, 2021
Member

Sorry if I wasn't clear. The whole test for am/pm (line 98 - 118) can simply be:

            testMap(US, AM_PM, ALL_STYLES,
                    "AM", "PM",
                    RESET_INDEX,
                    "a", "p");

Since we now know that day periods strings won't be leaked into display names, then the map simply should contain only 4 entries (AM, PM, a, and p).

This comment has been minimized.

@jaikiran

jaikiran Apr 16, 2021
Author Member

Thank you for that clarification. I now understand what you mean. I've updated the test to follow this suggestion in the latest update to this PR.

@naotoj
naotoj approved these changes Apr 16, 2021
Copy link
Member

@naotoj naotoj left a comment

Looks good. Thank you for fixing the issue.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 16, 2021

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8262108: SimpleDateFormat formatting broken for sq_MK Locale

Reviewed-by: naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 66 new commits pushed to the master branch:

  • 3423f3e: 8265180: JvmtiCompiledMethodLoadEvent should include the stub section of nmethods
  • f6e54f2: 8258794: Support for CLDR version 39
  • e89fd15: 8261301: StringWriter.flush() is NOOP but documentation does not indicate it
  • 0b1b5c8: 8264373: javac hangs when annotation is declared with sealed public modifier
  • c7da64a: 8265302: ProblemList runtime/logging/RedefineClasses.java on linux-x64 -Xcomp
  • 838c11f: 8265293: ProblemList java/foreign/TestDowncall.java on macosx-aarch64
  • 325eecb: 8255273: jshell crashes with UnsupportedOperationException: Should not get here.
  • e43aee5: 8262900: ToolBasicTest fails to access HTTP server it starts
  • c70589c: 8265227: Move Proc.java from security/testlibrary to test/lib
  • 7b61a42: 8265173: [test] divert spurious log output away from stream under test in ProcessBuilder Basic test
  • ... and 56 more: https://git.openjdk.java.net/jdk/compare/008fc75a290f0286fb30c3b0a2f5abada441149c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Apr 16, 2021
@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Apr 16, 2021

Thank you Naoto for your reviews and guidance in fixing this issue.

@jaikiran
Copy link
Member Author

@jaikiran jaikiran commented Apr 16, 2021

/integrate

@openjdk openjdk bot closed this Apr 16, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 16, 2021

@jaikiran Since your change was applied there have been 66 commits pushed to the master branch:

  • 3423f3e: 8265180: JvmtiCompiledMethodLoadEvent should include the stub section of nmethods
  • f6e54f2: 8258794: Support for CLDR version 39
  • e89fd15: 8261301: StringWriter.flush() is NOOP but documentation does not indicate it
  • 0b1b5c8: 8264373: javac hangs when annotation is declared with sealed public modifier
  • c7da64a: 8265302: ProblemList runtime/logging/RedefineClasses.java on linux-x64 -Xcomp
  • 838c11f: 8265293: ProblemList java/foreign/TestDowncall.java on macosx-aarch64
  • 325eecb: 8255273: jshell crashes with UnsupportedOperationException: Should not get here.
  • e43aee5: 8262900: ToolBasicTest fails to access HTTP server it starts
  • c70589c: 8265227: Move Proc.java from security/testlibrary to test/lib
  • 7b61a42: 8265173: [test] divert spurious log output away from stream under test in ProcessBuilder Basic test
  • ... and 56 more: https://git.openjdk.java.net/jdk/compare/008fc75a290f0286fb30c3b0a2f5abada441149c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 64e2130.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants