Fix parameter type for Assert::isMap()

[gsteel]

isMap should accept mixed or array<array-key, T>, not array<string, T>. If the input was array<string, T>, we wouldn't need to use Assert::isMap().

[zerkms]

Isn't mixed|array<array-key, T> === mixed?

[gsteel]

The array<array-key, T> preserves the value type when it is asserted via @psalm-assert, so array<array-key, MyThing> becomes array<string, MyThing>

[zerkms]

I guess it's implementation dependent (the static analysis tool), but yep, agree.

[shadowhand]

I think this should just be mixed. As in, remove the @param declaration because the actual type is already mixed $array.

[shadowhand]

This is incorrect, it should be @return array<array-key, T>.

[shadowhand]

Also, this should be array<array-key, T>.

[gsteel]

The static analysis tests disagree with all of these points as do I:

assert/tests/static-analysis/assert-isMap.php

Lines 10 to 38 in a632bc7

	/**
	* @psalm-pure
	*
	* @param mixed $value
	*
	* @return array<string, mixed>
	*/
	function isMap(mixed $value): array
	{
	Assert::isMap($value);
	return $value;
	}
	/**
	* Verifying that the type of the elements in the array is preserved by the assertion
	*
	* @psalm-pure
	*
	* @param array<int|string, stdClass> $value
	*
	* @return array<string, stdClass>
	*/
	function isMapWithKnownType(array $value): array
	{
	Assert::isMap($value);
	return $value;
	}

[shadowhand]

@gsteel the static analysis tests verify expectations of example code, they are not unit tests.

The very first line of isMap is Assert::isArray, thereby ensuring that the input is an array, implying the type is not known beforehand. Thus, the only viable type is mixed $array. In other words:

The assertion isMap takes a mixed input, proves it is a map (aka associative array), then returns it.

[gsteel]

The good thing about the old parameter type mixed|array<array-key, T> and the return type of array<string, T>, is that the template preserves the value type of the input, therefore, if you have already asserted the value type, static analysis will retain the value type whilst also asserting the key type to be a string.

The SA tests here backup that perspective.

Returning or asserting array<array-key, T> means that the isMap method is effectively useless, because a map, by definition is at the very least array<string, mixed>.

This patch effectively reverts incorrect changes to this method.

If you feel like a map is defined by array<array-key, mixed> then please feel free to close this patch.

[shadowhand]

Maps can also be array<int, mixed> so long as the keys are not sequential numbers. Thus, array<string, mixed> would be fundamentally incorrect.

[shadowhand]

However, I see the message in the assertion is also incorrect in this regard.

And I do understand your point about preserving the template value, I'm just not quite sure what to do about it...

[theofidry]

Does this operator make any sense actually?

There is the first issue that keys are casted automatically by PHP, ie if you have the key "23", it will be casted into an integer instead of preserving the string value.

Additionally, a map could end up being a list, yet I do not think it should fail in that scenario.

So given this, is there really a difference between is a map and is an array?

[shadowhand]

@theofidry yes, it does make sense. This is a list: ['apple', 'pear'] and this is a map: ['apple' => 5, 'pear' => 1]. Effectively, Assert::isMap() asserts the inverse of array_is_list() PHP function.

[theofidry]

Isn't [10 => 'a', 20 => 'b'] a perfectly valid map too? In which case, by accident you could end up in a scenario when this maps ends up being a list too.

[gsteel]

In v1.x of this lib, isMap() asserted array<string, T> and I have a ton of Laminas and Mezzio libs that that'll need fixing for v2 compat; for years we've been using isMap() to assert array<string, T>.

I accept the position that a map could mean non-sequential integer keys, in which case the doc-blocks and SA tests are still wrong, and as @theofidry points out, this makes isMap() redundant, because isArray already asserts array<array-key, T>.

Perhaps, we need a new method such as isDict() so we still have something that can assert array<string, T>?

Given that the static analysis tests expect isMap() to assert array<string, T>, I was confused as to why they are not currently failing, but I took a look at psalm.xml and it's currently set to level 8. On level 1, things are markedly worse.