Skip to content

Commit

Permalink
ICU-22747 MF2: Validate values of integer-valued options for :number …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@catamorphism
catamorphism committed Apr 18, 2024
1 parent 43cde6b commit 1e0e151
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 56 deletions.
81 changes: 30 additions & 51 deletions icu4c/source/i18n/messageformat2_function_registry.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -405,83 +405,62 @@ 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<int32_t>(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<int32_t>(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<int32_t>(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<int32_t>(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<int32_t>(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()
// is an error.
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<int32_t>(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 {
Expand Down
11 changes: 6 additions & 5 deletions icu4c/source/i18n/messageformat2_function_registry_internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
30 changes: 30 additions & 0 deletions icu4c/source/test/intltest/messageformat2test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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\
Expand Down

0 comments on commit 1e0e151

Please sign in to comment.