Php-cs-fixer: Add a rule to root-namespace special compiled PHP functions

[nicolas-grekas]

Since PHP 7.0, some functions are replaced by opcodes, producing much faster code.
Yet, for this to work, these functions need to be referenced in the root namespace at compile time:
Either there is no namespace, or they are prefixed by a \.
php-cs-fixer should have a rule to list those functions and enforce adding this \ when used inside a namespaced file/class.

Here is the exact list of functions that have this behavior:
https://github.com/php/php-src/blob/f2db305fa4e9bd7d04d567822687ec714aedcdb5/Zend/zend_compile.c#L3872

(don't miss https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3048#issuecomment-332784692)

[bendavies]

I was just about to raise this. you beat me to it :)

[bendavies]

btw, this is already covered by the native_function_invocation rule:
https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/master/src/Fixer/FunctionNotation/NativeFunctionInvocationFixer.php

but as you said, it could have extra configuration to only replace the function calls you link to.

[keradus]

looks like a nice scenario
please open a PR to improve configuration of the fixer @nicolas-grekas :+1:

[SpacePossum]

"array_slice"
"assert"
"boolval"
"call_user_func"
"call_user_func_array"
"chr"
"count"
"defined"
"doubleval"
"floatval"
"func_get_args"
"func_num_args"
"get_called_class"
"get_class"
"gettype"
"in_array"
"intval"
"is_array"
"is_bool"
"is_double"
"is_float"
"is_int"
"is_integer"
"is_long"
"is_null"
"is_object"
"is_real"
"is_resource"
"is_string"
"ord"
"strlen"
"strval"

IIUC

[nicolas-grekas]

@SpacePossum correct, ~with some subtleties if we want to deal with them (just discovered that eg in_array is optimized only if its 3rd arg is set~)

[keradus]

haha, next scenario when it's always better to use strict comparisons ;)

[keradus]

@nicolas-grekas

native_function_invocation adds \ to each native function invocation.
this makes them to be handled faster, as some of them will be converted to opcodes, and the rest will be resolved immediately as root function instead of having a need to check if the function is also registered in current namespace.

is there anything left for this fixer ?

[nicolas-grekas]

For the purpose of Symfony, I may advocate that a rule that enforces the backslash when it provides something substential should be enabled.
But I will never advocate that all functions should be prefixed.
Is that what the linked PR provides? Would it be possible to select/configure the list of root functions that should be prefixed? If yes, then we might have a deal :)

[SpacePossum]

I have a branch that will provide what you are after, however it is built upon the one linked PR, the linked PR itself does not.

[nicolas-grekas]

Cool, thanks for doing the hard work on this.

[keradus]

@nicolas-grekas , what about later part?

and the rest will be resolved immediately as root function instead of having a need to check if the function is also registered in current namespace

[nicolas-grekas]

@keradus not sure what you mean? If you mean devs should be required to always add the backslash prefix, I certainly don't agree, there is zero perf benefit doing so, but only more visual debt in the code...

[keradus]

I'm saying that there is benefit, as if you add \ to all native functions, you will get benefit of php not needed to check if you have function in given namespace, as you explicitly say you don't care about current namespace, but you care about root-level native function. so there is benefit even if function call cannot be converted to opcode

[nicolas-grekas]

Did you try measuring it actually? Last time I did, I couldn't spot any difference at all, except for the specific functions listed above.

[keradus]

• wondering: https://twitter.com/Ocramius/status/811504929357660160
• indicator that it might make a difference: http://php.net/manual/en/language.namespaces.faq.php#language.namespaces.faq.shortname2 (basically what I said for 2-steps lookup)
• some benchmark: http://veewee.github.io/blog/optimizing-php-performance-by-fq-function-calls/
• some other benchmark even with \-adder https://github.com/Roave/FunctionFQNReplacer
• opcode difference between non-\ vs \ is INIT_NS_FCALL_BY_NAME vs INIT_FCALL and so (totally different opcodes of resolving function, passing arguments and calling functoin)

[SpacePossum]

If it does make a difference for all calls:

• why doesn't it show in the op codes?
• why is this fixer inconsistent, i.e. only fix native function calls? https://3v4l.org/Z0NuO/vld#output shows calling user defined functions in global namespace also benefit from the \

Why can't this be a different issue report so the same discussion is not done two times, here and in the PR? This issue is about a special subset of native calls, the PR about scoping the namespacing, I don't see the need to widen the scope.

[keradus]

why doesn't it show in the op codes?

it does, like INIT_NS_FCALL_BY_NAME vs INIT_FCALL
amount of opcodes are the same, but the opcodes itself are different

why is this fixer inconsistent, i.e. only fix native function calls?

