Skip to content

Fix #76093: Format strings w/o loss of precision w/ FORMAT_TYPE_DECIMAL #7782

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix #76093: Format strings w/o loss of precision w/ FORMAT_TYPE_DECIMAL #7782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a20342
Fix #76093: Format strings w/o loss of precision w/ FORMAT_TYPE_DECIMAL
cscott Jan 22, 2021
1520e11
Fix #76093, part 2: Format currency strings w/o loss of precision
cscott Jan 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions Zend/zend_API.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1324,6 +1324,8 @@ static zend_always_inline zval *zend_try_array_init(zval *zv)
_(Z_EXPECTED_ARRAY_OR_STRING_OR_NULL, "of type array|string|null") \
_(Z_EXPECTED_STRING_OR_LONG, "of type string|int") \
_(Z_EXPECTED_STRING_OR_LONG_OR_NULL, "of type string|int|null") \
_(Z_EXPECTED_STRING_OR_NUMBER, "of type string|int|float") \
_(Z_EXPECTED_STRING_OR_NUMBER_OR_NULL, "of type string|int|float|null") \
_(Z_EXPECTED_OBJECT_OR_CLASS_NAME, "an object or a valid class name") \
_(Z_EXPECTED_OBJECT_OR_CLASS_NAME_OR_NULL, "an object, a valid class name, or null") \
_(Z_EXPECTED_OBJECT_OR_STRING, "of type object|string") \
Expand Down Expand Up @@ -1922,6 +1924,17 @@ ZEND_API ZEND_COLD void zend_argument_value_error(uint32_t arg_num, const char *
#define Z_PARAM_STR_OR_LONG_OR_NULL(dest_str, dest_long, is_null) \
Z_PARAM_STR_OR_LONG_EX(dest_str, dest_long, is_null, 1);

#define Z_PARAM_STR_OR_NUMBER_EX(dest, check_null) \
Z_PARAM_PROLOGUE(0, 0); \
if (UNEXPECTED(!zend_parse_arg_str_or_number(_arg, &dest, check_null, _i))) { \
_expected_type = check_null ? Z_EXPECTED_STRING_OR_NUMBER_OR_NULL : Z_EXPECTED_STRING_OR_NUMBER; \
_error_code = ZPP_ERROR_WRONG_ARG; \
break; \
}

#define Z_PARAM_STR_OR_NUMBER(dest) \
Z_PARAM_STR_OR_NUMBER_EX(dest, 0)

/* End of new parameter parsing API */

/* Inlined implementations shared by new and old parameter parsing APIs */
Expand Down Expand Up @@ -2258,6 +2271,18 @@ static zend_always_inline bool zend_parse_arg_str_or_long(zval *arg, zend_string
return 1;
}

static zend_always_inline bool zend_parse_arg_str_or_number(zval *arg, zval **dest, bool check_null, uint32_t arg_num)
{
if (EXPECTED(Z_TYPE_P(arg) == IS_LONG || Z_TYPE_P(arg) == IS_DOUBLE || Z_TYPE_P(arg) == IS_STRING)) {
*dest = arg;
} else if (check_null && EXPECTED(Z_TYPE_P(arg) == IS_NULL)) {
*dest = NULL;
} else {
return zend_parse_arg_number_slow(arg, dest, arg_num);
}
return 1;
}

static zend_always_inline bool zend_parse_arg_obj_or_class_name(
zval *arg, zend_class_entry **destination, bool allow_null
) {
Expand Down
1 change: 1 addition & 0 deletions ext/intl/formatter/formatter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ void formatter_register_constants( INIT_FUNC_ARGS )
FORMATTER_EXPOSE_CUSTOM_CLASS_CONST( "TYPE_INT32", FORMAT_TYPE_INT32 );
FORMATTER_EXPOSE_CUSTOM_CLASS_CONST( "TYPE_INT64", FORMAT_TYPE_INT64 );
FORMATTER_EXPOSE_CUSTOM_CLASS_CONST( "TYPE_DOUBLE", FORMAT_TYPE_DOUBLE );
FORMATTER_EXPOSE_CUSTOM_CLASS_CONST( "TYPE_DECIMAL", FORMAT_TYPE_DECIMAL );
FORMATTER_EXPOSE_CUSTOM_CLASS_CONST( "TYPE_CURRENCY", FORMAT_TYPE_CURRENCY );

#undef FORMATTER_EXPOSE_CUSTOM_CLASS_CONST
Expand Down
4 changes: 2 additions & 2 deletions ext/intl/formatter/formatter.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public static function create(string $locale, int $style, ?string $pattern = nul
* @tentative-return-type
* @alias numfmt_format
*/
public function format(int|float $num, int $type = NumberFormatter::TYPE_DEFAULT): string|false {}
public function format(int|float|string $num, int $type = NumberFormatter::TYPE_DEFAULT): string|false {}

/**
* @param int $offset
Expand All @@ -30,7 +30,7 @@ public function parse(string $string, int $type = NumberFormatter::TYPE_DOUBLE,
* @tentative-return-type
* @alias numfmt_format_currency
*/
public function formatCurrency(float $amount, string $currency): string|false {}
public function formatCurrency(int|float|string $amount, string $currency): string|false {}

/**
* @param string $currency
Expand Down
6 changes: 3 additions & 3 deletions ext/intl/formatter/formatter_arginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* This is a generated file, edit the .stub.php file instead.
* Stub hash: f76ad76b08b7ca47883659fabfcc0882a2820c43 */
* Stub hash: 56694496058da02f52daf26a8f1588f604791f12 */

ZEND_BEGIN_ARG_INFO_EX(arginfo_class_NumberFormatter___construct, 0, 0, 2)
ZEND_ARG_TYPE_INFO(0, locale, IS_STRING, 0)
Expand All @@ -14,7 +14,7 @@ ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_OBJ_INFO_EX(arginfo_class_NumberFormatter_c
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_MASK_EX(arginfo_class_NumberFormatter_format, 0, 1, MAY_BE_STRING|MAY_BE_FALSE)
ZEND_ARG_TYPE_MASK(0, num, MAY_BE_LONG|MAY_BE_DOUBLE, NULL)
ZEND_ARG_TYPE_MASK(0, num, MAY_BE_LONG|MAY_BE_DOUBLE|MAY_BE_STRING, NULL)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, type, IS_LONG, 0, "NumberFormatter::TYPE_DEFAULT")
ZEND_END_ARG_INFO()

Expand All @@ -25,7 +25,7 @@ ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_MASK_EX(arginfo_class_NumberFormatter_
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_MASK_EX(arginfo_class_NumberFormatter_formatCurrency, 0, 2, MAY_BE_STRING|MAY_BE_FALSE)
ZEND_ARG_TYPE_INFO(0, amount, IS_DOUBLE, 0)
ZEND_ARG_TYPE_MASK(0, amount, MAY_BE_LONG|MAY_BE_DOUBLE|MAY_BE_STRING, NULL)
ZEND_ARG_TYPE_INFO(0, currency, IS_STRING, 0)
ZEND_END_ARG_INFO()

Expand Down
89 changes: 77 additions & 12 deletions ext/intl/formatter/formatter_format.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,22 @@ PHP_FUNCTION( numfmt_format )
int32_t formatted_len = USIZE(format_buf);
FORMATTER_METHOD_INIT_VARS;

object = getThis();

/* Parse parameters. */
if( zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "On|l",
&object, NumberFormatter_ce_ptr, &number, &type ) == FAILURE )
{
RETURN_THROWS();
if (object) {
ZEND_PARSE_PARAMETERS_START(1, 2)
Z_PARAM_STR_OR_NUMBER(number)
Z_PARAM_OPTIONAL
Z_PARAM_LONG(type)
ZEND_PARSE_PARAMETERS_END();
} else {
ZEND_PARSE_PARAMETERS_START(2, 3)
Z_PARAM_OBJECT_OF_CLASS(object, NumberFormatter_ce_ptr)
Z_PARAM_STR_OR_NUMBER(number)
Z_PARAM_OPTIONAL
Z_PARAM_LONG(type)
ZEND_PARSE_PARAMETERS_END();
Comment on lines -38 to +52
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this case it might make sense to stick with zend_parse_method_parameters(), but replace the n specifier with z, and to replace the EMPTY_SWITCH_DEFAULT_CASE() some lines below to throw a type error manually.

}

/* Fetch the object. */
Expand All @@ -53,10 +64,21 @@ PHP_FUNCTION( numfmt_format )
case IS_DOUBLE:
type = FORMAT_TYPE_DOUBLE;
break;
case IS_STRING:
type = FORMAT_TYPE_DECIMAL;
break;
EMPTY_SWITCH_DEFAULT_CASE();
}
}

// Avoid losing precision on 32-bit platforms where PHP's "long" isn't
// as long as the FORMAT_TYPE_INT64 which is requested.
if(sizeof(zend_long) < 8) {
if (Z_TYPE_P(number) == IS_STRING && type == FORMAT_TYPE_INT64) {
type = FORMAT_TYPE_DECIMAL;
}
}
Comment on lines +76 to +80
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should better be a compile directive:

Suggested change
if(sizeof(zend_long) < 8) {
if (Z_TYPE_P(number) == IS_STRING && type == FORMAT_TYPE_INT64) {
type = FORMAT_TYPE_DECIMAL;
}
}
#if SIZEOF_ZEND_LONG < 8
if (Z_TYPE_P(number) == IS_STRING && type == FORMAT_TYPE_INT64) {
type = FORMAT_TYPE_DECIMAL;
}
#endif


switch(type) {
case FORMAT_TYPE_INT32:
convert_to_long(number);
Expand All @@ -76,7 +98,13 @@ PHP_FUNCTION( numfmt_format )

case FORMAT_TYPE_INT64:
{
int64_t value = (Z_TYPE_P(number) == IS_DOUBLE)?(int64_t)Z_DVAL_P(number):Z_LVAL_P(number);
int64_t value;
if (Z_TYPE_P(number) == IS_DOUBLE) {
value = (int64_t)Z_DVAL_P(number);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, this conversion is undefined behavior if the double value is outside the range of int64_t. However, we already do that prior to this patch, so it may be okay for all supported platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could be justified in broadening the expected test result if platforms really do crazy things with this conversion. But IMO if you've asked for TYPE_INT64 you are explicitly asking for the result of casting the double to the int64, however that's defined on your platform. And as you note, this isn't new behavior in this patch, this is exactly how it worked previously.

Copy link
Contributor Author

@cscott cscott Dec 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it turns out, your comment was prescient: the s390 architecture got +9223372036854775808 instead of -9223372036854775808 as a result of that cast (https://app.travis-ci.com/github/php/php-src/jobs/552554610). So I've tweaked the test to allow any value for this case.

} else {
convert_to_long(number);
value = Z_LVAL_P(number);
}
formatted_len = unum_formatInt64(FORMATTER_OBJECT(nfo), value, formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
if (INTL_DATA_ERROR_CODE(nfo) == U_BUFFER_OVERFLOW_ERROR) {
intl_error_reset(INTL_DATA_ERROR_P(nfo));
Expand Down Expand Up @@ -104,6 +132,23 @@ PHP_FUNCTION( numfmt_format )
INTL_METHOD_CHECK_STATUS( nfo, "Number formatting failed" );
break;

case FORMAT_TYPE_DECIMAL:
if (!try_convert_to_string(number)) {
RETURN_THROWS();
}
// Convert string as a DecimalNumber, so we don't lose precision
formatted_len = unum_formatDecimal(FORMATTER_OBJECT(nfo), Z_STRVAL_P(number), Z_STRLEN_P(number), formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
if (INTL_DATA_ERROR_CODE(nfo) == U_BUFFER_OVERFLOW_ERROR) {
intl_error_reset(INTL_DATA_ERROR_P(nfo));
formatted = eumalloc(formatted_len);
unum_formatDecimal(FORMATTER_OBJECT(nfo), Z_STRVAL_P(number), Z_STRLEN_P(number), formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
if (U_FAILURE( INTL_DATA_ERROR_CODE(nfo) ) ) {
efree(formatted);
}
}
INTL_METHOD_CHECK_STATUS( nfo, "Number formatting failed" );
break;

default:
zend_argument_value_error(3, "must be a NumberFormatter::TYPE_* constant");
RETURN_THROWS();
Expand All @@ -116,7 +161,7 @@ PHP_FUNCTION( numfmt_format )
/* {{{ Format a number as currency. */
PHP_FUNCTION( numfmt_format_currency )
{
double number;
zval *number;
UChar format_buf[32];
UChar* formatted = format_buf;
int32_t formatted_len = USIZE(format_buf);
Expand All @@ -126,11 +171,20 @@ PHP_FUNCTION( numfmt_format_currency )
int32_t scurrency_len = 0;
FORMATTER_METHOD_INIT_VARS;

object = getThis();

/* Parse parameters. */
if( zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "Ods",
&object, NumberFormatter_ce_ptr, &number, &currency, &currency_len ) == FAILURE )
{
RETURN_THROWS();
if (object) {
ZEND_PARSE_PARAMETERS_START(2, 2)
Z_PARAM_STR_OR_NUMBER(number)
Z_PARAM_STRING(currency, currency_len)
ZEND_PARSE_PARAMETERS_END();
} else {
ZEND_PARSE_PARAMETERS_START(3, 3)
Z_PARAM_OBJECT_OF_CLASS(object, NumberFormatter_ce_ptr)
Z_PARAM_STR_OR_NUMBER(number)
Z_PARAM_STRING(currency, currency_len)
ZEND_PARSE_PARAMETERS_END();
}

/* Fetch the object. */
Expand All @@ -139,9 +193,16 @@ PHP_FUNCTION( numfmt_format_currency )
/* Convert currency to UTF-16. */
intl_convert_utf8_to_utf16(&scurrency, &scurrency_len, currency, currency_len, &INTL_DATA_ERROR_CODE(nfo));
INTL_METHOD_CHECK_STATUS( nfo, "Currency conversion to UTF-16 failed" );
unum_setTextAttribute(FORMATTER_OBJECT(nfo), UNUM_CURRENCY_CODE, scurrency, scurrency_len, &INTL_DATA_ERROR_CODE(nfo));
INTL_METHOD_CHECK_STATUS( nfo, "Setting currency code failed" );

/* Format the number using a fixed-length buffer. */
formatted_len = unum_formatDoubleCurrency(FORMATTER_OBJECT(nfo), number, scurrency, formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
if (Z_TYPE_P(number) == IS_STRING) {
formatted_len = unum_formatDecimal(FORMATTER_OBJECT(nfo), Z_STRVAL_P(number), Z_STRLEN_P(number), formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
} else {
convert_to_double(number);
formatted_len = unum_formatDoubleCurrency(FORMATTER_OBJECT(nfo), Z_DVAL_P(number), scurrency, formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
}

/* If the buffer turned out to be too small
* then allocate another buffer dynamically
Expand All @@ -150,7 +211,11 @@ PHP_FUNCTION( numfmt_format_currency )
if (INTL_DATA_ERROR_CODE(nfo) == U_BUFFER_OVERFLOW_ERROR) {
intl_error_reset(INTL_DATA_ERROR_P(nfo));
formatted = eumalloc(formatted_len);
unum_formatDoubleCurrency(FORMATTER_OBJECT(nfo), number, scurrency, formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
if (Z_TYPE_P(number) == IS_STRING) {
unum_formatDecimal(FORMATTER_OBJECT(nfo), Z_STRVAL_P(number), Z_STRLEN_P(number), formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
} else {
unum_formatDoubleCurrency(FORMATTER_OBJECT(nfo), Z_DVAL_P(number), scurrency, formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
}
}

if( U_FAILURE( INTL_DATA_ERROR_CODE((nfo)) ) ) {
Expand Down
1 change: 1 addition & 0 deletions ext/intl/formatter/formatter_format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,6 @@
#define FORMAT_TYPE_INT64 2
#define FORMAT_TYPE_DOUBLE 3
#define FORMAT_TYPE_CURRENCY 4
#define FORMAT_TYPE_DECIMAL 5

#endif // FORMATTER_FORMAT_H
4 changes: 2 additions & 2 deletions ext/intl/php_intl.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,12 +225,12 @@ function datefmt_get_error_message(IntlDateFormatter $formatter): string {}

function numfmt_create(string $locale, int $style, ?string $pattern = null): ?NumberFormatter {}

function numfmt_format(NumberFormatter $formatter, int|float $num, int $type = NumberFormatter::TYPE_DEFAULT): string|false {}
function numfmt_format(NumberFormatter $formatter, int|float|string $num, int $type = NumberFormatter::TYPE_DEFAULT): string|false {}

/** @param int $offset */
function numfmt_parse(NumberFormatter $formatter, string $string, int $type = NumberFormatter::TYPE_DOUBLE, &$offset = null): int|float|false {}

function numfmt_format_currency(NumberFormatter $formatter, float $amount, string $currency): string|false {}
function numfmt_format_currency(NumberFormatter $formatter, int|float|string $amount, string $currency): string|false {}

/**
* @param string $currency
Expand Down
4 changes: 2 additions & 2 deletions ext/intl/php_intl_arginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_numfmt_format, 0, 2, MAY_BE_STRING|MAY_BE_FALSE)
ZEND_ARG_OBJ_INFO(0, formatter, NumberFormatter, 0)
ZEND_ARG_TYPE_MASK(0, num, MAY_BE_LONG|MAY_BE_DOUBLE, NULL)
ZEND_ARG_TYPE_MASK(0, num, MAY_BE_LONG|MAY_BE_DOUBLE|MAY_BE_STRING, NULL)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, type, IS_LONG, 0, "NumberFormatter::TYPE_DEFAULT")
ZEND_END_ARG_INFO()

Expand All @@ -388,7 +388,7 @@ ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_numfmt_format_currency, 0, 3, MAY_BE_STRING|MAY_BE_FALSE)
ZEND_ARG_OBJ_INFO(0, formatter, NumberFormatter, 0)
ZEND_ARG_TYPE_INFO(0, amount, IS_DOUBLE, 0)
ZEND_ARG_TYPE_MASK(0, amount, MAY_BE_LONG|MAY_BE_DOUBLE|MAY_BE_STRING, NULL)
ZEND_ARG_TYPE_INFO(0, currency, IS_STRING, 0)
ZEND_END_ARG_INFO()

Expand Down
38 changes: 34 additions & 4 deletions ext/intl/tests/bug48227.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,50 @@ intl
--FILE--
<?php

$testcases = ['', 1, NULL, true, false, [], (object)[]];

echo("OOP\n");
$x = new NumberFormatter('en_US', NumberFormatter::DECIMAL);
foreach (['', 1, NULL, $x] as $value) {
foreach (array_merge($testcases, [$x]) as $value) {
try {
var_dump($x->format($value));
} catch (TypeError $ex) {
echo $ex->getMessage(), PHP_EOL;
}
}

echo("\nNon-OOP\n");
$x = numfmt_create('en_US', NumberFormatter::DECIMAL);
foreach (array_merge($testcases, [$x]) as $value) {
try {
var_dump(numfmt_format($x, $value));
} catch (TypeError $ex) {
echo $ex->getMessage(), PHP_EOL;
}
}

?>
--EXPECTF--
NumberFormatter::format(): Argument #1 ($num) must be of type int|float, string given
OOP
bool(false)
string(1) "1"

Deprecated: NumberFormatter::format(): Passing null to parameter #1 ($num) of type int|float is deprecated in %s on line %d
Deprecated: NumberFormatter::format(): Passing null to parameter #1 ($num) of type string|int|float is deprecated in %s on line %d
string(1) "0"
string(1) "1"
string(1) "0"
NumberFormatter::format(): Argument #1 ($num) must be of type string|int|float, array given
NumberFormatter::format(): Argument #1 ($num) must be of type string|int|float, stdClass given
NumberFormatter::format(): Argument #1 ($num) must be of type string|int|float, NumberFormatter given

Non-OOP
bool(false)
string(1) "1"

Deprecated: numfmt_format(): Passing null to parameter #2 ($num) of type string|int|float is deprecated in %s on line %d
string(1) "0"
string(1) "1"
string(1) "0"
NumberFormatter::format(): Argument #1 ($num) must be of type int|float, NumberFormatter given
numfmt_format(): Argument #2 ($num) must be of type string|int|float, array given
numfmt_format(): Argument #2 ($num) must be of type string|int|float, stdClass given
numfmt_format(): Argument #2 ($num) must be of type string|int|float, NumberFormatter given
Loading