Skip to content

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Oct 13, 2022

This would be the first new array modification function moved to Type. Most likely one of the simplest :)
As you can see and know anyways: it is a very specific thing, but it allows us to get rid of conditional code and getConstantArrays() calls, that I don't like much and are also messy with intersections/unions.

What about the name? There will be more to come, e.g. pop, shift, ... Does a common prefix like array for all of them make sense maybe? I'd definitely group them together everywhere, my worry is just that all of them are only meant for arrays and some could suggest that e.g. strings can be flipped or something like that.

@herndlm herndlm marked this pull request as ready for review October 13, 2022 14:08
@ondrejmirtes
Copy link
Member

A name that makes sense in English would be best.

So flipArray ? 😊

BTW I want to rename getIterableCount to getArraySize. Makes more sense to me 😊

@herndlm
Copy link
Contributor Author

herndlm commented Oct 13, 2022

BTW I want to rename getIterableCount to getArraySize. Makes more sense to me 😊

makes sense. I was also unsure about naming there and found iterable-related things I just kept using. maybe the maybe- and non- implementations should then be moved from the iterable traits to the array traits. not sure if counting iterables makes much sense anyways 🤔 in that case it doesn't even change anything. I'll open a PR in a bit 😊

@herndlm
Copy link
Contributor Author

herndlm commented Oct 13, 2022

I want to add another test case

@herndlm herndlm marked this pull request as draft October 13, 2022 18:03
@herndlm herndlm marked this pull request as ready for review October 13, 2022 18:43
@herndlm
Copy link
Contributor Author

herndlm commented Oct 13, 2022

now it's ok

@@ -139,7 +139,7 @@ public function unsetOffset(Type $offsetType): Type

public function flipArray(): Type
{
return $this;
return new MixedType();
Copy link
Contributor

@staabm staabm Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be more precise, like ArrayType($valueType, 0+$positiveInt)?

Copy link
Contributor Author

@herndlm herndlm Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that shouldn't be necessary because this type should come intersected with an array and intersecting with mixed then just removes it basically. the simpler the better, I think, but I'm also not sure and @rvanvelzen just recently improved performance by not being too precise in type specification. maybe he can confirm/explain it better :)

Copy link
Contributor

@staabm staabm Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it passes a non-constant-list test like

/**
 * @param list<string> $array
 */
function foo7($array)
{
	$flip = array_flip($array);
	assertType('array<string int<0, max>>', $flip);
}

we are good to go :)

@ondrejmirtes
Copy link
Member

makes sense. I was also unsure about naming there

It depends... For example isIterableAtLeastOnce() makes sense to me because it's about iteration, about whether foreach runs at least once.

@ondrejmirtes ondrejmirtes merged commit fe198fb into phpstan:1.9.x Oct 13, 2022
@ondrejmirtes
Copy link
Member

Beautiful, thank you :)

@herndlm herndlm deleted the array-flip branch October 14, 2022 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants