djoos / Symfony-coding-standard

Development repository for the Symfony coding standard
MIT License
401 stars 102 forks source link

ReturnTypeSniff and anonymous functions #119

Closed temp closed 6 years ago

temp commented 6 years ago

This code produces an error, Symfony.Functions.ReturnType.Invalid:

$x = array_map(function ($y) {
    return $y;
}, [1, 2, 3]);
170 | ERROR | Use return null; when a function explicitly returns null values and use return; when the function returns void values

Shouldn't anonymous functions be excluded from Return Type Sniffs?

ZhukV commented 6 years ago

Full code as an example:

Test class:

<?php

namespace GlobalPusher\Tests\Component\Tactician\Middleware;

use FiveLab\Component\Transactional\TransactionalInterface;
use GlobalPusher\Component\Tactician\DatabaseTransactionalCommandInterface;
use GlobalPusher\Component\Tactician\Middleware\DatabaseTransactionalMiddleware;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;

class DatabaseTransactionalMiddlewareTest extends TestCase
{
    /**
     * @var TransactionalInterface|MockObject
     */
    private $transactional;

    /**
     * @var DatabaseTransactionalMiddleware
     */
    private $middleware;

    /**
     * {@inheritdoc}
     */
    protected function setUp(): void
    {
        $this->transactional = $this->createMock(TransactionalInterface::class);
        $this->middleware = new DatabaseTransactionalMiddleware($this->transactional);
    }

    /**
     * @test
     */
    public function shouldSuccessWithoutTransaction(): void
    {
        $this->transactional->expects(self::never())
            ->method('execute');

        $callbackCalled = false;

        $callback = function () use (&$callbackCalled) {
            $callbackCalled = true;

            return 'command result';
        };

        $result = $this->middleware->execute(new \stdClass(), $callback);

        self::assertTrue($callbackCalled, 'The original callback not been called.');
        self::assertEquals('command result', $result);
    }

    /**
     * @test
     */
    public function shouldSuccessWithTransaction(): void
    {
        $this->transactional->expects(self::once())
            ->method('execute')
            ->with(self::isInstanceOf(\Closure::class))
            ->willReturnCallback(function (\Closure $closure) {
                return call_user_func($closure);
            });

        $callback = function () use (&$callbackCalled) {
            $callbackCalled = true;

            return 'command result';
        };

        $result = $this->middleware->execute($this->createMock(DatabaseTransactionalCommandInterface::class), $callback);

        self::assertTrue($callbackCalled, 'The original callback not been called.');
        self::assertEquals('command result', $result);
    }
}

Output of failures:

FILE: ...sts/Component/Tactician/Middleware/DatabaseTransactionalMiddlewareTest.php
--------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
--------------------------------------------------------------------------------
 45 | ERROR | Use return null; when a function explicitly returns null values
    |       | and use return; when the function returns void values
 63 | ERROR | Use return null; when a function explicitly returns null values
    |       | and use return; when the function returns void values
 70 | ERROR | Use return null; when a function explicitly returns null values
    |       | and use return; when the function returns void values
--------------------------------------------------------------------------------

The system check what the returns type should be a void, but we use other return statement in internal anonymous function.

soullivaneuh commented 6 years ago

Why not simply put return type on your function signature?

$publicComments = $incident->getComments()->filter(static function (IncidentComment $comment): bool {
    return true === $comment->getCustomerVisible();
});
stof commented 6 years ago

I think this is related to the same kind of issue than https://github.com/djoos/Symfony-coding-standard/issues/118: it does not properly distinguish return statements of the closure from return statements of the enclosing function.

djoos commented 6 years ago

126 merged!