Skip to content
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.

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

PHP 7.4: arrow functions #2523

Closed
3 of 6 tasks
jrfnl opened this issue Jun 2, 2019 · 12 comments
Closed
3 of 6 tasks

PHP 7.4: arrow functions #2523

jrfnl opened this issue Jun 2, 2019 · 12 comments
Milestone

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Jun 2, 2019

PHP 7.4 will contain the ability to declare arrow functions (short, one statement closures). This RFC has already been implemented in the PHP 7.4 branch.

Arrow functions will make code sniffing more complicated as the T_DOUBLE_ARROW token is now also used for those, quite apart from these functions being declared without "scope"/curly braces, making it interesting to determine the end of a statement, particularly when these would be used within arrays, ternaries or even in nested arrow functions.

I imagine, it may even be desirable to assign a different token to the T_DOUBLE_ARROW when used for arrow functions.

Based on the RFC and the implementation details, I foresee that - at least - the following changes will need to be made:

  • Back-fill the new T_FN token for PHP < 7.4.
  • Add T_FN to the condition related to anonymous structures in the File::getDeclarationName() method.
  • Allow for T_FN in the File::getMethodParameters() method.
  • Allow for T_FN in the File::getMethodProperties() method.
  • Register T_FN with various sniffs which sniff function declarations.
  • Any sniff/function relying on the T_DOUBLE_ARROW only being used for array key/value separation will also need adjusting (depending on whether or not a different token will be used or not).

Userland external standards will also need to be aware of this and will need to make similar changes in their sniffs for the last two action points.

There are a lot of code examples available in the RFC, but IMO, more will be needed to test the above identified issues around arrow functions in arrays, in ternaries and nested arrow functions.

@gsherwood
Copy link
Member

From #2570, Generic.WhiteSpace.ScopeIndent would need support:

<?php

declare(strict_types=1);

$numbers = [1, 2, 3];

$result = array_map(
    static fn(int $number) : int => $number + 1,
    $numbers
);

From #2571, PSR2.Methods.FunctionCallSignature would need support:

<?php

declare(strict_types=1);

$container
    ->add(
        'identifier',
        fn() : SomeClass => new SomeClass(
            [
                'option' => true,
                'option2' => false,
            ]
        ),
        true
    );

From #2572, PSR2.Methods.FunctionCallSignature would need support:

<?php

declare(strict_types=1);

throwException(
    fn () : Exception => new Exception('Message', 1)
);

function throwException(callable $function) : void
{
    throw $function();
}

@gsherwood gsherwood added this to the 3.5.3 milestone Nov 6, 2019
gsherwood added a commit that referenced this issue Nov 7, 2019
This changed the way the T_FN token was backfilled as it now needs to set before processAdditional kicks in so it also works in PHP 7.4. Scope information is added in processAdditional because the way the opener and closer is calulated is too specific to put into the generic scope mapping code.
gsherwood added a commit that referenced this issue Nov 7, 2019
@gsherwood
Copy link
Member

I've had a go at adding support for arrow functions, but that are a bit strange given they have no defined beginning and end of the scope. Despite that, I do have scope and parenthesis information being populated for them, I'm back-filling the token for older PHP versions to ensure they do the same, and I've fixed up the sniffs that were reporting errors once the arrow functions were processed.

I've done the processing in the processAdditional() method and I haven't added arrow functions as scope openers because their lack of real defined scope makes applying indentation rules (and everything else) pretty difficult. My feeling is that specific sniffs would need to be written for arrow function formatting instead of using the existing ones, so I've gone in the direction of having the existing sniffs effectively ignore their formatting.

The 3 pieces of sample code in this issue now process without error, and I've tested quite a few examples from the RFC, but I'm sure there are going to be cases where these aren't being processed correctly. I'll have to fix them as they come up.

For now, I'll leave this open so that anyone with time can have a test of master and see if the code is breaking. Please leave sample code snippets here and I'll get these fixed up before the next release.

@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 7, 2019

@gsherwood Thanks for all your work on this! I'll try to find the time to do some testing soonish.

Have you had any thoughts on the (re-)tokenizing of T_DOUBLE_ARROW in the context of what I mention above about it ?

@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 7, 2019

Quick test with a simple array with arrow functions shows that the Squiz.Arrays.ArrayDeclaration definitely has problems with it.

$array = [
    'a' => fn() => return 1,
    'bb' => fn() => return 2,
    'ccc' => ( true ) ?
        fn() => return 1 :
        fn() => return 2,
];

@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 7, 2019

Also wondering: as there is now a third "function" keyword, would it be useful to have a Tokens::$functionKeywords array containing T_FUNCTION, T_CLOSURE and T_FN ?

@gsherwood
Copy link
Member

Have you had any thoughts on the (re-)tokenizing of T_DOUBLE_ARROW in the context of what I mention above about it ?

