diff --git a/android/intltest/java/com/facebook/hermes/test/HermesIntlAndroidTest.java b/android/intltest/java/com/facebook/hermes/test/HermesIntlAndroidTest.java index 582a98ce6e4..ccc90bbeb32 100644 --- a/android/intltest/java/com/facebook/hermes/test/HermesIntlAndroidTest.java +++ b/android/intltest/java/com/facebook/hermes/test/HermesIntlAndroidTest.java @@ -17,6 +17,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.util.Locale; import java.util.stream.Collectors; import org.junit.Test; import org.junit.runner.RunWith; @@ -60,6 +61,50 @@ public void testNumberFormatSignificantDigitsFromAsset() throws IOException { } } + @Test + public void testLocaleCompareFromAsset() throws IOException { + AssetManager assets = + InstrumentationRegistry.getInstrumentation().getTargetContext().getAssets(); + InputStream is = assets.open("locale-compare.js"); + String script = + new BufferedReader(new InputStreamReader(is)).lines().collect(Collectors.joining("\n")); + try (JSRuntime rt = JSRuntime.makeHermesRuntime()) { + rt.evaluateJavaScript(script); + } + } + + @Test + public void testLocaleCompareFollowsDefaultLocale() { + Locale saved = Locale.getDefault(); + Locale savedFormat = Locale.getDefault(Locale.Category.FORMAT); + Locale savedDisplay = Locale.getDefault(Locale.Category.DISPLAY); + Locale swedish = new Locale("sv", "SE"); + try (JSRuntime rt = JSRuntime.makeHermesRuntime()) { + // Swedish sorts 'ä' after 'z', German before it. + Locale.setDefault(swedish); + rt.evaluateJavaScript("var sv = 'ä'.localeCompare('z');"); + Locale.setDefault(Locale.GERMANY); + rt.evaluateJavaScript("var de = 'ä'.localeCompare('z');"); + // Collators resolve against the default format locale. + Locale.setDefault(Locale.Category.FORMAT, swedish); + rt.evaluateJavaScript("var svFormat = 'ä'.localeCompare('z');"); + // An unsupported explicit locale falls back to the default locale. + Locale.setDefault(Locale.GERMANY); + rt.evaluateJavaScript("var deFallback = 'ä'.localeCompare('z', 'zz');"); + Locale.setDefault(swedish); + rt.evaluateJavaScript("var svFallback = 'ä'.localeCompare('z', 'zz');"); + assertThat(rt.getGlobalNumberProperty("sv")).isEqualTo(1); + assertThat(rt.getGlobalNumberProperty("de")).isEqualTo(-1); + assertThat(rt.getGlobalNumberProperty("svFormat")).isEqualTo(1); + assertThat(rt.getGlobalNumberProperty("deFallback")).isEqualTo(-1); + assertThat(rt.getGlobalNumberProperty("svFallback")).isEqualTo(1); + } finally { + Locale.setDefault(saved); + Locale.setDefault(Locale.Category.FORMAT, savedFormat); + Locale.setDefault(Locale.Category.DISPLAY, savedDisplay); + } + } + @Test public void testDateTimeFormat() { try (JSRuntime rt = JSRuntime.makeHermesRuntime()) { diff --git a/include/hermes/Platform/Intl/PlatformIntl.h b/include/hermes/Platform/Intl/PlatformIntl.h index 8a0b8eba71d..93b20177989 100644 --- a/include/hermes/Platform/Intl/PlatformIntl.h +++ b/include/hermes/Platform/Intl/PlatformIntl.h @@ -57,6 +57,12 @@ class Option { return str_; } + bool operator==(const Option &other) const { + if (kind_ != other.kind_) + return false; + return isString() ? str_ == other.str_ : num_ == other.num_; + } + private: Kind kind_; // I am too lazy to make this a union. @@ -122,6 +128,11 @@ class Collator : public vm::DecoratedObject::Decoration { Options resolvedOptions() noexcept; double compare(const std::u16string &x, const std::u16string &y) noexcept; + + /// Whether this collator uses a default locale that is no longer the + /// platform's default. A collator created now from the same locales and + /// options would then resolve to a different locale. + bool defaultLocaleChanged() noexcept; }; class DateTimeFormat : public vm::DecoratedObject::Decoration { diff --git a/include/hermes/VM/JSLib/JSLibStorage.h b/include/hermes/VM/JSLib/JSLibStorage.h index 0bdd5173066..4125a82f717 100644 --- a/include/hermes/VM/JSLib/JSLibStorage.h +++ b/include/hermes/VM/JSLib/JSLibStorage.h @@ -9,7 +9,11 @@ #define HERMES_VM_JSLIB_RUNTIMECOMMONSTORAGE_H #include "hermes/VM/JSLib/DateCache.h" +#ifdef HERMES_ENABLE_INTL +#include "hermes/Platform/Intl/PlatformIntl.h" +#endif +#include #include namespace hermes { @@ -31,6 +35,16 @@ struct JSLibStorage { /// Time zone offset cache used in conversion between UTC and local time. LocalTimeOffsetCache localTimeOffsetCache; + +#ifdef HERMES_ENABLE_INTL + /// The collator String.prototype.localeCompare created last, with the + /// normalized locales and options it was created from. + struct { + std::vector locales; + platform_intl::Options options; + std::unique_ptr collator; + } localeCompareCollator; +#endif }; } // namespace vm diff --git a/lib/Platform/Intl/PlatformIntlAndroid.cpp b/lib/Platform/Intl/PlatformIntlAndroid.cpp index ce89d8d6070..c25428d5690 100644 --- a/lib/Platform/Intl/PlatformIntlAndroid.cpp +++ b/lib/Platform/Intl/PlatformIntlAndroid.cpp @@ -316,6 +316,12 @@ class JCollator : public jni::JavaClass { jni::alias_ref, jni::alias_ref)>("compare"); return method(self(), x, y); } + + bool defaultLocaleChanged() { + static const auto method = + javaClassStatic()->getMethod("defaultLocaleChanged"); + return method(self()); + } }; class CollatorAndroid : public Collator { @@ -338,6 +344,10 @@ class CollatorAndroid : public Collator { return jCollator_->compare(stringToJava(x), stringToJava(y)); } + bool defaultLocaleChanged() noexcept { + return jCollator_->defaultLocaleChanged(); + } + private: jni::global_ref jCollator_; }; @@ -398,6 +408,10 @@ double Collator::compare( return static_cast(this)->compare(x, y); } +bool Collator::defaultLocaleChanged() noexcept { + return static_cast(this)->defaultLocaleChanged(); +} + namespace { class JDateTimeFormat : public jni::JavaClass { diff --git a/lib/Platform/Intl/PlatformIntlApple.mm b/lib/Platform/Intl/PlatformIntlApple.mm index 41fe3d7b8de..3f4fffac82f 100644 --- a/lib/Platform/Intl/PlatformIntlApple.mm +++ b/lib/Platform/Intl/PlatformIntlApple.mm @@ -1263,6 +1263,11 @@ uint8_t getCurrencyDigits(std::u16string_view code) { return static_cast(this)->compare(x, y); } +bool Collator::defaultLocaleChanged() noexcept { + // getDefaultLocale() reads the default locale once per process. + return false; +} + namespace { // Implementation of // https://402.ecma-international.org/8.0/#datetimeformat-objects diff --git a/lib/Platform/Intl/java/com/facebook/hermes/intl/Collator.java b/lib/Platform/Intl/java/com/facebook/hermes/intl/Collator.java index 11732c1aafa..c71fd8d6935 100644 --- a/lib/Platform/Intl/java/com/facebook/hermes/intl/Collator.java +++ b/lib/Platform/Intl/java/com/facebook/hermes/intl/Collator.java @@ -7,6 +7,7 @@ package com.facebook.hermes.intl; +import android.icu.util.ULocale; import android.os.Build; import com.facebook.proguard.annotations.DoNotStrip; import java.util.ArrayList; @@ -14,6 +15,7 @@ import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; +import java.util.Locale; import java.util.Map; /** @@ -78,6 +80,19 @@ public class Collator { // [[InitializedCollator]] private IPlatformCollator mPlatformCollatorObject; + // The default locale that locale resolution fell back to, or null if a requested locale was used. + private Object mDefaultLocale; + + // Must read the same locale as LocaleObject.createDefault(), or defaultLocaleChanged() compares + // the wrong value. + private static Object currentDefaultLocale() { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { + return ULocale.getDefault(ULocale.Category.FORMAT); + } else { + return Locale.getDefault(); + } + } + // https://tc39.es/ecma402/#sec-initializecollator private void initializeCollator(List locales, Map options) throws JSRangeErrorException { @@ -141,6 +156,7 @@ private void initializeCollator(List locales, Map option HashMap r = LocaleResolver.resolveLocale(locales, opt, relevantExtensionKeys); mResolvedLocaleObject = (ILocaleObject) JSObjects.getJavaMap(r).get("locale"); + mDefaultLocale = JSObjects.getJavaMap(r).get("defaultLocale"); mResolvedLocaleObjectForResolvedOptions = mResolvedLocaleObject.cloneObject(); Object collation = JSObjects.Get(r, "co"); @@ -297,4 +313,11 @@ public Map resolvedOptions() throws JSRangeErrorException { public double compare(String source, String target) { return mPlatformCollatorObject.compare(source, target); } + + // Whether this collator uses a default locale that is no longer the default, in which case a + // collator created now from the same locales and options would resolve differently. + @DoNotStrip + public boolean defaultLocaleChanged() { + return mDefaultLocale != null && !mDefaultLocale.equals(currentDefaultLocale()); + } } diff --git a/lib/Platform/Intl/java/com/facebook/hermes/intl/LocaleMatcher.java b/lib/Platform/Intl/java/com/facebook/hermes/intl/LocaleMatcher.java index b43f6e27c1c..13d197a758a 100644 --- a/lib/Platform/Intl/java/com/facebook/hermes/intl/LocaleMatcher.java +++ b/lib/Platform/Intl/java/com/facebook/hermes/intl/LocaleMatcher.java @@ -19,6 +19,8 @@ public class LocaleMatcher { public static class LocaleMatchResult { public ILocaleObject matchedLocale; public HashMap extensions = new HashMap<>(); + // The default locale the match fell back to because no requested locale is available, or null. + public Object defaultLocale; } // https://tc39.es/ecma402/#sec-bestavailablelocale @@ -62,6 +64,7 @@ public static LocaleMatchResult lookupMatch(String[] requestedLocales, String[] } result.matchedLocale = LocaleObject.createDefault(); + result.defaultLocale = result.matchedLocale.getLocale(); return result; } @@ -145,6 +148,7 @@ public static LocaleMatchResult bestFitMatch(String[] requestedLocales) } result.matchedLocale = LocaleObjectICU.createDefault(); + result.defaultLocale = result.matchedLocale.getLocale(); return result; } diff --git a/lib/Platform/Intl/java/com/facebook/hermes/intl/LocaleResolver.java b/lib/Platform/Intl/java/com/facebook/hermes/intl/LocaleResolver.java index e90b98a7e03..0b2bd3056ce 100644 --- a/lib/Platform/Intl/java/com/facebook/hermes/intl/LocaleResolver.java +++ b/lib/Platform/Intl/java/com/facebook/hermes/intl/LocaleResolver.java @@ -105,6 +105,7 @@ public static HashMap resolveLocale( } result.put("locale", localeMatchResult.matchedLocale); + result.put("defaultLocale", localeMatchResult.defaultLocale); return result; } diff --git a/lib/VM/JSLib/Intl.cpp b/lib/VM/JSLib/Intl.cpp index c0e5841dca1..47529ea037a 100644 --- a/lib/VM/JSLib/Intl.cpp +++ b/lib/VM/JSLib/Intl.cpp @@ -13,6 +13,7 @@ #include "hermes/VM/ArrayLike.h" #include "hermes/VM/JSLib/DateUtil.h" +#include "hermes/VM/JSLib/JSLibStorage.h" #include "hermes/VM/PrimitiveBox.h" #include "hermes/VM/Runtime.h" #include "hermes/VM/StackFrame-inline.h" @@ -1672,14 +1673,25 @@ CallResult intlStringPrototypeLocaleCompare( return ExecutionStatus::EXCEPTION; } - CallResult> collatorRes = - platform_intl::Collator::create(runtime, *localesRes, *optionsRes); - if (LLVM_UNLIKELY(collatorRes == ExecutionStatus::EXCEPTION)) { - return ExecutionStatus::EXCEPTION; + // Reuse the previous collator when its inputs are equal: creating one costs + // far more than a comparison. This must stay after every read and conversion + // of the arguments, so the reuse is not observable. + auto &cached = runtime.getJSLibStorage()->localeCompareCollator; + if (!cached.collator || cached.locales != *localesRes || + cached.options != *optionsRes || + cached.collator->defaultLocaleChanged()) { + CallResult> collatorRes = + platform_intl::Collator::create(runtime, *localesRes, *optionsRes); + if (LLVM_UNLIKELY(collatorRes == ExecutionStatus::EXCEPTION)) { + return ExecutionStatus::EXCEPTION; + } + cached.collator = std::move(*collatorRes); + cached.locales = std::move(*localesRes); + cached.options = std::move(*optionsRes); } return HermesValue::encodeTrustedNumberValue( - (*collatorRes)->compare(*thisRes, *thatRes)); + cached.collator->compare(*thisRes, *thatRes)); } CallResult intlStringPrototypeToLocaleLowerCase( diff --git a/test/hermes/intl/locale-compare.js b/test/hermes/intl/locale-compare.js new file mode 100644 index 00000000000..ca3c436d190 --- /dev/null +++ b/test/hermes/intl/locale-compare.js @@ -0,0 +1,86 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// RUN: %hermes %s +// REQUIRES: intl + +// String.prototype.localeCompare reuses its collator while the locales and +// options stay the same. A call with other arguments must not get the +// previous call's collator. + +function assertEq(actual, expected, what) { + if (actual !== expected) { + throw new Error(what + ': expected ' + expected + ', got ' + actual); + } +} + +function sorted(locales, options) { + return ['Z', 'a', 'z', 'ä'] + .sort((x, y) => x.localeCompare(y, locales, options)) + .join(); +} + +for (var i = 0; i < 2; i++) { + assertEq(sorted('de'), 'a,ä,z,Z', 'de'); + assertEq(sorted('sv'), 'a,z,Z,ä', 'sv'); + assertEq(sorted('sv', {caseFirst: 'upper'}), 'a,Z,z,ä', 'sv, upper first'); + assertEq(sorted('de', {caseFirst: 'upper'}), 'a,ä,Z,z', 'de, upper first'); + assertEq( + 'test10'.localeCompare('test2', 'en', {numeric: true}), + 1, + 'numeric', + ); + assertEq('test10'.localeCompare('test2', 'en'), -1, 'not numeric'); +} + +// The options are still read on every call, even when the collator is reused. +var reads = 0; +var counted = { + get sensitivity() { + reads++; + return 'base'; + }, +}; +for (var i = 0; i < 3; i++) { + assertEq('a'.localeCompare('A', 'en', counted), 0, 'counted getter'); +} +assertEq(reads, 3, 'sensitivity reads'); + +// A throwing getter throws with a warm cache, and the next call still works. +var threw = false; +try { + 'a'.localeCompare('A', 'en', { + get sensitivity() { + throw new Error('boom'); + }, + }); +} catch (e) { + threw = e.message === 'boom'; +} +assertEq(threw, true, 'throwing getter'); +assertEq('a'.localeCompare('A', 'en', counted), 0, 'after throwing getter'); + +// A getter that calls localeCompare with other arguments does not disturb the +// outer call. +var reentrant = { + get sensitivity() { + assertEq(sorted('sv'), 'a,z,Z,ä', 'inner sv'); + return 'base'; + }, +}; +assertEq('a'.localeCompare('A', 'de', reentrant), 0, 'outer de'); +assertEq('ä'.localeCompare('z', 'de', reentrant), -1, 'outer de after inner'); + +// An invalid option throws and leaves later calls correct. +threw = false; +try { + 'a'.localeCompare('b', 'en', {usage: 'order'}); +} catch (e) { + threw = e instanceof RangeError; +} +assertEq(threw, true, 'invalid usage'); +assertEq(sorted('de'), 'a,ä,z,Z', 'de after invalid usage');