Php_codesniffer: Wrong T_FN token when $this->fn

[BafS]

Having $xxx->fn in the code is reported as a T_FN, which is wrong.

Example:

<?php
class Test {
    public function foo() {
        $this->fn = 'a';
    }
}

$this->fn gives

^ array:8 [
  "code" => "PHPCS_T_FN"
  "type" => "T_FN"
  "content" => "fn"
  "line" => 7
  "column" => 16
  "length" => 2
  "level" => 2
  "conditions" => array:2 [
    2 => 361
    11 => 346
  ]
]

Related issue: https://github.com/slevomat/coding-standard/issues/896

[jrfnl]

@BafS You beat me to it :grinning: I was working on an issue write-up for this and some more similar issues. I'll try and find the time to write it up properly and add those here hopefully by tomorrow.

[BafS]

Thanks @jrfnl !

[jrfnl]

Related to #2523

I've tried to do an inventory of any situation I could think of for the fn keyword/arrow functions which wasn't yet covered by unit tests (and some which are).

As shown in the below table, there are a number of inconsistencies with the current tokenization of arrow functions:

1. Inconsistency between how PHP tokenizes and how PHPCS tokenizes them.
2. Inconsistency between how PHPCS tokenizes them on PHP 7.4 versus on older PHP versions.
3. Inconsistency with the reality of whether something is actually an arrow function.
4. Inconsistency with how PHPCS tokenizes the fn if used as a function name.
   For all other reserved keywords, PHPCS changes the token to T_STRING, but not for T_FN.

In part the current tokenization is incorrect:

As of PHP 7.0.0 these keywords are allowed as property, constant, and method names of classes, interfaces and traits, except that class may not be used as constant name.

See: https://www.php.net/manual/en/reserved.keywords.php

In part the current tokenization is unpredictable:
Different tokenization depending on which PHP version is used to run PHPCS.

In part the current tokenization is unexpected/makes sniffing hard:
The token may or may not be an actual arrow function and may or may not have the extra indexes set.

Proposal

For the same reason as array as a type declaration is set to the T_STRING token instead of the T_ARRAY token, it would make a lot of sense to only set a token to T_FN if it actually is an arrow function and set it to T_STRING in all other cases.

This includes tokenizing fn as T_STRING when used as function name, same as is already done for all other reserved keywords.

This will make life easier for sniff writers as you can then count on a T_FN token actually being an arrow function and on the extra indexes being set instead of still having to do significant extra checking when encountering a T_FN token.

I've got unit tests ready for this already.

@gsherwood If you agree with this proposal, I'd be happy to complete the work on fixing this.

Findings

Is Arrow Function ?	Tokenized in PHP 7.4.2 as	Tokenized in PHPCS master on PHP < 7.4 (7.3.12) as	Has extra indexes ?	Tokenized in PHPCS master on PHP 7.4(.2) as	Has extra indexes ?	Proposed token	Remarks
$fn1 = fn($x) => $x + $y;
:heavy_check_mark:	T_FN	T_FN	:white_check_mark:	T_FN	:white_check_mark:	T_FN
class Foo { public function bar($param) { $fn = fn($c) => $callable($factory($c), $c); } }
:heavy_check_mark:	T_FN	T_FN	:white_check_mark:	T_FN	:white_check_mark:	T_FN
function fn($param) { echo $param; }
:x:	T_FN	T_FN	:no_entry_sign:	T_FN	:no_entry_sign:	T_STRING	Illegal in PHP 7.4
function do($param) { echo $param; }
N/A	T_DO	T_STRING	N/A	T_STRING	N/A	T_STRING	Illegal
const FN = 'a';
:x:	T_FN	T_STRING	:no_entry_sign:	T_STRING	:no_entry_sign:	T_STRING
class Foo { public static function fn($param) { echo $param; } }
:x:	T_FN	T_FN	:no_entry_sign:	T_FN	:no_entry_sign:	T_STRING	Perfectly fine in PHP 7.4, keyword reservation does not apply
$anon = new class() { protected function fn($param) { echo $param; } }
:x:	T_FN	T_FN	:no_entry_sign:	T_FN	:no_entry_sign:	T_STRING	Perfectly fine in PHP 7.4, keyword reservation does not apply
class Foo { public function foo() { $this->fn = 'a'; } }
:x:	T_STRING	T_FN	:no_entry_sign:	T_FN	:no_entry_sign:	T_STRING
$a = Foo::fn($param);
:x:	T_FN	T_FN	:no_entry_sign:	T_STRING	:no_entry_sign:	T_STRING
$a = MyClass::FN;
:x:	T_FN	T_FN	:no_entry_sign:	T_STRING	:no_entry_sign:	T_STRING
$a = $obj->fn($param);
:x:	T_STRING	T_FN	:no_entry_sign:	T_FN	:no_entry_sign:	T_STRING
$a = MyNS\Sub\fn($param);
:x:	T_FN	T_FN	:no_entry_sign:	T_FN	:no_entry_sign:	T_STRING
$a = namespace\fn($param);
:x:	T_FN	T_FN	:no_entry_sign:	T_FN	:no_entry_sign:	T_STRING
/* testLiveCoding */ $fn = fn
:grey_question:	T_FN	T_FN	:no_entry_sign:	T_FN	:no_entry_sign:	T_STRING
Is Arrow Function ?	Tokenized in PHP 7.4.2 as	Tokenized in PHPCS master on PHP < 7.4 (7.3.12) as	Has extra indexes ?	Tokenized in PHPCS master on PHP 7.4(.2) as	Has extra indexes ?	Proposed token	Remarks

[jrfnl]

PR #2860 implements the proposal outlined above.

[jrfnl]

Oh and just to bring the point home: the proposed fix will also prevent nonsensical results from the File::getMethodProperties() and File::getMethodParameters() methods.

A call to File::getMethodProperties() for a non-arrow function T_FN token, like $this->fn (property access) or $obj->fn($param) (function call, not declaration), would now yield:

array(9) {
  ["scope"]=>
  string(6) "public"
  ["scope_specified"]=>
  bool(false)
  ["return_type"]=>
  string(0) ""
  ["return_type_token"]=>
  bool(false)
  ["nullable_return_type"]=>
  bool(false)
  ["is_abstract"]=>
  bool(false)
  ["is_final"]=>
  bool(false)
  ["is_static"]=>
  bool(false)
  ["has_body"]=>
  bool(true)
}

And for File::getMethodParameters() it would yield an empty array, not the RuntimeException.

[gsherwood]

@gsherwood If you agree with this proposal, I'd be happy to complete the work on fixing this.

I sure do. Thanks for the PR - I've left a couple of comments on there for testing.

[gsherwood]

Thanks @jrfnl for the thorough research into this, and for the PR to fix it all up.

[jrfnl]

@gsherwood I'm just happy we got it sorted as, as things were, sniffing for arrow functions was harder than it should be IMO.

I hope I caught everything, but either way, the currently implemented solution should make it stable for the future.