Yeah. Have been thinking about T_FN_ARROW but not 100% sure on it. At least PHP 7.4 doesn't assign a custom token to it, so PHPCS can do whatever it wants. Would make array stuff easier, so feels like something that should be done. I might give it a go as part of fixing the array sniffs and see how it looks.

Also wondering: as there is now a third "function" keyword

I think that's a good idea. Will add that too.

gsherwood added a commit that referenced this issue Nov 10, 2019
…W (ref #2523)

The reduces a lot of confusion with double arrows used in arrays.
michalbundyra added a commit to michalbundyra/Squizlabs_PHP_CodeSniffer that referenced this issue Nov 21, 2019
michalbundyra added a commit to michalbundyra/Squizlabs_PHP_CodeSniffer that referenced this issue Nov 26, 2019
michalbundyra added a commit to michalbundyra/Squizlabs_PHP_CodeSniffer that referenced this issue Nov 26, 2019
Related squizlabs#2523

I am not sure about this change as this is actually invalid code.
We cannot use `$this` even inside closure, as then closure is not gonna
be usable. The same in case of normal closure.

Maybe here we should have another fix then?
michalbundyra added a commit to michalbundyra/Squizlabs_PHP_CodeSniffer that referenced this issue Nov 26, 2019
@gsherwood
Copy link
Member

Core support appears to be working, so closing this now in preparation for a release.

michalbundyra added a commit to michalbundyra/Squizlabs_PHP_CodeSniffer that referenced this issue Jan 6, 2020
Related squizlabs#2523

I am not sure about this change as this is actually invalid code.
We cannot use `$this` even inside closure, as then closure is not gonna
be usable. The same in case of normal closure.

Maybe here we should have another fix then?
jrfnl added a commit to jrfnl/PHP_CodeSniffer that referenced this issue Feb 5, 2020
While as per [@gsherwood's comment](squizlabs#2523 (comment)) in 2523, arrow functions are not set up as scope openers, I think adding them to the `Tokens::$parenthesisOpeners` array would be justified.

I'm also wondering if the token should be added to the `Tokens::$weightings` array ?
@zlodes
Copy link

zlodes commented Feb 25, 2020

Hello there!

I have a problem.
Using 3.5.4 with PHP 7.4.2.

Code example:

<?php

declare(strict_types=1);

namespace Examples;

class ExampleClass
{
    public function doSomething(): void
    {
        print_r($this->filter());
    }

    private function cleanString(string $string, string $except): string
    {
        return str_replace($except, '', $string);
    }

    /**
     * @return array|string[]
     */
    private function filter(): array
    {
        $stringValues = [
            'foo',
            'bar',
        ];

        return array_filter(
            $stringValues,
            fn (string $value): string => $this->cleanString($value, 'b')
        );
    }
}
# vagrant @ homestead in ~/projects/php-quality-tools [14:00:15] C:2
$ ./vendor/bin/phpcs --version
PHP_CodeSniffer version 3.5.4 (stable) by Squiz (http://www.squiz.net)

# vagrant @ homestead in ~/projects/php-quality-tools [14:00:17]
$ ./vendor/bin/phpcs --standard="PSR2" testo.php -s

FILE: /home/vagrant/projects/php-quality-tools/testo.php
---------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------------------------
 31 | ERROR | [x] Only one argument is allowed per line in a multi-line function call
    |       |     (PSR2.Methods.FunctionCallSignature.MultipleArguments)
---------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------------------------------------

Time: 209ms; Memory: 6MB

gsherwood pushed a commit that referenced this issue Jul 21, 2020
Related #2523

I am not sure about this change as this is actually invalid code.
We cannot use `$this` even inside closure, as then closure is not gonna
be usable. The same in case of normal closure.

Maybe here we should have another fix then?
@tomterl
Copy link

tomterl commented May 9, 2022

I have an older Sniff (I didn't wrote myself), that I would like to adept to T_FN with minimal effort on my part :), it seems to me that this point from the original Issue text

Add T_FN to the [condition related to anonymous structures](https://github.com/squizlabs/PHP_CodeSniffer/blob/200eae562f8c449107d2378a7f886586e77c6c8e/src/Files/File.php#L1235) in the File::getDeclarationName() method.

would help me to do that (variable scope is determined by looping over 'conditions') - is this change still considered (adding T_FN to conditions)?

@jrfnl
Copy link
Contributor Author

jrfnl commented May 9, 2022

@tomterl Adding the T_FN token to the File::getDeclarationName() method is something completely different from adding it to the conditions array in the token stack. And AFAIK, the latter is not planned - also see #2523 (comment)

@tomterl
Copy link

tomterl commented May 9, 2022

Thanks for clarifying that and the pointer -- will have to dig deeper :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants