Duplicated errors when that() is reused for multiple should()

[Wirone]

Bug Report

Q	A
BC Break	no?
Library Version	0.2.32
PHP version	7.4.32

Summary

Consider such configuration:

return static function (Config $config): void {
    $srcClassSet = ClassSet::fromDir(__DIR__ . '/src');
    $rules = [];

    $rectors = Rule::allClasses()->that(new ResideInOneOfTheseNamespaces('Codito\Rector\Money\Rule'));
    $rules[] = $rectors->should(new HaveNameMatching('*Rector'))->because('this is Rector convention');
    $rules[] = $rectors
        ->should(new Extend(AbstractRector::class))
        ->because('we need satisfy Rector\'s contract for rules');

    $config->add($srcClassSet, ...$rules);
};

Since result of the should() is a BecauseParser it does not allow multiple expectations for the same selection, I wanted to reuse result of that() to make 2 separate rules. Unfortunately, it does not behave as I expected.

Current behavior

For class that does not satisfy both rules (empty Codito\Rector\Money\Rule\Foo class) I get:

Codito\Rector\Money\Rule\Foo has 4 violations
  should have a name that matches *Rector because this is Rector convention
  should extend Rector\Core\Rector\AbstractRector because this is Rector convention
  should have a name that matches *Rector because we need satisfy Rector's contract for rules
  should extend Rector\Core\Rector\AbstractRector because we need satisfy Rector's contract for rules

How to reproduce

• Fork codito/rector-money
• Create empty class in src/Rule directory
• Run make arkitect

Expected behavior

I would expect only 2 errors for such scenario:

Codito\Rector\Money\Rule\Foo has 2 violations
  should have a name that matches *Rector because this is Rector convention
  should extend Rector\Core\Rector\AbstractRector because we need satisfy Rector's contract for rules

Ideally, it would be great to be able to define rule like this:

return static function (Config $config): void {
    $srcClassSet = ClassSet::fromDir(__DIR__ . '/src');
    $rules = [];

    $rules[] = Rule::allClasses()
        ->that(new ResideInOneOfTheseNamespaces('Codito\Rector\Money\Rule'))
        ->should(new HaveNameMatching('*Rector'))
        ->andShould(new Extend(AbstractRector::class))
        ->because('we need to satisfy Rector\'s contract for rules and keep Rector naming convention');

    $config->add($srcClassSet, ...$rules);
};

and set multiple expectations for single rule.

[AlessandroMinoccheri]

Hi @Wirone thanks for this interesting issue, I have a little question.
If you write multiple expectations and a class violates them, Do you expect an error or more errors for that class?
Because if you expect only one error the message could be not useful for the user if we show only one single violation.
It's better to return multiple errors for the class with all the violations to understand well, what do you think?

[Wirone]

@AlessandroMinoccheri it depends 😅 I believe there are scenarios when you would like to have multiple expectations but only one error if any of the expectations is not met (but print only those failed). For that "ideal" example, when there is no Rector suffix it could be something like:

Codito\Rector\Money\Rule\Foo has 1 violation
  should have a name that matches *Rector because we need to satisfy Rector's contract for rules and keep Rector naming convention

But when both expectation are not met, it could be:

Codito\Rector\Money\Rule\Foo has 1 violation
  should have a name that matches *Rector AND should extend AbstractRector because we need to satisfy Rector's contract for rules and keep Rector naming convention

Probably because should be defined better (shorter, like "it's a convention for rules"), but you get the point 😉

In general, I would like to avoid duplicating Rule::allClasses()->that(new ResideInOneOfTheseNamespaces('Codito\Rector\Money\Rule')). Since it's the same selector, IMHO it should be ready for reusing for different expectations. But currently the analysis result is invalid (2 out of 4 errors does not make sense because they mix expectations and explanations from different concerns).

[fain182]

We could make the class return from the that immutable, so that can be reused in different "shoulds".