Fix Android Intl to clear needDefault flag correctly

[Maturia]

Fixed an issue where needDefault flag was not being cleared when the option weekday was set.

Summary

The current Intl implementation on Android does not correctly follow the RFC for Intl implementation. Specifying a value for the option weekday, year, month, or day should clear the needDefaults flag, which will prevent the auto-formatted date from automatically being appended to the formatted string (4a here: https://tc39.es/ecma402/#sec-todatetimeoptions).

Current implementation:

const date = new Date(Date.UTC(2012, 11, 20, 3, 0, 0, 200));
// request a narrow weekday format
let options = {
  weekday: "narrow"
};
console.log(new Intl.DateTimeFormat("en-US", options).format(date)); 
// outputs "W, 12/19/2012" which is wrong, this should just be "W" since using "weekday" negates the need for defaults.

With fix:

const date = new Date(Date.UTC(2012, 11, 20, 3, 0, 0, 200));
let options = {
  weekday: "narrow"
};
console.log(new Intl.DateTimeFormat("en-US", options).format(date)); 
// outputs "W" which is correct.

[neildhar]

Hey @Maturia, thanks for reporting and investigating this. I have a question regarding the test case you've shared. While this seems to be a legitimate bug, running new Intl.DateTimeFormat("en-US", options).format(date) does not exercise the code modified here, and seems to work correctly. However, I am able to reproduce the bug you describe by running console.log(date.toLocaleString('en-US', options));.

Can you confirm the issue you're seeing?

[dlebedynskyi]

I believe issues in this PR is same from #926 (comment)

[neildhar]

@dlebedynskyi I understand the issue being reported here. This fixes a legitimate bug in Hermes. I'm just confused because the example code deals with DateTimeFormat, when the bug should appear with toLocaleString. So it would be helpful to get confirmation that this is indeed the bug you're observing, and you aren't using any polyfills perhaps that might in turn delegate the work to toLocaleString.

[dlebedynskyi]

I've done some quick test on 0.71 demo app

date = new Date(Date.UTC(2012, 11, 20, 3, 0, 0, 200));
options = {
  weekday: "narrow"
};
new Intl.DateTimeFormat("en-US", options).format(date);
// 'W' - Correct 

console.log(date.toLocaleString('en-US', options))
// W, 12/19/2012, 7:00:00 PM  - WRONG, expected W instead

@neildhar looks like you are correct and fix should be in toLocaleString part only.

However looking at code intlDatePrototypeToSomeLocaleString seems to be using kDTFOptions for normalization of inputs. so toLocaleString => intlDatePrototypeToSomeLocaleString => normalizeOptions

hermes/lib/VM/JSLib/Intl.cpp

Line 1469 in de48d91

normalizeOptions(runtime, args.getArgHandle(1), kDTFOptions);

I see that same options kDTFOptions are used for both constructor and intlDatePrototypeToSomeLocaleString

Should fix be in toDateTimeOptions instead then? I see another call for toDateTimeOptions there and I guess this lines now take effect due to change in pull? Any suggestion on fix?

[neildhar]

@dlebedynskyi Thank you for confirming. This fix is correct for the bug in toLocaleString, I just wanted to make sure we were looking at the same problem.

@Maturia could you update the summary and add a test for this? The easiest place to do it would be to add it to date-time-format-apple.js, which will test that we get the right behaviour on macOS (the bug is also present there).

[facebook-github-bot]

@neildhar has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@neildhar merged this pull request in 64daca6.