As in scope of file, we are unaware of all user-defined functions :( we know for sure that native functions are defined in root scope and hope they are not overwritten in namespace (which could be in different file). We cannot be sure that user-defined function is declared in root scope.

Why can't this be a different issue report so the same discussion is not done two times (...)

For sure it's worth to not work on it twice, yet before implementing I want to be sure this part it's needed (configuring to fix only subset). Thus I took discussion-first approach.

[nicolas-grekas]

On the very first call, opcache resolves the namespace once for all and replaces the slower opcode by the fully qualified one. This hot replacement is free compared to running whatever the rest of the code does.
The benchmark you linked doesn't say anything about opcache being enabled or not. The php --version doesn't show it, and CLI disables it by default, so it's likely it was not enabled.
In short, I don't buy it at all. And the other links prove absolutely nothing.

[keradus]

you see, it's not hard to discuss ;)
in that case, feel free to open a PR @nicolas-grekas

[nicolas-grekas]

@keradus ??????

[keradus]

what's up?
here, I challenged current fixer we do have and reasoning we had behind him.
with your last reasoning, I think we could change default behaviour of it to fix only opcache-friendly functions by default (3.x line)

[leofeyer]

There is the exclude option already, so why not add another configuration option in the 2.x line?

[
    'native_function_invocation' => [
        'exclude' => [],
        'opcache-only' => true,
    ]
]

That would be forward compatible with version 3.

[SpacePossum]

or add an include option, WIP https://github.com/SpacePossum/PHP-CS-Fixer/commit/2490fb1694dcebaa8cb8e14d971d009ecd13af07

[nicolas-grekas]

Here is new stuff I just learned about from @jpauli: OPcache does additional inlining, see
https://github.com/php/php-src/blob/master/ext/opcache/Optimizer/pass1_5.c

extra list is:

• function_exists
• is_callable
• extension_loaded
• dirname
• constant
• define

[SpacePossum]

Thanks, I'll update the functions list in my branch :)

Btw. my POC PR @ SF did already only change the subset of functions as listed here and only within a user-defined-namespace.

[leofeyer]

I have added a PR that implements the opcache-only option: #3222

[theofidry]

Maybe instead of escaping native functions what about adding them with a use statement?

use function is_array;

is_array($x);

// Instead of:

\is_array($x);

It feels way more natural and make it easier to read IMO. Also for reference this is already used in https://github.com/doctrine/coding-standard/pull/15 and you also can enable it via your IDE (cf the doctrine coding standard issue)

[stof]

@theofidry this cannot be done in the LTS branch of Symfony, as it makes the code incompatible with PHP 5.x. Using the fully-qualified function name works on PHP 5.3+ (it won't have the same optimizations on older versions, but that's not an issue). So Symfony is more interested in qualifying the call (making this configurable is an option though)

[theofidry]

Sure, but not everyone is stuck with that 5.x compatibility issue. I simply worry that everyone will jump on that option when available making a lot of code unreadable for no reason; Ideally a choice between the two options would be given so that depending of your needs you can pick the right solution.

Also I would argue you should use use, use const & use function for anything, not just to optimise some native calls

[stof]

@theofidry the SF coding standards are also to avoid the class use statement for the global namespace btw, so using \ for function would even be consistent. So this definitely need to be cpnfigurable IMO.

[gggeek]

Maybe it's just the mental burden of having to abandon long time habits, but the idea of having to prefix usage of every "standard" function call (or even just a small but not negligible part of them) makes me shudder. The solution proposed by @theofidry seems much cleaner. Or maybe just wait for the php engine to implement a pragma that disables dynamic function name resolution for the current file and have this fixed with a one-liner change...

[SpacePossum]

We can offer both options and than people/communities can decide what style they want to us,
this typically works best with high opinionated CS preferences (like Yoda-style as well ;)
It would move the discussion about the CS to the communities and away from here where the exposure might be less visible.

[derrabus]

Or maybe just wait for the php engine to implement a pragma that disables dynamic function name resolution for the current file

We don't know if that will ever happen. And even if, it would take years until that feature will be usable for frameworks and libraries. That does not sound like a good plan.

[gggeek]

"We don't know if that will ever happen" => sure, that was more of a tongue-in-cheek joke than a real proposal ;-)

[glensc]

@nicolas-grekas pointed out to this issue, that not all functions are slashed by the fixer.

https://github.com/symfony/polyfill-mbstring/commit/3296adf6a6454a050679cde90f95350ad604b171

• \floor
• \gettype
• \strpos
• \iconv
• \trigger_error

[ntzm]

Are they compiler optimised functions?

[stof]

@glensc this feature request is precisely about not prefixing them all, but prefixing only the compiler-optimized ones. Prefixing them all is already possible (but is not what the Symfony team wants to do).

[gharlan]

Maybe instead of escaping native functions what about adding them with a use statement?

use function is_array;

is_array($x);

// Instead of:

\is_array($x);

I would like to use this style. Should it be an option in this fixer? Or maybe better another fixer, something like import_root_namespaced_functions?

[jaydiablo]

@gharlan There's some discussion about that in this issue: https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/2739

Seems like it needs to be decided if it's an option to the native_function_invocation fixer, or a separate fixer, as you suggested.

[gharlan]

@gharlan There's some discussion about that in this issue: #2739

Ah thanks! 👍