squizlabs / PHP_CodeSniffer

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

Anonymous classes with interfaces missing space before opening bracket cause parse error with PSR-12 ruleset #3790

Closed jwittorf closed 11 months ago

jwittorf commented 1 year ago

Describe the bug In some cases anonymous classes with extends and implements cause a ParseError after "fixing" the file.

It happens if there is no space between the last interface-name and the {, see following code samples.

The following happens, see code samples at the end:

  1. The new keyword gets additional () braces
  2. The interface-names don't get indented and lined up correctly
  3. The opening { for the class gets removed
  4. The methods get wrong

Code sample

<?php

namespace Jwittorf\Psr12Linting\Psr12;

use Jwittorf\Psr12Linting\UseableClass;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfaces;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfacesB;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfacesC;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfacesD;

$instance = new class() extends UseableClass implements MultipleInterfaces,
    MultipleInterfacesB,
    MultipleInterfacesC, MultipleInterfacesD{
    public function __construct()
    {
        echo "Created anonymous class in file '" . __FILE__ . "'.\n";
    }

    public function someFunctionToBeImplemented(): void
    {
        echo "This text was created with the function " . __FUNCTION__ . " in file '" . __FILE__ . "'.\n";
    }
};

$instance2 = new class extends UseableClass implements MultipleInterfaces,
                                                       MultipleInterfacesB,
                                                       MultipleInterfacesC, MultipleInterfacesD{
    public function __construct()
    {
        echo "Created anonymous class in file '" . __FILE__ . "'.\n";
    }

    public function someFunctionToBeImplemented(): void
    {
        echo "This text was created with the function " . __FUNCTION__ . " in file '" . __FILE__ . "'.\n";
    }
};

Custom ruleset

<?xml version="1.0"?>
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="PHP_CodeSniffer">
    <file>src</file>

    <!-- Don't hide tokenizer exceptions -->
    <rule ref="Internal.Tokenizer.Exception">
        <type>error</type>
    </rule>

    <rule ref="PSR12"/>
</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 messed up PHP code below
<?php

namespace Jwittorf\Psr12Linting\Psr12;

use Jwittorf\Psr12Linting\UseableClass;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfaces;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfacesB;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfacesC;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfacesD;

$instance = new() class () extends UseableClass implements
MultipleInterfaces,
    MultipleInterfacesB,
    MultipleInterfacesC, MultipleInterfacesD

public function __construct()
{
    echo "Created anonymous class in file '" . __FILE__ . "'.\n";
}

public function someFunctionToBeImplemented(): void
{
    echo "This text was created with the function " . __FUNCTION__ . " in file '" . __FILE__ . "'.\n";
}
};

$instance2 = new() class extends UseableClass implements
MultipleInterfaces,
    MultipleInterfacesB,
    MultipleInterfacesC, MultipleInterfacesD

public function __construct()
{
    echo "Created anonymous class in file '" . __FILE__ . "'.\n";
}

public function someFunctionToBeImplemented(): void
{
    echo "This text was created with the function " . __FUNCTION__ . " in file '" . __FILE__ . "'.\n";
}
};

Expected behavior I would expect the correct formatting like the code below with following "steps":

  1. Put every interface-name on an own line, indented 4 spaces
  2. Move the opening { into an own line, no indention
  3. DON'T try to add () after the new keyword
<?php

namespace Jwittorf\Psr12Linting\Psr12;

use Jwittorf\Psr12Linting\UseableClass;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfaces;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfacesB;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfacesC;
use Jwittorf\Psr12Linting\Psr4\MultipleInterfacesD;

$instance = new class() extends UseableClass implements
    MultipleInterfaces,
    MultipleInterfacesB,
    MultipleInterfacesC,
    MultipleInterfacesD
{
    public function __construct()
    {
        echo "Created anonymous class in file '" . __FILE__ . "'.\n";
    }

    public function someFunctionToBeImplemented(): void
    {
        echo "This text was created with the function " . __FUNCTION__ . " in file '" . __FILE__ . "'.\n";
    }
};

$instance2 = new class extends UseableClass implements
    MultipleInterfaces,
    MultipleInterfacesB,
    MultipleInterfacesC,
    MultipleInterfacesD
{
    public function __construct()
    {
        echo "Created anonymous class in file '" . __FILE__ . "'.\n";
    }

    public function someFunctionToBeImplemented(): void
    {
        echo "This text was created with the function " . __FUNCTION__ . " in file '" . __FILE__ . "'.\n";
    }
};

Versions:

jrfnl commented 1 year ago

@jwittorf Thank you for reporting this. I've been able to reproduce the issues and am working on a fix.

jrfnl commented 1 year ago

Okay, done. PR #3791 should fix this. Testing appreciated.

jwittorf commented 1 year ago

Thanks for your quick response and fix @jrfnl! I've tested it in my setup, looks good :)

But the checks for PHP 8.2 still fail?

jrfnl commented 1 year ago

Thanks for your quick response and fix @jrfnl! I've tested it in my setup, looks good :)

But the checks for PHP 8.2 still fail?

Thanks for testing, Re: the PHP 8.2 test run, that is unrelated to this issue. See #3731

fredden commented 1 year ago

But the checks for PHP 8.2 still fail?

Thanks for testing, Re: the PHP 8.2 test run, that is unrelated to this issue. See #3731

This specific failure seems to be what is being fixed in https://github.com/squizlabs/PHP_CodeSniffer/pull/3773, not related to #3731.

jrfnl commented 1 year ago

@fredden As I pointed out before, they are related. Also: different discussion, not in this issue.

fredden commented 1 year ago

@jrfnl it looks like #3791 only addresses one of the four bullet points raised in this issue. Did you determine that the other three are false-alerts / side effects?

  1. The new keyword gets additional () braces
  2. The interface-names don't get indented and lined up correctly
  3. The opening { for the class gets removed
  4. The methods get wrong

From what I can tell:

  1. This seems unrelated to the fix. Perhaps there's a quirk somewhere that adds these parenthesises when the code is mal-formed / invalid?
  2. The test case that you've added in #3791 doesn't cover this. Is it covered elsewhere and a side effect of the invalid code?
  3. This is directly fixed by #3791.
  4. This is fixed by #3791, if I'm reading "wrong" correctly as "not indented enough", because it seems like a side effect from removing the opening brace by mistake.
jrfnl commented 1 year ago

@jrfnl it looks like #3791 only addresses one of the four bullet points raised in this issue. Did you determine that the other three are false-alerts / side effects?

Yes, I did, see my comment in the PR. They are all side-effects. There is nothing else to do.

fredden commented 1 year ago

Sorry @jrfnl, I've now seen the note on the pull request which specifically addresses my query.

jrfnl commented 10 months ago

FYI: the fix for this issue is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).