Skip to content

Add regression test#3066

Merged
ondrejmirtes merged 1 commit intophpstan:1.11.xfrom
herndlm:regression-test-bug-3300
May 30, 2024
Merged

Add regression test#3066
ondrejmirtes merged 1 commit intophpstan:1.11.xfrom
herndlm:regression-test-bug-3300

Conversation

@herndlm
Copy link
Copy Markdown
Contributor

@herndlm herndlm commented May 12, 2024 •
edited
Loading

Closes phpstan/phpstan#3300

See also https://phpstan.org/r/f16bc296-b26d-4081-a308-7dd0416def71

Not sure if more precision for constant arrays is needed? For such an edge case the current result might be enough? My PR #2117 adds some complexity, including changes on Type::getKeysArray() and rebasing those changes on latest 1.11.x is not trivial after #2516 and I'm worried they would make things even more complex..

The reason why this is fixed is the early exit in

phpstan-src/src/Type/Php/ArrayKeysFunctionDynamicReturnTypeExtension.php

Lines 30 to 32 in d5a4746

if (count($functionCall->getArgs()) !== 1) {
return null;
}
when more than one argument is given which did not exist yet when the bug was reported.

@herndlm herndlm force-pushed the regression-test-bug-3300 branch from 56f0e9c to 803d153 Compare May 12, 2024 12:39
@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented May 15, 2024

How does this compare to your older PR? #2117

Is this issue really fixed?

@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented May 15, 2024 •
edited
Loading

How does this compare to your older PR? #2117

Is this issue really fixed?

before, when the issue was reported ArrayKeysFunctionDynamicReturnTypeExtension was incorrectly specifying types also when array_keys was called with more than only one arg -> even if people wanted to filter for specific keys we were always returning all constant array keys, see

phpstan-src/src/Type/Php/ArrayKeysFunctionDynamicReturnTypeExtension.php

Line 32 in 1c6ce5d

return $valueType->getKeysArray();

Then I did some refactoring which didn't change behaviour.

Then @staabm added in 89cfc49#diff-a408f91196cfe8e476bde977d3ceab1867fddee0b39a9bf1bb4e94ff7c1a2eb3 the check where the extension would only handle calls of array_keys with a single arg anymore.

And that basically fixed it, I'd say. Since then the extension didn't wrongfully use the constant array keys anymore, the type specification must be falling back to the default function definition, where the types are wider and that doesn't make problems.

Difference to my solution? I tried to add support to array_keys with more than 1 arg and properly use the array keys to have a more specific / narrower type. But that's a nice-to-have thing which adds most likely only unnecessary complexity at the moment, especially after #2516.

@ondrejmirtes ondrejmirtes mentioned this pull request May 30, 2024
Closed
@ondrejmirtes ondrejmirtes merged commit b543e8f into phpstan:1.11.x May 30, 2024
@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented May 30, 2024

Thank you.

@herndlm herndlm deleted the regression-test-bug-3300 branch May 31, 2024 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

array_keys doesn't always return all keys

2 participants

@herndlm @ondrejmirtes