Implement array list type #1751

rvanvelzen · 2022-09-23T15:03:53Z

No description provided.

ondrejmirtes · 2022-09-23T15:05:31Z

Just casually drop this bomb on a Friday afternoon 😀 I had some ideas about this myself, gonna dive right into the code.

ondrejmirtes

This looks great so far :) It's really tedious to go through all array-related functions and other operations and decide:

Does this create a list out of an array that wasn't a list before?
Does this preserve a list when it was already a list?
Does this make list stop being a list?

I'd like some tests around:

Setting an offset to an array. If we overwrite an existing offset (constant array), the list is preserved, if we add a new offset (doesn't matter if it's through [] or [5]), the list should also be preserved. We should also work with some confidence around the value - different tests for the offset value being 5 vs. int.
Unsetting offsets. I'd argue this should always stop being a list. Because if we have [1, 2, 3] and remove the last element, the array still looks like a list but we can't append a new offset to it: https://3v4l.org/JUWKE
Functions hardcoded in NodeScopeResolver - array_push, array_unshift, array_pop, array_shift.
What happens in array_walk

src/Type/Accessory/ListArrayType.php

src/PhpDoc/TypeNodeResolver.php

src/Type/Accessory/ListArrayType.php

ondrejmirtes · 2022-09-23T15:40:26Z

src/Type/Accessory/ListArrayType.php

+
+	public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $unionValues = true): Type
+	{
+		if ($offsetType === null || (new ConstantIntegerType(0))->isSuperTypeOf($offsetType)->yes()) {


Non-zero assignment might still preserve a list? https://3v4l.org/pWCs0

Yes, but because we don't have knowledge about other keys it's not safe to assume it remains a list for non-zero offsets.

ondrejmirtes · 2022-09-23T15:41:39Z

src/Type/Constant/ConstantArrayType.php

@@ -80,6 +81,7 @@ public function __construct(
 		private array $valueTypes,
 		int|array $nextAutoIndexes = [0],
 		private array $optionalKeys = [],
+		private bool $isList = false,


Does this need a new property? Can't we always figure it out from keyTypes and nextAutoIndexes?

I tried that at first but it was a bit buggy. I've reworked some other parts so it might be doable now.

The property is necessary, or at the very least I don't know how to resolve issues that arise otherwise:

getAllArrays() should only return lists if the array is a list, which is unknowable otherwise

other code sometimes relies on ConstantArrayType existing on its own and produces worse type inference with the accessory type separately

ondrejmirtes · 2022-09-23T17:14:32Z

I just remembered an idea from phpstan/phpstan#3499 - if we have $a of list<string> and we check count($a) === 3, $a should be narrowed to array{string, string, string} :) This should be done somewhere in TypeSpecifier around the current count() handling.

herndlm · 2022-09-26T18:22:07Z

I noticed that the webmozart-assert run was also green, but I would have expected that https://github.com/phpstan/phpstan-webmozart-assert/blob/1.2.x/tests/Type/WebMozartAssert/data/array.php#L104 starts failing.
Internally this is done with an expression using array_filter, see https://github.com/phpstan/phpstan-webmozart-assert/blob/1.2.x/src/Type/WebMozartAssert/AssertTypeSpecifyingExtension.php#L347

Maybe a list can be inferred for that as well :)

ondrejmirtes · 2022-09-30T13:10:35Z

What I want to try here (I can do that by myself in a few days): PHPStan without bleeding edge will behave exactly as on 1.8.x, PHPStan with bleeding edge will fully support the list type. Because this is too disruptive to enable for everyone right now, but I want to merge it, and I want to gather feedback from bleeding edge users.

rvanvelzen · 2022-09-30T14:49:51Z

Unfortunately some global state is needed, but this should make it completely disabled for non-bleeding-edge.

ondrejmirtes · 2022-09-30T18:29:01Z

Nice :) Did you observe the failures in the integration projects when the list type was enabled for everyone? Were they legit bugs, or false positives caused by PHPStan changes?

rvanvelzen · 2022-10-01T08:09:29Z

I can't get these tests to fail locally. No idea what is going on :/

Nice :) Did you observe the failures in the integration projects when the list type was enabled for everyone? Were they legit bugs, or false positives caused by PHPStan changes?

I looked at them when they failed and as far as I recall they were all legit failures.

ondrejmirtes · 2022-10-01T10:36:06Z

I can't get these tests to fail locally. No idea what is going on :/

I know, probably. Because there's global state, it's easy to switch it off and make tests fail. Some tests don't use bleedingEdge (RuleTestCase::getAdditionalConfigFiles() - depends on if the child calls parent and merges the value), so the ContainerFactory switches it off, and because the containers are cached, the factory isn't called again to switch it on again.

I think the solution would be to additionally switch it on/off in PHPStanTestCase::getContainer() after the caching if statement. That should make it deterministic.

ondrejmirtes · 2022-10-01T10:37:23Z

Also feel free to fix the getAdditionalConfigFiles() in rule tests that don't explicitly have "NoBleedingEdge" in their name - those should call the parent and merge the value :)

…ecause of named args

ondrejmirtes · 2022-10-02T13:25:51Z

Thank you 🎉 Please expect some follow-up work as I test this in the real world and come up with new ideas :) For example, we could update functionMap.php with list types from here: https://github.com/vimeo/psalm/tree/4.x/dictionaries

The functionMap.php file in PHPStan is roughly for PHP 7.3, so the delta files for that version and before should go to functionMap.php, the delta files after 7.3 should go to respective PHPStan functionMap delta files.

ondrejmirtes · 2022-10-02T13:26:52Z

Oh, we should actually take the list types from the master branch because that's going to be more up to date: https://github.com/vimeo/psalm/tree/master/dictionaries

ondrejmirtes · 2022-10-02T13:31:47Z

non-empty-array<0|1|2|3, literal-string&non-falsy-string>&list should be non-empty-list<0|1|2|3, literal-string&non-falsy-string>. Regexing type description isn't probably a good idea :) phpstan/phpstan#7856 (comment)

ondrejmirtes · 2022-10-02T13:37:48Z

Variadic parameter array should be list<string> instead of array<int, string> here: phpstan/phpstan#5968

It should work in these situations:

Always before PHP 8
With PHP 8.0+ and @no-named-arguments

It shouldn't work:

On PHP 8.0+ without @no-named-arguments (because that's array<int|string, string>)

herndlm · 2022-10-02T13:43:53Z

Nice! I'll see what I can do to infer lists via the webmozart/assert extension soon :)

ondrejmirtes · 2022-10-02T16:24:04Z

An idea - is this how we want to collapse list<mixed>|array<string, mixed>? https://phpstan.org/r/b7ffe028-2aca-472b-a61f-8d7c00b825c1

rvanvelzen · 2022-10-03T09:28:52Z

non-empty-array<0|1|2|3, literal-string&non-falsy-string>&list should be non-empty-list<0|1|2|3, literal-string&non-falsy-string>.

This was on purpose, to always have list<...> equal array<int<0, max>, ...>. To change this the input syntax should also be extended, and I'm not really convinced that that is useful.

An idea - is this how we want to collapse list<mixed>|array<string, mixed>? https://phpstan.org/r/b7ffe028-2aca-472b-a61f-8d7c00b825c1

I think that is the best we can do in general. Trying to keep list separate introduces a lot of complexity in the code that handles unions of constant arrays, because array{} can be seen as both a list and a regular array. I don't think it's worth the complexity.

ondrejmirtes · 2022-10-03T09:35:34Z

Oh, I get and agree with your arguments, thanks for letting me know!

ondrejmirtes requested changes Sep 23, 2022

View reviewed changes

rvanvelzen force-pushed the array-list branch 5 times, most recently from f5ca50a to e308711 Compare September 30, 2022 08:18

rvanvelzen marked this pull request as ready for review September 30, 2022 09:03

ondrejmirtes changed the base branch from 1.8.x to 1.9.x September 30, 2022 12:18

ondrejmirtes force-pushed the array-list branch from e308711 to 01056dd Compare September 30, 2022 13:01

rvanvelzen force-pushed the array-list branch from 01056dd to fe82fef Compare September 30, 2022 14:48

rvanvelzen force-pushed the array-list branch 2 times, most recently from 266375d to c9b678b Compare October 1, 2022 08:07

rvanvelzen and others added 9 commits October 2, 2022 14:59

Implement array list type

Loading
Loading status checks…

9dd7728

Fix array_chunk extension

f0b52f3

ConstantArrayType::getAllArrays() should only return lists for list

043f9f1

Fix issues in PHPStan itself

598741f

Fix ConstantArrayType::getKeysArray() and ::getValuesArray()

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

32f98fe

Fix PHP<8 tests

9b5f425

Always parse list in signature maps as list

a3daa66

Completely disable list type when feature toggle is disabled

3a04cc3

Fix new tests

2829175

NativeFunctionReflectionProvider - caching array no longer static

9832147

ondrejmirtes force-pushed the array-list branch from c9b678b to 9832147 Compare October 2, 2022 13:09

Fix test - container is created by no longer crashes on parse error b…

beda12b

…ecause of named args

ondrejmirtes approved these changes Oct 2, 2022

View reviewed changes

ondrejmirtes merged commit e29ffc2 into phpstan:1.9.x Oct 2, 2022

rvanvelzen deleted the array-list branch October 3, 2022 06:26

ondrejmirtes mentioned this pull request Oct 20, 2022

List todos phpstan/phpstan#8185

Closed

ondrejmirtes mentioned this pull request Aug 22, 2024

Fix ConstantArrayType not accepting NeverType #3327

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement array list type #1751

Implement array list type #1751

rvanvelzen commented Sep 23, 2022

ondrejmirtes commented Sep 23, 2022

ondrejmirtes left a comment

ondrejmirtes Sep 23, 2022

rvanvelzen Sep 26, 2022

ondrejmirtes Sep 23, 2022

rvanvelzen Sep 26, 2022

rvanvelzen Sep 30, 2022

ondrejmirtes commented Sep 23, 2022

herndlm commented Sep 26, 2022

ondrejmirtes commented Sep 30, 2022

rvanvelzen commented Sep 30, 2022

ondrejmirtes commented Sep 30, 2022

rvanvelzen commented Oct 1, 2022

ondrejmirtes commented Oct 1, 2022

ondrejmirtes commented Oct 1, 2022

ondrejmirtes commented Oct 2, 2022

ondrejmirtes commented Oct 2, 2022

ondrejmirtes commented Oct 2, 2022

ondrejmirtes commented Oct 2, 2022

herndlm commented Oct 2, 2022

ondrejmirtes commented Oct 2, 2022

rvanvelzen commented Oct 3, 2022

ondrejmirtes commented Oct 3, 2022

Implement array list type #1751

Implement array list type #1751

Conversation

rvanvelzen commented Sep 23, 2022

ondrejmirtes commented Sep 23, 2022

ondrejmirtes left a comment

Choose a reason for hiding this comment

ondrejmirtes Sep 23, 2022

Choose a reason for hiding this comment

rvanvelzen Sep 26, 2022

Choose a reason for hiding this comment

ondrejmirtes Sep 23, 2022

Choose a reason for hiding this comment

rvanvelzen Sep 26, 2022

Choose a reason for hiding this comment

rvanvelzen Sep 30, 2022

Choose a reason for hiding this comment

ondrejmirtes commented Sep 23, 2022

herndlm commented Sep 26, 2022

ondrejmirtes commented Sep 30, 2022

rvanvelzen commented Sep 30, 2022

ondrejmirtes commented Sep 30, 2022

rvanvelzen commented Oct 1, 2022

ondrejmirtes commented Oct 1, 2022

ondrejmirtes commented Oct 1, 2022

ondrejmirtes commented Oct 2, 2022

ondrejmirtes commented Oct 2, 2022

ondrejmirtes commented Oct 2, 2022

ondrejmirtes commented Oct 2, 2022

herndlm commented Oct 2, 2022

ondrejmirtes commented Oct 2, 2022

rvanvelzen commented Oct 3, 2022

ondrejmirtes commented Oct 3, 2022