Fix ext-ds object diffing and merging

[simPod]

I have not found a way to test this on my project yet.

Rather than creating issue I tried to fix stubs or at least show the idea. The values do not need to have the same type and this should address it.

[ondrejmirtes]

/cc @enumag

[enumag]

It doesn't make much sense to me to use the data structures this way but technically this is of course correct. 👍

I don't particularly like "TValue2" but I'm not sure what to replace it with.

[simPod]

I'll write some tests to make the behavior 100% right

…

On Sat, Jan 11, 2020, 08:37 Jáchym Toušek ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In stubs/ext-ds.stub <#95 (comment)>: > @@ -153,16 +154,18 @@ final class Map implements IteratorAggregate, ArrayAccess, Collection } /** - * @param Map<TKey, TValue> $map - * @return Map<TKey, TValue> + * @template TValue2 + * @param Map<TKey, TValue2> $map + * @return Map<TKey, TValue|TValue2> This is wrong too. Since this is intersect the result values have to be TValue&TValue2 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#95?email_source=notifications&email_token=AACQAJN2MDHGUKF4GYMJRB3Q5FZMTA5CNFSM4KFOXRQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCRNQPCI#pullrequestreview-341510025>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACQAJNKGY35BCEBR7PIC3TQ5FZMTANCNFSM4KFOXRQQ> .

[enumag]

Tests aren't necessary in my opinion. What do you think @ondrejmirtes?

[ondrejmirtes]

You can write some using NodeScopeResolverTest::testFileAsserts() if you want to, it can’t hurt.

[simPod]

To sum up:

diff() and intersect() only modify current map based on keys
merge(), xor(), union() result in some mixed kind of map C from maps A and B

I think that that enumerates all methods that need to be modified here.

[simPod]

I agree TValue2 sucks a bit. Looked around for some inspiration but found none...

[enumag]

Another thought for map: shouldn't there be a TKey2 used as well? There is nothing preventing the other map to have different type of keys, is there? Map::intersect() would return Map<TKey&TKey2, TValue>.

[simPod]

@enumag intersect afaik returns only values whose keys are in both maps. So the keys must be of the same type to return anything.

But for the second group there should indeed be TKey2 and the resulting type is Map<TKey|TKey2, TValue|TValue2>

( merge(), xor(), union() )

[enumag]

intersect afaik returns only values whose keys are in both maps

yup that's why the keys in resulting map need to be both, meaning TKey&TKey2

[simPod]

But both is union |, not intersection &, hm?

[simPod]

Also

$map = new Map([1=>1]);
$map->put('2', '2');

modifies the type but I have no idea how to implement that. Maybe through some extension?

Not sure why tests fail, can't spot a bug. Any idea?

[enumag]

But both is union |, not intersection &, hm?

For the intersect method result to contain a key it needs to be in both maps. Meaning it needs to be of both type TKey and type TKey2 at the same time. Hence &.

[enumag]

As for #95 (comment) you should annotate such map with /** @var Map<string|int, string|int>. It's illegal to change the type later.

[simPod]

you should annotate such map with /** @var Map<string|int, string|int>.

not having to do that is desired, I'm trying to avoid all comments or unnecessary assertions in code if possible.

It's possible with array but I'm not sure how that's implemented.

<?php declare(strict_types = 1);

$array = ['a']; // initially array<string>
$array[] = 1; // phpstan now knows it's array<string|int>

foreach($array as $entry) {
	if(is_string($entry) || is_int($entry)) {
		continue;
	}
	
	throw new \Exception();
}

https://phpstan.org/r/f8a9f24b-de0b-424c-93da-02dafa902560

[enumag]

@simPod I don't know how that's done either. @ondrejmirtes ?

[ondrejmirtes]

@enumag Can you formulate your question? I don't know what you're asking about. This code seems OK: https://phpstan.org/r/f8a9f24b-de0b-424c-93da-02dafa902560

[enumag]

@ondrejmirtes #95 (comment)

[ondrejmirtes]

Is the question how does this piece of code modifies the original object?

$map = new Map([1=>1]);
$map->put('2', '2');

How is the object modified? What's the result of this following code?

$map = new Map([1=>1]);
$map === 'aaa';
$map->put('2', '2');
$map === 'aaa';

[simPod]

I got lost a bit but the question is how to make this work:

<?php declare(strict_types = 1);

$set = new Set('a'); // initially Set<string>
$set->add(1); // phpstan now does not know it's Set<string|int>. How to make PHPStan know?

[ondrejmirtes]

The actual dilema is:

1. Do you want to report it as a problem?
2. Or do you want to modify the type of the set?

I think it makes sense to do only one, and right now it's doing 1). Which is what you want from generics...

[simPod]

It is reported as a problem now and I'd like to modify the type of the set instead. Same behaviour as array has.

[ondrejmirtes]

If you want to make it Set<string|int> from the beginning you should use @var.

[ondrejmirtes]

I don't think this is how generics should work. When people write Set<string> they usually want to enforce that.

[ondrejmirtes]

Otherwise you should use mixed here and not TValue: https://github.com/phpstan/phpstan-src/blob/master/stubs/ext-ds.stub#L567-L572

[simPod]

When people write Set they usually want to enforce that.

Sure but what if I don't write it?
It is still the same with arrays. If I declare array<string> I definitely want to enforce it. But if I write $arr = []; the inner type is not defined. Then this is possible: $arr[] = 1; $arr[] = '1'.

// this should be possible
$set = new Set();
$set->add(1, '1');

// this is error
/** @var Set<string> $set */
$set = new Set();
$set->add(1, '1');

[ondrejmirtes]

I don't think this PR should be blocked on this request. Can I review it?

[simPod]

Definitely agree, I'll just address this #95 (comment) in 15m

[simPod]

Ok, I have not touched this https://github.com/phpstan/phpstan-src/blob/master/stubs/ext-ds.stub#L567-L572 so we can discuss it later.

Same would then be with eg. https://github.com/phpstan/phpstan-src/blob/master/stubs/ext-ds.stub#L227-L233.

You can review, thanks.

[ondrejmirtes]

The test needs to be skipped if Ds extension isn't loaded. (You can for example return an empty array from the data provider.) Otherwise it's good to merge 👍

[ondrejmirtes]

Next step should probably be something that would detect wrong usage of Map::intersect() and Set::intersect(). Some type intersections (like string&int) end up as NeverType and with the current stubs they wouldn't get detected.

It might be interesting to do this generally as part of generics implementation.

[ondrejmirtes]

Thank you!

[ondrejmirtes]

Could you maybe send some similar stubs to https://github.com/JetBrains/phpstorm-stubs? Then PHPStan could analyze the code even without the ds extension loaded. They're probably not intereted in generics since PhpStorm doesn't support them, but even with the usual @param mixed etc. it would be beneficial (not just for PHPStan).

[enumag]

Yeah I was planning to send them there because right now PHPStorm doesn't know about Ds classes at all so I have to use composer install --dev php-ds/php-ds for PHPStorm to even know that these classes exist. I was just waiting for this to be merged. I'll try to do it tomorrow.

[enumag]

See JetBrains/phpstorm-stubs#741