Add NativeConstantInvocationFixer #3127

Slamdunk · 2017-10-04T14:56:53Z

Following NativeFunctionInvocationFixer here I propose:

-echo M_PI;
+echo \M_PI;

VLD before: https://3v4l.org/43Sci/vld
VLD after: https://3v4l.org/FXkg2/vld

Slamdunk · 2017-10-04T14:59:18Z

src/Fixer/ConstantNotation/NativeConstantInvocationFixer.php

+            }
+
+            $constantNamePrefix = $tokens->getPrevMeaningfulToken($index);
+            if ($tokens[$constantNamePrefix]->isGivenKind([CT::T_USE_TRAIT, T_CLASS, T_CONST, T_DOUBLE_COLON, T_EXTENDS, T_FUNCTION, T_IMPLEMENTS, T_INTERFACE, T_NAMESPACE, T_NEW, T_NS_SEPARATOR, T_OBJECT_OPERATOR, T_TRAIT, T_USE])) {


I don't like this whitelist very much, it's too brittle: suggestions welcome

Slamdunk · 2017-10-04T15:20:55Z

There was 1 failure:

1) PhpCsFixer\Tests\Fixer\ConstantNotation\NativeConstantInvocationFixerTest::testFixWithDefaultConfiguration with data set #3 ('<?php namespace M_PI; use M_P...n; } }')
Failed asserting that 'PHP Fatal error:  Cannot declare class M_PI\M_PI because the name is already in use in /home/travis/build/FriendsOfPHP/PHP-CS-Fixer/cs_fixer_tmp_R5f8K8 on line 1

Source:
<?php namespace M_PI; use M_PI; class M_PI extends M_PI implements M_PI { const M_PI = 1; use M_PI; public $M_PI = 1; public function M_PI($M_PI) { $M_PI = M_PI(); return; } }' is null.

/home/travis/build/FriendsOfPHP/PHP-CS-Fixer/tests/Test/AbstractFixerTestCase.php:175
/home/travis/build/FriendsOfPHP/PHP-CS-Fixer/tests/Fixer/ConstantNotation/NativeConstantInvocationFixerTest.php:109

I'll fix this tomorrow, splitting all the stuff in separate cases.

keradus · 2017-10-04T16:09:18Z

cc @nicolas-grekas , you might find this usefull

nicolas-grekas · 2017-10-04T16:12:18Z

Can we set a list for the ones we'd like to always namespace?
I don't thing we will want to add a \ to all of them in Symfony, but only to a selected few that could contribute to OPcache dead code elimination (eg PHP_VERSION_ID)

keradus · 2017-10-04T16:28:06Z

so, you would say there is no need to add \ for most of the constants in terms of performance ?

nicolas-grekas · 2017-10-04T16:29:58Z

I think I will not advocate that the maybe perf benefit is worth the noise when writing code yes.

keradus · 2017-10-04T16:33:46Z

Then maybe try to add a combined behavior described in other ticket you raised @nicolas-grekas

option to add \ to preset constants where it makes sense (like that PHP_VERSION_ID, need to find all)
option to change all constants to be applied only before phar building

?

Slamdunk · 2017-10-05T06:58:41Z

I think I will not advocate that the maybe perf benefit is worth the noise when writing code yes.

I have to say I don't understand this statement. In my humble opinion:

The extra \ is always a pain in the *** in therm of noise when writing code: since the introduction of NativeFunctionInvocationFixer I have never seen a single programmer adding it manually, the automatism of this tool is here for this very purpose. Since the introduction of this PHP-CS-Fixer I have ended ordering use import manually, taking care of newlines, etc: the code deserves only my creativity, not my attention to this smallnesses.
Stated the first point, since all Symfony code (afaik) is all namespaced there will always be a performance improvement.
To me it will be very confusing to have half constants escaped and half not (exclude list apart): I am against this approach, if you don't like it don't use the fixer.

As far as I can see, there is no difference, in theory and in practice, between this fixer and NativeFunctionInvocationFixer, and shouldn't be.

keradus · 2017-10-05T13:55:35Z

ref #3048
for functions: there is a difference between adding ` everywhere and for subset of functions that are boosted by compiler to opcode directly. from what I understand from @nicolas-grekas it's somehow similar here

Slamdunk · 2017-10-05T14:31:07Z

I admit I know nothing about php source code and opcode sources.
But here are my tests:

$ php71 -r 'namespace A\B\C; for ($i=0;$i<100000000;++$i) {M_PI;}'

real    0m1.545s
user    0m1.541s
sys     0m0.002s

$ php71 -r 'namespace A\B\C; for ($i=0;$i<100000000;++$i) {\M_PI;}'

real    0m0.905s
user    0m0.903s
sys     0m0.000s

Repeated tests always show noticeable improvements around 40%, and with all constants.

Slamdunk · 2017-10-05T14:34:07Z

Tests do the same with opcache present, opcache.enable=1, opcache.enable_cli=1

nicolas-grekas · 2017-10-05T15:20:52Z

Writing is one thing, reading another. Reading a code full of \ is no go to me.
I'm very thankful to the PHP internal team that they didn't make the \ a requirement, PHP would become a joke to read.
The benchmark isn't representative of anything. I know no app that access PI 100M times.
A bench on a real app would be the only way to prove this could be worth the huge readability downgrade.

Slamdunk · 2017-10-05T15:42:34Z

I understand your point, and I agree this fixer should be adopted only where it makes sense.

This library access T_* constants millions of times, I would be very happy to apply my fixers in half the time.

Slamdunk · 2017-11-08T13:45:15Z

@nicolas-grekas continuing what written in #3222 we could use the $categorize parameter in get_defined_constants to get this fixer easy to configure and to escape only the certain constants:

print_r(array_keys(get_defined_constants(true)));
/*
Array
(
    [0] => Core
    [1] => date
    [2] => libxml
    [3] => pcre
    [4] => sqlite3
    [5] => dom
    [6] => fileinfo
    [7] => filter
    [8] => hash
    [9] => iconv
    [10] => json
    [11] => posix
    [12] => session
    [13] => standard
    [14] => tokenizer
    [15] => xml
    [16] => openssl
    [17] => curl
    [18] => gd
    [19] => imap
    [20] => mbstring
    [21] => mysqli
    [22] => zlib
)
*/

By default only Core would be fixed, but the user could specify a different subset, or all of them.

WDYT?

nicolas-grekas · 2017-11-08T13:48:56Z

I can't tell for everyone else, but on Symfony, we might just cherry-pick a few constants only (and not by "group"), less than 10 for sure (DIRECTORY_SEPARATOR, PHP_VERSION_ID, PHP_SAPI, maybe no more). I don't know how to help much more sorry.

Slamdunk · 2017-11-08T14:05:52Z

Symfony is a leader project not only for its adoption, but also for the good standards it spreads and all the edge cases it faces.

I am willing to make this fixer configurable this way:

Escape all the constants
Escape a custom subset of constants
Escape the Symfony sybset of constants

@nicolas-grekas would you be so kind to research what would be the Symfony needs and list an "official" Symfony list of the constants to be escaped?

Slamdunk · 2017-11-10T14:01:58Z

If can be of any help, at current commit at master
https://github.com/symfony/symfony/tree/61b753443d6903d8a9644f62caf020fdfca3ef6f

EDITED

Excluding the /Tests/ folders, the same count gives, the constants with 10 or more occurrences in all symfony are:

Constant	Count
DIRECTORY_SEPARATOR	95
E_USER_DEPRECATED	22
PHP_INT_MAX	18
PHP_SAPI	16
PHP_EOL	14
E_ERROR	12
PATHINFO_EXTENSION	11
STDIN	10

Of course a constants may occur only one time but can be called a lot of times, but still this may help.

nicolas-grekas · 2017-11-10T14:05:46Z

Cool, thanks for the analysis. PHP_VERSION_ID is missing, maybe because you did that on master? What about 3.4?
Of those constants, we should select only those involved in conditional branching.
That means DIRECTORY_SEPARATOR, PHP_SAPI and PHP_VERSION_ID only.

Slamdunk · 2017-11-10T14:19:23Z

Yes the previous statistic was for master, where PHP_VERSION_ID appears only 7 times.
On 3.4 we have (still excluding /Tests/):

Constant	Count
E_USER_DEPRECATED	352
DIRECTORY_SEPARATOR	115
PHP_VERSION_ID	62
PHP_INT_MAX	18
PHP_SAPI	17
E_ERROR	15
PHP_EOL	14
PHP_INT_SIZE	13
PATHINFO_EXTENSION	12
STDIN	10

nicolas-grekas · 2017-11-10T14:21:42Z

OK, so I confirm: only DIRECTORY_SEPARATOR, PHP_SAPI and PHP_VERSION_ID.

Slamdunk · 2017-11-10T16:10:01Z

I've thought about what you said "we should select only those involved in conditional branching" and in fact I've found various projects where conditional branching vary a lot from the Symfony one.

Thus I've added the include parameter to allow arbitrary custom whitelists.

keradus · 2017-11-10T17:27:33Z

@Symfony ruleset shall be adjusted

Slamdunk · 2017-11-11T14:35:57Z

I've added the rule in the @Symfony:risky set and applied it.

keradus · 2017-11-11T15:09:49Z

that's great. please regenerate readme, as it has to update rule now belongs to ruleset

staabm · 2018-02-12T10:32:55Z

whats open to get this feature landed?

stof · 2018-03-30T12:42:07Z

src/Fixer/ConstantNotation/NativeConstantInvocationFixer.php

+        // Case sensitive constants handling
+        $constantsToEscape = array_values($this->configuration['include']);
+        if (true === $this->configuration['fix_built_in']) {
+            $constantsToEscape = array_merge($constantsToEscape, \array_keys(\get_defined_constants()));


get_defined_constants also includes userland constants (which would be constants define by the run of the PHP-CS-Fixer in such case). This could cause issues (and contradicts the fixer name saying native)
A solution would be to use the categorized output of get_defined_constants, to remove constants belonging to the user category.

user category now skipped

stof · 2018-03-30T12:43:34Z

src/Fixer/ConstantNotation/NativeConstantInvocationFixer.php

+
+            if (!isset($this->constantsToEscape[$tokenContent]) && !isset($this->caseInsensitiveConstantsToEscape[strtolower($tokenContent)])) {
+                continue;
+            }


This fixer should also identify the constants imported with use const to avoid prefixing them. It would make the fixer much less risky (and it is necessary to support use statements if you want to later make this fixer configurable to allow injecting such use statements instead of injecting a \).

Slamdunk · 2018-03-30T12:58:49Z

@stof you are right on both your statements. By the way this feature seems controversial, and it's almost 6 months is stalled by the maintainers; I'm not going to develop any further improvement without a clear direction by them.

If you are willing to fix them, I'll be happy to review & merge them as a PR upon my branch in my fork 👍

keradus · 2018-06-01T12:15:38Z

sadly, whole \ thing got spread across multiple issues and PRs, each of them with conflicting ideas. I believe we got some conclusion finally (not only about constants) in the end and @SpacePossum did implement part of them.
I believe this is now good time to revive the PR. Thank you for understanding.

Can you, please:

merge/rebase the PR with master
point out what directions are still missing, so we could make a call on them

Slamdunk · 2018-06-01T12:38:38Z

point out what directions are still missing, so we could make a call on them

Strip user defined constants returned by get_defined_constants: easy pick, I'll face this soon
Do not fix constants imported with use statement: is there an helper class to retrieve imports?

Slamdunk · 2018-06-01T14:58:19Z

src/Fixer/ConstantNotation/NativeConstantInvocationFixer.php

+        $useDeclarations = (new NamespaceUsesAnalyzer())->getDeclarationsFromTokens($tokens);
+        $useConstantDeclarations = [];
+        foreach ($useDeclarations as $use) {
+            if ($use->isConstant()) {


@keradus in order to have this clean API, I extended the NamespaceUsesAnalyzer behaviour, which is backward compatible

Slamdunk · 2018-06-01T14:59:11Z

Strip user defined constants returned by get_defined_constants: easy pick, I'll face this soon

Do not fix constants imported with use statement: is there an helper class to retrieve imports?

Done both.

keradus · 2018-06-02T08:24:06Z

src/Fixer/ConstantNotation/NativeConstantInvocationFixer.php

+            $indexes[] = $index;
+        }
+
+        $indexes = \array_reverse($indexes);


keradus · 2018-06-02T08:25:07Z

src/Tokenizer/Analyzer/Analysis/NamespaceUseAnalysis.php

     */
-    public function __construct($fullName, $shortName, $isAliased, $startIndex, $endIndex)
+    public function __construct($fullName, $shortName, $isAliased, $startIndex, $endIndex, $type)


👍 for change in this file

keradus · 2018-06-02T08:27:21Z

tests/Tokenizer/Analyzer/NamespaceUsesAnalyzerTest.php

-            // Function and constant imports
-            // ['<?php use function My\count;', [], []],
-            // ['<?php use function My\count as myCount;', [], []],
-            // ['<?php use const My\Full\CONSTANT;', [], []],


nice side effect of this PR 👍

keradus · 2018-06-02T17:12:42Z

Thank you @Slamdunk.

This PR was squashed before being merged into the 2.12-dev branch (closes #3127). Discussion ---------- Add NativeConstantInvocationFixer Following [NativeFunctionInvocationFixer](#2462) here I propose: ```diff -echo M_PI; +echo \M_PI; ``` VLD before: https://3v4l.org/43Sci/vld VLD after: https://3v4l.org/FXkg2/vld Commits ------- aabbbc2 Add NativeConstantInvocationFixer

@nicolas-grekas

This PR was squashed before being merged into the 2.8 branch (closes #27852). Discussion ---------- Fix coding standards | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This PR is mostly about running the PHP-CS-Fixer (v2.12.1) in the whole codebase. - I updated the exclude rule to avoid some false positives for the `error_suppression` fixer (we have more files triggering unsilenced deprecations on purpose than when building the initial whitelist, mostly). - I ran the fixer with this updated config. Most changes were related to fully-qualifying some constants, with the new fixer implemented in PHP-CS-Fixer/PHP-CS-Fixer#3127, for which @nicolas-grekas and I suggested a config to include in the Symfony ruleset. Based on the output, I suggested a feature request in PHP-CS-Fixer/PHP-CS-Fixer#3872 as we might want to avoid the `\` in non-namespaced files to improve readability. We might want to remove the second commit of this PR if we decide to wait for the feature to be implemented (update: implementation is contributed in PHP-CS-Fixer/PHP-CS-Fixer#3876) - I added the `native_function_invocation` fixer explicitly, to automatically fully-qualify calls to compiler-optimized functions. This feature was implemented in PHP-CS-Fixer based on our feature request (as currently, we do such thing only manually in some hot path, because it could not be automated). I opened PHP-CS-Fixer/PHP-CS-Fixer#3873 to include it in the ruleset automatically. TODOs: - [x] agree on the updated rules - [x] update fabbot to use the new version of PHP-CS-Fixer - [ ] make separate PRs for newer branches with their own updates (exclude rules, and CS fixes), once this PR gets merged. Commits ------- 538c69d Fix Clidumper tests 04654cf Enable the fixer enforcing fully-qualified calls for compiler-optimized functions f00b327 Apply fixers 720ed4d Disable the native_constant_invocation fixer until it can be scoped 8892b98 Update the list of excluded files for the CS fixer

@nicolas-grekas

…e set (kubawerlos) This PR was merged into the 2.17-dev branch. Discussion ---------- NativeConstantInvocation - Add "PHP_INT_SIZE" to SF rule set As [requested](#4943 (comment)) by @nicolas-grekas. Added as a feature because it is [change](#3127 (comment)) request. Commits ------- fd92a7e Add "PHP_INT_SIZE" to "native_constant_invocation" in Symfony rule set configuration

@nicolas-grekas

This PR was squashed before being merged into the 2.8 branch (closes #27852). Discussion ---------- Fix coding standards | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This PR is mostly about running the PHP-CS-Fixer (v2.12.1) in the whole codebase. - I updated the exclude rule to avoid some false positives for the `error_suppression` fixer (we have more files triggering unsilenced deprecations on purpose than when building the initial whitelist, mostly). - I ran the fixer with this updated config. Most changes were related to fully-qualifying some constants, with the new fixer implemented in PHP-CS-Fixer/PHP-CS-Fixer#3127, for which @nicolas-grekas and I suggested a config to include in the Symfony ruleset. Based on the output, I suggested a feature request in PHP-CS-Fixer/PHP-CS-Fixer#3872 as we might want to avoid the `\` in non-namespaced files to improve readability. We might want to remove the second commit of this PR if we decide to wait for the feature to be implemented (update: implementation is contributed in PHP-CS-Fixer/PHP-CS-Fixer#3876) - I added the `native_function_invocation` fixer explicitly, to automatically fully-qualify calls to compiler-optimized functions. This feature was implemented in PHP-CS-Fixer based on our feature request (as currently, we do such thing only manually in some hot path, because it could not be automated). I opened PHP-CS-Fixer/PHP-CS-Fixer#3873 to include it in the ruleset automatically. TODOs: - [x] agree on the updated rules - [x] update fabbot to use the new version of PHP-CS-Fixer - [ ] make separate PRs for newer branches with their own updates (exclude rules, and CS fixes), once this PR gets merged. Commits ------- 538c69dc26 Fix Clidumper tests 39d2079 Enable the fixer enforcing fully-qualified calls for compiler-optimized functions f00b3279ea Apply fixers 720ed4d379 Disable the native_constant_invocation fixer until it can be scoped 8892b98627 Update the list of excluded files for the CS fixer

Slamdunk force-pushed the native_constant_invocation branch from eab0454 to d5e498f Compare October 4, 2017 14:58

Slamdunk commented Oct 4, 2017

View reviewed changes

SpacePossum added the kind/feature label Oct 6, 2017

Slamdunk force-pushed the native_constant_invocation branch from 0ea4229 to 1f6c7b9 Compare October 9, 2017 07:55

Slamdunk force-pushed the native_constant_invocation branch from 1f6c7b9 to 5217656 Compare October 16, 2017 14:48

leofeyer mentioned this pull request Nov 8, 2017

Add the "opcache-only" option to the native function invocation fixer #3222

Closed

Slamdunk force-pushed the native_constant_invocation branch from 5217656 to 867724c Compare November 10, 2017 15:52

Slamdunk force-pushed the native_constant_invocation branch from 6b65db4 to a3c4642 Compare January 8, 2018 15:29

Slamdunk force-pushed the native_constant_invocation branch from a3c4642 to baf07ff Compare February 5, 2018 07:53

Slamdunk force-pushed the native_constant_invocation branch 2 times, most recently from d49fae5 to 542bfd3 Compare February 15, 2018 08:03

stof reviewed Mar 30, 2018

View reviewed changes

Slamdunk force-pushed the native_constant_invocation branch from 542bfd3 to d86f790 Compare June 1, 2018 12:27

Slamdunk commented Jun 1, 2018

View reviewed changes

keradus reviewed Jun 2, 2018

View reviewed changes

src/Fixer/ConstantNotation/NativeConstantInvocationFixer.php

$indexes[] = $index;

}

$indexes = \array_reverse($indexes);

Copy link

Member

keradus Jun 2, 2018

Choose a reason for hiding this comment

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

👍

keradus reviewed Jun 2, 2018

View reviewed changes

keradus added this to the 2.12.0 milestone Jun 2, 2018

keradus approved these changes Jun 2, 2018

View reviewed changes

Add NativeConstantInvocationFixer

aabbbc2

keradus force-pushed the native_constant_invocation branch from d8c458c to aabbbc2 Compare June 2, 2018 17:12

keradus merged commit aabbbc2 into PHP-CS-Fixer:master Jun 2, 2018

Slamdunk deleted the native_constant_invocation branch June 2, 2018 18:38

sanmai mentioned this pull request Jun 4, 2018

Run cs fixer infection/infection#371

Merged

Jean85 mentioned this pull request Jun 15, 2018

[2.0] Fix build cs namespaced constants getsentry/sentry-php#614

Merged

stof mentioned this pull request Jul 5, 2018

Fix coding standards symfony/symfony#27852

Merged

3 tasks

kubawerlos mentioned this pull request May 8, 2020

NativeConstantInvocation - Add "PHP_INT_SIZE" to SF rule set #4947

Merged

Uh oh!

Add NativeConstantInvocationFixer #3127

Add NativeConstantInvocationFixer #3127

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EDITED

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!