PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
938 stars 57 forks source link

PSR2.ControlStructures.SwitchDeclaration.TerminatingComment false positive with never returning function call #657

Closed anthonyryan1 closed 2 hours ago

anthonyryan1 commented 3 hours ago

Describe the bug

I'm not certain if this is in scope, but I'm curious if phpcs could treat never functions as comparable to exit;

Code sample

<?php

declare(strict_types=1);

function error(): never
{
    // Do logging
    exit;
}

switch ($Var) {
    case 'one':
        // Do stuff
        break;
    case 'two':
        error();
    case 'three':
        // Do stuff
        break;
}

Custom ruleset

<?xml version="1.0"?>
<ruleset name="PSR12">
    <config name="testVersion" value="8.3-"/>
    <arg name="extensions" value="php"/>
    <arg name="ignore" value="vendor/"/>
    <arg name="colors" />
    <arg value="sp"/>
    <rule ref="PSR12">
        <exclude name="Generic.Files.LineLength.TooLong" />
    </rule>
    <rule ref="Generic.PHP.RequireStrictTypes" />
    <rule ref="Generic.PHP.DisallowShortOpenTag" />
</ruleset>

To reproduce

Steps to reproduce the behavior:

  1. Create a file called test.php with the code sample above...
  2. Run phpcs test.php ...
  3. See error message displayed
    
    ---------------------------------------------------------------------------------------------------------------
    FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
    ---------------------------------------------------------------------------------------------------------------
    1 | WARNING | A file should declare new symbols (classes, functions, constants, etc.) and cause no other
    |         | side effects, or it should execute logic with side effects, but should not do both. The first
    |         | symbol is defined on line 5 and the first side effect is on line 12.
    |         | (PSR1.Files.SideEffects.FoundWithSymbols)
    16 | ERROR   | There must be a comment when fall-through is intentional in a non-empty case body
    |         | (PSR2.ControlStructures.SwitchDeclaration.TerminatingComment)
    ---------------------------------------------------------------------------------------------------------------

## Expected behavior

There should be no error on line 16, because it's unreachable.

The obvious workarounds of doing:
```php
error();
break;

regrettably causes the 3rd party project phpstan to rightly throw an error than the break statement is unreachable. So it fixes the phpcs error, and creates a phpstan one. Making it a bit inconvenient.

Versions (please complete the following information)

Operating System Gentoo Linux
PHP version 8.3.13
PHP_CodeSniffer version 3.10.3
Standard PSR12
Install type composer

Additional context

Add any other context about the problem here.

Please confirm:

jrfnl commented 2 hours ago

@anthonyryan1 Interesting question, but not something which PHPCS is suitable to ignore.

PHPCS scans are file-based, not project-based, so as soon as the error() function from the example would be declared in a different file, PHPCS would not be able to determine anymore whether something is a never function.

I suggest you apply either a // phpcs:ignore PSR2.ControlStructures.SwitchDeclaration.TerminatingComment -- function exits comment or a PHPStan ignore comment.