djoos / Symfony-coding-standard

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

structure: else, elseif, break (bugfix) #90

Closed wickedOne closed 7 years ago

wickedOne commented 7 years ago

fixes several bugs in the return or throw sniff

should now raise appropriate errors when tested with for example

<?php

namespace Symfony;

class Ex
{
    public function __construct()
    {
        if ($a === $b) {
            if ($b === $c) {
                return null;
            }
        } else { # this one was a false positive
            $b = $a;
        }
    }

    public function baz($foo, $bar)
    {
        if ($foo === 1) {
            throw new \Exception('Bar'.$foo);
        } else {
            $foo = $bar;
        }
    }

    public function foo()
    {
        switch($a)
        {
            case 1:
                return null;
            case 2:
                $a = $b;
                return null;
                break; # this one was ignored
        }
    }
}
VincentLanglet commented 7 years ago

@wickedOne I think you have trouble with :

        if ('string' === $dummy) {
            if ('values' === $mergedOptions['some_default']) {
                return substr($dummy, 0, 5); <--- THIS RETURN
            }

            return ucwords($dummy);
        }

        if ('int' === $dummy) {
            if ('values' === $mergedOptions['some_default']) {
                substr($dummy, 0, 5);
            } elseif (true) {   <-- THIS $conditions FOUND ON THE SAME LEVEL
                return ucwords($dummy);
            }
        }

FYI, It seems good with this code:

        $tokens = $phpcsFile->getTokens();
        $opener = $phpcsFile->findPrevious($this->openers, $stackPtr);

        if ($opener && $stackPtr <= $tokens[$opener]['scope_closer']) {
            $isClosure = $phpcsFile->findPrevious(T_CLOSURE, $stackPtr, $opener);

            if (false !== $isClosure) {
                return;
            }

            $condition = $phpcsFile->findNext($this->conditions, $stackPtr + 1);

            if (false !== $condition) {
                $start = $stackPtr;
                $end = $condition;

                $next = $phpcsFile->findNext($this->openers, $start + 1, $end);
                while (false !== $next) {
                    if ($tokens[$condition]['level'] >= $tokens[$next]['level']) {
                        $err = false;
                        break;
                    }

                    $start = $next;
                    $next = $phpcsFile->findNext($this->openers, $start + 1, $end);
                }

                if (!isset($err)) {
                    $err = $tokens[$condition]['level'] === $tokens[$opener]['level'];
                }

                if (true === $err) {
                    $phpcsFile->addError(
                        'Do not use else, elseif, break after if and case conditions which return or throw something',
                        $condition,
                        'Invalid'
                    );
                }
            }
        }
wickedOne commented 7 years ago

thanks @VincentLanglet ! i'll try to have a look at it somewhere this week.

wickedOne commented 7 years ago

@VincentLanglet i've modified the sniff so it expects the else(if) statement on the same line as scope closer of the if statement.

so that is assuming nobody does something horrible like

// ...
if ($foo === $bar) {
    return null;
}
else {
    $bar = $foo;
}
djoos commented 7 years ago

Thanks @wickedOne!