feat: Add sprintf in the list of compiler optimized functions

[stof]

A compiler optimization has been added for sprintf in PHP 8.4.

[coveralls]

coverage: 95.168%. remained the same
when pulling b7cc7bc on stof:patch-2
into fa3b1ed on PHP-CS-Fixer:master.

[coveralls]

coverage: 95.168%. remained the same
when pulling 143e17a on stof:patch-2
into fa3b1ed on PHP-CS-Fixer:master.

[mvorisek]

what about other functions like vsprintf?

[stof]

the optimization has only been implemented for sprintf. There is no optimization for vsprintf for now

[mvorisek]

I have questioned this in php/php-src#14546 (comment)

[stof]

My guess is that vsprintf is harder to transform into a concatenation because the compiler sees a single argument (the array of values) and has no easy way to destructure it at compile time.

Thus, to me, the use case for vsprintf compared to sprintf is likely to be the case where the array of values comes from a variable (using vsprintf with an array literal is just adding the overhead of creating an array and then destroying it, compared to sprintf). And the case of a variable would not be optimizable anyway as you cannot destructure it at compile time as it requires having the variable value.

[Wirone]

Quick questions before I scroll through all leading \ in ~200 files 😅.

[Wirone]

Shouldn't we have test case for sprintf to see whether \ was added?

[stof]

the test is not testing each of the compiler optimized functions explicitly, so I haven't added a new test for this function.

[Wirone]

I know there wasn't such a convention before, but wouldn't it make sense to add sprintf only if PHP >= 8.4? For previous versions it does not add value, so probably people running older PHP versions not necessarily want to apply potentially huge diff that does not improve performance for them?

[mvorisek]

I do not support this idea, as running PHP CS Fixer in different environments is even worse - if not detected from composer.json of course.

[Wirone]

@mvorisek I don't understand what you said. The suggested usage of Fixer is to run it on the PHP version you use for your app. So if the app is still on 7.4 for example, there's no point in applying \ to sprintf as it does not bring the performance boost, but can lead to sudden CI crashes when Fixer is updated.

[mvorisek]

I mean if we condition it on executable PHP version, I do not want different fixer output locally vs. CI when the minor PHP version does not match.

[stof]

@Wirone open-source package will need to support multiple versions of PHP. They don't have a single target PHP version. And they should be fully-qualifying calls to sprintf to benefit the subset of their users running on PHP >=8.4 even when their min target version is lower than 8.4 (qualifying the call does not break code on PHP 8.3. It simply does not unlock a performance optimization there)

[Wirone]

@mvorisek but there are rules that behave differently depending on PHP runtime, that's why I wanted to discuss it.

@stof yeah, for packages it's different, but still it'll affect codebases with pinned PHP versions. I don't want to enforce this change, just wanted to figure out the best approach.

[mvorisek]

@mvorisek but there are rules that behave differently depending on PHP runtime, that's why I wanted to discuss it.

The desired behaviour is too have PHP CS Fixer stable across PHP runtime versions once the codebase is fixed for the highest supported version. This is quite working for now for ex. with attributes which are ignored in lower PHP versions.

If we really want sprintf to condition based on PHP version, we will have to add it for PHP >=8.4 version and allow both variants below such version if configured so.

If this is too complicated, we must either always fix it or not.

[keradus]

there is no harm of applying \ on PHP7 etc, apply it regardless of runtime.

[stof]

@Wirone if you want a smaller diff, check the first commit. I intentionally kept the run of PHP-CS-Fixer in a separate commit to make the review easier.