False positives for IsAbstract expression

[davidcv91]

Bug Report

Q	A
BC Break	yes
Library Version	0.3.33
PHP version	8.1.28

Summary

IsAbstract() finds classes that are not abstract

Apparently, the bug was added on this commit

Current behavior

App\MyClass has 1 violations
  should have a name that matches Abstract* because we want to prefix abstract classes

How to reproduce

Add the following rule:

Rule::allClasses()
        ->that(new IsAbstract())
        ->should(new HaveNameMatching('Abstract*'))
        ->because('we want to prefix abstract classes');

And create a class like:

final class MyClass
{
}

Expected behavior

No errors reported

[courtney-miles]

I have also found this to be unexpected matching and am compensating by specifically excluding everything else:

Rule::allClasses()
        ->that(new IsAbstract())
        ->that(new IsNotInterface())
        ->that(new IsNotTrait())
        ->that(new IsNotFinal())
        ->should(new HaveNameMatching('Abstract*'))
        ->because('we want to prefix abstract classes');

I'm trying to understand what problem #425 will have fixed and can't see it.

[micheleorselli]

@davidcv91 This should be fixed by #454. TL;DR: a new method was added to allow skipping classes where a certain rule is not applicable.

[sukei]

The v0.5.4 breaks it again. The following class is not flagged as abstract.

abstract class AbstractBackedEnumTestCase extends TestCase { }

[micheleorselli]

@sukei does the class AbstractBackedEnumTestCase make use of anonymous classes? If yes, a bugfix is on the way for that, otherwise, would it be possibile to have a snippet of the content of the class itself? Thanks!

[sukei]

Nope, but it does extends an other abstract class.

[micheleorselli]

would you mind trying with the latest version (0.6.0) if the problem still exists? Thanks 🙇

[sukei]

Yep, it is fixed! My bad, that sneaky characters where hidden somewhere in: new class {}.

[sukei]

I got a new weird case where final classes are excluded by the predicates and do not go further:

->that(new ResideInOneOfTheseNamespaces('My\Namespace'))->andThat(new IsNotTrait())->andThat(new IsNotAbstract())->andThat(new IsNotInterface())->andThat(new IsNotEnum())
->should(...)

The following class did not trigger the should rule:

namespace My\Namespace;

final readonly class TreeTypeID extends ID
{
}

[micheleorselli]

I was able to reproduce it with the test below

It seems those constraints does not play well together: when using IsAbstract\IsNotAbstract the current implementation does not consider

• interfaces
• final classes
• enums
• traits

because they cannot be abstract by definition (see IsNotAbstract::appliesTo). Maybe we could change the implementation for IsNotAbstract, let me tinker a bit with it. In the meantime as a workaround you could add an additional rule to check explicitly for final classes so you got those covered.

public function test_it_fails():void
    {
        $runner = TestRunner::create('8.4');

        $rule = Rule::allClasses()
            ->that(new ResideInOneOfTheseNamespaces('My\Namespace'))
            ->andThat(new IsNotTrait())
            ->andThat(new IsNotAbstract())
            ->andThat(new IsNotInterface())
            ->andThat(new IsNotEnum())
            ->should(new HaveNameMatching('My*'))
            ->because('we want to prefix classes in this namespace with My');

        $structure = [
            'My' => [
                'Namespace' => [
                    'TreeTypeID.php' => '<?php

                        namespace My\Namespace;

                        final readonly class TreeTypeID extends ID { }
                    '
                ],
            ]
        ];

        $runner->run(vfsStream::setup('root', null, $structure)->url(), $rule);

        dump($runner->getViolations()); // you get 0 violations
        
    }

[sukei]

There was a over-optimisation of some rules that are completely fine by themselves but do not play well when composed together. Maybe they should be removed to be the as straightforward as possible. It could have an impact on performances but it should be easier to reason about.