phpstan: Check for duplicate keys in multiple yield operators

Summary of a problem or a feature request

When I use, for example, data providers in PHPUnit, I use them by yielding each data set, like this:

/** @dataProvider provider */
public function test_xxx(int $int, string $string) {}

public function provider(): \Generator
{
    yield 'Description of the data set #1' => [123, 'xy'];

    yield 'Description of the data set #2' => [456, 'ab'];
}

When you have a large provider, it’s easy to make copy-paste error and end up with the duplicate key:

public function provider(): \Generator
{
    // ...
    yield 'Duplicate key' => [123, 'xy'];
    // ...
    yield 'Duplicate key' => [456, 'ab'];
}

PHPUnit in this case ignores the first one, and we loose 1 test case.

Code snippet that reproduces the problem

https://phpstan.org/r/6eef732e950c1001e202097358d510cf

Expected output

Expected ouput from PHPStan:

Method sayHello() has 2 duplicate keys in yield with value 'Duplicate Key'

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 18 (18 by maintainers)

Most upvoted comments

phpstan/phpstan-strict-rules is not a catch-all repo for rules that do not fit phpstan/phpstan 😃

This repository contains additional rules that revolve around strictly and strongly typed code with no loose casting for those who want additional safety in extremely defensive programming

As has been said above, duplicate keys from iterators and generators are OK. Does PHPUnit handle them correctly (e.g. running a test case for each item, even with a duplicate key?) Maybe the issue should be reported there.

Of course, anyone is welcome to write and use the proposed rule on their own, but it’s not something generally applicable…

ondrejmirtes on Dec 22, 2018

Just because it’s a weird language quirk doesn’t mean it’s incorrect.

I wouldn’t call it a quirk. There are valid use cases and you can always avoid the behaviour if you don’t like it. Besides, if you assume generators/iterators produce unique array keys, you are already working with broken assumptions since even though they produce different values, they may end up the same in an array (think of 5, "5" and 5.0).

I can see use cases in long-running code where you can create infinite generator that yields updates where key would be updated user entity and value would be a diff / value changed. Just because you don’t use it or don’t find valid use case doesn’t mean it should be forbidden.

I’m strongly against including this in strict-rules, it has nothing to do with strict code.

Majkl578 on Dec 21, 2018

@Majkl578

but complicates things and makes them a lot less readable, or even performant (in critical code).

If you need high performance you probably shouldn’t use generators with a state machine behind them.

but then it becomes harder to comprehend from API and also harder to consume.

How are either of those statements true? The code looks more obvious, it’s typed, you’ll get nice IDE autocompletion, it’s just as easy to consume. You’ll get some small overhead from the heap allocation for the Change object. That’s it.

IMO this is a valid feature for strict-rules.

@borNfreee

could you please transfer this issue to that repository using GitHub API?

I’m just a contributor, @ondrejmirtes decides what makes it into which repository. Let’s wait until he states his opinion on where this should go.

Also, PRs are always welcome! Open source only works when people contribute.

iluuu1994 on Dec 22, 2018

Yielding the same key is not a bug and may be perfectly valid for some use cases. You should instead open feature request for PHPUnit to report such data providers as broken.

JanTvrdik on Dec 11, 2018