From 1e0e151c16aa1fc1a920ea0e6cbb80afe552ba02 Mon Sep 17 00:00:00 2001 From: Tim Chevalier Date: Thu, 18 Apr 2024 15:37:29 -0700 Subject: [PATCH] ICU-22747 MF2: Validate values of integer-valued options for :number and :integer Previously, it wasn't an error to write a message like:: .local $foo = {1 :integer minimumIntegerDigits=foo} {{$foo}} This is an error according to the spec, because the `minimumIntegerDigits` option to `:integer` must have a value that's a non-negative integer. This change adds validation of options that must be integer-valued to the implementations of the built-in `:integer` and `:number` functions. Other options (most of which have small sets of string values that are allowed as valid options) are not validated yet. --- .../i18n/messageformat2_function_registry.cpp | 81 +++++++------------ ...essageformat2_function_registry_internal.h | 11 +-- .../test/intltest/messageformat2test.cpp | 30 +++++++ 3 files changed, 66 insertions(+), 56 deletions(-) diff --git a/icu4c/source/i18n/messageformat2_function_registry.cpp b/icu4c/source/i18n/messageformat2_function_registry.cpp index 18401d4844c7..3f9812b38eb9 100644 --- a/icu4c/source/i18n/messageformat2_function_registry.cpp +++ b/icu4c/source/i18n/messageformat2_function_registry.cpp @@ -291,11 +291,11 @@ MFFunctionRegistry::~MFFunctionRegistry() { } } - int32_t maxSignificantDigits = number.maximumSignificantDigits(opts); + int32_t maxSignificantDigits = number.maximumSignificantDigits(opts, status); if (!isInteger) { - int32_t minFractionDigits = number.minimumFractionDigits(opts); - int32_t maxFractionDigits = number.maximumFractionDigits(opts); - int32_t minSignificantDigits = number.minimumSignificantDigits(opts); + int32_t minFractionDigits = number.minimumFractionDigits(opts, status); + int32_t maxFractionDigits = number.maximumFractionDigits(opts, status); + int32_t minSignificantDigits = number.minimumSignificantDigits(opts, status); Precision p = Precision::minMaxFraction(minFractionDigits, maxFractionDigits); if (minSignificantDigits > 0) { p = p.minSignificantDigits(minSignificantDigits); @@ -314,7 +314,7 @@ MFFunctionRegistry::~MFFunctionRegistry() { } // All other options apply to both `:number` and `:integer` - int32_t minIntegerDigits = number.minimumIntegerDigits(opts); + int32_t minIntegerDigits = number.minimumIntegerDigits(opts, status); nf = nf.integerWidth(IntegerWidth::zeroFillTo(minIntegerDigits)); // signDisplay @@ -405,62 +405,48 @@ static FormattedPlaceholder stringAsNumber(const number::LocalizedNumberFormatte return FormattedPlaceholder(input, FormattedValue(std::move(result))); } -int32_t StandardFunctions::Number::maximumFractionDigits(const FunctionOptions& opts) const { +int32_t StandardFunctions::Number::intValuedOption(const FunctionOptions& opts, + const UnicodeString& optionName, + int32_t defaultVal, + UErrorCode& status) const { + Formattable opt; + + if (opts.getFunctionOption(optionName, opt)) { + return static_cast(getInt64Value(locale, opt, status)); + } + return defaultVal; +} + +int32_t StandardFunctions::Number::maximumFractionDigits(const FunctionOptions& opts, UErrorCode& status) const { Formattable opt; if (isInteger) { return 0; } - if (opts.getFunctionOption(UnicodeString("maximumFractionDigits"), opt)) { - UErrorCode localErrorCode = U_ZERO_ERROR; - int64_t val = getInt64Value(locale, opt, localErrorCode); - if (U_SUCCESS(localErrorCode)) { - return static_cast(val); - } - } - return number::impl::kMaxIntFracSig; + return intValuedOption(opts, UnicodeString("maximumFractionDigits"), number::impl::kMaxIntFracSig, status); } -int32_t StandardFunctions::Number::minimumFractionDigits(const FunctionOptions& opts) const { +int32_t StandardFunctions::Number::minimumFractionDigits(const FunctionOptions& opts, UErrorCode& status) const { Formattable opt; - if (!isInteger) { - if (opts.getFunctionOption(UnicodeString("minimumFractionDigits"), opt)) { - UErrorCode localErrorCode = U_ZERO_ERROR; - int64_t val = getInt64Value(locale, opt, localErrorCode); - if (U_SUCCESS(localErrorCode)) { - return static_cast(val); - } - } + if (isInteger) { + return 0; } - return 0; + return intValuedOption(opts, UnicodeString("minimumFractionDigits"), 0, status); } -int32_t StandardFunctions::Number::minimumIntegerDigits(const FunctionOptions& opts) const { +int32_t StandardFunctions::Number::minimumIntegerDigits(const FunctionOptions& opts, UErrorCode& status) const { Formattable opt; - if (opts.getFunctionOption(UnicodeString("minimumIntegerDigits"), opt)) { - UErrorCode localErrorCode = U_ZERO_ERROR; - int64_t val = getInt64Value(locale, opt, localErrorCode); - if (U_SUCCESS(localErrorCode)) { - return static_cast(val); - } - } - return 0; + return intValuedOption(opts, UnicodeString("minimumIntegerDigits"), 0, status); } -int32_t StandardFunctions::Number::minimumSignificantDigits(const FunctionOptions& opts) const { +int32_t StandardFunctions::Number::minimumSignificantDigits(const FunctionOptions& opts, UErrorCode& status) const { Formattable opt; if (!isInteger) { - if (opts.getFunctionOption(UnicodeString("minimumSignificantDigits"), opt)) { - UErrorCode localErrorCode = U_ZERO_ERROR; - int64_t val = getInt64Value(locale, opt, localErrorCode); - if (U_SUCCESS(localErrorCode)) { - return static_cast(val); - } - } + return intValuedOption(opts, UnicodeString("minimumSignificantDigits"), 0, status); } // Returning 0 indicates that the option wasn't provided or was a non-integer. // The caller needs to check for that case, since passing 0 to Precision::minSignificantDigits() @@ -468,20 +454,13 @@ int32_t StandardFunctions::Number::minimumSignificantDigits(const FunctionOption return 0; } -int32_t StandardFunctions::Number::maximumSignificantDigits(const FunctionOptions& opts) const { +int32_t StandardFunctions::Number::maximumSignificantDigits(const FunctionOptions& opts, UErrorCode& status) const { Formattable opt; - if (opts.getFunctionOption(UnicodeString("maximumSignificantDigits"), opt)) { - UErrorCode localErrorCode = U_ZERO_ERROR; - int64_t val = getInt64Value(locale, opt, localErrorCode); - if (U_SUCCESS(localErrorCode)) { - return static_cast(val); - } - } - // Returning 0 indicates that the option wasn't provided or was a non-integer. + // Returning 0 indicates that the option wasn't provided. // The caller needs to check for that case, since passing 0 to Precision::maxSignificantDigits() // is an error. - return 0; // Not a valid value for Precision; has to be checked + return intValuedOption(opts, UnicodeString("maximumSignificantDigits"), 0, status); } bool StandardFunctions::Number::usePercent(const FunctionOptions& opts) const { diff --git a/icu4c/source/i18n/messageformat2_function_registry_internal.h b/icu4c/source/i18n/messageformat2_function_registry_internal.h index b34cb9b01421..84140e501dda 100644 --- a/icu4c/source/i18n/messageformat2_function_registry_internal.h +++ b/icu4c/source/i18n/messageformat2_function_registry_internal.h @@ -122,11 +122,12 @@ namespace message2 { static Number integer(const Locale& loc); // These options have their own accessor methods, since they have different default values. - int32_t maximumFractionDigits(const FunctionOptions& options) const; - int32_t minimumFractionDigits(const FunctionOptions& options) const; - int32_t minimumSignificantDigits(const FunctionOptions& options) const; - int32_t maximumSignificantDigits(const FunctionOptions& options) const; - int32_t minimumIntegerDigits(const FunctionOptions& options) const; + int32_t intValuedOption(const FunctionOptions&, const UnicodeString&, int32_t, UErrorCode&) const; + int32_t maximumFractionDigits(const FunctionOptions& options, UErrorCode& status) const; + int32_t minimumFractionDigits(const FunctionOptions& options, UErrorCode& status) const; + int32_t minimumSignificantDigits(const FunctionOptions& options, UErrorCode& status) const; + int32_t maximumSignificantDigits(const FunctionOptions& options, UErrorCode& status) const; + int32_t minimumIntegerDigits(const FunctionOptions& options, UErrorCode& status) const; bool usePercent(const FunctionOptions& options) const; const Locale& locale; diff --git a/icu4c/source/test/intltest/messageformat2test.cpp b/icu4c/source/test/intltest/messageformat2test.cpp index e674c94857bc..79a4050ffd04 100644 --- a/icu4c/source/test/intltest/messageformat2test.cpp +++ b/icu4c/source/test/intltest/messageformat2test.cpp @@ -768,6 +768,36 @@ void TestMessageFormat2::testResolutionErrors() { testRuntimeErrorPattern(++i, ".local $bar = {|42| ~plural} .match {|horse| :string} * {{{$bar}}}", U_MF_UNSUPPORTED_EXPRESSION_ERROR); + // Bad options. The spec is unclear about this -- see https://github.com/unicode-org/message-format-wg/issues/738 + // The current behavior is to set a U_MF_FORMATTING_ERROR for any invalid options. + testRuntimeErrorPattern(++i, + ".local $foo = {1 :number minimumIntegerDigits=-1} {{bar {$foo}}}", + U_MF_FORMATTING_ERROR); + testRuntimeErrorPattern(++i, + ".local $foo = {1 :number minimumIntegerDigits=foo} {{bar {$foo}}}", + U_MF_FORMATTING_ERROR); + testRuntimeErrorPattern(++i, + ".local $foo = {1 :number minimumFractionDigits=foo} {{bar {$foo}}}", + U_MF_FORMATTING_ERROR); + testRuntimeErrorPattern(++i, + ".local $foo = {1 :number maximumFractionDigits=foo} {{bar {$foo}}}", + U_MF_FORMATTING_ERROR); + testRuntimeErrorPattern(++i, + ".local $foo = {1 :number minimumSignificantDigits=foo} {{bar {$foo}}}", + U_MF_FORMATTING_ERROR); + testRuntimeErrorPattern(++i, + ".local $foo = {1 :number maximumSignificantDigits=foo} {{bar {$foo}}}", + U_MF_FORMATTING_ERROR); + testRuntimeErrorPattern(++i, + ".local $foo = {1 :integer minimumIntegerDigits=foo} {{bar {$foo}}}", + U_MF_FORMATTING_ERROR); + testRuntimeErrorPattern(++i, + ".local $foo = {1 :integer maximumSignificantDigits=foo} {{bar {$foo}}}", + U_MF_FORMATTING_ERROR); + + // Ideally this would cover all the options, but since there are no spec tests yet for other options, + // they are omitted. See https://github.com/unicode-org/message-format-wg/issues/752 + // Selector error // Here, the plural selector returns "no match" so the * variant matches testRuntimeWarningPattern(++i, ".match {|horse| :number}\n\